Add network interface parameters to instance creation

- Add helpers to check if an address provided by a user can be
  requested, i.e., is in the subnet and is not a reserved address.
- Add a query used to insert a network interface into the database,
  handling both IP allocation and validation of any provided instance
  (i.e., that the instance spans exactly one VPC). This subsumes the
  previous IP allocation query.
- Adds parameters for setting up network interfaces (possibly multiple)
  in the instance create request, with parameters for each.
- Adds more robust and clear database error handling for detecting
  failures when inserting network interfaces. This code detects specific
  error messages from the database that we intentionally insert, and
  makes clear the relationship between the errors we expect, and the
  invariants they are designed to ensure. This includes special handling
  of a duplicate primary key, which is only expected during sagas and
  helps ensure their idempotency.
- Ensures idempotency of multi-NIC allocation and rollback during sagas,
  by adding separate ID-allocation nodes and NIC-creation nodes. The
  rollback for the latter is a no-op, and the former deletes all NICs
  for the instance to be created. There are also tests to ensure that we
  completely any NICs created as a result of failure partway through,
  and that replaying the saga works as expected.
- Adds some more tests to the NIC creation Nexus API
- Adds convenience method to delete all NICs from an instance.

Author: bnaecker

README.adoc

@@ -645,6 +645,11 @@ documents being checked in.
 In general, changes any service API **require the following set of build steps**:
 
 * Make changes to the service API
+* Build the package for the modified service alone. This can be done by changing
+  directories there, or `cargo build -p <package>`. This is step is important,
+  to avoid the circular dependency at this point. One needs to update this one
+  OpenAPI document, without rebuilding the other components that depend on a
+  now-outdated spec.
 * Update the OpenAPI document by running the relevant test with overwrite set:
   `EXPECTORATE=overwrite cargo test test_nexus_openapi_internal` (changing the
   test name as necessary)

common/src/api/external/mod.rs

@@ -1774,20 +1774,22 @@ pub struct NetworkInterface {
     #[serde(flatten)]
     pub identity: IdentityMetadata,
 
-    /** The Instance to which the interface belongs. */
+    /// The Instance to which the interface belongs.
     pub instance_id: Uuid,
 
-    /** The VPC to which the interface belongs. */
+    /// The VPC to which the interface belongs.
     pub vpc_id: Uuid,
 
-    /** The subnet to which the interface belongs. */
+    /// The subnet to which the interface belongs.
     pub subnet_id: Uuid,
 
-    /** The MAC address assigned to this interface. */
+    /// The MAC address assigned to this interface.
     pub mac: MacAddr,
 
-    /** The IP address assigned to this interface. */
+    /// The IP address assigned to this interface.
     pub ip: IpAddr,
+    // TODO-correctness: We need to split this into an optional V4 and optional
+    // V6 address, at least one of which must be specified.
 }
 
 #[cfg(test)]

common/src/sql/dbinit.sql

@@ -465,8 +465,13 @@ CREATE TABLE omicron.public.network_interface (
     time_modified TIMESTAMPTZ NOT NULL,
     /* Indicates that the object has been deleted */
     time_deleted TIMESTAMPTZ,
-    /* FK into Instance table. */
+
+    /* FK into Instance table.
+     * Note that interfaces are always attached to a particular instance.
+     * IP addresses may be reserved, but this is a different resource.
+     */
     instance_id UUID NOT NULL,
+
     /* FK into VPC table */
     vpc_id UUID NOT NULL,
     /* FK into VPCSubnet table. */
@@ -483,12 +488,6 @@ CREATE TABLE omicron.public.network_interface (
  * as moving IPs between NICs on different instances, etc.
  */
 
-CREATE UNIQUE INDEX ON omicron.public.network_interface (
-    vpc_id,
-    name
-) WHERE
-    time_deleted IS NULL;
-
 /* Ensure we do not assign the same address twice within a subnet */
 CREATE UNIQUE INDEX ON omicron.public.network_interface (
     subnet_id,
@@ -505,6 +504,18 @@ CREATE UNIQUE INDEX ON omicron.public.network_interface (
 ) WHERE
     time_deleted IS NULL;
 
+/*
+ * Index used to verify that an Instance's networking is contained
+ * within a single VPC.
+ */
+CREATE UNIQUE INDEX ON omicron.public.network_interface (
+    instance_id,
+    name
+)
+STORING (vpc_id)
+WHERE
+    time_deleted IS NULL;
+
 CREATE TYPE omicron.public.vpc_router_kind AS ENUM (
     'system',
     'custom'

nexus/src/db/datastore.rs

@@ -79,8 +79,9 @@ use crate::db::{
     },
     pagination::paginated,
     pagination::paginated_multicolumn,
-    subnet_allocation::AllocateIpQuery,
     subnet_allocation::FilterConflictingVpcSubnetRangesQuery,
+    subnet_allocation::InsertNetworkInterfaceQuery,
+    subnet_allocation::NetworkInterfaceError,
     subnet_allocation::SubnetError,
     update_and_check::{UpdateAndCheck, UpdateStatus},
 };
@@ -1698,109 +1699,153 @@ impl DataStore {
     /*
      * Network interfaces
      */
-
     pub async fn instance_create_network_interface(
         &self,
         interface: IncompleteNetworkInterface,
-    ) -> CreateResult<NetworkInterface> {
+    ) -> Result<NetworkInterface, NetworkInterfaceError> {
         use db::schema::network_interface::dsl;
+        let query = InsertNetworkInterfaceQuery {
+            interface: interface.clone(),
+            now: Utc::now(),
+        };
+        diesel::insert_into(dsl::network_interface)
+            .values(query)
+            .returning(NetworkInterface::as_returning())
+            .get_result_async(self.pool())
+            .await
+            .map_err(|e| NetworkInterfaceError::from_pool(e, &interface))
+    }
 
-        // TODO: Longer term, it would be nice to decouple the IP allocation
-        // (and MAC allocation) from the NetworkInterface table, so that
-        // retrying from parallel inserts doesn't need to happen here.
-
-        let name = interface.identity.name.clone();
-        match interface.ip {
-            // Attempt an insert with a requested IP address
-            Some(ip) => {
-                interface.subnet.contains(ip)?;
-                let row = NetworkInterface {
-                    identity: interface.identity,
-                    instance_id: interface.instance_id,
-                    vpc_id: interface.vpc_id,
-                    subnet_id: interface.subnet.id(),
-                    mac: interface.mac,
-                    ip: ip.into(),
-                };
-                diesel::insert_into(dsl::network_interface)
-                    .values(row)
-                    .returning(NetworkInterface::as_returning())
-                    .get_result_async(self.pool())
-                    .await
-                    .map_err(|e| {
-                        public_error_from_diesel_pool(
-                            e,
-                            ErrorHandler::Conflict(
-                                ResourceType::NetworkInterface,
-                                name.as_str(),
-                            ),
-                        )
-                    })
-            }
-            // Insert and allocate an IP address
-            None => {
-                let allocation_query = AllocateIpQuery {
-                    block: ipnetwork::IpNetwork::V4(
-                        interface.subnet.ipv4_block.0 .0,
+    /// Delete all network interfaces attached to the given instance.
+    // NOTE: This is mostly useful in the context of sagas, but might be helpful
+    // in other situations, such as moving an instance between VPC Subnets.
+    pub async fn instance_delete_all_network_interfaces(
+        &self,
+        instance_id: &Uuid,
+    ) -> DeleteResult {
+        use db::schema::network_interface::dsl;
+        let now = Utc::now();
+        diesel::update(dsl::network_interface)
+            .filter(dsl::instance_id.eq(*instance_id))
+            .filter(dsl::time_deleted.is_null())
+            .set(dsl::time_deleted.eq(now))
+            .execute_async(self.pool())
+            .await
+            .map_err(|e| {
+                public_error_from_diesel_pool(
+                    e,
+                    ErrorHandler::NotFoundByLookup(
+                        ResourceType::Instance,
+                        LookupType::ById(*instance_id),
                     ),
-                    interface,
-                    now: Utc::now(),
-                };
-                diesel::insert_into(dsl::network_interface)
-                    .values(allocation_query)
-                    .returning(NetworkInterface::as_returning())
-                    .get_result_async(self.pool())
-                    .await
-                    .map_err(|e| {
-                        if let PoolError::Connection(ConnectionError::Query(
-                            diesel::result::Error::NotFound,
-                        )) = e
-                        {
-                            Error::InvalidRequest {
-                                message: "no available IP addresses"
-                                    .to_string(),
-                            }
-                        } else {
-                            public_error_from_diesel_pool(
-                                e,
-                                ErrorHandler::Conflict(
-                                    ResourceType::NetworkInterface,
-                                    name.as_str(),
-                                ),
-                            )
-                        }
-                    })
-            }
-        }
+                )
+            })?;
+        Ok(())
     }
 
     pub async fn instance_delete_network_interface(
         &self,
-        network_interface_id: &Uuid,
+        interface_id: &Uuid,
     ) -> DeleteResult {
         use db::schema::network_interface::dsl;
+        let now = Utc::now();
+        let result = diesel::update(dsl::network_interface)
+            .filter(dsl::id.eq(*interface_id))
+            .filter(dsl::time_deleted.is_null())
+            .set((dsl::time_deleted.eq(now),))
+            .check_if_exists::<db::model::NetworkInterface>(*interface_id)
+            .execute_and_check(self.pool())
+            .await
+            .map_err(|e| {
+                public_error_from_diesel_pool(
+                    e,
+                    ErrorHandler::NotFoundByLookup(
+                        ResourceType::NetworkInterface,
+                        LookupType::ById(*interface_id),
+                    ),
+                )
+            })?;
+        match result.status {
+            UpdateStatus::Updated => Ok(()),
+            UpdateStatus::NotUpdatedButExists => {
+                let interface = &result.found;
+                if interface.time_deleted().is_some() {
+                    // Already deleted
+                    Ok(())
+                } else {
+                    Err(Error::internal_error(&format!(
+                        "failed to delete network interface: {}",
+                        interface_id
+                    )))
+                }
+            }
+        }
+    }
+
+    pub async fn subnet_lookup_network_interface(
+        &self,
+        subnet_id: &Uuid,
+        interface_name: &Name,
+    ) -> LookupResult<db::model::NetworkInterface> {
+        use db::schema::network_interface::dsl;
+
+        dsl::network_interface
+            .filter(dsl::subnet_id.eq(*subnet_id))
+            .filter(dsl::time_deleted.is_null())
+            .filter(dsl::name.eq(interface_name.clone()))
+            .select(db::model::NetworkInterface::as_select())
+            .get_result_async(self.pool())
+            .await
+            .map_err(|e| {
+                public_error_from_diesel_pool(
+                    e,
+                    ErrorHandler::NotFoundByLookup(
+                        ResourceType::NetworkInterface,
+                        LookupType::ByName(interface_name.to_string()),
+                    ),
+                )
+            })
+    }
 
-        // TODO-correctness: Do not allow deleting interfaces on running
-        // instances until we support hotplug
+    /// List network interfaces associated with a given instance.
+    pub async fn instance_list_network_interfaces(
+        &self,
+        instance_id: &Uuid,
+        pagparams: &DataPageParams<'_, Name>,
+    ) -> ListResultVec<NetworkInterface> {
+        use db::schema::network_interface::dsl;
+        paginated(dsl::network_interface, dsl::name, &pagparams)
+            .filter(dsl::time_deleted.is_null())
+            .filter(dsl::instance_id.eq(*instance_id))
+            .select(NetworkInterface::as_select())
+            .load_async::<NetworkInterface>(self.pool())
+            .await
+            .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
+    }
 
-        let now = Utc::now();
-        diesel::update(dsl::network_interface)
+    /// Get a network interface by name attached to an instance
+    pub async fn instance_lookup_network_interface(
+        &self,
+        instance_id: &Uuid,
+        interface_name: &Name,
+    ) -> LookupResult<NetworkInterface> {
+        use db::schema::network_interface::dsl;
+        dsl::network_interface
+            .filter(dsl::instance_id.eq(*instance_id))
+            .filter(dsl::name.eq(interface_name.clone()))
             .filter(dsl::time_deleted.is_null())
-            .filter(dsl::id.eq(*network_interface_id))
-            .set(dsl::time_deleted.eq(now))
-            .returning(NetworkInterface::as_returning())
+            .select(NetworkInterface::as_select())
             .get_result_async(self.pool())
             .await
             .map_err(|e| {
                 public_error_from_diesel_pool(
                     e,
                     ErrorHandler::NotFoundByLookup(
                         ResourceType::NetworkInterface,
-                        LookupType::ById(*network_interface_id),
+                        LookupType::ByName(interface_name.to_string()),
                     ),
                 )
-            })?;
-        Ok(())
+            })
     }
 
     // Create a record for a new Oximeter instance