Skip to content

Add API endpoint for updating network interfaces #1186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion common/src/sql/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
31 changes: 31 additions & 0 deletions nexus/src/app/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
126 changes: 125 additions & 1 deletion nexus/src/db/datastore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
},
Expand Down Expand Up @@ -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,
Expand Down
28 changes: 28 additions & 0 deletions nexus/src/db/model/network_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
}
}
}
39 changes: 37 additions & 2 deletions nexus/src/external_api/http_entrypoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand Down Expand Up @@ -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}",
Expand Down Expand Up @@ -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.
Expand Down
27 changes: 27 additions & 0 deletions nexus/src/external_api/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions nexus/tests/integration_tests/endpoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -875,6 +884,9 @@ lazy_static! {
allowed_methods: vec![
AllowedMethod::Get,
AllowedMethod::Delete,
AllowedMethod::Put(
serde_json::to_value(&*DEMO_INSTANCE_NIC_PUT).unwrap()
),
],
},

Expand Down
Loading