From d83abad7bc1eb81221fc1686aab72b1c7a784773 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Fri, 27 May 2022 05:11:09 +0000 Subject: [PATCH 1/2] Adds concept of a primary network interface - Adds the `is_primary` column to the `network_interface` table, and a corresponding field `primary` in the database model and external `NetworkInterface` objects. Primary interfaces are used for NAT and appear in DNS records. - Updates the `InsertNetworkInterfaceQuery` to automatically decide if this interface should be considered the primary. It considers the new NIC primary iff there are zero existing NICs for the instance it's to be attached to. That means that the first NIC added to an instance, either during a provision or later, is the primary. Future work could allow changing which NIC is the primary. - Adds a new query for deleting a network interface from an instance, with improved validation. This now checks that the instance is stopped inside the query, fixing a TOCTOU bug. It also verifies that the instance either has exactly 1 interface (which must be the primary) or that the instance has 2 or more (we're deleting a secondary). This means that the primary interface cannot be deleted until all secondary interfaces are deleted. The reason for this restriction is that instances _must_ have a primary interface, and it's not clear how to pick a new primary from the remaining secondaries if we allow deletion of the primary. We force the client to make the choice. - Adds a special error type for handling the above validation failures. - Adds tests for this deletion behavior to the instance integration tests --- common/src/api/external/mod.rs | 3 + common/src/sql/dbinit.sql | 9 +- nexus/src/app/instance.rs | 20 +- nexus/src/app/sagas/instance_create.rs | 28 +- nexus/src/db/datastore.rs | 54 +-- nexus/src/db/model/network_interface.rs | 3 + nexus/src/db/queries/network_interface.rs | 426 +++++++++++++++++++-- nexus/src/db/schema.rs | 1 + nexus/src/defaults.rs | 4 + nexus/src/external_api/http_entrypoints.rs | 5 + nexus/src/external_api/params.rs | 11 +- nexus/tests/integration_tests/endpoints.rs | 3 +- nexus/tests/integration_tests/instances.rs | 155 ++++++-- openapi/nexus.json | 10 +- 14 files changed, 612 insertions(+), 120 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 3a5890ecd3b..2f74c8e5597 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 99d7a9712af..e2682044d4b 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -674,7 +674,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 754bfddcf9d..3cc77606d4b 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -13,7 +13,8 @@ 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::DeleteNetworkInterfaceError; +use crate::db::queries::network_interface::InsertNetworkInterfaceError; use crate::external_api::params; use omicron_common::api::external; use omicron_common::api::external::CreateResult; @@ -642,7 +643,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 +682,7 @@ impl super::Nexus { interface, ) .await - .map_err(NetworkInterfaceError::into_external)?; + .map_err(InsertNetworkInterfaceError::into_external)?; Ok(interface) } @@ -724,6 +726,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 +751,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 +761,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(DeleteNetworkInterfaceError::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 fa04ad373c4..ecff380cf4b 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::InsertNetworkInterfaceError; +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, @@ -347,7 +348,7 @@ async fn sic_create_custom_network_interfaces( // 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(InsertNetworkInterfaceError::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(InsertNetworkInterfaceError::into_external) .map_err(ActionError::action_failed)?; Ok(()) } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 5c844e5806c..d5314b1fd4e 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -40,8 +40,10 @@ 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::DeleteNetworkInterfaceError; +use crate::db::queries::network_interface::DeleteNetworkInterfaceQuery; +use crate::db::queries::network_interface::InsertNetworkInterfaceError; use crate::db::queries::network_interface::InsertNetworkInterfaceQuery; -use crate::db::queries::network_interface::NetworkInterfaceError; use crate::db::queries::vpc::InsertVpcQuery; use crate::db::queries::vpc_subnet::FilterConflictingVpcSubnetRangesQuery; use crate::db::queries::vpc_subnet::SubnetError; @@ -1575,15 +1577,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(InsertNetworkInterfaceError::External)?; opctx .authorize(authz::Action::CreateChild, authz_subnet) .await - .map_err(NetworkInterfaceError::External)?; + .map_err(InsertNetworkInterfaceError::External)?; self.instance_create_network_interface_raw(&opctx, interface).await } @@ -1591,7 +1593,7 @@ impl DataStore { &self, opctx: &OpContext, interface: IncompleteNetworkInterface, - ) -> Result { + ) -> Result { use db::schema::network_interface::dsl; let query = InsertNetworkInterfaceQuery::new(interface.clone()); diesel::insert_into(dsl::network_interface) @@ -1600,10 +1602,10 @@ impl DataStore { .get_result_async( self.pool_authorized(opctx) .await - .map_err(NetworkInterfaceError::External)?, + .map_err(InsertNetworkInterfaceError::External)?, ) .await - .map_err(|e| NetworkInterfaceError::from_pool(e, &interface)) + .map_err(|e| InsertNetworkInterfaceError::from_pool(e, &interface)) } /// Delete all network interfaces attached to the given instance. @@ -1634,28 +1636,32 @@ 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<(), DeleteNetworkInterfaceError> { + opctx + .authorize(authz::Action::Delete, authz_interface) .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByResource(authz_interface), - ) - })?; + .map_err(DeleteNetworkInterfaceError::External)?; + let query = DeleteNetworkInterfaceQuery::new( + authz_instance.id(), + authz_interface.id(), + ); + query + .clone() + .execute_async( + self.pool_authorized(opctx) + .await + .map_err(DeleteNetworkInterfaceError::External)?, + ) + .await + .map_err(|e| DeleteNetworkInterfaceError::from_pool(e, &query))?; Ok(()) } diff --git a/nexus/src/db/model/network_interface.rs b/nexus/src/db/model/network_interface.rs index 7afb6e3f25a..b9a42b26b19 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 95c752f4975..36457f451d7 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 InsertNetworkInterfaceError { /// The instance specified for this interface is already associated with a /// different VPC from this interface. InstanceSpansMultipleVpcs(Uuid), @@ -48,8 +51,8 @@ pub enum NetworkInterfaceError { External(external::Error), } -impl NetworkInterfaceError { - /// Construct a `NetworkInterfaceError` from a database error +impl InsertNetworkInterfaceError { + /// Construct a `InsertNetworkInterfaceError` from a database error /// /// This catches the various errors that the `InsertNetworkInterfaceQuery` /// can generate, especially the intentional errors that indicate either IP @@ -70,7 +73,7 @@ impl NetworkInterfaceError { Error::DatabaseError(_, _), )) => decode_database_error(e, interface), // Any other error at all is a bug - _ => NetworkInterfaceError::External( + _ => InsertNetworkInterfaceError::External( error::public_error_from_diesel_pool( e, error::ErrorHandler::Server, @@ -82,24 +85,24 @@ impl NetworkInterfaceError { /// Convert this error into an external one. pub fn into_external(self) -> external::Error { match self { - NetworkInterfaceError::NoAvailableIpAddresses => { + InsertNetworkInterfaceError::NoAvailableIpAddresses => { external::Error::invalid_request( "No available IP addresses for interface", ) } - NetworkInterfaceError::InstanceSpansMultipleVpcs(_) => { + InsertNetworkInterfaceError::InstanceSpansMultipleVpcs(_) => { external::Error::invalid_request(concat!( "Networking may not span multiple VPCs, but the ", "requested instance is associated with another VPC" )) } - NetworkInterfaceError::IpAddressNotAvailable(ip) => { + InsertNetworkInterfaceError::IpAddressNotAvailable(ip) => { external::Error::invalid_request(&format!( "The IP address '{}' is not available", ip )) } - NetworkInterfaceError::DuplicatePrimaryKey(id) => { + InsertNetworkInterfaceError::DuplicatePrimaryKey(id) => { external::Error::InternalError { internal_message: format!( "Found duplicate primary key '{}' when inserting network interface", @@ -107,18 +110,18 @@ impl NetworkInterfaceError { ), } } - NetworkInterfaceError::NoSlotsAvailable => { + InsertNetworkInterfaceError::NoSlotsAvailable => { external::Error::invalid_request(&format!( "Instances may not have more than {} network interfaces", MAX_NICS_PER_INSTANCE )) } - NetworkInterfaceError::NoMacAddrressesAvailable => { + InsertNetworkInterfaceError::NoMacAddrressesAvailable => { external::Error::invalid_request( "No available MAC addresses for interface", ) } - NetworkInterfaceError::External(e) => e, + InsertNetworkInterfaceError::External(e) => e, } } } @@ -138,7 +141,7 @@ impl NetworkInterfaceError { fn decode_database_error( err: async_bb8_diesel::PoolError, interface: &IncompleteNetworkInterface, -) -> NetworkInterfaceError { +) -> InsertNetworkInterfaceError { use crate::db::error; use async_bb8_diesel::ConnectionError; use async_bb8_diesel::PoolError; @@ -198,7 +201,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::NotNullViolation, ref info), )) if info.message() == IP_EXHAUSTION_ERROR_MESSAGE => { - NetworkInterfaceError::NoAvailableIpAddresses + InsertNetworkInterfaceError::NoAvailableIpAddresses } // This catches the error intentionally introduced by the @@ -208,7 +211,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::Unknown, ref info), )) if info.message() == MULTIPLE_VPC_ERROR_MESSAGE => { - NetworkInterfaceError::InstanceSpansMultipleVpcs( + InsertNetworkInterfaceError::InstanceSpansMultipleVpcs( interface.instance_id, ) } @@ -218,7 +221,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 + InsertNetworkInterfaceError::NoSlotsAvailable } // If the MAC allocation subquery fails, we'll attempt to insert NULL @@ -227,7 +230,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::NotNullViolation, ref info), )) if info.message() == MAC_EXHAUSTION_ERROR_MESSAGE => { - NetworkInterfaceError::NoMacAddrressesAvailable + InsertNetworkInterfaceError::NoMacAddrressesAvailable } // This path looks specifically at constraint names. @@ -240,13 +243,13 @@ fn decode_database_error( let ip = interface .ip .unwrap_or_else(|| std::net::Ipv4Addr::UNSPECIFIED.into()); - NetworkInterfaceError::IpAddressNotAvailable(ip) + InsertNetworkInterfaceError::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( + InsertNetworkInterfaceError::External( error::public_error_from_diesel_pool( err, error::ErrorHandler::Conflict( @@ -259,13 +262,13 @@ fn decode_database_error( // Primary key constraint violation. See notes above. Some(constraint) if constraint == PRIMARY_KEY_CONSTRAINT => { - NetworkInterfaceError::DuplicatePrimaryKey( + InsertNetworkInterfaceError::DuplicatePrimaryKey( interface.identity.id, ) } // Any other constraint violation is a bug - _ => NetworkInterfaceError::External( + _ => InsertNetworkInterfaceError::External( error::public_error_from_diesel_pool( err, error::ErrorHandler::Server, @@ -274,7 +277,7 @@ fn decode_database_error( }, // Any other error at all is a bug - _ => NetworkInterfaceError::External( + _ => InsertNetworkInterfaceError::External( error::public_error_from_diesel_pool( err, error::ErrorHandler::Server, @@ -517,8 +520,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,7 +605,7 @@ fn push_ensure_unique_vpc_expression<'a>( /// Errors /// ------ /// -/// See [`NetworkInterfaceError`] for the errors caught and propagated by this +/// See [`InsertNetworkInterfaceError`] for the errors caught and propagated by this /// query. /// /// Notes @@ -637,7 +638,6 @@ fn push_interface_allocation_subquery<'a>( mut out: AstPass<'_, 'a, Pg>, query: &'a InsertNetworkInterfaceQuery, ) -> 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 +738,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,7 +811,7 @@ 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`] +/// with a saga (for PK uniqueness violations). See [`InsertNetworkInterfaceError`] /// 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)] @@ -825,6 +831,7 @@ pub struct InsertNetworkInterfaceQuery { next_mac_subquery: NextGuestMacAddress, next_ipv4_address_subquery: NextGuestIpv4Address, next_slot_subquery: NextNicSlot, + is_primary_subquery: IsPrimaryNic, } impl InsertNetworkInterfaceQuery { @@ -837,6 +844,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 +854,7 @@ impl InsertNetworkInterfaceQuery { next_mac_subquery, next_ipv4_address_subquery, next_slot_subquery, + is_primary_subquery, } } } @@ -876,7 +886,6 @@ impl QueryFragment for InsertNetworkInterfaceQuery { &'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 +911,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(()) }; @@ -954,7 +965,6 @@ impl QueryFragment for InsertNetworkInterfaceQueryValues { &'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 +989,342 @@ 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(1) +/// FROM +/// network_interface +/// WHERE +/// instance_id = AND +/// name = +/// time_deleted IS NULL +/// ) = 1, +/// '', +/// 'has-secondary' +/// ) AS UUID) +/// ) +/// instance AS MATERIALIZED ( +/// SELECT CAST(IF( +/// EXISTS( +/// SELECT +/// id +/// FROM +/// instance +/// WHERE +/// id = AND +/// time_deleted IS NULL AND +/// state = 'stopped' +/// ), +/// '', +/// 'inst-running' +/// ) AS UUID) +/// ) +/// UPDATE +/// network_interface +/// SET +/// time_deleted = NOW() +/// WHERE +/// id = AND +/// time_deleted IS NULL +/// ``` +#[derive(Debug, Clone)] +pub struct DeleteNetworkInterfaceQuery { + interface_id: Uuid, + instance_id: Uuid, + instance_id_str: String, + instance_state: db::model::InstanceState, +} + +impl DeleteNetworkInterfaceQuery { + 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 DeleteNetworkInterfaceQuery { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl QueryFragment for DeleteNetworkInterfaceQuery { + 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(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) = 1, "); + out.push_bind_param::(&self.instance_id_str)?; + out.push_sql(", "); + out.push_bind_param::( + &DeleteNetworkInterfaceError::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::( + &DeleteNetworkInterfaceError::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 DeleteNetworkInterfaceQuery {} + +/// Errors related to deleting a network interface from an instance +#[derive(Debug)] +pub enum DeleteNetworkInterfaceError { + /// 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 DeleteNetworkInterfaceError { + const HAS_SECONDARIES_SENTINEL: &'static str = "secondaries"; + const INSTANCE_RUNNING_SENTINEL: &'static str = "running"; + + /// Construct a `DeleteNetworkInterfaceError` from a database error + /// + /// This catches the various errors that the `DeleteNetworkInterfaceQuery` + /// 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: &DeleteNetworkInterfaceQuery, + ) -> 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 + _ => DeleteNetworkInterfaceError::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 { + DeleteNetworkInterfaceError::InstanceHasSecondaryInterfaces(_) => { + external::Error::invalid_request( + "The primary interface for an instance \ + may not be deleted while secondary interfaces \ + are still attached", + ) + } + DeleteNetworkInterfaceError::InstanceMustBeStopped(_) => { + external::Error::invalid_request( + "Instance must be stopped to detach a network interface", + ) + } + DeleteNetworkInterfaceError::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, +) -> DeleteNetworkInterfaceError { + 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 => { + DeleteNetworkInterfaceError::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 => { + DeleteNetworkInterfaceError::InstanceMustBeStopped(instance_id) + } + + // Any other error at all is a bug + _ => DeleteNetworkInterfaceError::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::InsertNetworkInterfaceError; use super::MAX_NICS_PER_INSTANCE; use crate::context::OpContext; use crate::db::model::IncompleteNetworkInterface; @@ -1126,7 +1462,7 @@ mod tests { assert!( matches!( result, - Err(NetworkInterfaceError::IpAddressNotAvailable(_)) + Err(InsertNetworkInterfaceError::IpAddressNotAvailable(_)) ), "Requesting an interface with an existing IP should fail" ); @@ -1151,7 +1487,7 @@ mod tests { assert!( matches!( result, - Err(NetworkInterfaceError::External(Error::ObjectAlreadyExists { .. })), + Err(InsertNetworkInterfaceError::External(Error::ObjectAlreadyExists { .. })), ), "Requesting an interface with the same name on the same instance should fail" ); @@ -1175,7 +1511,7 @@ mod tests { .instance_create_network_interface_raw(&opctx, interface) .await; assert!( - matches!(result, Err(NetworkInterfaceError::InstanceSpansMultipleVpcs(_))), + matches!(result, Err(InsertNetworkInterfaceError::InstanceSpansMultipleVpcs(_))), "Attaching an interface to an instance which already has one in a different VPC should fail" ); } @@ -1226,7 +1562,7 @@ mod tests { assert!( matches!( result, - Err(NetworkInterfaceError::NoAvailableIpAddresses) + Err(InsertNetworkInterfaceError::NoAvailableIpAddresses) ), "Address exhaustion should be detected and handled" ); @@ -1343,11 +1679,13 @@ 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(InsertNetworkInterfaceError::DuplicatePrimaryKey(key)) = + result + { assert_eq!(key, inserted_interface.identity.id); } else { panic!( - "Expected a NetworkInterfaceError::DuplicatePrimaryKey \ + "Expected a InsertNetworkInterfaceError::DuplicatePrimaryKey \ error when inserting the exact same interface" ); } @@ -1415,6 +1753,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,12 +1780,18 @@ 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, + InsertNetworkInterfaceError::NoSlotsAvailable, + )); db.cleanup().await.unwrap(); logctx.cleanup_successful(); } + #[tokio::test] + async fn test_instance_network_interface_delete() {} + #[test] fn test_last_address_offset() { let subnet = "172.30.0.0/28".parse().unwrap(); diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index 931b6789154..31b6493eac2 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -130,6 +130,7 @@ table! { mac -> Int8, ip -> Inet, slot -> Int2, + is_primary -> Bool, } } diff --git a/nexus/src/defaults.rs b/nexus/src/defaults.rs index 15769c86611..c7af06cca01 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 61d358d5edb..d2299771d19 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1759,6 +1759,11 @@ 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. #[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 f654b393caf..760dadee946 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -93,12 +93,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 ff547f13c1f..ab04dcba9aa 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 dcead063843..2b7f8eea151 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 cbfb4f14d9d..16a0c9cb617 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": [ { @@ -6266,7 +6267,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": { @@ -6288,7 +6289,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": { @@ -6467,6 +6468,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", @@ -6495,6 +6500,7 @@ "ip", "mac", "name", + "primary", "subnet_id", "time_created", "time_modified", From ae2298209974d32c4558e0c5e4bf73d87193849b Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Fri, 3 Jun 2022 16:50:31 +0000 Subject: [PATCH 2/2] Review feedback - Cleanup comments - Simplify NIC query/error type names - Remove stale test --- nexus/src/app/instance.rs | 7 +- nexus/src/app/sagas/instance_create.rs | 8 +- nexus/src/db/datastore.rs | 33 +-- nexus/src/db/queries/network_interface.rs | 238 ++++++++++----------- nexus/src/external_api/http_entrypoints.rs | 2 + 5 files changed, 135 insertions(+), 153 deletions(-) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 3cc77606d4b..6e3228049c1 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -13,8 +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::DeleteNetworkInterfaceError; -use crate::db::queries::network_interface::InsertNetworkInterfaceError; +use crate::db::queries::network_interface; use crate::external_api::params; use omicron_common::api::external; use omicron_common::api::external::CreateResult; @@ -682,7 +681,7 @@ impl super::Nexus { interface, ) .await - .map_err(InsertNetworkInterfaceError::into_external)?; + .map_err(network_interface::InsertError::into_external)?; Ok(interface) } @@ -767,7 +766,7 @@ impl super::Nexus { &authz_interface, ) .await - .map_err(DeleteNetworkInterfaceError::into_external) + .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 ecff380cf4b..3b543edeea8 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -9,7 +9,7 @@ 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::InsertNetworkInterfaceError; +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; @@ -341,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(InsertNetworkInterfaceError::DuplicatePrimaryKey(_)) => { + Err(InsertNicError::DuplicatePrimaryKey(_)) => { // TODO-observability: We should bump a counter here. let log = osagactx.log(); warn!( @@ -439,7 +439,7 @@ async fn sic_create_default_primary_network_interface( interface, ) .await - .map_err(InsertNetworkInterfaceError::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 d5314b1fd4e..2898d0cbb72 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -40,10 +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::DeleteNetworkInterfaceError; -use crate::db::queries::network_interface::DeleteNetworkInterfaceQuery; -use crate::db::queries::network_interface::InsertNetworkInterfaceError; -use crate::db::queries::network_interface::InsertNetworkInterfaceQuery; +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; @@ -1577,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(InsertNetworkInterfaceError::External)?; + .map_err(network_interface::InsertError::External)?; opctx .authorize(authz::Action::CreateChild, authz_subnet) .await - .map_err(InsertNetworkInterfaceError::External)?; + .map_err(network_interface::InsertError::External)?; self.instance_create_network_interface_raw(&opctx, interface).await } @@ -1593,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(InsertNetworkInterfaceError::External)?, + .map_err(network_interface::InsertError::External)?, ) .await - .map_err(|e| InsertNetworkInterfaceError::from_pool(e, &interface)) + .map_err(|e| { + network_interface::InsertError::from_pool(e, &interface) + }) } /// Delete all network interfaces attached to the given instance. @@ -1644,12 +1643,12 @@ impl DataStore { opctx: &OpContext, authz_instance: &authz::Instance, authz_interface: &authz::NetworkInterface, - ) -> Result<(), DeleteNetworkInterfaceError> { + ) -> Result<(), network_interface::DeleteError> { opctx .authorize(authz::Action::Delete, authz_interface) .await - .map_err(DeleteNetworkInterfaceError::External)?; - let query = DeleteNetworkInterfaceQuery::new( + .map_err(network_interface::DeleteError::External)?; + let query = network_interface::DeleteQuery::new( authz_instance.id(), authz_interface.id(), ); @@ -1658,10 +1657,12 @@ impl DataStore { .execute_async( self.pool_authorized(opctx) .await - .map_err(DeleteNetworkInterfaceError::External)?, + .map_err(network_interface::DeleteError::External)?, ) .await - .map_err(|e| DeleteNetworkInterfaceError::from_pool(e, &query))?; + .map_err(|e| { + network_interface::DeleteError::from_pool(e, &query) + })?; Ok(()) } diff --git a/nexus/src/db/queries/network_interface.rs b/nexus/src/db/queries/network_interface.rs index 36457f451d7..225adddf74d 100644 --- a/nexus/src/db/queries/network_interface.rs +++ b/nexus/src/db/queries/network_interface.rs @@ -32,7 +32,7 @@ use uuid::Uuid; /// Errors related to inserting or attaching a NetworkInterface #[derive(Debug)] -pub enum InsertNetworkInterfaceError { +pub enum InsertError { /// The instance specified for this interface is already associated with a /// different VPC from this interface. InstanceSpansMultipleVpcs(Uuid), @@ -51,10 +51,10 @@ pub enum InsertNetworkInterfaceError { External(external::Error), } -impl InsertNetworkInterfaceError { - /// Construct a `InsertNetworkInterfaceError` 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. @@ -73,36 +73,34 @@ impl InsertNetworkInterfaceError { Error::DatabaseError(_, _), )) => decode_database_error(e, interface), // Any other error at all is a bug - _ => InsertNetworkInterfaceError::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 { - InsertNetworkInterfaceError::NoAvailableIpAddresses => { + InsertError::NoAvailableIpAddresses => { external::Error::invalid_request( "No available IP addresses for interface", ) } - InsertNetworkInterfaceError::InstanceSpansMultipleVpcs(_) => { + InsertError::InstanceSpansMultipleVpcs(_) => { external::Error::invalid_request(concat!( "Networking may not span multiple VPCs, but the ", "requested instance is associated with another VPC" )) } - InsertNetworkInterfaceError::IpAddressNotAvailable(ip) => { + InsertError::IpAddressNotAvailable(ip) => { external::Error::invalid_request(&format!( "The IP address '{}' is not available", ip )) } - InsertNetworkInterfaceError::DuplicatePrimaryKey(id) => { + InsertError::DuplicatePrimaryKey(id) => { external::Error::InternalError { internal_message: format!( "Found duplicate primary key '{}' when inserting network interface", @@ -110,25 +108,25 @@ impl InsertNetworkInterfaceError { ), } } - InsertNetworkInterfaceError::NoSlotsAvailable => { + InsertError::NoSlotsAvailable => { external::Error::invalid_request(&format!( "Instances may not have more than {} network interfaces", MAX_NICS_PER_INSTANCE )) } - InsertNetworkInterfaceError::NoMacAddrressesAvailable => { + InsertError::NoMacAddrressesAvailable => { external::Error::invalid_request( "No available MAC addresses for interface", ) } - InsertNetworkInterfaceError::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 @@ -141,7 +139,7 @@ impl InsertNetworkInterfaceError { fn decode_database_error( err: async_bb8_diesel::PoolError, interface: &IncompleteNetworkInterface, -) -> InsertNetworkInterfaceError { +) -> InsertError { use crate::db::error; use async_bb8_diesel::ConnectionError; use async_bb8_diesel::PoolError; @@ -201,7 +199,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::NotNullViolation, ref info), )) if info.message() == IP_EXHAUSTION_ERROR_MESSAGE => { - InsertNetworkInterfaceError::NoAvailableIpAddresses + InsertError::NoAvailableIpAddresses } // This catches the error intentionally introduced by the @@ -211,9 +209,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::Unknown, ref info), )) if info.message() == MULTIPLE_VPC_ERROR_MESSAGE => { - InsertNetworkInterfaceError::InstanceSpansMultipleVpcs( - interface.instance_id, - ) + InsertError::InstanceSpansMultipleVpcs(interface.instance_id) } // This checks the constraint on the interface slot numbers, used to @@ -221,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 => { - InsertNetworkInterfaceError::NoSlotsAvailable + InsertError::NoSlotsAvailable } // If the MAC allocation subquery fails, we'll attempt to insert NULL @@ -230,7 +226,7 @@ fn decode_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::NotNullViolation, ref info), )) if info.message() == MAC_EXHAUSTION_ERROR_MESSAGE => { - InsertNetworkInterfaceError::NoMacAddrressesAvailable + InsertError::NoMacAddrressesAvailable } // This path looks specifically at constraint names. @@ -243,46 +239,38 @@ fn decode_database_error( let ip = interface .ip .unwrap_or_else(|| std::net::Ipv4Addr::UNSPECIFIED.into()); - InsertNetworkInterfaceError::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 => { - InsertNetworkInterfaceError::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 => { - InsertNetworkInterfaceError::DuplicatePrimaryKey( - interface.identity.id, - ) + InsertError::DuplicatePrimaryKey(interface.identity.id) } // Any other constraint violation is a bug - _ => InsertNetworkInterfaceError::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 - _ => InsertNetworkInterfaceError::External( - error::public_error_from_diesel_pool( - err, - error::ErrorHandler::Server, - ), - ), + _ => InsertError::External(error::public_error_from_diesel_pool( + err, + error::ErrorHandler::Server, + )), } } @@ -605,8 +593,7 @@ fn push_ensure_unique_vpc_expression<'a>( /// Errors /// ------ /// -/// See [`InsertNetworkInterfaceError`] for the errors caught and propagated by this -/// query. +/// See [`InsertError`] for the errors caught and propagated by this query. /// /// Notes /// ----- @@ -636,7 +623,7 @@ 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<()> { // Push the CTE that ensures that any other interface with the same // instance_id also has the same vpc_id. See @@ -811,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 [`InsertNetworkInterfaceError`] -/// 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, @@ -834,7 +821,7 @@ pub struct InsertNetworkInterfaceQuery { 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()); @@ -866,22 +853,20 @@ 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>, @@ -942,25 +927,23 @@ 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>, @@ -1059,16 +1042,15 @@ const INSTANCE_FROM_CLAUSE: InstanceFromClause = InstanceFromClause::new(); /// OR /// ( /// SELECT -/// COUNT(1) +/// COUNT(*) /// FROM /// network_interface /// WHERE /// instance_id = AND -/// name = /// time_deleted IS NULL /// ) = 1, /// '', -/// 'has-secondary' +/// 'secondaries' /// ) AS UUID) /// ) /// instance AS MATERIALIZED ( @@ -1084,7 +1066,7 @@ const INSTANCE_FROM_CLAUSE: InstanceFromClause = InstanceFromClause::new(); /// state = 'stopped' /// ), /// '', -/// 'inst-running' +/// 'running' /// ) AS UUID) /// ) /// UPDATE @@ -1095,15 +1077,33 @@ const INSTANCE_FROM_CLAUSE: InstanceFromClause = InstanceFromClause::new(); /// 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 DeleteNetworkInterfaceQuery { +pub struct DeleteQuery { interface_id: Uuid, instance_id: Uuid, instance_id_str: String, instance_state: db::model::InstanceState, } -impl DeleteNetworkInterfaceQuery { +impl DeleteQuery { pub fn new(instance_id: Uuid, interface_id: Uuid) -> Self { Self { interface_id, @@ -1116,12 +1116,12 @@ impl DeleteNetworkInterfaceQuery { } } -impl QueryId for DeleteNetworkInterfaceQuery { +impl QueryId for DeleteQuery { type QueryId = (); const HAS_STATIC_QUERY_ID: bool = false; } -impl QueryFragment for DeleteNetworkInterfaceQuery { +impl QueryFragment for DeleteQuery { fn walk_ast<'a>( &'a self, mut out: AstPass<'_, 'a, Pg>, @@ -1138,7 +1138,7 @@ impl QueryFragment for DeleteNetworkInterfaceQuery { 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(1) FROM "); + 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)?; @@ -1150,7 +1150,7 @@ impl QueryFragment for DeleteNetworkInterfaceQuery { out.push_bind_param::(&self.instance_id_str)?; out.push_sql(", "); out.push_bind_param::( - &DeleteNetworkInterfaceError::HAS_SECONDARIES_SENTINEL, + &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)?; @@ -1170,7 +1170,7 @@ impl QueryFragment for DeleteNetworkInterfaceQuery { out.push_bind_param::(&self.instance_id_str)?; out.push_sql(", "); out.push_bind_param::( - &DeleteNetworkInterfaceError::INSTANCE_RUNNING_SENTINEL, + &DeleteError::INSTANCE_RUNNING_SENTINEL, )?; out.push_sql(") AS UUID))"); out.push_sql(" UPDATE "); @@ -1188,11 +1188,11 @@ impl QueryFragment for DeleteNetworkInterfaceQuery { } } -impl RunQueryDsl for DeleteNetworkInterfaceQuery {} +impl RunQueryDsl for DeleteQuery {} /// Errors related to deleting a network interface from an instance #[derive(Debug)] -pub enum DeleteNetworkInterfaceError { +pub enum DeleteError { /// Attempting to delete the primary interface, while there still exist /// secondary interfaces. InstanceHasSecondaryInterfaces(Uuid), @@ -1202,19 +1202,19 @@ pub enum DeleteNetworkInterfaceError { External(external::Error), } -impl DeleteNetworkInterfaceError { +impl DeleteError { const HAS_SECONDARIES_SENTINEL: &'static str = "secondaries"; const INSTANCE_RUNNING_SENTINEL: &'static str = "running"; - /// Construct a `DeleteNetworkInterfaceError` from a database error + /// Construct a `DeleteError` from a database error /// - /// This catches the various errors that the `DeleteNetworkInterfaceQuery` + /// 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: &DeleteNetworkInterfaceQuery, + query: &DeleteQuery, ) -> Self { use crate::db::error; use async_bb8_diesel::ConnectionError; @@ -1230,31 +1230,29 @@ impl DeleteNetworkInterfaceError { query.instance_id, ), // Any other error at all is a bug - _ => DeleteNetworkInterfaceError::External( - error::public_error_from_diesel_pool( - e, - error::ErrorHandler::Server, - ), - ), + _ => 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 { - DeleteNetworkInterfaceError::InstanceHasSecondaryInterfaces(_) => { + DeleteError::InstanceHasSecondaryInterfaces(_) => { external::Error::invalid_request( "The primary interface for an instance \ may not be deleted while secondary interfaces \ are still attached", ) } - DeleteNetworkInterfaceError::InstanceMustBeStopped(_) => { + DeleteError::InstanceMustBeStopped(_) => { external::Error::invalid_request( "Instance must be stopped to detach a network interface", ) } - DeleteNetworkInterfaceError::External(e) => e, + DeleteError::External(e) => e, } } } @@ -1269,7 +1267,7 @@ impl DeleteNetworkInterfaceError { fn decode_delete_network_interface_database_error( err: async_bb8_diesel::PoolError, instance_id: Uuid, -) -> DeleteNetworkInterfaceError { +) -> DeleteError { use crate::db::error; use async_bb8_diesel::ConnectionError; use async_bb8_diesel::PoolError; @@ -1296,9 +1294,7 @@ fn decode_delete_network_interface_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::Unknown, ref info), )) if info.message() == HAS_SECONDARIES_ERROR_MESSAGE => { - DeleteNetworkInterfaceError::InstanceHasSecondaryInterfaces( - instance_id, - ) + DeleteError::InstanceHasSecondaryInterfaces(instance_id) } // This catches the error intentionally introduced by the @@ -1307,16 +1303,14 @@ fn decode_delete_network_interface_database_error( PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::Unknown, ref info), )) if info.message() == INSTANCE_RUNNING_RUNNING_ERROR_MESSAGE => { - DeleteNetworkInterfaceError::InstanceMustBeStopped(instance_id) + DeleteError::InstanceMustBeStopped(instance_id) } // Any other error at all is a bug - _ => DeleteNetworkInterfaceError::External( - error::public_error_from_diesel_pool( - err, - error::ErrorHandler::Server, - ), - ), + _ => DeleteError::External(error::public_error_from_diesel_pool( + err, + error::ErrorHandler::Server, + )), } } @@ -1324,7 +1318,7 @@ fn decode_delete_network_interface_database_error( mod tests { use super::first_available_address; use super::last_address_offset; - use super::InsertNetworkInterfaceError; + use super::InsertError; use super::MAX_NICS_PER_INSTANCE; use crate::context::OpContext; use crate::db::model::IncompleteNetworkInterface; @@ -1460,10 +1454,7 @@ mod tests { .instance_create_network_interface_raw(&opctx, interface) .await; assert!( - matches!( - result, - Err(InsertNetworkInterfaceError::IpAddressNotAvailable(_)) - ), + matches!(result, Err(InsertError::IpAddressNotAvailable(_))), "Requesting an interface with an existing IP should fail" ); @@ -1487,7 +1478,7 @@ mod tests { assert!( matches!( result, - Err(InsertNetworkInterfaceError::External(Error::ObjectAlreadyExists { .. })), + Err(InsertError::External(Error::ObjectAlreadyExists { .. })), ), "Requesting an interface with the same name on the same instance should fail" ); @@ -1511,7 +1502,7 @@ mod tests { .instance_create_network_interface_raw(&opctx, interface) .await; assert!( - matches!(result, Err(InsertNetworkInterfaceError::InstanceSpansMultipleVpcs(_))), + matches!(result, Err(InsertError::InstanceSpansMultipleVpcs(_))), "Attaching an interface to an instance which already has one in a different VPC should fail" ); } @@ -1560,10 +1551,7 @@ mod tests { .instance_create_network_interface_raw(&opctx, interface) .await; assert!( - matches!( - result, - Err(InsertNetworkInterfaceError::NoAvailableIpAddresses) - ), + matches!(result, Err(InsertError::NoAvailableIpAddresses)), "Address exhaustion should be detected and handled" ); @@ -1679,13 +1667,11 @@ mod tests { let result = db_datastore .instance_create_network_interface_raw(&opctx, interface.clone()) .await; - if let Err(InsertNetworkInterfaceError::DuplicatePrimaryKey(key)) = - result - { + if let Err(InsertError::DuplicatePrimaryKey(key)) = result { assert_eq!(key, inserted_interface.identity.id); } else { panic!( - "Expected a InsertNetworkInterfaceError::DuplicatePrimaryKey \ + "Expected a InsertError::DuplicatePrimaryKey \ error when inserting the exact same interface" ); } @@ -1780,18 +1766,12 @@ 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, - InsertNetworkInterfaceError::NoSlotsAvailable, - )); + assert!(matches!(result, InsertError::NoSlotsAvailable,)); db.cleanup().await.unwrap(); logctx.cleanup_successful(); } - #[tokio::test] - async fn test_instance_network_interface_delete() {} - #[test] fn test_last_address_offset() { let subnet = "172.30.0.0/28".parse().unwrap(); diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d2299771d19..353913e59fc 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1764,6 +1764,8 @@ pub struct NetworkInterfacePathParam { /// 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}",