Add API endpoint for updating network interfaces (#1186)

* Add API endpoint for updating network interfaces

- Adds the HTTP endpoint to PUT updates to an instance's network
  interfaces
- Adds transaction-based method for applying these updates. The
  transaction is mainly necessary to correctly make a new primary
  interface, specifically marking the old primary as now secondary. It
  also helps with a potential race when checking if the instance being
  operated on is stopped. The interactive transaction is unfortunate,
  but appears required, since the entire operation of setting a new
  primary cannot be expressed as a single query.
- Adds tests, especially verifying making a new primary interface
- Adds PUT endpoint to auth tests

* Review feedback

common/src/sql/dbinit.sql

@@ -814,15 +814,20 @@ CREATE UNIQUE INDEX ON omicron.public.network_interface (
  * Index used to verify that an Instance's networking is contained
  * within a single VPC, and that all interfaces are in unique VPC
  * Subnets.
+ *
+ * This is also used to quickly find the primary interface for an
+ * instance, since we store the `is_primary` column. Such queries are
+ * mostly used when setting a new primary interface for an instance.
  */
 CREATE UNIQUE INDEX ON omicron.public.network_interface (
     instance_id,
     name
 )
-STORING (vpc_id, subnet_id)
+STORING (vpc_id, subnet_id, is_primary)
 WHERE
     time_deleted IS NULL;
 
+
 CREATE TYPE omicron.public.vpc_router_kind AS ENUM (
     'system',
     'custom'

nexus/src/app/instance.rs

@@ -751,6 +751,37 @@ impl super::Nexus {
         Ok(db_interface)
     }
 
+    /// Update a network interface for the given instance.
+    pub async fn network_interface_update(
+        &self,
+        opctx: &OpContext,
+        organization_name: &Name,
+        project_name: &Name,
+        instance_name: &Name,
+        interface_name: &Name,
+        updates: params::NetworkInterfaceUpdate,
+    ) -> UpdateResult<db::model::NetworkInterface> {
+        let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore)
+            .organization_name(organization_name)
+            .project_name(project_name)
+            .instance_name(instance_name)
+            .lookup_for(authz::Action::Modify)
+            .await?;
+        let (.., authz_interface) = LookupPath::new(opctx, &self.db_datastore)
+            .instance_id(authz_instance.id())
+            .network_interface_name(interface_name)
+            .lookup_for(authz::Action::Modify)
+            .await?;
+        self.db_datastore
+            .instance_update_network_interface(
+                opctx,
+                &authz_instance,
+                &authz_interface,
+                db::model::NetworkInterfaceUpdate::from(updates),
+            )
+            .await
+    }
+
     /// Delete a network interface from the provided instance.
     ///
     /// Note that the primary interface for an instance cannot be deleted if

nexus/src/db/datastore.rs

@@ -39,6 +39,8 @@ use crate::db::fixed_data::silo::DEFAULT_SILO;
 use crate::db::lookup::LookupPath;
 use crate::db::model::DatabaseString;
 use crate::db::model::IncompleteVpc;
+use crate::db::model::NetworkInterfaceUpdate;
+use crate::db::model::Vpc;
 use crate::db::queries::network_interface;
 use crate::db::queries::vpc::InsertVpcQuery;
 use crate::db::queries::vpc_subnet::FilterConflictingVpcSubnetRangesQuery;
@@ -56,7 +58,7 @@ use crate::db::{
         OrganizationUpdate, OximeterInfo, ProducerEndpoint, Project,
         ProjectUpdate, Rack, Region, RoleAssignment, RoleBuiltin, RouterRoute,
         RouterRouteUpdate, Service, Silo, SiloUser, Sled, SshKey,
-        UpdateAvailableArtifact, UserBuiltin, Volume, Vpc, VpcFirewallRule,
+        UpdateAvailableArtifact, UserBuiltin, Volume, VpcFirewallRule,
         VpcRouter, VpcRouterUpdate, VpcSubnet, VpcSubnetUpdate, VpcUpdate,
         Zpool,
     },
@@ -1944,6 +1946,128 @@ impl DataStore {
             .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
     }
 
+    /// Update a network interface associated with a given instance.
+    pub async fn instance_update_network_interface(
+        &self,
+        opctx: &OpContext,
+        authz_instance: &authz::Instance,
+        authz_interface: &authz::NetworkInterface,
+        updates: NetworkInterfaceUpdate,
+    ) -> UpdateResult<NetworkInterface> {
+        use crate::db::schema::network_interface::dsl;
+
+        // This database operation is surprisingly subtle. It's possible to
+        // express this in a single query, with multiple common-table
+        // expressions for the updated rows. For example, if we're setting a new
+        // primary interface, we need to set the `is_primary` column to false
+        // for the current primary, and then set it to true, along with any
+        // other updates, for the new primary.
+        //
+        // That's feasible, but there's a CRDB bug that affects some queries
+        // with multiple update statements. It's possible that this query isn't
+        // in that bucket, but we'll still avoid it for now. Instead, we'll bite
+        // the bullet and use a transaction.
+        //
+        // See https://github.com/oxidecomputer/omicron/issues/1204 for the
+        // issue tracking the work to move this into a CTE.
+
+        // Build up some of the queries first, outside the transaction.
+        //
+        // This selects the existing primary interface.
+        let instance_id = authz_instance.id();
+        let interface_id = authz_interface.id();
+        let find_primary_query = dsl::network_interface
+            .filter(dsl::instance_id.eq(instance_id))
+            .filter(dsl::is_primary.eq(true))
+            .filter(dsl::time_deleted.is_null())
+            .select(NetworkInterface::as_select());
+
+        // This returns the state of the associated instance.
+        let instance_query = db::schema::instance::dsl::instance
+            .filter(db::schema::instance::dsl::id.eq(instance_id))
+            .filter(db::schema::instance::dsl::time_deleted.is_null())
+            .select(Instance::as_select());
+        let stopped =
+            db::model::InstanceState::new(external::InstanceState::Stopped);
+
+        // This is the actual query to update the target interface.
+        let make_primary = matches!(updates.make_primary, Some(true));
+        let update_target_query = diesel::update(dsl::network_interface)
+            .filter(dsl::id.eq(interface_id))
+            .filter(dsl::time_deleted.is_null())
+            .set(updates)
+            .returning(NetworkInterface::as_returning());
+
+        // Errors returned from the below transactions.
+        #[derive(Debug)]
+        enum NetworkInterfaceUpdateError {
+            InstanceNotStopped,
+            FailedToUnsetPrimary(diesel::result::Error),
+        }
+        type TxnError = TransactionError<NetworkInterfaceUpdateError>;
+
+        let pool = self.pool_authorized(opctx).await?;
+        if make_primary {
+            pool.transaction(move |conn| {
+                let instance_state =
+                    instance_query.get_result(conn)?.runtime_state.state;
+                if instance_state != stopped {
+                    return Err(TxnError::CustomError(
+                        NetworkInterfaceUpdateError::InstanceNotStopped,
+                    ));
+                }
+
+                // First, get the primary interface
+                let primary_interface = find_primary_query.get_result(conn)?;
+
+                // If the target and primary are different, we need to toggle
+                // the primary into a secondary.
+                if primary_interface.identity.id != interface_id {
+                    if let Err(e) = diesel::update(dsl::network_interface)
+                        .filter(dsl::id.eq(primary_interface.identity.id))
+                        .filter(dsl::time_deleted.is_null())
+                        .set(dsl::is_primary.eq(false))
+                        .execute(conn)
+                    {
+                        return Err(TxnError::CustomError(
+                            NetworkInterfaceUpdateError::FailedToUnsetPrimary(
+                                e,
+                            ),
+                        ));
+                    }
+                }
+
+                // In any case, update the actual target
+                Ok(update_target_query.get_result(conn)?)
+            })
+        } else {
+            // In this case, we can just directly apply the updates. By
+            // construction, `updates.make_primary` is `None`, so nothing will
+            // be done there. The other columns always need to be updated, and
+            // we're only hitting a single row. Note that we still need to
+            // verify the instance is stopped.
+            pool.transaction(move |conn| {
+                let instance_state =
+                    instance_query.get_result(conn)?.runtime_state.state;
+                if instance_state != stopped {
+                    return Err(TxnError::CustomError(
+                        NetworkInterfaceUpdateError::InstanceNotStopped,
+                    ));
+                }
+                Ok(update_target_query.get_result(conn)?)
+            })
+        }
+        .await
+        .map_err(|e| match e {
+            TxnError::CustomError(
+                NetworkInterfaceUpdateError::InstanceNotStopped,
+            ) => Error::invalid_request(
+                "Instance must be stopped to update its network interfaces",
+            ),
+            _ => Error::internal_error(&format!("Transaction error: {:?}", e)),
+        })
+    }
+
     // Create a record for a new Oximeter instance
     pub async fn oximeter_create(
         &self,

nexus/src/db/model/network_interface.rs

@@ -4,8 +4,13 @@
 
 use super::{MacAddr, VpcSubnet};
 use crate::db::identity::Resource;
+use crate::db::model::Name;
 use crate::db::schema::network_interface;
+use crate::external_api::params;
+use chrono::DateTime;
+use chrono::Utc;
 use db_macros::Resource;
+use diesel::AsChangeset;
 use omicron_common::api::external;
 use uuid::Uuid;
 
@@ -71,3 +76,26 @@ impl IncompleteNetworkInterface {
         Ok(Self { identity, instance_id, subnet, vpc_id, ip })
     }
 }
+
+/// Describes a set of updates for the [`NetworkInterface`] model.
+#[derive(AsChangeset, Debug, Clone)]
+#[diesel(table_name = network_interface)]
+pub struct NetworkInterfaceUpdate {
+    pub name: Option<Name>,
+    pub description: Option<String>,
+    pub time_modified: DateTime<Utc>,
+    #[diesel(column_name = is_primary)]
+    pub make_primary: Option<bool>,
+}
+
+impl From<params::NetworkInterfaceUpdate> for NetworkInterfaceUpdate {
+    fn from(params: params::NetworkInterfaceUpdate) -> Self {
+        let make_primary = if params.make_primary { Some(true) } else { None };
+        Self {
+            name: params.identity.name.map(|n| n.into()),
+            description: params.identity.description,
+            time_modified: Utc::now(),
+            make_primary,
+        }
+    }
+}

nexus/src/external_api/http_entrypoints.rs

@@ -155,6 +155,7 @@ pub fn external_api() -> NexusApiDescription {
         api.register(instance_network_interfaces_post)?;
         api.register(instance_network_interfaces_get)?;
         api.register(instance_network_interfaces_get_interface)?;
+        api.register(instance_network_interfaces_put_interface)?;
         api.register(instance_network_interfaces_delete_interface)?;
 
         api.register(vpc_routers_get)?;
@@ -1875,8 +1876,6 @@ 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}",
@@ -1942,6 +1941,42 @@ async fn instance_network_interfaces_get_interface(
     apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
 }
 
+/// Update information about an instance's network interface
+#[endpoint {
+    method = PUT,
+    path = "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/network-interfaces/{interface_name}",
+    tags = ["instances"],
+}]
+async fn instance_network_interfaces_put_interface(
+    rqctx: Arc<RequestContext<Arc<ServerContext>>>,
+    path_params: Path<NetworkInterfacePathParam>,
+    updated_iface: TypedBody<params::NetworkInterfaceUpdate>,
+) -> Result<HttpResponseOk<NetworkInterface>, HttpError> {
+    let apictx = rqctx.context();
+    let nexus = &apictx.nexus;
+    let path = path_params.into_inner();
+    let organization_name = &path.organization_name;
+    let project_name = &path.project_name;
+    let instance_name = &path.instance_name;
+    let interface_name = &path.interface_name;
+    let updated_iface = updated_iface.into_inner();
+    let handler = async {
+        let opctx = OpContext::for_external_api(&rqctx).await?;
+        let interface = nexus
+            .network_interface_update(
+                &opctx,
+                organization_name,
+                project_name,
+                instance_name,
+                interface_name,
+                updated_iface,
+            )
+            .await?;
+        Ok(HttpResponseOk(NetworkInterface::from(interface)))
+    };
+    apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
+}
+
 // Snapshots
 
 /// List snapshots in a project.

nexus/src/external_api/params.rs

@@ -284,6 +284,33 @@ pub struct NetworkInterfaceCreate {
     pub ip: Option<IpAddr>,
 }
 
+/// Parameters for updating a
+/// [`NetworkInterface`](omicron_common::api::external::NetworkInterface).
+///
+/// Note that modifying IP addresses for an interface is not yet supported, a
+/// new interface must be created instead.
+#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
+pub struct NetworkInterfaceUpdate {
+    #[serde(flatten)]
+    pub identity: IdentityMetadataUpdateParams,
+
+    /// Make a secondary interface the instance's primary interface.
+    ///
+    /// If applied to a secondary interface, that interface will become the
+    /// primary on the next reboot of the instance. Note that this may have
+    /// implications for routing between instances, as the new primary interface
+    /// will be on a distinct subnet from the previous primary interface.
+    ///
+    /// Note that this can only be used to select a new primary interface for an
+    /// instance. Requests to change the primary interface into a secondary will
+    /// return an error.
+    // TODO-completeness TODO-docs: When we get there, this should note that a
+    // change in the primary interface will result in changes to the DNS records
+    // for the instance, though not the name.
+    #[serde(default)]
+    pub make_primary: bool,
+}
+
 // INSTANCES
 
 pub const MIN_MEMORY_SIZE_BYTES: u32 = 1 << 30; // 1 GiB

nexus/tests/integration_tests/endpoints.rs

@@ -225,6 +225,15 @@ lazy_static! {
             subnet_name: DEMO_VPC_SUBNET_NAME.clone(),
             ip: None,
         };
+    pub static ref DEMO_INSTANCE_NIC_PUT: params::NetworkInterfaceUpdate = {
+        params::NetworkInterfaceUpdate {
+            identity: IdentityMetadataUpdateParams {
+                name: None,
+                description: Some(String::from("an updated description")),
+            },
+            make_primary: false,
+        }
+    };
 }
 
 // Separate lazy_static! blocks to avoid hitting some recursion limit when compiling
@@ -875,6 +884,9 @@ lazy_static! {
             allowed_methods: vec![
                 AllowedMethod::Get,
                 AllowedMethod::Delete,
+                AllowedMethod::Put(
+                    serde_json::to_value(&*DEMO_INSTANCE_NIC_PUT).unwrap()
+                ),
             ],
         },