diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 3a5890ecd3..2f74c8e559 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1811,6 +1811,9 @@ pub struct NetworkInterface { 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. + /// True if this interface is the primary for the instance to which it's + /// attached. + pub primary: bool, } #[derive( diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index b486fdaf11..b031d900be 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -675,7 +675,14 @@ CREATE TABLE omicron.public.network_interface ( * Limited to 8 NICs per instance. This value must be kept in sync with * `crate::nexus::MAX_NICS_PER_INSTANCE`. */ - slot INT2 NOT NULL CHECK (slot >= 0 AND slot < 8) + slot INT2 NOT NULL CHECK (slot >= 0 AND slot < 8), + + /* True if this interface is the primary interface for the instance. + * + * The primary interface appears in DNS and its address is used for external + * connectivity for the instance. + */ + is_primary BOOL NOT NULL ); /* TODO-completeness diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 754bfddcf9..6e3228049c 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -13,7 +13,7 @@ use crate::db; use crate::db::identity::Resource; use crate::db::lookup::LookupPath; use crate::db::model::Name; -use crate::db::queries::network_interface::NetworkInterfaceError; +use crate::db::queries::network_interface; use crate::external_api::params; use omicron_common::api::external; use omicron_common::api::external::CreateResult; @@ -642,7 +642,8 @@ impl super::Nexus { // instance between this check and when we actually create the NIC // record. One solution is to place the state verification in the query // to create the NIC. Unfortunately, that query is already very - // complicated. + // complicated. See + // https://github.com/oxidecomputer/omicron/issues/1134. let stopped = db::model::InstanceState::new(external::InstanceState::Stopped); if db_instance.runtime_state.state != stopped { @@ -680,7 +681,7 @@ impl super::Nexus { interface, ) .await - .map_err(NetworkInterfaceError::into_external)?; + .map_err(network_interface::InsertError::into_external)?; Ok(interface) } @@ -724,6 +725,9 @@ impl super::Nexus { } /// Delete a network interface from the provided instance. + /// + /// Note that the primary interface for an instance cannot be deleted if + /// there are any secondary interfaces. pub async fn instance_delete_network_interface( &self, opctx: &OpContext, @@ -746,6 +750,8 @@ impl super::Nexus { .await?; // TODO-completeness: We'd like to relax this once hot-plug is supported + // TODO-correctness: There's a race condition here. Someone may start + // the instance after this check but before we actually delete the NIC. let stopped = db::model::InstanceState::new(external::InstanceState::Stopped); if db_instance.runtime_state.state != stopped { @@ -754,8 +760,13 @@ impl super::Nexus { )); } self.db_datastore - .instance_delete_network_interface(opctx, &authz_interface) + .instance_delete_network_interface( + opctx, + &authz_instance, + &authz_interface, + ) .await + .map_err(network_interface::DeleteError::into_external) } /// Invoked by a sled agent to publish an updated runtime state for an diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index fa04ad373c..3b543edeea 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -9,7 +9,8 @@ use crate::app::{MAX_DISKS_PER_INSTANCE, MAX_NICS_PER_INSTANCE}; use crate::context::OpContext; use crate::db::identity::Resource; use crate::db::lookup::LookupPath; -use crate::db::queries::network_interface::NetworkInterfaceError; +use crate::db::queries::network_interface::InsertError as InsertNicError; +use crate::defaults::DEFAULT_PRIMARY_NIC_NAME; use crate::external_api::params; use crate::saga_interface::SagaContext; use crate::{authn, authz, db}; @@ -242,7 +243,7 @@ async fn sic_create_network_interfaces( match sagactx.saga_params().create_params.network_interfaces { params::InstanceNetworkInterfaceAttachment::None => Ok(()), params::InstanceNetworkInterfaceAttachment::Default => { - sic_create_default_network_interface(&sagactx).await + sic_create_default_primary_network_interface(&sagactx).await } params::InstanceNetworkInterfaceAttachment::Create( ref create_params, @@ -340,14 +341,14 @@ async fn sic_create_custom_network_interfaces( // 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). + // `db::queries::network_interface::InsertQuery` 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(_)) => { + Err(InsertNicError::DuplicatePrimaryKey(_)) => { // TODO-observability: We should bump a counter here. let log = osagactx.log(); warn!( @@ -369,8 +370,9 @@ async fn sic_create_custom_network_interfaces( Ok(()) } -/// Create the default network interface for an instance during the create saga -async fn sic_create_default_network_interface( +/// Create a default primary network interface for an instance during the create +/// saga. +async fn sic_create_default_primary_network_interface( sagactx: &ActionContext, ) -> Result<(), ActionError> { let osagactx = sagactx.user_data(); @@ -379,13 +381,23 @@ async fn sic_create_default_network_interface( let opctx = OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); let instance_id = sagactx.lookup::("instance_id")?; + + // The literal name "default" is currently used for the VPC and VPC Subnet, + // when not specified in the client request. + // TODO-completeness: We'd like to select these from Project-level defaults. + // See https://github.com/oxidecomputer/omicron/issues/1015. let default_name = Name::try_from("default".to_string()).unwrap(); let internal_default_name = db::model::Name::from(default_name.clone()); + + // The name of the default primary interface. + let iface_name = + Name::try_from(DEFAULT_PRIMARY_NIC_NAME.to_string()).unwrap(); + let interface_params = params::NetworkInterfaceCreate { identity: IdentityMetadataCreateParams { - name: default_name.clone(), + name: iface_name.clone(), description: format!( - "default interface for {}", + "default primary interface for {}", saga_params.create_params.identity.name, ), }, @@ -427,7 +439,7 @@ async fn sic_create_default_network_interface( interface, ) .await - .map_err(NetworkInterfaceError::into_external) + .map_err(InsertNicError::into_external) .map_err(ActionError::action_failed)?; Ok(()) } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 5c844e5806..2898d0cbb7 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -40,8 +40,7 @@ use crate::db::lookup::LookupPath; use crate::db::model::DatabaseString; use crate::db::model::IncompleteVpc; use crate::db::model::Vpc; -use crate::db::queries::network_interface::InsertNetworkInterfaceQuery; -use crate::db::queries::network_interface::NetworkInterfaceError; +use crate::db::queries::network_interface; use crate::db::queries::vpc::InsertVpcQuery; use crate::db::queries::vpc_subnet::FilterConflictingVpcSubnetRangesQuery; use crate::db::queries::vpc_subnet::SubnetError; @@ -1575,15 +1574,15 @@ impl DataStore { authz_subnet: &authz::VpcSubnet, authz_instance: &authz::Instance, interface: IncompleteNetworkInterface, - ) -> Result { + ) -> Result { opctx .authorize(authz::Action::CreateChild, authz_instance) .await - .map_err(NetworkInterfaceError::External)?; + .map_err(network_interface::InsertError::External)?; opctx .authorize(authz::Action::CreateChild, authz_subnet) .await - .map_err(NetworkInterfaceError::External)?; + .map_err(network_interface::InsertError::External)?; self.instance_create_network_interface_raw(&opctx, interface).await } @@ -1591,19 +1590,21 @@ impl DataStore { &self, opctx: &OpContext, interface: IncompleteNetworkInterface, - ) -> Result { + ) -> Result { use db::schema::network_interface::dsl; - let query = InsertNetworkInterfaceQuery::new(interface.clone()); + let query = network_interface::InsertQuery::new(interface.clone()); diesel::insert_into(dsl::network_interface) .values(query) .returning(NetworkInterface::as_returning()) .get_result_async( self.pool_authorized(opctx) .await - .map_err(NetworkInterfaceError::External)?, + .map_err(network_interface::InsertError::External)?, ) .await - .map_err(|e| NetworkInterfaceError::from_pool(e, &interface)) + .map_err(|e| { + network_interface::InsertError::from_pool(e, &interface) + }) } /// Delete all network interfaces attached to the given instance. @@ -1634,27 +1635,33 @@ impl DataStore { } /// Delete a `NetworkInterface` attached to a provided instance. + /// + /// Note that the primary interface for an instance cannot be deleted if + /// there are any secondary interfaces. pub async fn instance_delete_network_interface( &self, opctx: &OpContext, + authz_instance: &authz::Instance, authz_interface: &authz::NetworkInterface, - ) -> DeleteResult { - opctx.authorize(authz::Action::Delete, authz_interface).await?; - - use db::schema::network_interface::dsl; - let now = Utc::now(); - let interface_id = authz_interface.id(); - diesel::update(dsl::network_interface) - .filter(dsl::id.eq(interface_id)) - .filter(dsl::time_deleted.is_null()) - .set((dsl::time_deleted.eq(now),)) - .execute_async(self.pool_authorized(opctx).await?) + ) -> Result<(), network_interface::DeleteError> { + opctx + .authorize(authz::Action::Delete, authz_interface) + .await + .map_err(network_interface::DeleteError::External)?; + let query = network_interface::DeleteQuery::new( + authz_instance.id(), + authz_interface.id(), + ); + query + .clone() + .execute_async( + self.pool_authorized(opctx) + .await + .map_err(network_interface::DeleteError::External)?, + ) .await .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByResource(authz_interface), - ) + network_interface::DeleteError::from_pool(e, &query) })?; Ok(()) } diff --git a/nexus/src/db/model/network_interface.rs b/nexus/src/db/model/network_interface.rs index 7afb6e3f25..b9a42b26b1 100644 --- a/nexus/src/db/model/network_interface.rs +++ b/nexus/src/db/model/network_interface.rs @@ -26,6 +26,8 @@ pub struct NetworkInterface { // If neither is specified, auto-assign one of each? pub ip: ipnetwork::IpNetwork, pub slot: i16, + #[diesel(column_name = is_primary)] + pub primary: bool, } impl From for external::NetworkInterface { @@ -37,6 +39,7 @@ impl From for external::NetworkInterface { subnet_id: iface.subnet_id, ip: iface.ip.ip(), mac: *iface.mac, + primary: iface.primary, } } } diff --git a/nexus/src/db/queries/network_interface.rs b/nexus/src/db/queries/network_interface.rs index 95c752f497..225adddf74 100644 --- a/nexus/src/db/queries/network_interface.rs +++ b/nexus/src/db/queries/network_interface.rs @@ -8,8 +8,10 @@ use crate::app::MAX_NICS_PER_INSTANCE; use crate::db; use crate::db::model::IncompleteNetworkInterface; use crate::db::model::MacAddr; +use crate::db::pool::DbConnection; use crate::db::queries::next_item::DefaultShiftGenerator; use crate::db::queries::next_item::NextItem; +use crate::db::schema::network_interface::dsl; use crate::defaults::NUM_INITIAL_RESERVED_IP_ADDRESSES; use chrono::DateTime; use chrono::Utc; @@ -21,6 +23,7 @@ use diesel::query_builder::QueryId; use diesel::sql_types; use diesel::Insertable; use diesel::QueryResult; +use diesel::RunQueryDsl; use ipnetwork::IpNetwork; use ipnetwork::Ipv4Network; use omicron_common::api::external; @@ -29,7 +32,7 @@ use uuid::Uuid; /// Errors related to inserting or attaching a NetworkInterface #[derive(Debug)] -pub enum NetworkInterfaceError { +pub enum InsertError { /// The instance specified for this interface is already associated with a /// different VPC from this interface. InstanceSpansMultipleVpcs(Uuid), @@ -48,10 +51,10 @@ pub enum NetworkInterfaceError { External(external::Error), } -impl NetworkInterfaceError { - /// Construct a `NetworkInterfaceError` from a database error +impl InsertError { + /// Construct a `InsertError` from a database error /// - /// This catches the various errors that the `InsertNetworkInterfaceQuery` + /// This catches the various errors that the `InsertQuery` /// 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. @@ -70,36 +73,34 @@ impl NetworkInterfaceError { 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, - ), - ), + _ => InsertError::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 => { + InsertError::NoAvailableIpAddresses => { external::Error::invalid_request( "No available IP addresses for interface", ) } - NetworkInterfaceError::InstanceSpansMultipleVpcs(_) => { + InsertError::InstanceSpansMultipleVpcs(_) => { external::Error::invalid_request(concat!( "Networking may not span multiple VPCs, but the ", "requested instance is associated with another VPC" )) } - NetworkInterfaceError::IpAddressNotAvailable(ip) => { + InsertError::IpAddressNotAvailable(ip) => { external::Error::invalid_request(&format!( "The IP address '{}' is not available", ip )) } - NetworkInterfaceError::DuplicatePrimaryKey(id) => { + InsertError::DuplicatePrimaryKey(id) => { external::Error::InternalError { internal_message: format!( "Found duplicate primary key '{}' when inserting network interface", @@ -107,25 +108,25 @@ impl NetworkInterfaceError { ), } } - NetworkInterfaceError::NoSlotsAvailable => { + InsertError::NoSlotsAvailable => { external::Error::invalid_request(&format!( "Instances may not have more than {} network interfaces", MAX_NICS_PER_INSTANCE )) } - NetworkInterfaceError::NoMacAddrressesAvailable => { + InsertError::NoMacAddrressesAvailable => { external::Error::invalid_request( "No available MAC addresses for interface", ) } - NetworkInterfaceError::External(e) => e, + InsertError::External(e) => e, } } } /// Decode an error from the database to determine why our NIC query failed. /// -/// When inserting network interfaces, we use the `InsertNetworkInterfaceQuery`, +/// When inserting network interfaces, we use the `InsertQuery`, /// 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 @@ -138,7 +139,7 @@ impl NetworkInterfaceError { fn decode_database_error( err: async_bb8_diesel::PoolError, interface: &IncompleteNetworkInterface, -) -> NetworkInterfaceError { +) -> InsertError { use crate::db::error; use async_bb8_diesel::ConnectionError; use async_bb8_diesel::PoolError; @@ -198,7 +199,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::NotNullViolation, ref info), )) if info.message() == IP_EXHAUSTION_ERROR_MESSAGE => { - NetworkInterfaceError::NoAvailableIpAddresses + InsertError::NoAvailableIpAddresses } // This catches the error intentionally introduced by the @@ -208,9 +209,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::Unknown, ref info), )) if info.message() == MULTIPLE_VPC_ERROR_MESSAGE => { - NetworkInterfaceError::InstanceSpansMultipleVpcs( - interface.instance_id, - ) + InsertError::InstanceSpansMultipleVpcs(interface.instance_id) } // This checks the constraint on the interface slot numbers, used to @@ -218,7 +217,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::CheckViolation, ref info), )) if info.message() == NO_SLOTS_AVAILABLE_ERROR_MESSAGE => { - NetworkInterfaceError::NoSlotsAvailable + InsertError::NoSlotsAvailable } // If the MAC allocation subquery fails, we'll attempt to insert NULL @@ -227,7 +226,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::NotNullViolation, ref info), )) if info.message() == MAC_EXHAUSTION_ERROR_MESSAGE => { - NetworkInterfaceError::NoMacAddrressesAvailable + InsertError::NoMacAddrressesAvailable } // This path looks specifically at constraint names. @@ -240,46 +239,38 @@ fn decode_database_error( let ip = interface .ip .unwrap_or_else(|| std::net::Ipv4Addr::UNSPECIFIED.into()); - NetworkInterfaceError::IpAddressNotAvailable(ip) + InsertError::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(), - ), + InsertError::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, - ) + InsertError::DuplicatePrimaryKey(interface.identity.id) } // Any other constraint violation is a bug - _ => NetworkInterfaceError::External( - error::public_error_from_diesel_pool( - err, - error::ErrorHandler::Server, - ), - ), + _ => InsertError::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, - ), - ), + _ => InsertError::External(error::public_error_from_diesel_pool( + err, + error::ErrorHandler::Server, + )), } } @@ -517,8 +508,6 @@ fn push_ensure_unique_vpc_expression<'a>( vpc_id_str: &'a String, instance_id: &'a 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 "); @@ -604,8 +593,7 @@ fn push_ensure_unique_vpc_expression<'a>( /// Errors /// ------ /// -/// See [`NetworkInterfaceError`] for the errors caught and propagated by this -/// query. +/// See [`InsertError`] for the errors caught and propagated by this query. /// /// Notes /// ----- @@ -635,9 +623,8 @@ fn push_ensure_unique_vpc_expression<'a>( /// the instance-validation check passes. fn push_interface_allocation_subquery<'a>( mut out: AstPass<'_, 'a, Pg>, - query: &'a InsertNetworkInterfaceQuery, + query: &'a InsertQuery, ) -> 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 @@ -738,7 +725,13 @@ fn push_interface_allocation_subquery<'a>( out.push_sql(") AS "); out.push_identifier(dsl::slot::NAME)?; - Ok(()) + // Push the subquery used to detect whether this interface is the primary. + // That's true iff there are zero interfaces for this instance at the time + // this interface is inserted. + out.push_sql(", ("); + query.is_primary_subquery.walk_ast(out.reborrow())?; + out.push_sql(") AS "); + out.push_identifier(dsl::is_primary::NAME) } /// Type used to insert conditionally insert a network interface. @@ -805,11 +798,11 @@ fn push_interface_allocation_subquery<'a>( /// `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 +/// with a saga (for PK uniqueness violations). See [`InsertError`] 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 struct InsertQuery { interface: IncompleteNetworkInterface, now: DateTime, @@ -825,9 +818,10 @@ pub struct InsertNetworkInterfaceQuery { next_mac_subquery: NextGuestMacAddress, next_ipv4_address_subquery: NextGuestIpv4Address, next_slot_subquery: NextNicSlot, + is_primary_subquery: IsPrimaryNic, } -impl InsertNetworkInterfaceQuery { +impl InsertQuery { pub fn new(interface: IncompleteNetworkInterface) -> Self { let vpc_id_str = interface.vpc_id.to_string(); let ip_sql = interface.ip.map(|ip| ip.into()); @@ -837,6 +831,8 @@ impl InsertNetworkInterfaceQuery { interface.subnet.identity.id, ); let next_slot_subquery = NextNicSlot::new(interface.instance_id); + let is_primary_subquery = + IsPrimaryNic { instance_id: interface.instance_id }; Self { interface, now: Utc::now(), @@ -845,6 +841,7 @@ impl InsertNetworkInterfaceQuery { next_mac_subquery, next_ipv4_address_subquery, next_slot_subquery, + is_primary_subquery, } } } @@ -856,27 +853,24 @@ type NetworkInterfaceFromClause = const NETWORK_INTERFACE_FROM_CLAUSE: NetworkInterfaceFromClause = NetworkInterfaceFromClause::new(); -impl QueryId for InsertNetworkInterfaceQuery { +impl QueryId for InsertQuery { type QueryId = (); const HAS_STATIC_QUERY_ID: bool = false; } -impl Insertable - for InsertNetworkInterfaceQuery -{ - type Values = InsertNetworkInterfaceQueryValues; +impl Insertable for InsertQuery { + type Values = InsertQueryValues; fn values(self) -> Self::Values { - InsertNetworkInterfaceQueryValues(self) + InsertQueryValues(self) } } -impl QueryFragment for InsertNetworkInterfaceQuery { +impl QueryFragment for InsertQuery { fn walk_ast<'a>( &'a self, mut out: AstPass<'_, 'a, Pg>, ) -> diesel::QueryResult<()> { - use db::schema::network_interface::dsl; let push_columns = |mut out: AstPass<'_, 'a, Pg>| -> diesel::QueryResult<()> { out.push_identifier(dsl::id::NAME)?; @@ -902,6 +896,8 @@ impl QueryFragment for InsertNetworkInterfaceQuery { out.push_identifier(dsl::ip::NAME)?; out.push_sql(", "); out.push_identifier(dsl::slot::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::is_primary::NAME)?; Ok(()) }; @@ -931,30 +927,27 @@ impl QueryFragment for InsertNetworkInterfaceQuery { } } -/// Type used to add the results of the `InsertNetworkInterfaceQuery` as values +/// Type used to add the results of the `InsertQuery` as values /// in a Diesel statement, e.g., `insert_into(network_interface).values(query).` /// Not for direct use. -pub struct InsertNetworkInterfaceQueryValues(InsertNetworkInterfaceQuery); +pub struct InsertQueryValues(InsertQuery); -impl QueryId for InsertNetworkInterfaceQueryValues { +impl QueryId for InsertQueryValues { type QueryId = (); const HAS_STATIC_QUERY_ID: bool = false; } -impl diesel::insertable::CanInsertInSingleQuery - for InsertNetworkInterfaceQueryValues -{ +impl diesel::insertable::CanInsertInSingleQuery for InsertQueryValues { fn rows_to_insert(&self) -> Option { Some(1) } } -impl QueryFragment for InsertNetworkInterfaceQueryValues { +impl QueryFragment for InsertQueryValues { fn walk_ast<'a>( &'a self, mut out: AstPass<'_, 'a, Pg>, ) -> diesel::QueryResult<()> { - use db::schema::network_interface::dsl; out.push_sql("("); out.push_identifier(dsl::id::NAME)?; out.push_sql(", "); @@ -979,16 +972,353 @@ impl QueryFragment for InsertNetworkInterfaceQueryValues { out.push_identifier(dsl::ip::NAME)?; out.push_sql(", "); out.push_identifier(dsl::slot::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::is_primary::NAME)?; out.push_sql(") "); self.0.walk_ast(out) } } +/// A small helper subquery that automatically assigns the `is_primary` column +/// for a new network interface. +/// +/// An instance with any network interfaces must have exactly one primary. (An +/// instance may have zero interfaces, however.) This subquery is used to insert +/// the value `true` if there are no extant interfaces on an instance, or +/// `false` if there are. +#[derive(Debug, Clone, Copy)] +struct IsPrimaryNic { + instance_id: Uuid, +} + +impl QueryId for IsPrimaryNic { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl QueryFragment for IsPrimaryNic { + fn walk_ast<'a>( + &'a self, + mut out: AstPass<'_, 'a, Pg>, + ) -> diesel::QueryResult<()> { + out.push_sql("SELECT NOT EXISTS(SELECT 1 FROM"); + NETWORK_INTERFACE_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(dsl::instance_id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.instance_id)?; + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL LIMIT 1)"); + Ok(()) + } +} + +type InstanceFromClause = FromClause; +const INSTANCE_FROM_CLAUSE: InstanceFromClause = InstanceFromClause::new(); + +/// Delete a network interface from an instance. +/// +/// There are a few preconditions that need to be checked when deleting a NIC. +/// First, the instance must currently be stopped, though we may relax this in +/// the future. Second, while an instance may have zero or more interfaces, if +/// it has one or more, exactly one of those must be the primary interface. That +/// means we can only delete the primary interface if there are no secondary +/// interfaces. The full query is: +/// +/// ```sql +/// WITH +/// interface AS MATERIALIZED ( +/// SELECT CAST(IF( +/// ( +/// SELECT +/// NOT is_primary +/// FROM +/// network_interface +/// WHERE +/// id = AND +/// time_deleted IS NULL +/// ) +/// OR +/// ( +/// SELECT +/// COUNT(*) +/// FROM +/// network_interface +/// WHERE +/// instance_id = AND +/// time_deleted IS NULL +/// ) = 1, +/// '', +/// 'secondaries' +/// ) AS UUID) +/// ) +/// instance AS MATERIALIZED ( +/// SELECT CAST(IF( +/// EXISTS( +/// SELECT +/// id +/// FROM +/// instance +/// WHERE +/// id = AND +/// time_deleted IS NULL AND +/// state = 'stopped' +/// ), +/// '', +/// 'running' +/// ) AS UUID) +/// ) +/// UPDATE +/// network_interface +/// SET +/// time_deleted = NOW() +/// WHERE +/// id = AND +/// time_deleted IS NULL +/// ``` +/// +/// Notes +/// ----- +/// +/// As with some of the other queries in this module, this uses some casting +/// trickery to learn why the query fails. This is why we store the +/// `instance_id` as a string in this type. For example, to check that the +/// instance is currently stopped, we see the subquery: +/// +/// ```sql +/// CAST(IF(, 'instance_id', 'running') AS UUID) +/// ``` +/// +/// The string `'running'` is not a valid UUID, so the query will fail with a +/// message including a string like: `could not parse "running" as type uuid`. +/// That string is included in the error message, and lets us determine that the +/// query failed because the instance was not stopped, as opposed to, say, +/// trying to delete the primary interface when there are still secondaries. +#[derive(Debug, Clone)] +pub struct DeleteQuery { + interface_id: Uuid, + instance_id: Uuid, + instance_id_str: String, + instance_state: db::model::InstanceState, +} + +impl DeleteQuery { + pub fn new(instance_id: Uuid, interface_id: Uuid) -> Self { + Self { + interface_id, + instance_id, + instance_id_str: instance_id.to_string(), + instance_state: db::model::InstanceState( + external::InstanceState::Stopped, + ), + } + } +} + +impl QueryId for DeleteQuery { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl QueryFragment for DeleteQuery { + fn walk_ast<'a>( + &'a self, + mut out: AstPass<'_, 'a, Pg>, + ) -> diesel::QueryResult<()> { + out.push_sql( + "WITH interface AS MATERIALIZED (SELECT CAST(IF((SELECT NOT ", + ); + out.push_identifier(dsl::is_primary::NAME)?; + out.push_sql(" FROM "); + 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_id)?; + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL) OR (SELECT COUNT(*) FROM "); + NETWORK_INTERFACE_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(dsl::instance_id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.instance_id)?; + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL) = 1, "); + out.push_bind_param::(&self.instance_id_str)?; + out.push_sql(", "); + out.push_bind_param::( + &DeleteError::HAS_SECONDARIES_SENTINEL, + )?; + out.push_sql(") AS UUID)), instance AS MATERIALIZED (SELECT CAST(IF(EXISTS(SELECT "); + out.push_identifier(db::schema::instance::dsl::id::NAME)?; + out.push_sql(" FROM "); + INSTANCE_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(db::schema::instance::dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.instance_id)?; + out.push_sql(" AND "); + out.push_identifier(db::schema::instance::dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL AND "); + out.push_identifier(db::schema::instance::dsl::state::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.instance_state)?; + out.push_sql("), "); + out.push_bind_param::(&self.instance_id_str)?; + out.push_sql(", "); + out.push_bind_param::( + &DeleteError::INSTANCE_RUNNING_SENTINEL, + )?; + out.push_sql(") AS UUID))"); + out.push_sql(" UPDATE "); + NETWORK_INTERFACE_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" SET "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" = NOW() WHERE "); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.interface_id)?; + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL"); + Ok(()) + } +} + +impl RunQueryDsl for DeleteQuery {} + +/// Errors related to deleting a network interface from an instance +#[derive(Debug)] +pub enum DeleteError { + /// Attempting to delete the primary interface, while there still exist + /// secondary interfaces. + InstanceHasSecondaryInterfaces(Uuid), + /// Instance must be stopped prior to deleting interfaces from it + InstanceMustBeStopped(Uuid), + /// Any other error + External(external::Error), +} + +impl DeleteError { + const HAS_SECONDARIES_SENTINEL: &'static str = "secondaries"; + const INSTANCE_RUNNING_SENTINEL: &'static str = "running"; + + /// Construct a `DeleteError` from a database error + /// + /// This catches the various errors that the `DeleteQuery` + /// can generate, specifically the intentional errors that indicate that + /// either the instance is still running, or that the instance has one or + /// more secondary interfaces. + pub fn from_pool( + e: async_bb8_diesel::PoolError, + query: &DeleteQuery, + ) -> 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_delete_network_interface_database_error( + e, + query.instance_id, + ), + // Any other error at all is a bug + _ => DeleteError::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 { + DeleteError::InstanceHasSecondaryInterfaces(_) => { + external::Error::invalid_request( + "The primary interface for an instance \ + may not be deleted while secondary interfaces \ + are still attached", + ) + } + DeleteError::InstanceMustBeStopped(_) => { + external::Error::invalid_request( + "Instance must be stopped to detach a network interface", + ) + } + DeleteError::External(e) => e, + } + } +} + +/// Decode an error from the database to determine why deleting an interface +/// failed. +/// +/// 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_delete_network_interface_database_error( + err: async_bb8_diesel::PoolError, + instance_id: Uuid, +) -> DeleteError { + 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're attempting to delete a primary + // interface, and that instance also has one or more secondary interfaces + const HAS_SECONDARIES_ERROR_MESSAGE: &'static str = + "could not parse \"secondaries\" as type uuid: uuid: \ + incorrect UUID length: secondaries"; + + // Error message generated when we're attempting to delete a secondary + // interface from an instance that is not stopped. + const INSTANCE_RUNNING_RUNNING_ERROR_MESSAGE: &'static str = + "could not parse \"running\" as type uuid: uuid: \ + incorrect UUID length: running"; + + match err { + // This catches the error intentionally introduced by the + // first CTE, which generates a UUID parsing error if we're trying to + // delete the primary interface, and the instance also has one or more + // secondaries. + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError(DatabaseErrorKind::Unknown, ref info), + )) if info.message() == HAS_SECONDARIES_ERROR_MESSAGE => { + DeleteError::InstanceHasSecondaryInterfaces(instance_id) + } + + // This catches the error intentionally introduced by the + // second CTE, which generates a UUID parsing error if we're trying to + // delete any interface from an instance that is not stopped. + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError(DatabaseErrorKind::Unknown, ref info), + )) if info.message() == INSTANCE_RUNNING_RUNNING_ERROR_MESSAGE => { + DeleteError::InstanceMustBeStopped(instance_id) + } + + // Any other error at all is a bug + _ => DeleteError::External(error::public_error_from_diesel_pool( + err, + error::ErrorHandler::Server, + )), + } +} + #[cfg(test)] mod tests { use super::first_available_address; use super::last_address_offset; - use super::NetworkInterfaceError; + use super::InsertError; use super::MAX_NICS_PER_INSTANCE; use crate::context::OpContext; use crate::db::model::IncompleteNetworkInterface; @@ -1124,10 +1454,7 @@ mod tests { .instance_create_network_interface_raw(&opctx, interface) .await; assert!( - matches!( - result, - Err(NetworkInterfaceError::IpAddressNotAvailable(_)) - ), + matches!(result, Err(InsertError::IpAddressNotAvailable(_))), "Requesting an interface with an existing IP should fail" ); @@ -1151,7 +1478,7 @@ mod tests { assert!( matches!( result, - Err(NetworkInterfaceError::External(Error::ObjectAlreadyExists { .. })), + Err(InsertError::External(Error::ObjectAlreadyExists { .. })), ), "Requesting an interface with the same name on the same instance should fail" ); @@ -1175,7 +1502,7 @@ mod tests { .instance_create_network_interface_raw(&opctx, interface) .await; assert!( - matches!(result, Err(NetworkInterfaceError::InstanceSpansMultipleVpcs(_))), + matches!(result, Err(InsertError::InstanceSpansMultipleVpcs(_))), "Attaching an interface to an instance which already has one in a different VPC should fail" ); } @@ -1224,10 +1551,7 @@ mod tests { .instance_create_network_interface_raw(&opctx, interface) .await; assert!( - matches!( - result, - Err(NetworkInterfaceError::NoAvailableIpAddresses) - ), + matches!(result, Err(InsertError::NoAvailableIpAddresses)), "Address exhaustion should be detected and handled" ); @@ -1343,11 +1667,11 @@ mod tests { let result = db_datastore .instance_create_network_interface_raw(&opctx, interface.clone()) .await; - if let Err(NetworkInterfaceError::DuplicatePrimaryKey(key)) = result { + if let Err(InsertError::DuplicatePrimaryKey(key)) = result { assert_eq!(key, inserted_interface.identity.id); } else { panic!( - "Expected a NetworkInterfaceError::DuplicatePrimaryKey \ + "Expected a InsertError::DuplicatePrimaryKey \ error when inserting the exact same interface" ); } @@ -1415,6 +1739,14 @@ mod tests { slot, actual_slot, "Failed to allocate next available interface slot" ); + + // Check that only the first NIC is designated the primary + assert_eq!( + inserted_interface.primary, + slot == 0, + "Only the first NIC inserted for an instance should \ + be marked the primary" + ); } // The next one should fail @@ -1434,7 +1766,7 @@ mod tests { .instance_create_network_interface_raw(&opctx, interface.clone()) .await .expect_err("Should not be able to insert more than 8 interfaces"); - assert!(matches!(result, NetworkInterfaceError::NoSlotsAvailable,)); + assert!(matches!(result, InsertError::NoSlotsAvailable,)); db.cleanup().await.unwrap(); logctx.cleanup_successful(); diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index 2eb058bd10..37a9164228 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -131,6 +131,7 @@ table! { mac -> Int8, ip -> Inet, slot -> Int2, + is_primary -> Bool, } } diff --git a/nexus/src/defaults.rs b/nexus/src/defaults.rs index 15769c8661..c7af06cca0 100644 --- a/nexus/src/defaults.rs +++ b/nexus/src/defaults.rs @@ -21,6 +21,10 @@ pub const MIN_VPC_IPV4_SUBNET_PREFIX: u8 = 8; /// The number of reserved addresses at the beginning of a subnet range. pub const NUM_INITIAL_RESERVED_IP_ADDRESSES: usize = 5; +/// The name provided for a default primary network interface for a guest +/// instance. +pub const DEFAULT_PRIMARY_NIC_NAME: &str = "net0"; + 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 345726e217..e3c86717fd 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1758,6 +1758,13 @@ pub struct NetworkInterfacePathParam { } /// Detach a network interface from an instance. +/// +/// Note that the primary interface for an instance cannot be deleted if there +/// are any secondary interfaces. A new primary interface must be designated +/// first. The primary interface can be deleted if there are no secondary +/// interfaces. +// TODO-completeness: Add API for modifying an interface, including setting as +// new primary. See https://github.com/oxidecomputer/omicron/issues/1153. #[endpoint { method = DELETE, path = "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces/{interface_name}", diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 59014e2498..7f6f3ddb2b 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -94,12 +94,15 @@ pub struct NetworkInterfaceCreate { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] #[serde(tag = "type", content = "params", rename_all = "snake_case")] pub enum InstanceNetworkInterfaceAttachment { - /// Create one or more `NetworkInterface`s for the `Instance` + /// Create one or more `NetworkInterface`s for the `Instance`. + /// + /// If more than one interface is provided, then the first will be + /// designated the primary interface for the instance. Create(Vec), - /// Default networking setup, which creates a single interface with an - /// auto-assigned IP address from project's "default" VPC and "default" VPC - /// Subnet. + /// The default networking configuration for an instance is to create a + /// single primary interface with an automatically-assigned IP address. The + /// IP will be pulled from the Project's default VPC / VPC Subnet. Default, /// No network interfaces at all will be created for the instance. diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 9dc8c82592..362e781e84 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -211,7 +211,8 @@ lazy_static! { }; // The instance needs a network interface, too. - pub static ref DEMO_INSTANCE_NIC_NAME: Name = "default".parse().unwrap(); + pub static ref DEMO_INSTANCE_NIC_NAME: Name = + omicron_nexus::defaults::DEFAULT_PRIMARY_NIC_NAME.parse().unwrap(); pub static ref DEMO_INSTANCE_NIC_URL: String = format!("{}/{}", *DEMO_INSTANCE_NICS_URL, *DEMO_INSTANCE_NIC_NAME); pub static ref DEMO_INSTANCE_NIC_CREATE: params::NetworkInterfaceCreate = diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index dcead06384..2b7f8eea15 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -182,7 +182,10 @@ async fn test_instances_create_reboot_halt( .items; assert_eq!(network_interfaces.len(), 1); assert_eq!(network_interfaces[0].instance_id, instance.identity.id); - assert_eq!(network_interfaces[0].identity.name, "default"); + assert_eq!( + network_interfaces[0].identity.name, + omicron_nexus::defaults::DEFAULT_PRIMARY_NIC_NAME + ); // Now, simulate completion of instance boot and check the state reported. instance_simulate(nexus, &instance.identity.id).await; @@ -915,18 +918,27 @@ async fn test_instance_create_delete_network_interface( "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"), + // Parameters for the interfaces to create/attach + let if_params = vec![ + 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()), }, - 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()); + params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: "if1".parse().unwrap(), + description: String::from("a new nic"), + }, + vpc_name: "default".parse().unwrap(), + subnet_name: "default".parse().unwrap(), + ip: Some("172.30.0.11".parse().unwrap()), + }, + ]; // We should not be able to create an interface while the instance is running. // @@ -937,7 +949,7 @@ async fn test_instance_create_delete_network_interface( http::Method::POST, url_interfaces.as_str(), ) - .body(Some(&if_params)) + .body(Some(&if_params[0])) .expect_status(Some(http::StatusCode::BAD_REQUEST)); let err = NexusRequest::new(builder) .authn_as(AuthnMode::PrivilegedUser) @@ -957,33 +969,83 @@ async fn test_instance_create_delete_network_interface( 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()); + // Verify we can now make the requests again + let mut interfaces = Vec::with_capacity(2); + for (i, params) in if_params.iter().enumerate() { + let response = NexusRequest::objects_post( + client, + url_interfaces.as_str(), + ¶ms, + ) + .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, params.identity.name); + assert_eq!(iface.ip, params.ip.unwrap()); + assert_eq!( + iface.primary, + i == 0, + "Only the first interface should be primary" + ); + interfaces.push(iface); + } - // Restart the instance, verify the interface is still correct. + // Restart the instance, verify the interfaces are 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()) + for iface in interfaces.iter() { + let url_interface = + format!("{}/{}", url_interfaces, iface.identity.name.as_str()); + 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); + assert_eq!(iface.primary, iface0.primary); + } + + // Verify we cannot delete either interface while the instance is running + for iface in interfaces.iter() { + let url_interface = + format!("{}/{}", url_interfaces, iface.identity.name.as_str()); + let err = NexusRequest::expect_failure( + client, + http::StatusCode::BAD_REQUEST, + http::Method::DELETE, + 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); + .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" + ); + } - // Verify we cannot delete the interface while the instance is running + // 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; + + // We should not be able to delete the primary interface, while the + // secondary still exists + let url_interface = + format!("{}/{}", url_interfaces, interfaces[0].identity.name.as_str()); let err = NexusRequest::expect_failure( client, http::StatusCode::BAD_REQUEST, @@ -994,25 +1056,40 @@ async fn test_instance_create_delete_network_interface( .execute() .await .expect( - "Should not be able to delete network interface on running instance", + "Should not be able to delete the primary network interface \ + while secondary interfaces still exist", ) .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" + "The primary interface for an instance \ + may not be deleted while secondary interfaces \ + are still attached", + "Expected an InvalidRequest response when detaching \ + the primary interface from an instance with at least one \ + secondary interface", ); - // 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; + // Verify that we can delete the secondary. + let url_interface = + format!("{}/{}", url_interfaces, interfaces[1].identity.name.as_str()); + NexusRequest::object_delete(client, url_interface.as_str()) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to delete secondary interface from stopped instance"); + + // Now verify that we can delete the primary + let url_interface = + format!("{}/{}", url_interfaces, interfaces[0].identity.name.as_str()); NexusRequest::object_delete(client, url_interface.as_str()) .authn_as(AuthnMode::PrivilegedUser) .execute() .await - .expect("Failed to delete interface from stopped instance"); + .expect( + "Failed to delete sole primary interface from stopped instance", + ); } /// This test specifically creates two NICs, the second of which will fail the diff --git a/openapi/nexus.json b/openapi/nexus.json index fe1ff4c66d..2c2f74f0ae 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -2209,6 +2209,7 @@ "instances" ], "summary": "Detach a network interface from an instance.", + "description": "Note that the primary interface for an instance cannot be deleted if there are any secondary interfaces. A new primary interface must be designated first. The primary interface can be deleted if there are no secondary interfaces.", "operationId": "instance_network_interfaces_delete_interface", "parameters": [ { @@ -6341,7 +6342,7 @@ "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`", + "description": "Create one or more `NetworkInterface`s for the `Instance`.\n\nIf more than one interface is provided, then the first will be designated the primary interface for the instance.", "type": "object", "properties": { "params": { @@ -6363,7 +6364,7 @@ ] }, { - "description": "Default networking setup, which creates a single interface with an auto-assigned IP address from project's \"default\" VPC and \"default\" VPC Subnet.", + "description": "The default networking configuration for an instance is to create a single primary interface with an automatically-assigned IP address. The IP will be pulled from the Project's default VPC / VPC Subnet.", "type": "object", "properties": { "type": { @@ -6542,6 +6543,10 @@ } ] }, + "primary": { + "description": "True if this interface is the primary for the instance to which it's attached.", + "type": "boolean" + }, "subnet_id": { "description": "The subnet to which the interface belongs.", "type": "string", @@ -6570,6 +6575,7 @@ "ip", "mac", "name", + "primary", "subnet_id", "time_created", "time_modified",