From b099b9b5c4f4915d8a322b3c88222775d66b2d73 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Fri, 18 Feb 2022 02:39:58 +0000 Subject: [PATCH 1/2] Add network interface parameters to instance creation - Add helpers to check if an address provided by a user can be requested, i.e., is in the subnet and is not a reserved address. - Add a query used to insert a network interface into the database, handling both IP allocation and validation of any provided instance (i.e., that the instance spans exactly one VPC). This subsumes the previous IP allocation query. - Adds parameters for setting up network interfaces (possibly multiple) in the instance create request, with parameters for each. - Adds more robust and clear database error handling for detecting failures when inserting network interfaces. This code detects specific error messages from the database that we intentionally insert, and makes clear the relationship between the errors we expect, and the invariants they are designed to ensure. This includes special handling of a duplicate primary key, which is only expected during sagas and helps ensure their idempotency. - Ensures idempotency of multi-NIC allocation and rollback during sagas, by adding separate ID-allocation nodes and NIC-creation nodes. The rollback for the latter is a no-op, and the former deletes all NICs for the instance to be created. There are also tests to ensure that we completely any NICs created as a result of failure partway through, and that replaying the saga works as expected. - Adds some more tests to the NIC creation Nexus API - Adds convenience method to delete all NICs from an instance. --- README.adoc | 5 + common/src/api/external/mod.rs | 12 +- common/src/sql/dbinit.sql | 25 +- nexus/src/db/datastore.rs | 205 ++- nexus/src/db/model.rs | 150 +- nexus/src/db/subnet_allocation.rs | 1441 +++++++++++++---- nexus/src/defaults.rs | 3 + nexus/src/external_api/http_entrypoints.rs | 163 +- nexus/src/external_api/params.rs | 66 +- nexus/src/nexus.rs | 156 +- nexus/src/sagas.rs | 277 +++- nexus/test-utils/src/http_testing.rs | 3 +- nexus/test-utils/src/lib.rs | 1 + nexus/test-utils/src/resource_helpers.rs | 2 + nexus/tests/config.test.toml | 2 + nexus/tests/integration_tests/instances.rs | 473 +++++- .../integration_tests/subnet_allocation.rs | 5 +- nexus/tests/integration_tests/unauthorized.rs | 1 + nexus/tests/output/nexus_tags.txt | 6 +- openapi/nexus.json | 394 ++++- sled-agent/src/bin/sled-agent-sim.rs | 1 + sled-agent/src/bin/sled-agent.rs | 1 + tools/oxapi_demo | 54 +- 23 files changed, 2920 insertions(+), 526 deletions(-) diff --git a/README.adoc b/README.adoc index 9c555d19571..812a53e7168 100644 --- a/README.adoc +++ b/README.adoc @@ -645,6 +645,11 @@ documents being checked in. In general, changes any service API **require the following set of build steps**: * Make changes to the service API +* Build the package for the modified service alone. This can be done by changing + directories there, or `cargo build -p `. This is step is important, + to avoid the circular dependency at this point. One needs to update this one + OpenAPI document, without rebuilding the other components that depend on a + now-outdated spec. * Update the OpenAPI document by running the relevant test with overwrite set: `EXPECTORATE=overwrite cargo test test_nexus_openapi_internal` (changing the test name as necessary) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 5df89b2da28..a0aec6172f0 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1774,20 +1774,22 @@ pub struct NetworkInterface { #[serde(flatten)] pub identity: IdentityMetadata, - /** The Instance to which the interface belongs. */ + /// The Instance to which the interface belongs. pub instance_id: Uuid, - /** The VPC to which the interface belongs. */ + /// The VPC to which the interface belongs. pub vpc_id: Uuid, - /** The subnet to which the interface belongs. */ + /// The subnet to which the interface belongs. pub subnet_id: Uuid, - /** The MAC address assigned to this interface. */ + /// The MAC address assigned to this interface. pub mac: MacAddr, - /** The IP address assigned to this interface. */ + /// The IP address assigned to this interface. pub ip: IpAddr, + // TODO-correctness: We need to split this into an optional V4 and optional + // V6 address, at least one of which must be specified. } #[cfg(test)] diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 531f37f26f0..d332f60b030 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -465,8 +465,13 @@ CREATE TABLE omicron.public.network_interface ( time_modified TIMESTAMPTZ NOT NULL, /* Indicates that the object has been deleted */ time_deleted TIMESTAMPTZ, - /* FK into Instance table. */ + + /* FK into Instance table. + * Note that interfaces are always attached to a particular instance. + * IP addresses may be reserved, but this is a different resource. + */ instance_id UUID NOT NULL, + /* FK into VPC table */ vpc_id UUID NOT NULL, /* FK into VPCSubnet table. */ @@ -483,12 +488,6 @@ CREATE TABLE omicron.public.network_interface ( * as moving IPs between NICs on different instances, etc. */ -CREATE UNIQUE INDEX ON omicron.public.network_interface ( - vpc_id, - name -) WHERE - time_deleted IS NULL; - /* Ensure we do not assign the same address twice within a subnet */ CREATE UNIQUE INDEX ON omicron.public.network_interface ( subnet_id, @@ -505,6 +504,18 @@ CREATE UNIQUE INDEX ON omicron.public.network_interface ( ) WHERE time_deleted IS NULL; +/* + * Index used to verify that an Instance's networking is contained + * within a single VPC. + */ +CREATE UNIQUE INDEX ON omicron.public.network_interface ( + instance_id, + name +) +STORING (vpc_id) +WHERE + time_deleted IS NULL; + CREATE TYPE omicron.public.vpc_router_kind AS ENUM ( 'system', 'custom' diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 591c677ef7d..43a392eadc5 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -79,8 +79,9 @@ use crate::db::{ }, pagination::paginated, pagination::paginated_multicolumn, - subnet_allocation::AllocateIpQuery, subnet_allocation::FilterConflictingVpcSubnetRangesQuery, + subnet_allocation::InsertNetworkInterfaceQuery, + subnet_allocation::NetworkInterfaceError, subnet_allocation::SubnetError, update_and_check::{UpdateAndCheck, UpdateStatus}, }; @@ -1698,97 +1699,142 @@ impl DataStore { /* * Network interfaces */ - pub async fn instance_create_network_interface( &self, interface: IncompleteNetworkInterface, - ) -> CreateResult { + ) -> Result { use db::schema::network_interface::dsl; + let query = InsertNetworkInterfaceQuery { + interface: interface.clone(), + now: Utc::now(), + }; + diesel::insert_into(dsl::network_interface) + .values(query) + .returning(NetworkInterface::as_returning()) + .get_result_async(self.pool()) + .await + .map_err(|e| NetworkInterfaceError::from_pool(e, &interface)) + } - // TODO: Longer term, it would be nice to decouple the IP allocation - // (and MAC allocation) from the NetworkInterface table, so that - // retrying from parallel inserts doesn't need to happen here. - - let name = interface.identity.name.clone(); - 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, - vpc_id: interface.vpc_id, - subnet_id: interface.subnet.id(), - mac: interface.mac, - ip: ip.into(), - }; - diesel::insert_into(dsl::network_interface) - .values(row) - .returning(NetworkInterface::as_returning()) - .get_result_async(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::Conflict( - ResourceType::NetworkInterface, - name.as_str(), - ), - ) - }) - } - // Insert and allocate an IP address - None => { - let allocation_query = AllocateIpQuery { - block: ipnetwork::IpNetwork::V4( - interface.subnet.ipv4_block.0 .0, + /// Delete all network interfaces attached to the given instance. + // NOTE: This is mostly useful in the context of sagas, but might be helpful + // in other situations, such as moving an instance between VPC Subnets. + pub async fn instance_delete_all_network_interfaces( + &self, + instance_id: &Uuid, + ) -> DeleteResult { + use db::schema::network_interface::dsl; + let now = Utc::now(); + diesel::update(dsl::network_interface) + .filter(dsl::instance_id.eq(*instance_id)) + .filter(dsl::time_deleted.is_null()) + .set(dsl::time_deleted.eq(now)) + .execute_async(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::Instance, + LookupType::ById(*instance_id), ), - interface, - now: Utc::now(), - }; - diesel::insert_into(dsl::network_interface) - .values(allocation_query) - .returning(NetworkInterface::as_returning()) - .get_result_async(self.pool()) - .await - .map_err(|e| { - if let PoolError::Connection(ConnectionError::Query( - diesel::result::Error::NotFound, - )) = e - { - Error::InvalidRequest { - message: "no available IP addresses" - .to_string(), - } - } else { - public_error_from_diesel_pool( - e, - ErrorHandler::Conflict( - ResourceType::NetworkInterface, - name.as_str(), - ), - ) - } - }) - } - } + ) + })?; + Ok(()) } pub async fn instance_delete_network_interface( &self, - network_interface_id: &Uuid, + interface_id: &Uuid, ) -> DeleteResult { use db::schema::network_interface::dsl; + let now = Utc::now(); + let result = diesel::update(dsl::network_interface) + .filter(dsl::id.eq(*interface_id)) + .filter(dsl::time_deleted.is_null()) + .set((dsl::time_deleted.eq(now),)) + .check_if_exists::(*interface_id) + .execute_and_check(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::NetworkInterface, + LookupType::ById(*interface_id), + ), + ) + })?; + match result.status { + UpdateStatus::Updated => Ok(()), + UpdateStatus::NotUpdatedButExists => { + let interface = &result.found; + if interface.time_deleted().is_some() { + // Already deleted + Ok(()) + } else { + Err(Error::internal_error(&format!( + "failed to delete network interface: {}", + interface_id + ))) + } + } + } + } + + pub async fn subnet_lookup_network_interface( + &self, + subnet_id: &Uuid, + interface_name: &Name, + ) -> LookupResult { + use db::schema::network_interface::dsl; + + dsl::network_interface + .filter(dsl::subnet_id.eq(*subnet_id)) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::name.eq(interface_name.clone())) + .select(db::model::NetworkInterface::as_select()) + .get_result_async(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::NetworkInterface, + LookupType::ByName(interface_name.to_string()), + ), + ) + }) + } - // TODO-correctness: Do not allow deleting interfaces on running - // instances until we support hotplug + /// List network interfaces associated with a given instance. + pub async fn instance_list_network_interfaces( + &self, + instance_id: &Uuid, + pagparams: &DataPageParams<'_, Name>, + ) -> ListResultVec { + use db::schema::network_interface::dsl; + paginated(dsl::network_interface, dsl::name, &pagparams) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::instance_id.eq(*instance_id)) + .select(NetworkInterface::as_select()) + .load_async::(self.pool()) + .await + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + } - let now = Utc::now(); - diesel::update(dsl::network_interface) + /// Get a network interface by name attached to an instance + pub async fn instance_lookup_network_interface( + &self, + instance_id: &Uuid, + interface_name: &Name, + ) -> LookupResult { + use db::schema::network_interface::dsl; + dsl::network_interface + .filter(dsl::instance_id.eq(*instance_id)) + .filter(dsl::name.eq(interface_name.clone())) .filter(dsl::time_deleted.is_null()) - .filter(dsl::id.eq(*network_interface_id)) - .set(dsl::time_deleted.eq(now)) - .returning(NetworkInterface::as_returning()) + .select(NetworkInterface::as_select()) .get_result_async(self.pool()) .await .map_err(|e| { @@ -1796,11 +1842,10 @@ impl DataStore { e, ErrorHandler::NotFoundByLookup( ResourceType::NetworkInterface, - LookupType::ById(*network_interface_id), + LookupType::ByName(interface_name.to_string()), ), ) - })?; - Ok(()) + }) } // Create a record for a new Oximeter instance diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index 8870ff0b6f1..e9615092fd2 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -33,6 +33,8 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::convert::TryFrom; use std::net::IpAddr; +use std::net::Ipv4Addr; +use std::net::Ipv6Addr; use std::net::SocketAddr; use uuid::Uuid; @@ -320,6 +322,23 @@ pub struct Ipv4Net(pub external::Ipv4Net); NewtypeFrom! { () pub struct Ipv4Net(external::Ipv4Net); } NewtypeDeref! { () pub struct Ipv4Net(external::Ipv4Net); } +impl Ipv4Net { + /// Check if an address is a valid user-requestable address for this subnet + pub fn check_requestable_addr(&self, addr: Ipv4Addr) -> bool { + self.contains(addr) + && ( + // First N addresses are reserved + self.iter() + .take(defaults::NUM_INITIAL_RESERVED_IP_ADDRESSES) + .all(|this| this != addr) + ) + && ( + // Last address in the subnet is reserved + addr != self.broadcast() + ) + } +} + impl ToSql for Ipv4Net where DB: Backend, @@ -404,6 +423,16 @@ impl Ipv6Net { .expect("Failed to create random subnet"); Some(Self(external::Ipv6Net(net))) } + + /// Check if an address is a valid user-requestable address for this subnet + pub fn check_requestable_addr(&self, addr: Ipv6Addr) -> bool { + // Only the first N addresses are reserved + self.contains(addr) + && self + .iter() + .take(defaults::NUM_INITIAL_RESERVED_IP_ADDRESSES) + .all(|this| this != addr) + } } impl ToSql for Ipv6Net @@ -1004,7 +1033,7 @@ impl_enum_type!( #[postgres(type_name = "instance_state", type_schema = "public")] pub struct InstanceStateEnum; - #[derive(Clone, Debug, AsExpression, FromSqlRow)] + #[derive(Clone, Debug, PartialEq, AsExpression, FromSqlRow)] #[sql_type = "InstanceStateEnum"] pub struct InstanceState(pub external::InstanceState); @@ -1413,29 +1442,29 @@ impl VpcSubnet { /// /// - 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 { + /// - The address is not reserved. + pub fn check_requestable_addr( + &self, + addr: IpAddr, + ) -> Result<(), external::Error> { + let subnet = 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, - ))) + if self.ipv4_block.check_requestable_addr(addr) { + return Ok(()); } + ipnetwork::IpNetwork::V4(self.ipv4_block.0 .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, - ))) + if self.ipv6_block.check_requestable_addr(addr) { + return Ok(()); } + ipnetwork::IpNetwork::V6(self.ipv6_block.0 .0) } - } + }; + Err(external::Error::invalid_request(&format!( + "Address '{}' not in subnet '{}' or is reserved for rack services", + addr, subnet, + ))) } } @@ -2005,12 +2034,14 @@ impl IncompleteNetworkInterface { vpc_id: Uuid, subnet: VpcSubnet, mac: MacAddr, + identity: external::IdentityMetadataCreateParams, ip: Option, - params: params::NetworkInterfaceCreate, - ) -> Self { - let identity = - NetworkInterfaceIdentity::new(interface_id, params.identity); - Self { identity, instance_id, subnet, vpc_id, mac, ip } + ) -> Result { + if let Some(ip) = ip { + subnet.check_requestable_addr(ip)?; + }; + let identity = NetworkInterfaceIdentity::new(interface_id, identity); + Ok(Self { identity, instance_id, subnet, vpc_id, mac, ip }) } } @@ -2025,6 +2056,11 @@ pub struct NetworkInterface { pub subnet_id: Uuid, pub mac: MacAddr, pub ip: ipnetwork::IpNetwork, + // TODO-correctness: We need to split this into an optional V4 and optional V6 address, at + // least one of which will always be specified. + // + // If user requests an address of either kind, give exactly that and not the other. + // If neither is specified, auto-assign one of each? } impl From for external::NetworkInterface { @@ -2173,7 +2209,7 @@ mod tests { use std::net::Ipv6Addr; #[test] - fn test_vpc_subnet_contains() { + fn test_vpc_subnet_check_requestable_addr() { let ipv4_block = Ipv4Net("192.168.0.0/16".parse::().unwrap()); let ipv6_block = Ipv6Net("fd00::/48".parse::().unwrap()); @@ -2181,27 +2217,54 @@ mod tests { name: "net-test-vpc".parse().unwrap(), description: "A test VPC".parse().unwrap(), }; - let vpc = VpcSubnet::new( + let subnet = VpcSubnet::new( Uuid::new_v4(), Uuid::new_v4(), identity, ipv4_block, ipv6_block, ); - assert!(vpc - .contains(IpAddr::from(Ipv4Addr::new(192, 168, 1, 1))) + // Within subnet + assert!(subnet + .check_requestable_addr(IpAddr::from(Ipv4Addr::new( + 192, 168, 1, 10 + ))) .is_ok()); - assert!(vpc - .contains(IpAddr::from(Ipv4Addr::new(192, 160, 1, 1))) + // Network address is reserved + assert!(subnet + .check_requestable_addr(IpAddr::from(Ipv4Addr::new(192, 168, 0, 0))) .is_err()); - assert!(vpc - .contains(IpAddr::from(Ipv6Addr::new( - 0xfd00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 + // Broadcast address is reserved + assert!(subnet + .check_requestable_addr(IpAddr::from(Ipv4Addr::new( + 192, 168, 255, 255 + ))) + .is_err()); + // Within subnet, but reserved + assert!(subnet + .check_requestable_addr(IpAddr::from(Ipv4Addr::new(192, 168, 0, 1))) + .is_err()); + // Not within subnet + assert!(subnet + .check_requestable_addr(IpAddr::from(Ipv4Addr::new(192, 160, 1, 1))) + .is_err()); + + // Within subnet + assert!(subnet + .check_requestable_addr(IpAddr::from(Ipv6Addr::new( + 0xfd00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10 ))) .is_ok()); - assert!(vpc - .contains(IpAddr::from(Ipv6Addr::new( - 0xfc00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 + // Within subnet, but reserved + assert!(subnet + .check_requestable_addr(IpAddr::from(Ipv6Addr::new( + 0xfd00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 + ))) + .is_err()); + // Not within subnet + assert!(subnet + .check_requestable_addr(IpAddr::from(Ipv6Addr::new( + 0xfc00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10 ))) .is_err()); } @@ -2238,4 +2301,21 @@ mod tests { ); assert_eq!(base.random_subnet(base.prefix()), Some(base)); } + + #[test] + fn test_ip_subnet_check_requestable_address() { + let subnet = super::Ipv4Net(Ipv4Net("192.168.0.0/16".parse().unwrap())); + assert!(subnet.check_requestable_addr("192.168.0.10".parse().unwrap())); + assert!(subnet.check_requestable_addr("192.168.1.0".parse().unwrap())); + assert!(!subnet.check_requestable_addr("192.168.0.0".parse().unwrap())); + assert!(subnet.check_requestable_addr("192.168.0.255".parse().unwrap())); + assert!( + !subnet.check_requestable_addr("192.168.255.255".parse().unwrap()) + ); + + let subnet = super::Ipv6Net(Ipv6Net("fd00::/64".parse().unwrap())); + assert!(subnet.check_requestable_addr("fd00::a".parse().unwrap())); + assert!(!subnet.check_requestable_addr("fd00::1".parse().unwrap())); + assert!(subnet.check_requestable_addr("fd00::1:1".parse().unwrap())); + } } diff --git a/nexus/src/db/subnet_allocation.rs b/nexus/src/db/subnet_allocation.rs index ece69aa0e4d..b1b1bba0bb7 100644 --- a/nexus/src/db/subnet_allocation.rs +++ b/nexus/src/db/subnet_allocation.rs @@ -12,217 +12,32 @@ use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; use diesel::sql_types; +use ipnetwork::IpNetwork; use omicron_common::api::external; use std::convert::TryFrom; use uuid::Uuid; -/// Used for allocating an IP as part of [`NetworkInterface`] construction. -/// -/// This is a query equivalent to: -/// SELECT AS id, AS name, AS description, -/// AS time_created, AS time_modified, -/// AS instance_id, AS vpc_id, -/// AS subnet_id, AS mac, + off AS ip -/// FROM -/// generate_series(5, ) AS off -/// LEFT OUTER JOIN -/// network_interface -/// ON (subnet_id, ip, time_deleted IS NULL) = -/// (, + off, TRUE) -/// 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. 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 -// retries. -pub struct AllocateIpQuery { - pub interface: IncompleteNetworkInterface, - pub block: ipnetwork::IpNetwork, - pub now: DateTime, -} - -/// Used for using AllocateIpQuery with an INSERT statement. Do not use this -/// directly, instead pass an instance of [`AllocateIpQuery`] to -/// [`InsertStatement::values`]. -pub struct AllocateIpQueryValues(AllocateIpQuery); - -impl QueryId for AllocateIpQuery { - type QueryId = (); - const HAS_STATIC_QUERY_ID: bool = false; -} - -impl Insertable for AllocateIpQuery { - type Values = AllocateIpQueryValues; - - fn values(self) -> Self::Values { - AllocateIpQueryValues(self) - } -} - -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) => { - // If we're allocating from a v6 subnet with more than 2^63 - 1 - // addresses, just cap the size we'll explore. This will never - // fail in practice since we're never going to be storing 2^64 - // rows in the network_interface table. - i64::try_from(network.size() - 1).unwrap_or(i64::MAX) - } - }; - - out.push_sql("SELECT "); - - out.push_bind_param::( - &self.interface.identity.id, - )?; - out.push_sql(" AS "); - out.push_identifier(dsl::id::NAME)?; - out.push_sql(", "); - - out.push_bind_param::( - &self.interface.identity.name.to_string(), - )?; - out.push_sql(" AS "); - out.push_identifier(dsl::name::NAME)?; - out.push_sql(", "); - - out.push_bind_param::( - &self.interface.identity.description, - )?; - out.push_sql(" AS "); - out.push_identifier(dsl::description::NAME)?; - out.push_sql(", "); - - out.push_bind_param::(&self.now)?; - out.push_sql(" AS "); - out.push_identifier(dsl::time_created::NAME)?; - out.push_sql(", "); - - out.push_bind_param::(&self.now)?; - out.push_sql(" AS "); - out.push_identifier(dsl::time_modified::NAME)?; - out.push_sql(", "); - - out.push_bind_param::( - &self.interface.instance_id, - )?; - out.push_sql(" AS "); - out.push_identifier(dsl::instance_id::NAME)?; - out.push_sql(", "); - - out.push_bind_param::(&self.interface.vpc_id)?; - out.push_sql(" AS "); - out.push_identifier(dsl::vpc_id::NAME)?; - out.push_sql(", "); - - out.push_bind_param::( - &self.interface.subnet.id(), - )?; - out.push_sql(" AS "); - out.push_identifier(dsl::subnet_id::NAME)?; - out.push_sql(", "); - - out.push_bind_param::( - &self.interface.mac.to_string(), - )?; - out.push_sql(" AS "); - out.push_identifier(dsl::mac::NAME)?; - out.push_sql(", "); - - out.push_bind_param::( - &self.block.network().into(), - )?; - out.push_sql(" + "); - out.push_identifier("off")?; - out.push_sql(" AS "); - out.push_identifier(dsl::ip::NAME)?; - - // Skip the initial reserved addresses and the broadcast address. - out.push_sql(" FROM generate_series(5, "); - out.push_bind_param::( - &(last_address_offset - 1), - )?; - out.push_sql(") AS "); - out.push_identifier("off")?; - out.push_sql(" LEFT OUTER JOIN "); - dsl::network_interface.from_clause().walk_ast(out.reborrow())?; - - // ON (subnet_id, ip, time_deleted IS NULL) = - // (, + off, TRUE) - out.push_sql(" ON ("); - out.push_identifier(dsl::subnet_id::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::ip::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::time_deleted::NAME)?; - out.push_sql(" IS NULL) = ("); - out.push_bind_param::( - &self.interface.subnet.id(), - )?; - out.push_sql(", "); - out.push_bind_param::( - &self.block.network().into(), - )?; - out.push_sql(" + "); - out.push_identifier("off")?; - out.push_sql(", TRUE) "); - // WHERE ip IS NULL LIMIT 1; - out.push_sql("WHERE "); - out.push_identifier(dsl::ip::NAME)?; - out.push_sql(" IS NULL LIMIT 1"); - Ok(()) - } -} - -impl QueryId for AllocateIpQueryValues { - type QueryId = (); - const HAS_STATIC_QUERY_ID: bool = false; -} +// Helper to return the offset of the last valid/allocatable IP in a subnet. +fn generate_last_address_offset(subnet: &ipnetwork::IpNetwork) -> i64 { + // 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 subnet { + ipnetwork::IpNetwork::V4(network) => network.size() as i64 - 1, + ipnetwork::IpNetwork::V6(network) => { + // If we're allocating from a v6 subnet with more than 2^63 - 1 + // addresses, just cap the size we'll explore. This will never + // fail in practice since we're never going to be storing 2^64 + // rows in the network_interface table. + i64::try_from(network.size() - 1).unwrap_or(i64::MAX) + } + }; -impl diesel::insertable::CanInsertInSingleQuery for AllocateIpQueryValues { - fn rows_to_insert(&self) -> Option { - Some(1) - } -} - -impl QueryFragment for AllocateIpQueryValues { - fn walk_ast(&self, mut out: AstPass) -> diesel::QueryResult<()> { - use db::schema::network_interface::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::instance_id::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::vpc_id::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::subnet_id::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::mac::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::ip::NAME)?; - out.push_sql(") "); - self.0.walk_ast(out) - } + // This subtraction is because the last address in a subnet is + // explicitly reserved for Oxide use. + last_address_offset - 1 } /// Errors related to allocating VPC Subnets. @@ -490,142 +305,771 @@ impl QueryFragment for FilterConflictingVpcSubnetRangesQueryValues { } } +/// Errors related to inserting or attaching a NetworkInterface +#[derive(Debug)] +pub enum NetworkInterfaceError { + /// The instance specified for this interface is already associated with a + /// different VPC from this interface. + InstanceSpansMultipleVpcs(Uuid), + /// There are no available IP addresses in the requested subnet + NoAvailableIpAddresses, + /// An explicitly-requested IP address is already in use + IpAddressNotAvailable(std::net::IpAddr), + /// A primary key violation, which is intentionally caused in some cases + /// during instance creation sagas. + DuplicatePrimaryKey(Uuid), + /// Any other error + External(external::Error), +} + +impl NetworkInterfaceError { + /// Construct a `NetworkInterfaceError` from a database error + /// + /// This catches the various errors that the `InsertNetworkInterfaceQuery` + /// can generate, especially the intentional errors that indicate either IP + /// address exhaustion or an attempt to attach an interface to an instance + /// that is already associated with another VPC. + pub fn from_pool( + e: async_bb8_diesel::PoolError, + interface: &IncompleteNetworkInterface, + ) -> Self { + use crate::db::error; + use async_bb8_diesel::ConnectionError; + use async_bb8_diesel::PoolError; + use diesel::result::Error; + match e { + // Catch the specific errors designed to communicate the failures we + // want to distinguish + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError(_, _), + )) => decode_database_error(e, interface), + // Any other error at all is a bug + _ => NetworkInterfaceError::External( + error::public_error_from_diesel_pool( + e, + error::ErrorHandler::Server, + ), + ), + } + } + + /// Convert this error into an external one. + pub fn into_external(self) -> external::Error { + match self { + NetworkInterfaceError::NoAvailableIpAddresses => { + external::Error::invalid_request( + "No available IP addresses for interface", + ) + } + NetworkInterfaceError::InstanceSpansMultipleVpcs(_) => { + external::Error::invalid_request(concat!( + "Networking may not span multiple VPCs, but the ", + "requested instance is associated with another VPC" + )) + } + NetworkInterfaceError::IpAddressNotAvailable(ip) => { + external::Error::invalid_request(&format!( + "The IP address '{}' is not available", + ip + )) + } + NetworkInterfaceError::DuplicatePrimaryKey(id) => { + external::Error::InternalError { + internal_message: format!( + "Found duplicate primary key '{}' when inserting network interface", + id + ), + } + } + NetworkInterfaceError::External(e) => e, + } + } +} + +/// Decode an error from the database to determine why our NIC query failed. +/// +/// When inserting network interfaces, we use the `InsertNetworkInterfaceQuery`, +/// which is designed to fail in particular ways depending on the requested +/// data. For example, if the client requests a new NIC on an instance, where +/// that instance already has a NIC from a VPC that's different from the new +/// one, we handle that here. +/// +/// This function works by inspecting the detailed error messages, including +/// indexes used or constraints violated, to determine the cause of the failure. +/// As such, it naturally is extremely tightly coupled to the database itself, +/// including the software version and our schema. +fn decode_database_error( + err: async_bb8_diesel::PoolError, + interface: &IncompleteNetworkInterface, +) -> NetworkInterfaceError { + use crate::db::error; + use async_bb8_diesel::ConnectionError; + use async_bb8_diesel::PoolError; + use diesel::result::DatabaseErrorKind; + use diesel::result::Error; + + // Error message generated when we attempt to insert an interface in a + // different VPC from the interface(s) already associated with the instance + const MULTIPLE_VPC_ERROR_MESSAGE: &str = + r#"could not parse "" as type uuid: uuid: incorrect UUID length: "#; + + // Error message generated when we attempt to insert NULL in the `ip` + // column, which only happens when we run out of IPs in the subnet. + const IP_EXHAUSTION_ERROR_MESSAGE: &str = + r#"null value in column "ip" violates not-null constraint"#; + + // The name of the index whose uniqueness is violated if we try to assign an + // IP that is already allocated to another interface in the same subnet. + const IP_NOT_AVAILABLE_CONSTRAINT: &str = + "network_interface_subnet_id_ip_key"; + + // The name of the index whose uniqueness is violated if we try to assign a + // name to an interface that is already used for another interface on the + // same instance. + const NAME_CONFLICT_CONSTRAINT: &str = + "network_interface_instance_id_name_key"; + + // The primary key constraint. This is intended only to be caught, and + // usually ignored, in sagas. UUIDs are allocated in one node of the saga, + // and then the NICs in a following node. In that case, primary key + // conflicts would be expected, if we're recovering from a crash during that + // node and replaying it, without first having unwound the whole saga. This + // should _only_ be ignored in that case. In any other circumstance, this + // should likely be converted to a 500-level server error + const PRIMARY_KEY_CONSTRAINT: &str = "primary"; + + 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 + // that colum has been violated. + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError(DatabaseErrorKind::NotNullViolation, ref info), + )) if info.message() == IP_EXHAUSTION_ERROR_MESSAGE => { + NetworkInterfaceError::NoAvailableIpAddresses + } + + // This catches the error intentionally introduced by the + // `push_ensure_unique_vpc_expression` subquery, which generates a + // UUID parsing error if an instance is already associated with + // another VPC. + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError(DatabaseErrorKind::Unknown, info), + )) if info.message() == MULTIPLE_VPC_ERROR_MESSAGE => { + NetworkInterfaceError::InstanceSpansMultipleVpcs( + interface.instance_id, + ) + } + + // This path looks specifically at constraint names. + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError(DatabaseErrorKind::UniqueViolation, ref info), + )) => match info.constraint_name() { + // Constraint violated if a user-requested IP address has + // already been assigned within the same VPC Subnet. + Some(constraint) if constraint == IP_NOT_AVAILABLE_CONSTRAINT => { + let ip = interface + .ip + .unwrap_or_else(|| std::net::Ipv4Addr::UNSPECIFIED.into()); + NetworkInterfaceError::IpAddressNotAvailable(ip) + } + + // Constraint violated if the user-requested name is already + // assigned to an interface on this instance. + Some(constraint) if constraint == NAME_CONFLICT_CONSTRAINT => { + NetworkInterfaceError::External( + error::public_error_from_diesel_pool( + err, + error::ErrorHandler::Conflict( + external::ResourceType::NetworkInterface, + interface.identity.name.as_str(), + ), + ), + ) + } + + // Primary key constraint violation. See notes above. + Some(constraint) if constraint == PRIMARY_KEY_CONSTRAINT => { + NetworkInterfaceError::DuplicatePrimaryKey( + interface.identity.id, + ) + } + + // Any other constraint violation is a bug + _ => NetworkInterfaceError::External( + error::public_error_from_diesel_pool( + err, + error::ErrorHandler::Server, + ), + ), + }, + + // Any other error at all is a bug + _ => NetworkInterfaceError::External( + error::public_error_from_diesel_pool( + err, + error::ErrorHandler::Server, + ), + ), + } +} + +/// Add a subquery intended to verify that an Instance's networking does not +/// span multiple VPCs. +/// +/// As described in RFD 21, an Instance's networking is confined to a single +/// VPC. That is, any NetworkInterfaces attached to an Instance must all have +/// the same VPC ID. This function adds a subquery, shown below, that fails in a +/// specific way (parsing error) if that invariant is violated. The basic +/// structure of the query is: +/// +/// ```text +/// CAST(IF(, '', '') AS UUID) +/// ``` +/// +/// This selects either the actual VPC UUID (as a string) or the empty string, +/// if any existing VPC IDs for this instance are the same. If true, we cast the +/// VPC ID string back to a UUID. If false, we try to cast the empty string, +/// which fails in a detectable way. +/// +/// Details +/// ------- +/// +/// The exact query generated looks like this: +/// +/// ```sql +/// CAST(IF( +/// COALESCE( +/// ( +/// SELECT vpc_id +/// FROM network_interface +/// WHERE +/// time_deleted IS NULL AND +/// instance_id = +/// LIMIT 1 +/// ), +/// +/// ) = , +/// '', -- UUID as a string +/// '' +/// ) AS UUID) +/// ``` +/// +/// This uses a partial index on the `network_interface` table to look up the +/// first record with the provided `instance_id`, if any. It then compares that +/// stored `vpc_id` to the one provided to this query. If those IDs match, then +/// the ID is returned. If they do _not_ match, the `IF` statement returns an +/// empty string, which it tries to cast as a UUID. That fails, in a detectable +/// way, so that we can check this case as distinct from other errors. +/// +/// Note that the `COALESCE` expression is there to handle the case where there +/// _is_ no record with the given `instance_id`. In that case, the `vpc_id` +/// provided is returned directly, so everything works as if the IDs matched. +fn push_ensure_unique_vpc_expression( + mut out: AstPass, + vpc_id: &Uuid, + instance_id: &Uuid, +) -> diesel::QueryResult<()> { + use db::schema::network_interface::dsl; + + out.push_sql("CAST(IF(COALESCE((SELECT "); + out.push_identifier(dsl::vpc_id::NAME)?; + out.push_sql(" FROM "); + dsl::network_interface.from_clause().walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL AND "); + out.push_identifier(dsl::instance_id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(instance_id)?; + out.push_sql(" LIMIT 1), "); + out.push_bind_param::(vpc_id)?; + out.push_sql(") = "); + out.push_bind_param::(vpc_id)?; + out.push_sql(", "); + + // NOTE: This bind-parameter is intentionally a string, rather than a UUID. + // + // This query relies on the fact that it generates a parsing error in the + // case where there is an interface attached to a VPC that's _different_ + // from the VPC of the candidate interface. This is so that we can + // distinguish this error case from the one where there is no IP address + // available. + // + // To do that, we generate a query like: + // + // ``` + // CAST(IF(, '', '') AS UUID) + // ``` + // + // That empty-string cannot be cast to a UUID, so we get a parsing error, + // but only if the condition _succeeds_. It's not evaluated otherwise. + // + // However, if we push this parameter as a UUID explicitly, the database + // looks at the parts of the `IF` statement, and tries to make them a common + // type, a UUID. That's the exact error we're trying to produce, but it's + // evaluated too early. So we ensure both are strings here, and then ask the + // DB to cast them after that condition is evaluated. + out.push_bind_param::(&vpc_id.to_string())?; + out.push_sql(", '') AS UUID)"); + Ok(()) +} + +/// Push a subquery that selects the next available IP address from a subnet. +/// +/// This adds a subquery like: +/// +/// ```sql +/// SELECT +/// + address_offset AS ip +/// FROM +/// generate_series(5, ) AS address_offset +/// LEFT OUTER JOIN +/// network_interface +/// ON +/// (subnet_id, ip, time_deleted IS NULL) = +/// ( + address_offset, TRUE) +/// WHERE ip IS NULL LIMIT 1 +/// ``` +/// +/// This is a linear, sequential scan for an IP from the subnet that's not yet +/// been allocated. We'd ultimately like a better-performing allocation +/// strategy; for example, we might be able to keep the lowest unallocated +/// address for each subnet, and atomically return and increment that. +/// +/// This would work fine, but explicit reservations of IP addresses complicate +/// the picture. We'd need a more complex data structure to manage the ranges of +/// available address for each subnet, especially to manage coalescing those +/// ranges as addresses are released back to the pool. +fn push_select_next_available_ip_subquery( + mut out: AstPass, + subnet: &IpNetwork, + subnet_id: &Uuid, +) -> diesel::QueryResult<()> { + use db::schema::network_interface::dsl; + let last_address_offset = generate_last_address_offset(&subnet); + let network_address = IpNetwork::from(subnet.network()); + out.push_sql("SELECT "); + out.push_bind_param::(&network_address)?; + out.push_sql(" + "); + out.push_identifier("address_offset")?; + out.push_sql(" AS "); + out.push_identifier(dsl::ip::NAME)?; + + out.push_sql( + format!( + " FROM generate_series({}, ", + crate::defaults::NUM_INITIAL_RESERVED_IP_ADDRESSES + ) + .as_str(), + ); + out.push_bind_param::(&last_address_offset)?; + out.push_sql(") AS "); + out.push_identifier("address_offset")?; + out.push_sql(" LEFT OUTER JOIN "); + dsl::network_interface.from_clause().walk_ast(out.reborrow())?; + out.push_sql(" ON ("); + out.push_identifier(dsl::subnet_id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::ip::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL) = ("); + out.push_bind_param::(&subnet_id)?; + out.push_sql(", "); + out.push_bind_param::(&network_address)?; + out.push_sql(" + "); + out.push_identifier("address_offset")?; + out.push_sql(", TRUE) "); + out.push_sql("WHERE "); + out.push_identifier(dsl::ip::NAME)?; + out.push_sql(" IS NULL LIMIT 1"); + Ok(()) +} + +/// Subquery used to insert a _new_ `NetworkInterface` from parameters. +/// +/// This function is used to construct a query that allows inserting a +/// `NetworkInterface`, supporting both optionally allocating a new IP address +/// and verifying that the attached instance's networking is contained within a +/// single VPC. The general query looks like: +/// +/// ```sql +/// +/// SELECT AS id, AS name, AS description, +/// AS time_created, AS time_modified, +/// NULL AS time_deleted, AS instance_id, AS vpc_id, +/// AS subnet_id, AS mac, +/// ``` +/// +/// Instance validation +/// ------------------- +/// +/// This query generates a CTE that checks that the requested instance is not +/// already associated with another VPC (since an instance's networking cannot +/// span multiple VPCs). This query is designed to fail in a particular way if +/// that invariant is violated, so that we can detect and report that case to +/// the user. See [`push_ensure_unique_vpc_expression`] for details of that +/// subquery, including how it fails. +/// +/// IP allocation subquery +/// ---------------------- +/// +/// If the user explicitly requests an IP address, this part of the query is +/// just that exact IP. The query may still fail if the IP is already in use, +/// which is detected and forwarded as a client error. +/// +/// If the user wants an address allocated, then this generates a subquery that +/// tries to find the next available IP address (if any). See +/// [`push_select_next_available_ip_subquery`] for details on that allocation +/// subquery. If that fails, due to address exhaustion, this is detected and +/// forwarded to the caller. +/// +/// Errors +/// ------ +/// +/// See [`NetworkInterfaceError`] for the errors caught and propagated by this +/// query. +/// +/// Notes +/// ----- +/// +/// This query is designed so that, if the instance-validation subquery fails, +/// we do not run the address allocation query. This is just for performance; +/// since the allocation query runs in a time proportional to the smallest +/// unallocated address in the subnet, we'd like to avoid that if the query will +/// just fail the other, VPC-validation check. +/// +/// It's not easy to verify that this is indeed the case, since running `EXPLAIN +/// ANALYZE` to get details about the number of rows read can't work, as the +/// query will fail. By putting this in a CTE, prior to the rest of the main +/// query, it seems likely that the database will run that portion first. In +/// particular, [this +/// note](https://www.cockroachlabs.com/docs/v21.2/subqueries#performance-best-practices) +/// claims that scalar subqueries, which generate a single value as this one +/// does, are completely executed and stored in memory before the surrounding +/// query starts. Thus the instance-validation subquery should run entirely +/// before the remainder of the query. +/// +/// It's still possible that the engine runs the IP allocation subquery too, +/// either before or concurrently with the instance-validation subquery. It's +/// not clear how to test for this. But if this does become obvious, that +/// portion of the query might need to be placed behind a conditional evaluation +/// expression, such as `IF` or `COALESCE`, which only runs the subquery when +/// the instance-validation check passes. +fn push_interface_allocation_subquery( + mut out: AstPass, + interface: &IncompleteNetworkInterface, + now: &DateTime, +) -> diesel::QueryResult<()> { + use db::schema::network_interface::dsl; + // Push the CTE that ensures that any other interface with the same + // instance_id also has the same vpc_id. See + // `push_ensure_unique_vpc_expression` for more details. This ultimately + // fails the query if the requested instance is already associated with + // a different VPC. + out.push_sql("WITH vpc("); + out.push_identifier(dsl::vpc_id::NAME)?; + out.push_sql(") AS "); + out.push_sql("(SELECT "); + push_ensure_unique_vpc_expression( + out.reborrow(), + &interface.vpc_id, + &interface.instance_id, + )?; + out.push_sql(") "); + + // Push the columns, values and names, that are named directly. These + // are known regardless of whether we're allocating an IP address. These + // are all written as `SELECT AS , AS , ... + out.push_sql("SELECT "); + out.push_bind_param::(&interface.identity.id)?; + out.push_sql(" AS "); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(", "); + + out.push_bind_param::( + &interface.identity.name.as_str(), + )?; + out.push_sql(" AS "); + out.push_identifier(dsl::name::NAME)?; + out.push_sql(", "); + + out.push_bind_param::( + &interface.identity.description.as_str(), + )?; + out.push_sql(" AS "); + out.push_identifier(dsl::description::NAME)?; + out.push_sql(", "); + + out.push_bind_param::>(&now)?; + out.push_sql(" AS "); + out.push_identifier(dsl::time_created::NAME)?; + out.push_sql(", "); + + out.push_bind_param::>(&now)?; + out.push_sql(" AS "); + out.push_identifier(dsl::time_modified::NAME)?; + out.push_sql(", "); + + out.push_bind_param::, Option>>(&None)?; + out.push_sql(" AS "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(", "); + + out.push_bind_param::(&interface.instance_id)?; + out.push_sql(" AS "); + out.push_identifier(dsl::instance_id::NAME)?; + out.push_sql(", "); + + // Tiny subquery to select the `vpc_id` from the preceding CTE. + out.push_sql("(SELECT vpc_id FROM vpc) AS "); + out.push_identifier(dsl::vpc_id::NAME)?; + out.push_sql(", "); + + out.push_bind_param::(&interface.subnet.id())?; + out.push_sql(" AS "); + out.push_identifier(dsl::subnet_id::NAME)?; + out.push_sql(", "); + + out.push_bind_param::(&interface.mac.to_string())?; + out.push_sql(" AS "); + out.push_identifier(dsl::mac::NAME)?; + out.push_sql(", "); + + // If the user specified an IP address, then insert it by value. If they + // did not, meaning we're allocating the next available one on their + // behalf, then insert that subquery here. + if let Some(ip) = interface.ip { + out.push_bind_param::(&ip.into())?; + } else { + let subnet = + ipnetwork::IpNetwork::from(interface.subnet.ipv4_block.0 .0); + out.push_sql("("); + push_select_next_available_ip_subquery( + out.reborrow(), + &subnet, + &interface.subnet.id(), + )?; + out.push_sql(")"); + } + out.push_sql(" AS "); + out.push_identifier(dsl::ip::NAME)?; + Ok(()) +} + +/// Type used to insert conditionally insert a network interface. +/// +/// This type implements a query that does one of two things +/// +/// - Insert a new network interface, performing validation and possibly IP +/// allocation +/// - Return an existing interface record, if it has the same primary key. +/// +/// The first case is implemented in the [`push_interface_allocation_subquery`] +/// function. See that function's documentation for the details. +/// +/// The second case is implemented in this type's `walk_ast` method. +/// +/// Details +/// ------- +/// +/// The `push_interface_allocation_subquery` performs a number of validations on +/// the data provided, such as verifying that a requested IP address isn't +/// already assigned, or ensuring that the instance that will receive this +/// interface isn't already associated with another VPC. +/// +/// However, the query is also meant to run during an instance creation saga. In +/// that case, the guardrails and unique indexes on this table make it +/// impossible for the query to both be idempotent and also catch these +/// constraints. +/// +/// For example, imaging the instance creation saga crashes partway through +/// allocation a list of NICs. That node of the saga will be replayed during +/// saga recovery. This will create a record with exactly the same UUID for each +/// interface, which will ulimately result in a conflicting primary key error +/// from the database. This is both intentional and integral to the sagas +/// correct functioning. We catch this error deliberately, assuming that the +/// uniqueness of 128-bit UUIDs guarantees that the only practical situation +/// under which this can occur is a saga node replay after a crash. +/// +/// Query structure +/// --------------- +/// +/// This query looks like the following: +/// +/// ```text +/// SELECT (candidate).* FROM (SELECT COALESCE( +/// , +/// +/// ) +/// ``` +/// +/// That is, we return the exact record that's already in the database, if there +/// is one, or run the entire validating query otherwise. In the context of +/// sagas, this is helpful because we generate the UUIDs for the interfaces in a +/// separate saga node, prior to inserting any interfaces. So if we have a +/// record with that exact UUID, we assert that it must be the record from the +/// saga itself. Note that, at this point, we return only the primary key, since +/// it's sufficiently unlikely that there's an existing key whose other data +/// does _not_ match the data we wanted to insert in a saga. +/// +/// The odd syntax in the initial section, `SELECT (candidate).*` is because the +/// result of the `COALESCE` expression is a tuple. That is CockroachDB's syntax +/// for expanding a tuple into its constituent columns. +/// +/// Note that the result of this expression is ultimately inserted into the +/// `network_interface` table. The way that fails (VPC-validation, IP +/// exhaustion, primary key violation), is used for either forwarding an error +/// on to the client (in the case of IP exhaustion, for example), or continuing +/// with a saga (for PK uniqueness violations). See [`NetworkInterfaceError`] +/// for a summary of the error conditions and their meaning, and the functions +/// constructing the subqueries in this type for more details. +#[derive(Debug, Clone)] +pub struct InsertNetworkInterfaceQuery { + pub interface: IncompleteNetworkInterface, + pub now: DateTime, +} + +impl QueryId for InsertNetworkInterfaceQuery { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl Insertable + for InsertNetworkInterfaceQuery +{ + type Values = InsertNetworkInterfaceQueryValues; + + fn values(self) -> Self::Values { + InsertNetworkInterfaceQueryValues(self) + } +} + +impl QueryFragment for InsertNetworkInterfaceQuery { + fn walk_ast(&self, mut out: AstPass) -> diesel::QueryResult<()> { + use db::schema::network_interface::dsl; + let push_columns = |mut out: AstPass| -> diesel::QueryResult<()> { + 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::instance_id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::vpc_id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::subnet_id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::mac::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::ip::NAME)?; + Ok(()) + }; + + out.push_sql("SELECT (candidate).* FROM (SELECT COALESCE(("); + + // Add subquery to find exactly the record we might have already + // inserted during a saga. + out.push_sql("SELECT "); + push_columns(out.reborrow())?; + out.push_sql(" FROM "); + dsl::network_interface.from_clause().walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::( + &self.interface.identity.id, + )?; + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL)"); + + // Push the main, data-validating subquery. + out.push_sql(", ("); + push_interface_allocation_subquery( + out.reborrow(), + &self.interface, + &self.now, + )?; + out.push_sql(")) AS candidate)"); + Ok(()) + } +} + +/// Type used to add the results of the `InsertNetworkInterfaceQuery` as values +/// in a Diesel statement, e.g., `insert_into(network_interface).values(query).` +/// Not for direct use. +pub struct InsertNetworkInterfaceQueryValues(InsertNetworkInterfaceQuery); + +impl QueryId for InsertNetworkInterfaceQueryValues { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl diesel::insertable::CanInsertInSingleQuery + for InsertNetworkInterfaceQueryValues +{ + fn rows_to_insert(&self) -> Option { + Some(1) + } +} + +impl QueryFragment for InsertNetworkInterfaceQueryValues { + fn walk_ast(&self, mut out: AstPass) -> diesel::QueryResult<()> { + use db::schema::network_interface::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::instance_id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::vpc_id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::subnet_id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::mac::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::ip::NAME)?; + out.push_sql(") "); + self.0.walk_ast(out) + } +} + #[cfg(test)] mod test { - use super::AllocateIpQuery; use super::FilterConflictingVpcSubnetRangesQuery; + use super::NetworkInterfaceError; use super::SubnetError; use crate::db::model::{ - IncompleteNetworkInterface, NetworkInterface, VpcSubnet, + self, IncompleteNetworkInterface, NetworkInterface, VpcSubnet, }; - use crate::db::schema::network_interface; - use crate::external_api::params; - 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, Ipv6Net, MacAddr, Name, + Error, IdentityMetadataCreateParams, Ipv4Net, Ipv6Net, Name, }; use omicron_test_utils::dev; use std::convert::TryInto; use std::sync::Arc; use uuid::Uuid; - #[test] - fn test_verify_query() { - let interface_id = - uuid::Uuid::parse_str("223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d0") - .unwrap(); - let instance_id = - uuid::Uuid::parse_str("223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d1") - .unwrap(); - let vpc_id = - uuid::Uuid::parse_str("223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d2") - .unwrap(); - let subnet_id = - uuid::Uuid::parse_str("223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d3") - .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, - 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])) - .into(); - let interface = IncompleteNetworkInterface::new( - interface_id, - instance_id, - vpc_id, - subnet, - mac, - None, - params::NetworkInterfaceCreate { - identity: IdentityMetadataCreateParams { - name: "test-iface".to_string().try_into().unwrap(), - description: "interface description".to_string(), - }, - }, - ); - let select = AllocateIpQuery { - interface, - block: ipv4_block.into(), - now: DateTime::::from_utc( - NaiveDateTime::from_timestamp(0, 0), - Utc, - ), - }; - let query = diesel::debug_query::(&select).to_string(); - - let expected_query = "SELECT \ - $1 AS \"id\", $2 AS \"name\", $3 AS \"description\", \ - $4 AS \"time_created\", $5 AS \"time_modified\", \ - $6 AS \"instance_id\", $7 AS \"vpc_id\", $8 AS \"subnet_id\", \ - $9 AS \"mac\", $10 + \"off\" AS \"ip\" \ - FROM generate_series(5, $11) AS \"off\" LEFT OUTER JOIN \ - \"network_interface\" ON \ - (\"subnet_id\", \"ip\", \"time_deleted\" IS NULL) = \ - ($12, $13 + \"off\", TRUE) \ - WHERE \"ip\" IS NULL LIMIT 1 -- \ - binds: [223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d0, \"test-iface\", \ - \"interface description\", 1970-01-01T00:00:00Z, \ - 1970-01-01T00:00:00Z, \ - 223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d1, \ - 223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d2, \ - 223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d3, \"A8:40:25:00:00:01\", \ - V4(Ipv4Network { addr: 192.168.1.0, prefix: 32 }), 254, \ - 223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d3, \ - V4(Ipv4Network { addr: 192.168.1.0, prefix: 32 })]"; - assert_eq!(query, expected_query); - - let insert = diesel::insert_into(network_interface::table) - .values(select) - .returning(NetworkInterface::as_returning()); - let query = diesel::debug_query::(&insert).to_string(); - let expected_query = "INSERT INTO \"network_interface\" \ - (\"id\", \"name\", \"description\", \"time_created\", \ - \"time_modified\", \"instance_id\", \"vpc_id\", \"subnet_id\", \ - \"mac\", \"ip\") \ - SELECT $1 AS \"id\", $2 AS \"name\", $3 AS \"description\", \ - $4 AS \"time_created\", $5 AS \"time_modified\", \ - $6 AS \"instance_id\", $7 AS \"vpc_id\", $8 AS \"subnet_id\", \ - $9 AS \"mac\", $10 + \"off\" AS \"ip\" \ - FROM generate_series(5, $11) AS \"off\" LEFT OUTER JOIN \ - \"network_interface\" ON \ - (\"subnet_id\", \"ip\", \"time_deleted\" IS NULL) = \ - ($12, $13 + \"off\", TRUE) \ - WHERE \"ip\" IS NULL LIMIT 1 \ - RETURNING \"network_interface\".\"id\", \ - \"network_interface\".\"name\", \ - \"network_interface\".\"description\", \ - \"network_interface\".\"time_created\", \ - \"network_interface\".\"time_modified\", \ - \"network_interface\".\"time_deleted\", \ - \"network_interface\".\"instance_id\", \ - \"network_interface\".\"vpc_id\", \ - \"network_interface\".\"subnet_id\", \ - \"network_interface\".\"mac\", \"network_interface\".\"ip\" -- \ - binds: [223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d0, \"test-iface\", \ - \"interface description\", 1970-01-01T00:00:00Z, \ - 1970-01-01T00:00:00Z, \ - 223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d1, \ - 223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d2, \ - 223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d3, \"A8:40:25:00:00:01\", \ - V4(Ipv4Network { addr: 192.168.1.0, prefix: 32 }), 254, \ - 223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d3, \ - 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; @@ -802,4 +1246,347 @@ mod test { db.cleanup().await.unwrap(); logctx.cleanup_successful(); } + + #[tokio::test] + async fn test_insert_network_interface_query() { + // Setup the test database + let logctx = dev::test_setup_log("test_insert_network_interface_query"); + 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))); + + // Two test VpcSubnets, in different VPCs. The IPv4 range has space for + // 16 addresses, less the 6 that are reserved. + let ipv4_block = Ipv4Net("172.30.0.0/28".parse().unwrap()); + let ipv6_block = Ipv6Net("fd12:3456:7890::/64".parse().unwrap()); + let subnet_name = "subnet-a".to_string().try_into().unwrap(); + let other_subnet_name = "subnet-b".to_string().try_into().unwrap(); + let description = "some description".to_string(); + 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 subnet = VpcSubnet::new( + subnet_id, + vpc_id, + IdentityMetadataCreateParams { + name: subnet_name, + description: description.to_string(), + }, + ipv4_block, + ipv6_block, + ); + let other_subnet = VpcSubnet::new( + other_subnet_id, + other_vpc_id, + IdentityMetadataCreateParams { + name: other_subnet_name, + description: description.to_string(), + }, + ipv4_block, + ipv6_block, + ); + + // Insert a network interface with a known valid IP address, attached to + // a specific instance. + let instance_id = + "90d8542f-52dc-cacb-fa2b-ea0940d6bcb7".parse().unwrap(); + let requested_ip = "172.30.0.5".parse().unwrap(); + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + instance_id, + vpc_id, + subnet.clone(), + model::MacAddr::new().unwrap(), + IdentityMetadataCreateParams { + name: "interface-a".parse().unwrap(), + description: String::from("description"), + }, + Some(requested_ip), + ) + .unwrap(); + let inserted_interface = db_datastore + .instance_create_network_interface(interface.clone()) + .await + .expect("Failed to insert interface with known-good IP address"); + assert_interfaces_eq(&interface, &inserted_interface); + assert_eq!( + inserted_interface.ip.ip(), + requested_ip, + "The requested IP address should be available when no interfaces exist in the table" + ); + + // Insert an interface on the same instance, but with an + // automatically-assigned IP address. It should have the next address. + let expected_address = + "172.30.0.6".parse::().unwrap(); + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + instance_id, + vpc_id, + subnet.clone(), + model::MacAddr::new().unwrap(), + IdentityMetadataCreateParams { + name: "interface-b".parse().unwrap(), + description: String::from("description"), + }, + None, + ) + .unwrap(); + let inserted_interface = db_datastore + .instance_create_network_interface(interface.clone()) + .await + .expect("Failed to insert interface with known-good IP address"); + assert_interfaces_eq(&interface, &inserted_interface); + assert_eq!( + inserted_interface.ip.ip(), + expected_address, + "Failed to automatically assign the next available IP address" + ); + + // Inserting an interface with the same IP should fail. + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + instance_id, + vpc_id, + subnet.clone(), + model::MacAddr::new().unwrap(), + IdentityMetadataCreateParams { + name: "interface-c".parse().unwrap(), + description: String::from("description"), + }, + Some(requested_ip), + ) + .unwrap(); + let result = + db_datastore.instance_create_network_interface(interface).await; + assert!( + matches!( + result, + Err(NetworkInterfaceError::IpAddressNotAvailable(_)) + ), + "Requesting an interface with an existing IP should fail" + ); + + // Inserting an interface with a new IP but the same name should + // generate an invalid request error. + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + instance_id, + vpc_id, + subnet.clone(), + model::MacAddr::new().unwrap(), + IdentityMetadataCreateParams { + name: "interface-b".parse().unwrap(), + description: String::from("description"), + }, + None, + ) + .unwrap(); + let result = + db_datastore.instance_create_network_interface(interface).await; + assert!( + matches!( + result, + Err(NetworkInterfaceError::External(Error::ObjectAlreadyExists { .. })), + ), + "Requesting an interface with the same name on the same instance should fail" + ); + + // Inserting an interface that is attached to the same instance, but in a different VPC, + // should fail regardless of whether the IP is explicitly requested or allocated. + for addr in [Some(expected_address), None] { + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + instance_id, + other_vpc_id, + other_subnet.clone(), + model::MacAddr::new().unwrap(), + IdentityMetadataCreateParams { + name: "interface-a".parse().unwrap(), + description: String::from("description"), + }, + addr, + ) + .unwrap(); + let result = + db_datastore.instance_create_network_interface(interface).await; + assert!( + matches!(result, Err(NetworkInterfaceError::InstanceSpansMultipleVpcs(_))), + "Attaching an interface to an instance which already has one in a different VPC should fail" + ); + } + + // At this point, we should have allocated 2 addresses in this VPC + // Subnet. That has a subnet of 172.30.0.0/28, so 16 total addresses are + // available, but there are 6 reserved. Assert that we fail after + // allocating 16 - 6 - 2 = 8 more interfaces, with an address exhaustion error. + for i in 0..8 { + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + instance_id, + vpc_id, + subnet.clone(), + model::MacAddr::new().unwrap(), + IdentityMetadataCreateParams { + name: format!("interface-{}", i).try_into().unwrap(), + description: String::from("description"), + }, + None, + ) + .unwrap(); + let result = + db_datastore.instance_create_network_interface(interface).await; + assert!( + result.is_ok(), + "We should be able to allocate 8 more interfaces successfully", + ); + } + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + instance_id, + vpc_id, + subnet.clone(), + model::MacAddr::new().unwrap(), + IdentityMetadataCreateParams { + name: "interface-d".parse().unwrap(), + description: String::from("description"), + }, + None, + ) + .unwrap(); + let result = + db_datastore.instance_create_network_interface(interface).await; + assert!( + matches!( + result, + Err(NetworkInterfaceError::NoAvailableIpAddresses) + ), + "Address exhaustion should be detected and handled" + ); + + // We should _not_ fail to allocate two interfaces with the same name if + // they're in the same VPC and VPC Subnet, but on different instances. + // another instance. + for _ in 0..2 { + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + 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"), + }, + None, + ) + .unwrap(); + let result = + db_datastore.instance_create_network_interface(interface).await; + assert!( + result.is_ok(), + concat!( + "Should be able to allocate multiple interfaces with the same name ", + "as long as they're attached to different instances", + ), + ); + } + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } + + // Test equality of a complete/inserted interface, for parts that are always known. + fn assert_interfaces_eq( + incomplete: &IncompleteNetworkInterface, + inserted: &NetworkInterface, + ) { + use crate::db::identity::Resource; + assert_eq!(inserted.id(), incomplete.identity.id); + assert_eq!(inserted.name(), &incomplete.identity.name); + assert_eq!(inserted.description(), incomplete.identity.description); + 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); + } + + // Test that inserting a record into the database with the same primary key + // returns the exact same record. + // + // This is an explicit test for the first expression within the `COALESCE` + // part of the query. That is specifically designed to be executed during + // sagas. In that case, we construct the UUIDs of each interface in one + // action, and then in the next, create and insert each interface. + #[tokio::test] + async fn test_insert_network_interface_with_identical_primary_key() { + // Setup the test database + let logctx = dev::test_setup_log( + "test_insert_network_interface_with_identical_primary_key", + ); + 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))); + let ipv4_block = Ipv4Net("172.30.0.0/28".parse().unwrap()); + let ipv6_block = Ipv6Net("fd12:3456:7890::/64".parse().unwrap()); + let subnet_name = "subnet-a".to_string().try_into().unwrap(); + let description = "some description".to_string(); + let vpc_id = "d402369d-c9ec-c5ad-9138-9fbee732d53e".parse().unwrap(); + let subnet_id = "093ad2db-769b-e3c2-bc1c-b46e84ce5532".parse().unwrap(); + let subnet = VpcSubnet::new( + subnet_id, + vpc_id, + IdentityMetadataCreateParams { + name: subnet_name, + description: description.to_string(), + }, + ipv4_block, + ipv6_block, + ); + let instance_id = + "90d8542f-52dc-cacb-fa2b-ea0940d6bcb7".parse().unwrap(); + let requested_ip = "172.30.0.5".parse().unwrap(); + let interface = IncompleteNetworkInterface::new( + Uuid::new_v4(), + instance_id, + vpc_id, + subnet, + model::MacAddr::new().unwrap(), + IdentityMetadataCreateParams { + name: "interface-a".parse().unwrap(), + description: String::from("description"), + }, + Some(requested_ip), + ) + .unwrap(); + let inserted_interface = db_datastore + .instance_create_network_interface(interface.clone()) + .await + .expect("Failed to insert interface with known-good IP address"); + + // Attempt to insert the exact same record again. + let result = db_datastore + .instance_create_network_interface(interface.clone()) + .await; + if let Err(NetworkInterfaceError::DuplicatePrimaryKey(key)) = result { + assert_eq!(key, inserted_interface.identity.id); + } else { + panic!( + "Expected a NetworkInterfaceError::DuplicatePrimaryKey \ + error when inserting the exact same interface" + ); + } + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } } diff --git a/nexus/src/defaults.rs b/nexus/src/defaults.rs index 88a8a3166d6..d7c22abfc04 100644 --- a/nexus/src/defaults.rs +++ b/nexus/src/defaults.rs @@ -23,6 +23,9 @@ pub const MIN_VPC_IPV4_SUBNET_PREFIX: u8 = 8; /// NOTE: This is the maximum _prefix_, which sets the minimum subnet size. pub const MAX_VPC_IPV4_SUBNET_PREFIX: u8 = 26; +/// The number of reserved addresses at the beginning of a subnet range. +pub const NUM_INITIAL_RESERVED_IP_ADDRESSES: usize = 5; + 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 diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 369ace2a406..3a9ca9fed18 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -115,7 +115,12 @@ pub fn external_api() -> NexusApiDescription { api.register(vpc_subnets_delete_subnet)?; api.register(vpc_subnets_put_subnet)?; - api.register(subnets_ips_get)?; + api.register(subnet_network_interfaces_get)?; + + api.register(instance_network_interfaces_post)?; + api.register(instance_network_interfaces_get)?; + api.register(instance_network_interfaces_get_interface)?; + api.register(instance_network_interfaces_delete_interface)?; api.register(vpc_routers_get)?; api.register(vpc_routers_get_router)?; @@ -1111,6 +1116,151 @@ async fn instance_disks_detach( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// List network interfaces attached to this instance. +#[endpoint { + method = GET, + path = "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces", + tags = ["instances"], +}] +async fn instance_network_interfaces_get( + rqctx: Arc>>, + query_params: Query, + path_params: Path, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let query = query_params.into_inner(); + let path = path_params.into_inner(); + let organization_name = &path.organization_name; + let project_name = &path.project_name; + let instance_name = &path.instance_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let interfaces = nexus + .instance_list_network_interfaces( + &opctx, + &organization_name, + &project_name, + &instance_name, + &data_page_params_for(&rqctx, &query)? + .map_name(|n| Name::ref_cast(n)), + ) + .await? + .into_iter() + .map(|d| d.into()) + .collect(); + Ok(HttpResponseOk(ScanByName::results_page(&query, interfaces)?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Create a network interface for an instance. +#[endpoint { + method = POST, + path = "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces", + tags = ["instances"], +}] +async fn instance_network_interfaces_post( + rqctx: Arc>>, + path_params: Path, + interface_params: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let organization_name = &path.organization_name; + let project_name = &path.project_name; + let instance_name = &path.instance_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let iface = nexus + .instance_create_network_interface( + &opctx, + &organization_name, + &project_name, + &instance_name, + &interface_params.into_inner(), + ) + .await?; + Ok(HttpResponseCreated(iface.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)] +pub struct NetworkInterfacePathParam { + pub organization_name: Name, + pub project_name: Name, + pub instance_name: Name, + pub interface_name: Name, +} + +/// Detach a network interface from an instance. +#[endpoint { + method = DELETE, + path = "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces/{interface_name}", + tags = ["instances"], +}] +async fn instance_network_interfaces_delete_interface( + rqctx: Arc>>, + path_params: Path, +) -> Result { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let organization_name = &path.organization_name; + let project_name = &path.project_name; + let instance_name = &path.instance_name; + let interface_name = &path.interface_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + nexus + .instance_delete_network_interface( + &opctx, + organization_name, + project_name, + instance_name, + interface_name, + ) + .await?; + Ok(HttpResponseDeleted()) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Get an interface attached to an instance. +#[endpoint { + method = GET, + path = "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces/{interface_name}", + tags = ["instances"], +}] +async fn instance_network_interfaces_get_interface( + rqctx: Arc>>, + path_params: Path, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let organization_name = &path.organization_name; + let project_name = &path.project_name; + let instance_name = &path.instance_name; + let interface_name = &path.interface_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let interface = nexus + .instance_lookup_network_interface( + &opctx, + organization_name, + project_name, + instance_name, + interface_name, + ) + .await?; + Ok(HttpResponseOk(NetworkInterface::from(interface))) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + /* * VPCs */ @@ -1456,18 +1606,13 @@ async fn vpc_subnets_put_subnet( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/** - * List IP addresses on a VPC subnet. - */ -// TODO-correctness: This API has not actually been specified in an RFD yet, and -// may not actually be what we want. It is being implemented here to give our -// testing introspection into network interfaces. +/// List network interfaces in a VPC subnet. #[endpoint { method = GET, - path = "/organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name}/ips", + path = "/organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name}/network-interfaces", tags = ["subnets"], }] -async fn subnets_ips_get( +async fn subnet_network_interfaces_get( rqctx: Arc>>, query_params: Query, path_params: Path, diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 40f37ec63e4..6e9b48a7272 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -12,6 +12,7 @@ use omicron_common::api::external::{ }; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use std::net::IpAddr; use uuid::Uuid; /* @@ -70,12 +71,60 @@ pub struct ProjectUpdate { pub struct NetworkInterfaceCreate { #[serde(flatten)] pub identity: IdentityMetadataCreateParams, + /// The VPC in which to create the interface. + pub vpc_name: Name, + /// The VPC Subnet in which to create the interface. + pub subnet_name: Name, + /// The IP address for the interface. One will be auto-assigned if not provided. + pub ip: Option, } /* * INSTANCES */ +/// Describes an attachment of a `NetworkInterface` to an `Instance`, at the +/// time the instance is created. +// NOTE: VPC's are an organizing concept for networking resources, not for +// instances. It's true that all networking resources for an instance must +// belong to a single VPC, but we don't consider instances to be "scoped" to a +// VPC in the same way that they are scoped to projects, for example. +// +// This is slightly different than some other cloud providers, such as AWS, +// which use VPCs as both a networking concept, and a container more similar to +// our concept of a project. One example for why this is useful is that "moving" +// an instance to a new VPC can be done by detaching any interfaces in the +// original VPC and attaching interfaces in the new VPC. +// +// This type then requires the VPC identifiers, exactly because instances are +// _not_ scoped to a VPC, and so the VPC and/or VPC Subnet names are not present +// in the path of endpoints handling instance operations. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +#[serde(tag = "type", content = "params")] +pub enum InstanceNetworkInterfaceAttachment { + /// Create one or more `NetworkInterface`s for the `Instance` + Create(InstanceNetworkInterfaceCreate), + + /// Default networking setup, which creates a single interface with an + /// auto-assigned IP address from project's "default" VPC and "default" VPC + /// Subnet. + Default, + + /// No network interfaces at all will be created for the instance. + None, +} + +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct InstanceNetworkInterfaceCreate { + pub params: Vec, +} + +impl Default for InstanceNetworkInterfaceAttachment { + fn default() -> Self { + Self::Default + } +} + /** * Create-time parameters for an [`Instance`](omicron_common::api::external::Instance) */ @@ -86,6 +135,10 @@ pub struct InstanceCreate { pub ncpus: InstanceCpuCount, pub memory: ByteCount, pub hostname: String, /* TODO-cleanup different type? */ + + /// The network interfaces to be created for this instance. + #[serde(default)] + pub network_interfaces: InstanceNetworkInterfaceAttachment, } /** @@ -158,8 +211,9 @@ 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). + // TODO-correctness: These need to be removed. Changing these is effectively + // creating a new resource, so we should require explicit + // deletion/recreation by the client. pub ipv4_block: Option, pub ipv6_block: Option, } @@ -229,6 +283,14 @@ pub struct DiskIdentifier { pub disk: Name, } +/// Parameters for the +/// [`NetworkInterface`](omicron_common::api::external::NetworkInterface) to be +/// attached or detached to an instance. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct NetworkInterfaceIdentifier { + pub interface_name: Name, +} + /* * BUILT-IN USERS * diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 54bbb302e26..c365739e5c8 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::NetworkInterfaceError; use crate::db::subnet_allocation::SubnetError; use crate::defaults; use crate::external_api::params; @@ -1541,6 +1542,160 @@ impl Nexus { .map(|_| ()) } + /// Lists network interfaces attached to the instance. + pub async fn instance_list_network_interfaces( + &self, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, + instance_name: &Name, + pagparams: &DataPageParams<'_, Name>, + ) -> ListResultVec { + let authz_instance = self + .db_datastore + .instance_lookup_by_path( + organization_name, + project_name, + instance_name, + ) + .await?; + opctx.authorize(authz::Action::ListChildren, &authz_instance).await?; + self.db_datastore + .instance_list_network_interfaces(&authz_instance.id(), pagparams) + .await + } + + /// Create a network interface attached to the provided instance. + // TODO-performance: Add a version of this that accepts the instance ID + // directly. This will avoid all the internal database lookups in the event + // that we create many NICs for the same instance, such as in a saga. + pub async fn instance_create_network_interface( + &self, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, + instance_name: &Name, + params: ¶ms::NetworkInterfaceCreate, + ) -> CreateResult { + let authz_project = self + .db_datastore + .project_lookup_by_path(organization_name, project_name) + .await?; + let (authz_instance, db_instance) = self + .db_datastore + .instance_fetch(opctx, &authz_project, instance_name) + .await?; + opctx.authorize(authz::Action::Modify, &authz_instance).await?; + + // TODO-completeness: We'd like to relax this once hot-plug is supported + let stopped = + db::model::InstanceState::new(external::InstanceState::Stopped); + if db_instance.runtime_state.state != stopped { + return Err(external::Error::invalid_request( + "Instance must be stopped to attach a new network interface", + )); + } + + // Lookup both the VPC and VPC Subnet, since we need both IDs to create + // an interface. + let vpc_name = db::model::Name(params.vpc_name.clone()); + let subnet_name = db::model::Name(params.subnet_name.clone()); + let vpc = self + .project_lookup_vpc(organization_name, project_name, &vpc_name) + .await?; + let subnet = self + .db_datastore + .vpc_subnet_fetch_by_name(&vpc.id(), &subnet_name) + .await?; + let mac = db::model::MacAddr::new()?; + let interface_id = Uuid::new_v4(); + let interface = db::model::IncompleteNetworkInterface::new( + interface_id, + authz_instance.id(), + vpc.id(), + subnet, + mac, + params.identity.clone(), + params.ip, + )?; + let interface = self + .db_datastore + .instance_create_network_interface(interface) + .await + .map_err(NetworkInterfaceError::into_external)?; + Ok(interface) + } + + /// Delete a network interface from the provided instance. + pub async fn instance_delete_network_interface( + &self, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, + instance_name: &Name, + interface_name: &Name, + ) -> DeleteResult { + let authz_project = self + .db_datastore + .project_lookup_by_path(organization_name, project_name) + .await?; + let (authz_instance, db_instance) = self + .db_datastore + .instance_fetch(opctx, &authz_project, instance_name) + .await?; + opctx.authorize(authz::Action::Modify, &authz_instance).await?; + + // TODO-completeness: We'd like to relax this once hot-plug is supported + let stopped = + db::model::InstanceState::new(external::InstanceState::Stopped); + if db_instance.runtime_state.state != stopped { + return Err(external::Error::invalid_request( + "Instance must be stopped to detach a network interface", + )); + } + // TODO-cleanup: It's annoying that we need to look up the interface by + // name, which gets a single record, and then soft-delete that one + // record. We should be able to do both at once, but the + // `update_and_check` tools only operate on the primary key. That means + // we need to fetch the whole record first. + let interface = self + .db_datastore + .instance_lookup_network_interface( + &authz_instance.id(), + interface_name, + ) + .await?; + self.db_datastore + .instance_delete_network_interface(&interface.id()) + .await + } + + /// Fetch a network interface attached to the given instance. + pub async fn instance_lookup_network_interface( + &self, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, + instance_name: &Name, + interface_name: &Name, + ) -> LookupResult { + let authz_instance = self + .db_datastore + .instance_lookup_by_path( + organization_name, + project_name, + instance_name, + ) + .await?; + opctx.authorize(authz::Action::ListChildren, &authz_instance).await?; + self.db_datastore + .instance_lookup_network_interface( + &authz_instance.id(), + interface_name, + ) + .await + } + pub async fn project_list_vpcs( &self, opctx: &OpContext, @@ -1826,7 +1981,6 @@ impl Nexus { vpc_name: &Name, subnet_name: &Name, ) -> LookupResult { - // TODO: join projects, vpcs, and subnets and do this in one query let vpc = self .project_lookup_vpc(organization_name, project_name, vpc_name) .await?; diff --git a/nexus/src/sagas.rs b/nexus/src/sagas.rs index 4302857cbd4..ba97a8cc07b 100644 --- a/nexus/src/sagas.rs +++ b/nexus/src/sagas.rs @@ -37,6 +37,7 @@ use omicron_common::api::internal::sled_agent::InstanceHardware; use omicron_common::backoff::{self, BackoffError}; use serde::Deserialize; use serde::Serialize; +use slog::warn; use slog::Logger; use std::collections::BTreeMap; use std::convert::{TryFrom, TryInto}; @@ -149,14 +150,41 @@ pub fn saga_instance_create() -> SagaTemplate { new_action_noop_undo(sic_alloc_server), ); + // NOTE: The separation of the ID-allocation and NIC creation nodes is + // intentional. + // + // The Nexus API supports creating multiple network interfaces at the time + // an instance is provisioned. However, each NIC creation is independent, + // and each can fail. For example, someone might specify multiple NICs with + // the same IP address. The first will be created successfully, but the + // later ones will fail. We need to handle this case gracefully, and always + // delete any NICs we create, even if the NIC creation node itself fails. + // + // To do that, we create an action that only allocates the UUIDs for each + // interface. This has an undo action that actually deletes any NICs for the + // instance to be provisioned. The forward action is infallible, so this + // undo action will always run, even (and especially) if the NIC creation + // action fails. + // + // It's also important that we allocate the UUIDs first. It's possible that + // we crash partway through the NIC creation action. In this case, the saga + // recovery machinery will pick it up where it left off, without first + // destroying the NICs we created before crashing. By allocating the UUIDs + // first, we can make the insertion idempotent, by ignoring conflicts on the + // UUID. template_builder.append( - "network_interface", - "CreateNetworkInterface", + "network_interface_ids", + "NetworkInterfaceIds", ActionFunc::new_action( - sic_create_network_interface, - sic_create_network_interface_undo, + sic_allocate_network_interface_ids, + sic_create_network_interfaces_undo, ), ); + template_builder.append( + "network_interfaces", + "CreateNetworkInterfaces", + new_action_noop_undo(sic_create_network_interfaces), + ); template_builder.append( "initial_runtime", @@ -184,72 +212,235 @@ async fn sic_alloc_server( .map_err(ActionError::action_failed) } -async fn sic_create_network_interface( +async fn sic_allocate_network_interface_ids( + sagactx: ActionContext, +) -> Result, ActionError> { + match sagactx.saga_params().create_params.network_interfaces { + params::InstanceNetworkInterfaceAttachment::None => Ok(vec![]), + params::InstanceNetworkInterfaceAttachment::Default => { + Ok(vec![Uuid::new_v4()]) + } + params::InstanceNetworkInterfaceAttachment::Create( + ref create_params, + ) => { + let mut ids = Vec::with_capacity(create_params.params.len()); + for _ in 0..create_params.params.len() { + ids.push(Uuid::new_v4()); + } + Ok(ids) + } + } +} + +async fn sic_create_network_interfaces( sagactx: ActionContext, -) -> Result { +) -> Result>, ActionError> { + match sagactx.saga_params().create_params.network_interfaces { + params::InstanceNetworkInterfaceAttachment::None => Ok(None), + params::InstanceNetworkInterfaceAttachment::Default => { + sic_create_default_network_interface(&sagactx).await + } + params::InstanceNetworkInterfaceAttachment::Create( + ref create_params, + ) => { + sic_create_custom_network_interfaces( + &sagactx, + &create_params.params, + ) + .await + } + } +} + +/// Create one or more custom (non-default) network interfaces for the provided +/// instance. +async fn sic_create_custom_network_interfaces( + sagactx: &ActionContext, + interface_params: &[params::NetworkInterfaceCreate], +) -> Result>, ActionError> { + if interface_params.is_empty() { + return Ok(Some(vec![])); + } + let osagactx = sagactx.user_data(); - let params = sagactx.saga_params(); + let saga_params = sagactx.saga_params(); let instance_id = sagactx.lookup::("instance_id")?; + let ids = sagactx.lookup::>("network_interface_ids")?; + let vpc = osagactx + .datastore() + .vpc_fetch_by_name( + &saga_params.project_id, + &db::model::Name::from(interface_params[0].vpc_name.clone()), + ) + .await + .map_err(ActionError::action_failed)?; - let default_name = - db::model::Name(Name::try_from("default".to_string()).unwrap()); + // Check that all VPC names are the same. + // + // This isn't strictly necessary, as the queries would fail below, but it's + // easier to handle here. + if interface_params.iter().any(|p| p.vpc_name != vpc.name().0) { + return Err(ActionError::action_failed(Error::invalid_request( + "All interfaces must be in the same VPC", + ))); + } + + let mut interfaces = Vec::with_capacity(interface_params.len()); + if ids.len() != interface_params.len() { + return Err(ActionError::action_failed(Error::internal_error( + "found differing number of network interface IDs and interface parameters" + ))); + } + for (interface_id, params) in ids.into_iter().zip(interface_params.iter()) { + // TODO-correctness: It seems racy to fetch the subnet and create the + // interface in separate requests, but outside of a transaction. This + // should probably either be in a transaction, or the + // `subnet_create_network_interface` function/query needs some JOIN + // on the `vpc_subnet` table. + let subnet = osagactx + .datastore() + .vpc_subnet_fetch_by_name( + &vpc.id(), + &db::model::Name::from(params.subnet_name.clone()), + ) + .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, + vpc.id(), + subnet.clone(), + mac, + params.identity.clone(), + params.ip, + ) + .map_err(ActionError::action_failed)?; + let result = osagactx + .datastore() + .instance_create_network_interface(interface) + .await; + + use crate::db::subnet_allocation::NetworkInterfaceError; + let interface = match result { + Ok(interface) => Ok(interface), + + // Detect the specific error arising from this node being partially + // completed. + // + // The query used to insert network interfaces first checks for an + // existing record with the same primary key. It will attempt to + // insert that record if it exists, which obviously fails with a + // primary key violation. (If the record does _not_ exist, one will + // be inserted as usual, see + // `db::subnet_name::InsertNetworkInterfaceQuery` for details). + // + // In this one specific case, we're asserting that any primary key + // duplicate arises because this saga node ran partway and then + // crashed. The saga recovery machinery will replay just this node, + // without first unwinding it, so any previously-inserted interfaces + // will still exist. This is expected. + Err(NetworkInterfaceError::DuplicatePrimaryKey(_)) => { + // TODO-observability: We should bump a counter here. + let log = osagactx.log(); + warn!( + log, + "Detected duplicate primary key during saga to \ + create network interfaces for instance '{}'. \ + This likely occurred because \ + the saga action 'sic_create_custom_network_interfaces' crashed \ + and has been recovered.", + instance_id; + "primary_key" => interface_id.to_string(), + ); + + // Refetch the interface itself, to serialize it for the next + // saga node. + osagactx + .datastore() + .instance_lookup_network_interface(&instance_id, &db::model::Name(params.identity.name.clone())) + .await + } + Err(e) => Err(e.into_external()), + } + .map_err(ActionError::action_failed)?; + interfaces.push(NetworkInterface::from(interface)) + } + Ok(Some(interfaces)) +} + +/// Create the default network interface for an instance during the create saga +async fn sic_create_default_network_interface( + sagactx: &ActionContext, +) -> Result>, ActionError> { + let osagactx = sagactx.user_data(); + let saga_params = sagactx.saga_params(); + let instance_id = sagactx.lookup::("instance_id")?; + let default_name = Name::try_from("default".to_string()).unwrap(); + let internal_default_name = db::model::Name::from(default_name.clone()); + let interface_params = params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: default_name.clone(), + description: format!( + "default interface for {}", + saga_params.create_params.identity.name, + ), + }, + vpc_name: default_name.clone(), + subnet_name: default_name.clone(), + ip: None, // Request an IP address allocation + }; let vpc = osagactx .datastore() - .vpc_fetch_by_name(¶ms.project_id, &default_name) + .vpc_fetch_by_name( + &saga_params.project_id, + &internal_default_name.clone(), + ) .await .map_err(ActionError::action_failed)?; let subnet = osagactx .datastore() - .vpc_subnet_fetch_by_name(&vpc.id(), &default_name) + .vpc_subnet_fetch_by_name(&vpc.id(), &internal_default_name) .await .map_err(ActionError::action_failed)?; let mac = db::model::MacAddr::new().map_err(ActionError::action_failed)?; let interface_id = Uuid::new_v4(); - // Request an allocation - let ip = None; let interface = db::model::IncompleteNetworkInterface::new( interface_id, instance_id, - // TODO-correctness: vpc_id here is used for name uniqueness. Should - // interface names be unique to the subnet's VPC or to the - // VPC associated with the instance's default interface? vpc.id(), - subnet, + subnet.clone(), mac, - ip, - params::NetworkInterfaceCreate { - identity: IdentityMetadataCreateParams { - // By naming the interface after the instance id, we should - // avoid name conflicts on creation. - name: format!("default-{}", instance_id).parse().unwrap(), - description: format!( - "default interface for {}", - params.create_params.identity.name - ), - }, - }, - ); - + interface_params.identity.clone(), + interface_params.ip, + ) + .map_err(ActionError::action_failed)?; let interface = osagactx .datastore() .instance_create_network_interface(interface) .await + .map_err(db::subnet_allocation::NetworkInterfaceError::into_external) .map_err(ActionError::action_failed)?; - Ok(interface.into()) + Ok(Some(vec![interface.into()])) } -async fn sic_create_network_interface_undo( +async fn sic_create_network_interfaces_undo( sagactx: ActionContext, ) -> Result<(), anyhow::Error> { + // We issue a request to delete any interfaces associated with this instance. + // In the case we failed partway through allocating interfaces, we won't + // have cached the interface records in the saga log, but they're definitely + // still in the database. Just delete every interface that exists, even if + // there are zero such records. let osagactx = sagactx.user_data(); - let network_interface = - sagactx.lookup::("network_interface")?; - + let instance_id = sagactx.lookup::("instance_id")?; osagactx .datastore() - .instance_delete_network_interface(&network_interface.identity.id) - .await?; + .instance_delete_all_network_interfaces(&instance_id) + .await + .map_err(ActionError::action_failed)?; Ok(()) } @@ -261,8 +452,9 @@ async fn sic_create_instance_record( let sled_uuid = sagactx.lookup::("server_id"); let instance_id = sagactx.lookup::("instance_id"); let propolis_uuid = sagactx.lookup::("propolis_id"); - let network_interface = - sagactx.lookup::("network_interface")?; + let nics = sagactx + .lookup::>>("network_interfaces")? + .unwrap_or_default(); let runtime = InstanceRuntimeState { run_state: InstanceState::Creating, @@ -292,10 +484,7 @@ async fn sic_create_instance_record( .map_err(ActionError::action_failed)?; // See also: instance_set_runtime in nexus.rs for a similar construction. - Ok(InstanceHardware { - runtime: instance.runtime().clone().into(), - nics: vec![network_interface], - }) + Ok(InstanceHardware { runtime: instance.runtime().clone().into(), nics }) } async fn sic_instance_ensure( diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index 91bcb544556..210ea2db71e 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -18,7 +18,7 @@ use std::fmt::Debug; /// properties of the response for testing // // When testing an HTTP server, we make varying requests to the server and -// verify a bunch of properties about it's behavior. A lot of things can go +// verify a bunch of properties about its behavior. A lot of things can go // wrong along the way: // // - failed to serialize request body @@ -380,6 +380,7 @@ where } /// Represents a response from an HTTP server +#[derive(Debug)] pub struct TestResponse { pub status: http::StatusCode, pub headers: http::HeaderMap, diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs index 66edea81d78..60301bdd5db 100644 --- a/nexus/test-utils/src/lib.rs +++ b/nexus/test-utils/src/lib.rs @@ -179,6 +179,7 @@ pub async fn start_sled_agent( nexus_address, dropshot: ConfigDropshot { bind_address: SocketAddr::new("127.0.0.1".parse().unwrap(), 0), + request_body_max_bytes: 2048, ..Default::default() }, /* TODO-cleanup this is unused */ diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 637b49e809e..a7c6f7e3b26 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -141,6 +141,8 @@ pub async fn create_instance( ncpus: InstanceCpuCount(4), memory: ByteCount::from_mebibytes_u32(256), hostname: String::from("the_host"), + network_interfaces: + params::InstanceNetworkInterfaceAttachment::Default, }, ) .await diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index d6dd93e1a6f..4ded6ef38a4 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -31,10 +31,12 @@ url = "postgresql://root@127.0.0.1:0/omicron?sslmode=disable" # [dropshot_external] bind_address = "127.0.0.1:0" +request_body_max_bytes = 2048 # port must be 0. see above [dropshot_internal] bind_address = "127.0.0.1:0" +request_body_max_bytes = 2048 # # NOTE: for the test suite, if mode = "file", the file path MUST be the sentinel diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index e5baf5c81e6..e694abc63a8 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -11,15 +11,18 @@ use http::StatusCode; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; +use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Instance; use omicron_common::api::external::InstanceCpuCount; use omicron_common::api::external::InstanceState; +use omicron_common::api::external::Ipv4Net; use omicron_common::api::external::Name; use omicron_common::api::external::NetworkInterface; use omicron_nexus::TestInterfaces as _; use omicron_nexus::{external_api::params, Nexus}; use sled_agent_client::TestInterfaces as _; +use std::convert::TryFrom; use std::sync::Arc; use uuid::Uuid; @@ -141,6 +144,8 @@ async fn test_instances_create_reboot_halt( ncpus: instance.ncpus, memory: instance.memory, hostname: instance.hostname.clone(), + network_interfaces: + params::InstanceNetworkInterfaceAttachment::Default, })) .expect_status(Some(StatusCode::BAD_REQUEST)), ) @@ -164,17 +169,14 @@ async fn test_instances_create_reboot_halt( /* Check that the instance got a network interface */ let ips_url = format!( - "/organizations/{}/projects/{}/vpcs/default/subnets/default/ips", + "/organizations/{}/projects/{}/vpcs/default/subnets/default/network-interfaces", ORGANIZATION_NAME, PROJECT_NAME ); let network_interfaces = objects_list_page::(client, &ips_url).await.items; assert_eq!(network_interfaces.len(), 1); assert_eq!(network_interfaces[0].instance_id, instance.identity.id); - assert_eq!( - network_interfaces[0].identity.name, - format!("default-{}", instance.identity.id).parse::().unwrap() - ); + assert_eq!(network_interfaces[0].identity.name, "default"); /* * Now, simulate completion of instance boot and check the state reported. @@ -530,6 +532,467 @@ async fn test_instances_invalid_creation_returns_bad_request( .starts_with("unable to parse body: invalid value: integer `-3`")); } +// Basic test requesting an interface with a specific IP address. +#[nexus_test] +async fn test_instance_with_single_explicit_ip_address( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + // Create test organization and project + create_organization(&client, ORGANIZATION_NAME).await; + let url_instances = format!( + "/organizations/{}/projects/{}/instances", + ORGANIZATION_NAME, PROJECT_NAME + ); + let _ = create_project(&client, ORGANIZATION_NAME, PROJECT_NAME).await; + + // Create the parameters for the interface. + let default_name = "default".parse::().unwrap(); + let requested_address = "172.30.0.10".parse::().unwrap(); + let if0_params = params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("if0")).unwrap(), + description: String::from("first custom interface"), + }, + vpc_name: default_name.clone(), + subnet_name: default_name.clone(), + ip: Some(requested_address), + }; + let interface_params = params::InstanceNetworkInterfaceAttachment::Create( + params::InstanceNetworkInterfaceCreate { + params: vec![if0_params.clone()], + }, + ); + + // Create the parameters for the instance itself, and create it. + let instance_params = params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("nic-test-inst")).unwrap(), + description: String::from("instance to test multiple nics"), + }, + ncpus: InstanceCpuCount::try_from(2).unwrap(), + memory: ByteCount::from_mebibytes_u32(4), + hostname: String::from("nic-test"), + network_interfaces: interface_params, + }; + let response = + NexusRequest::objects_post(client, &url_instances, &instance_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create instance with two network interfaces"); + let instance = response.parsed_body::().unwrap(); + + // Get the interface, and verify it has the requested address + let url_interface = format!( + "{}/{}/network-interfaces/{}", + url_instances, instance_params.identity.name, if0_params.identity.name, + ); + let interface = NexusRequest::object_get(client, &url_interface) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to get network interface for new instance") + .parsed_body::() + .expect("Failed to parse a network interface"); + assert_eq!(interface.instance_id, instance.identity.id); + assert_eq!(interface.identity.name, if0_params.identity.name); + assert_eq!( + interface.ip, requested_address, + "Interface was not assigned the requested IP address" + ); +} + +// Test creating two new interfaces for an instance, at creation time. +#[nexus_test] +async fn test_instance_with_new_custom_network_interfaces( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + // Create test organization and project + create_organization(&client, ORGANIZATION_NAME).await; + let url_instances = format!( + "/organizations/{}/projects/{}/instances", + ORGANIZATION_NAME, PROJECT_NAME + ); + let _ = create_project(&client, ORGANIZATION_NAME, PROJECT_NAME).await; + + // Create a VPC Subnet other than the default. + // + // We'll create one interface in the default VPC Subnet and one in this new + // VPC Subnet. + let url_vpc_subnets = format!( + "/organizations/{}/projects/{}/vpcs/{}/subnets", + ORGANIZATION_NAME, PROJECT_NAME, "default", + ); + let default_name = Name::try_from(String::from("default")).unwrap(); + let non_default_subnet_name = + Name::try_from(String::from("non-default-subnet")).unwrap(); + let vpc_subnet_params = params::VpcSubnetCreate { + identity: IdentityMetadataCreateParams { + name: non_default_subnet_name.clone(), + description: String::from("A non-default subnet"), + }, + ipv4_block: Ipv4Net("172.31.0.0/24".parse().unwrap()), + ipv6_block: None, + }; + let _response = NexusRequest::objects_post( + client, + &url_vpc_subnets, + &vpc_subnet_params, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create custom VPC Subnet"); + + // TODO-testing: We'd like to assert things about this VPC Subnet we just + // created, but the `vpc_subnets_post` endpoint in Nexus currently returns + // the "private" `omicron_nexus::db::model::VpcSubnet` type. That should be + // converted to return the public `omicron_common::external` type, which is + // work tracked in https://github.com/oxidecomputer/omicron/issues/388. + + // Create the parameters for the interfaces. These will be created during + // the saga for instance creation. + let if0_params = params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("if0")).unwrap(), + description: String::from("first custom interface"), + }, + vpc_name: default_name.clone(), + subnet_name: default_name.clone(), + ip: None, + }; + let if1_params = params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("if1")).unwrap(), + description: String::from("second custom interface"), + }, + vpc_name: default_name.clone(), + subnet_name: non_default_subnet_name.clone(), + ip: None, + }; + let interface_params = params::InstanceNetworkInterfaceAttachment::Create( + params::InstanceNetworkInterfaceCreate { + params: vec![if0_params.clone(), if1_params.clone()], + }, + ); + + // Create the parameters for the instance itself, and create it. + let instance_params = params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("nic-test-inst")).unwrap(), + description: String::from("instance to test multiple nics"), + }, + ncpus: InstanceCpuCount::try_from(2).unwrap(), + memory: ByteCount::from_mebibytes_u32(4), + hostname: String::from("nic-test"), + network_interfaces: interface_params, + }; + let response = + NexusRequest::objects_post(client, &url_instances, &instance_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create instance with two network interfaces"); + let instance = response.parsed_body::().unwrap(); + + // Check that both interfaces actually appear correct. + let ip_url = |subnet_name: &Name| { + format!( + "/organizations/{}/projects/{}/vpcs/{}/subnets/{}/network-interfaces", + ORGANIZATION_NAME, PROJECT_NAME, "default", subnet_name + ) + }; + + // The first interface is in the default VPC Subnet + let interfaces = NexusRequest::iter_collection_authn::( + client, + ip_url(&default_name).as_str(), + "", + Some(100), + ) + .await + .expect("Failed to get interfaces in default VPC Subnet"); + assert_eq!( + interfaces.all_items.len(), + 1, + "Should be a single interface in the default subnet" + ); + let if0 = &interfaces.all_items[0]; + assert_eq!(if0.identity.name, if0_params.identity.name); + assert_eq!(if0.identity.description, if0_params.identity.description); + assert_eq!(if0.instance_id, instance.identity.id); + assert_eq!(if0.ip, std::net::IpAddr::V4("172.30.0.5".parse().unwrap())); + + let interfaces1 = NexusRequest::iter_collection_authn::( + client, + ip_url(&non_default_subnet_name).as_str(), + "", + Some(100), + ) + .await + .expect("Failed to get interfaces in non-default VPC Subnet"); + assert_eq!( + interfaces1.all_items.len(), + 1, + "Should be a single interface in the non-default subnet" + ); + let if1 = &interfaces1.all_items[0]; + + // TODO-testing: Add this test once the `VpcSubnet` type can be + // deserialized. + // assert_eq!(if1.subnet_id, non_default_vpc_subnet.id); + + assert_eq!(if1.identity.name, if1_params.identity.name); + assert_eq!(if1.identity.description, if1_params.identity.description); + assert_eq!(if1.ip, std::net::IpAddr::V4("172.31.0.5".parse().unwrap())); + assert_eq!(if1.instance_id, instance.identity.id); + assert_eq!(if0.vpc_id, if1.vpc_id); + assert_ne!( + if0.subnet_id, if1.subnet_id, + "Two interfaces should be created in different subnets" + ); +} + +#[nexus_test] +async fn test_instance_create_delete_network_interface( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; + + // Create test organization and project + create_organization(&client, ORGANIZATION_NAME).await; + let url_instances = format!( + "/organizations/{}/projects/{}/instances", + ORGANIZATION_NAME, PROJECT_NAME + ); + let _ = create_project(&client, ORGANIZATION_NAME, PROJECT_NAME).await; + + // Create an instance with no network interfaces + let instance_params = params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("nic-attach-test-inst")).unwrap(), + description: String::from("instance to test attaching new nic"), + }, + ncpus: InstanceCpuCount::try_from(2).unwrap(), + memory: ByteCount::from_mebibytes_u32(4), + hostname: String::from("nic-test"), + network_interfaces: params::InstanceNetworkInterfaceAttachment::None, + }; + let response = + NexusRequest::objects_post(client, &url_instances, &instance_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create instance with two network interfaces"); + let instance = response.parsed_body::().unwrap(); + let url_instance = + format!("{}/{}", url_instances, instance.identity.name.as_str()); + + // Verify there are no interfaces + let url_interfaces = format!( + "/organizations/{}/projects/{}/instances/{}/network-interfaces", + ORGANIZATION_NAME, PROJECT_NAME, instance.identity.name, + ); + let interfaces = NexusRequest::iter_collection_authn::( + client, + url_interfaces.as_str(), + "", + Some(100), + ) + .await + .expect("Failed to get interfaces for instance"); + assert!( + interfaces.all_items.is_empty(), + "Expected no network interfaces for instance" + ); + + // Parameters for the interface to create/attach + let if_params = params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: "if0".parse().unwrap(), + description: String::from("a new nic"), + }, + vpc_name: "default".parse().unwrap(), + subnet_name: "default".parse().unwrap(), + ip: Some("172.30.0.10".parse().unwrap()), + }; + let url_interface = + format!("{}/{}", url_interfaces, if_params.identity.name.as_str()); + + // We should not be able to create an interface while the instance is running. + // + // NOTE: Need to use RequestBuilder manually because `expect_failure` does not allow setting + // the body. + let builder = RequestBuilder::new( + client, + http::Method::POST, + url_interfaces.as_str(), + ) + .body(Some(&if_params)) + .expect_status(Some(http::StatusCode::BAD_REQUEST)); + let err = NexusRequest::new(builder) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Should not be able to create network interface on running instance") + .parsed_body::() + .expect("Failed to parse error response body"); + assert_eq!( + err.message, + "Instance must be stopped to attach a new network interface", + "Expected an InvalidRequest response when creating an interface on a running instance" + ); + + // Stop the instance + let instance = + instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + instance_simulate(nexus, &instance.identity.id).await; + + // Verify we can now make the request again + let response = + NexusRequest::objects_post(client, url_interfaces.as_str(), &if_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create network interface on stopped instance"); + let iface = response.parsed_body::().unwrap(); + assert_eq!(iface.identity.name, if_params.identity.name); + assert_eq!(iface.ip, if_params.ip.unwrap()); + + // Restart the instance, verify the interface is still correct. + let instance = + instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_simulate(nexus, &instance.identity.id).await; + + let iface0 = NexusRequest::object_get(client, url_interface.as_str()) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to get interface") + .parsed_body::() + .expect("Failed to parse network interface from body"); + assert_eq!(iface.identity.id, iface0.identity.id); + assert_eq!(iface.ip, iface0.ip); + + // Verify we cannot delete the interface while the instance is running + let err = NexusRequest::expect_failure( + client, + http::StatusCode::BAD_REQUEST, + http::Method::DELETE, + url_interface.as_str(), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect( + "Should not be able to delete network interface on running instance", + ) + .parsed_body::() + .expect("Failed to parse error response body"); + assert_eq!( + err.message, + "Instance must be stopped to detach a network interface", + "Expected an InvalidRequest response when detaching an interface from a running instance" + ); + + // Stop the instance and verify we can delete the interface + let instance = + instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + instance_simulate(nexus, &instance.identity.id).await; + NexusRequest::object_delete(client, url_interface.as_str()) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to delete interface from stopped instance"); +} + +/// This test specifically creates two NICs, the second of which will fail the +/// saga on purpose, since its IP address is the same. This is to verify that +/// the initial NIC is also deleted. +#[nexus_test] +async fn test_instance_with_multiple_nics_unwinds_completely( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + /* Create a project that we'll use for testing. */ + create_organization(&client, ORGANIZATION_NAME).await; + let url_instances = format!( + "/organizations/{}/projects/{}/instances", + ORGANIZATION_NAME, PROJECT_NAME + ); + let _ = create_project(&client, ORGANIZATION_NAME, PROJECT_NAME).await; + + // Create two interfaces, with the same IP addresses. + let default_name = "default".parse::().unwrap(); + let if0_params = params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("if0")).unwrap(), + description: String::from("first custom interface"), + }, + vpc_name: default_name.clone(), + subnet_name: default_name.clone(), + ip: Some("172.30.0.6".parse().unwrap()), + }; + let if1_params = params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("if1")).unwrap(), + description: String::from("second custom interface"), + }, + vpc_name: default_name.clone(), + subnet_name: default_name.clone(), + ip: Some("172.30.0.6".parse().unwrap()), + }; + let interface_params = params::InstanceNetworkInterfaceAttachment::Create( + params::InstanceNetworkInterfaceCreate { + params: vec![if0_params.clone(), if1_params.clone()], + }, + ); + + // Create the parameters for the instance itself, and create it. + let instance_params = params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("nic-fail-test-inst")).unwrap(), + description: String::from("instance to test multiple bad nics"), + }, + ncpus: InstanceCpuCount::try_from(2).unwrap(), + memory: ByteCount::from_mebibytes_u32(4), + hostname: String::from("nic-test"), + network_interfaces: interface_params, + }; + let builder = + RequestBuilder::new(client, http::Method::POST, &url_instances) + .body(Some(&instance_params)) + .expect_status(Some(http::StatusCode::BAD_REQUEST)); + let response = NexusRequest::new(builder) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Expected instance saga to fail"); + assert_eq!(response.status, http::StatusCode::BAD_REQUEST); + + // Verify that there are no NICs at all in the subnet. + let url_nics = format!( + "/organizations/{}/projects/{}/vpcs/{}/subnets/{}/network-interfaces", + ORGANIZATION_NAME, PROJECT_NAME, "default", "default" + ); + let interfaces = NexusRequest::iter_collection_authn::( + client, &url_nics, "", None, + ) + .await + .expect("failed to list network interfaces") + .all_items; + assert!( + interfaces.is_empty(), + "There should be no network interfaces in the subnet" + ); +} + async fn instance_get( client: &ClientTestContext, instance_url: &str, diff --git a/nexus/tests/integration_tests/subnet_allocation.rs b/nexus/tests/integration_tests/subnet_allocation.rs index a23cba39df4..7aaa55b9387 100644 --- a/nexus/tests/integration_tests/subnet_allocation.rs +++ b/nexus/tests/integration_tests/subnet_allocation.rs @@ -41,6 +41,7 @@ async fn create_instance_expect_failure( ncpus: InstanceCpuCount(1), memory: ByteCount::from_mebibytes_u32(256), hostname: name.to_string(), + network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, }; NexusRequest::new( @@ -104,10 +105,10 @@ async fn test_subnet_allocation(cptestctx: &ControlPlaneTestContext) { // This should fail from address exhaustion let error = create_instance_expect_failure(client, &url_instances, "i3").await; - assert_eq!(error.message, "no available IP addresses"); + assert_eq!(error.message, "No available IP addresses for interface"); // Verify the subnet lists the two addresses as in use - let url_ips = format!("{}/ips", url_subnet); + let url_ips = format!("{}/network-interfaces", url_subnet); let mut network_interfaces = objects_list_page::(client, &url_ips).await.items; assert_eq!(network_interfaces.len(), 2); diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 528eb2f9aa1..894706acfbc 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -211,6 +211,7 @@ lazy_static! { ncpus: InstanceCpuCount(1), memory: ByteCount::from_gibibytes_u32(16), hostname: String::from("demo-instance"), + network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, }; } diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index e7f4fa0fe11..2eae202df1c 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -21,6 +21,10 @@ OPERATION ID URL PATH instance_disks_attach /organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/disks/attach instance_disks_detach /organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/disks/detach instance_disks_get /organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/disks +instance_network_interfaces_delete_interface /organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces/{interface_name} +instance_network_interfaces_get /organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces +instance_network_interfaces_get_interface /organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces/{interface_name} +instance_network_interfaces_post /organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces project_instances_delete_instance /organizations/{organization_name}/projects/{project_name}/instances/{instance_name} project_instances_get /organizations/{organization_name}/projects/{project_name}/instances project_instances_get_instance /organizations/{organization_name}/projects/{project_name}/instances/{instance_name} @@ -88,7 +92,7 @@ hardware_sleds_get_sled /hardware/sleds/{sled_id} API operations found with tag "subnets" OPERATION ID URL PATH -subnets_ips_get /organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name}/ips +subnet_network_interfaces_get /organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name}/network-interfaces vpc_subnets_delete_subnet /organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name} vpc_subnets_get /organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets vpc_subnets_get_subnet /organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name} diff --git a/openapi/nexus.json b/openapi/nexus.json index 7f792a37277..b5aba3bab21 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -1483,6 +1483,278 @@ } } }, + "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces": { + "get": { + "tags": [ + "instances" + ], + "summary": "List network interfaces attached to this instance.", + "operationId": "instance_network_interfaces_get", + "parameters": [ + { + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + }, + "style": "form" + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retreive the subsequent page", + "schema": { + "nullable": true, + "type": "string" + }, + "style": "form" + }, + { + "in": "query", + "name": "sort_by", + "schema": { + "$ref": "#/components/schemas/NameSortMode" + }, + "style": "form" + }, + { + "in": "path", + "name": "instance_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "organization_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "project_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/NetworkInterfaceResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": true + }, + "post": { + "tags": [ + "instances" + ], + "summary": "Create a network interface for an instance.", + "operationId": "instance_network_interfaces_post", + "parameters": [ + { + "in": "path", + "name": "instance_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "organization_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "project_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/NetworkInterfaceCreate" + } + } + }, + "required": true + }, + "responses": { + "201": { + "description": "successful creation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/NetworkInterface" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces/{interface_name}": { + "get": { + "tags": [ + "instances" + ], + "summary": "Get an interface attached to an instance.", + "operationId": "instance_network_interfaces_get_interface", + "parameters": [ + { + "in": "path", + "name": "instance_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "interface_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "organization_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "project_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/NetworkInterface" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "delete": { + "tags": [ + "instances" + ], + "summary": "Detach a network interface from an instance.", + "operationId": "instance_network_interfaces_delete_interface", + "parameters": [ + { + "in": "path", + "name": "instance_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "interface_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "organization_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "project_name", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "responses": { + "204": { + "description": "successful deletion" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/reboot": { "post": { "tags": [ @@ -3134,13 +3406,13 @@ } } }, - "/organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name}/ips": { + "/organizations/{organization_name}/projects/{project_name}/vpcs/{vpc_name}/subnets/{subnet_name}/network-interfaces": { "get": { "tags": [ "subnets" ], - "summary": "List IP addresses on a VPC subnet.", - "operationId": "subnets_ips_get", + "summary": "List network interfaces in a VPC subnet.", + "operationId": "subnet_network_interfaces_get", "parameters": [ { "in": "query", @@ -4065,6 +4337,14 @@ }, "ncpus": { "$ref": "#/components/schemas/InstanceCpuCount" + }, + "network_interfaces": { + "description": "The network interfaces to be created for this instance.", + "allOf": [ + { + "$ref": "#/components/schemas/InstanceNetworkInterfaceAttachment" + } + ] } }, "required": [ @@ -4088,6 +4368,74 @@ "dst_sled_uuid" ] }, + "InstanceNetworkInterfaceAttachment": { + "description": "Describes an attachment of a `NetworkInterface` to an `Instance`, at the time the instance is created.", + "oneOf": [ + { + "description": "Create one or more `NetworkInterface`s for the `Instance`", + "type": "object", + "properties": { + "params": { + "$ref": "#/components/schemas/InstanceNetworkInterfaceCreate" + }, + "type": { + "type": "string", + "enum": [ + "Create" + ] + } + }, + "required": [ + "params", + "type" + ] + }, + { + "description": "Default networking setup, which creates a single interface with an auto-assigned IP address from project's \"default\" VPC and \"default\" VPC Subnet.", + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "Default" + ] + } + }, + "required": [ + "type" + ] + }, + { + "description": "No network interfaces at all will be created for the instance.", + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "None" + ] + } + }, + "required": [ + "type" + ] + } + ] + }, + "InstanceNetworkInterfaceCreate": { + "type": "object", + "properties": { + "params": { + "type": "array", + "items": { + "$ref": "#/components/schemas/NetworkInterfaceCreate" + } + } + }, + "required": [ + "params" + ] + }, "InstanceResultsPage": { "description": "A single page of results", "type": "object", @@ -4275,6 +4623,46 @@ "vpc_id" ] }, + "NetworkInterfaceCreate": { + "description": "Create-time parameters for a [`NetworkInterface`](omicron_common::api::external::NetworkInterface)", + "type": "object", + "properties": { + "description": { + "type": "string" + }, + "ip": { + "nullable": true, + "description": "The IP address for the interface. One will be auto-assigned if not provided.", + "type": "string", + "format": "ip" + }, + "name": { + "$ref": "#/components/schemas/Name" + }, + "subnet_name": { + "description": "The VPC Subnet in which to create the interface.", + "allOf": [ + { + "$ref": "#/components/schemas/Name" + } + ] + }, + "vpc_name": { + "description": "The VPC in which to create the interface.", + "allOf": [ + { + "$ref": "#/components/schemas/Name" + } + ] + } + }, + "required": [ + "description", + "name", + "subnet_name", + "vpc_name" + ] + }, "NetworkInterfaceResultsPage": { "description": "A single page of results", "type": "object", diff --git a/sled-agent/src/bin/sled-agent-sim.rs b/sled-agent/src/bin/sled-agent-sim.rs index 3cd7a84384f..c4c1a4a9d98 100644 --- a/sled-agent/src/bin/sled-agent-sim.rs +++ b/sled-agent/src/bin/sled-agent-sim.rs @@ -72,6 +72,7 @@ async fn do_run() -> Result<(), CmdError> { nexus_address: args.nexus_addr, dropshot: ConfigDropshot { bind_address: args.sled_agent_addr, + request_body_max_bytes: 2048, ..Default::default() }, log: ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Info }, diff --git a/sled-agent/src/bin/sled-agent.rs b/sled-agent/src/bin/sled-agent.rs index 4da820751ff..4edfe39f853 100644 --- a/sled-agent/src/bin/sled-agent.rs +++ b/sled-agent/src/bin/sled-agent.rs @@ -116,6 +116,7 @@ async fn do_run() -> Result<(), CmdError> { id: config.id, dropshot: ConfigDropshot { bind_address: config.bootstrap_address, + request_body_max_bytes: 2048, ..Default::default() }, log: ConfigLogging::StderrTerminal { diff --git a/tools/oxapi_demo b/tools/oxapi_demo index 4571c5c0e1e..fba15826caf 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -51,6 +51,13 @@ INSTANCES instance_detach_disk ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME DISK_NAME instance_list_disks ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME + instance_create_nic ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME VPC_NAME + SUBNET_NAME INTERFACE_NAME + instance_delete_nic ORGANIZATION_NAME PROJECT_NAME INTERFACE_NAME VPC_NAME + SUBNET_NAME INTERFACE_NAME + instance_list_nics ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME + instance_get_nic ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME + DISKS disk_create_demo ORGANIZATION_NAME PROJECT_NAME DISK_NAME @@ -67,10 +74,12 @@ VPC SUBNETS vpc_subnets_list ORGANIZATION_NAME PROJECT_NAME VPC_NAME vpc_subnet_get ORGANIZATION_NAME PROJECT_NAME VPC_NAME SUBNET_NAME + vpc_subnet_create_demo ORGANIZATION_NAME PROJECT_NAME VPC_NAME SUBNET_NAME + IPV4_NET -VPC SUBNETS IPS +VPC SUBNET NETWORK INTERFACES - subnet_ips_list ORGANIZATION_NAME PROJECT_NAME VPC_NAME SUBNET_NAME + subnet_list_nics ORGANIZATION_NAME PROJECT_NAME VPC_NAME SUBNET_NAME VPC FIREWALL @@ -380,10 +389,47 @@ function cmd_vpc_subnet_get do_curl "/organizations/$1/projects/$2/vpcs/$3/subnets/$4" } -function cmd_subnet_ips_list +function cmd_instance_create_nic +{ + [[ $# != 6 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME + VPC_NAME SUBNET_NAME INTERFACE_NAME" + mkjson \ + name="$6" \ + description="an interface called $6" \ + vpc_name="$4" \ + subnet_name="$5" | + do_curl_authn \ + "/organizations/$1/projects/$2/instances/$3/network-interfaces" \ + -X POST -T - +} + +function cmd_instance_delete_nic +{ + [[ $# != 4 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME + INTERFACE_NAME" + do_curl_authn \ + "/organizations/$1/projects/$2/instances/$3/network-interfaces/$4" -X \ + DELETE +} + +function cmd_subnet_list_nics { [[ $# != 4 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME VPC_NAME SUBNET_NAME" - do_curl "/organizations/$1/projects/$2/vpcs/$3/subnets/$4/ips" + do_curl "/organizations/$1/projects/$2/vpcs/$3/subnets/$4/network-interfaces" +} + +function cmd_instance_get_nic +{ + [[ $# != 5 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME INTERFACE_NAME" + do_curl \ + "/organizations/$1/projects/$2/instances/$3/network-interfaces/$4" +} + +function cmd_instance_list_nics +{ + [[ $# != 3 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME INSTANCE_NAME" + do_curl_authn \ + "/organizations/$1/projects/$2/instances/$3/network-interfaces" } function cmd_vpc_firewall_rules_get From 8f4f6e38b2c043046de034248016aa0240c4214c Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Mon, 14 Mar 2022 19:00:41 +0000 Subject: [PATCH 2/2] Small note fixup --- nexus/src/nexus.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index c365739e5c8..8f33b9635bc 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1596,8 +1596,8 @@ impl Nexus { )); } - // Lookup both the VPC and VPC Subnet, since we need both IDs to create - // an interface. + // NOTE: We need to lookup the VPC and VPC Subnet, since we need both + // IDs for creating the network interface. let vpc_name = db::model::Name(params.vpc_name.clone()); let subnet_name = db::model::Name(params.subnet_name.clone()); let vpc = self