Skip to content

Instance network interfaces don't need to be paginated (#1009) #1164

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

Closed
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
3 changes: 1 addition & 2 deletions nexus/src/app/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,6 @@ impl super::Nexus {
organization_name: &Name,
project_name: &Name,
instance_name: &Name,
pagparams: &DataPageParams<'_, Name>,
) -> ListResultVec<db::model::NetworkInterface> {
let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore)
.organization_name(organization_name)
Expand All @@ -737,7 +736,7 @@ impl super::Nexus {
.lookup_for(authz::Action::ListChildren)
.await?;
self.db_datastore
.instance_list_network_interfaces(opctx, &authz_instance, pagparams)
.instance_list_network_interfaces(opctx, &authz_instance)
.await
}

Expand Down
8 changes: 4 additions & 4 deletions nexus/src/db/datastore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1932,16 +1932,16 @@ impl DataStore {
&self,
opctx: &OpContext,
authz_instance: &authz::Instance,
pagparams: &DataPageParams<'_, Name>,
) -> ListResultVec<NetworkInterface> {
opctx.authorize(authz::Action::ListChildren, authz_instance).await?;

use db::schema::network_interface::dsl;
paginated(dsl::network_interface, dsl::name, &pagparams)

dsl::network_interface
.filter(dsl::time_deleted.is_null())
.filter(dsl::instance_id.eq(authz_instance.id()))
.order(dsl::slot.asc())
.select(NetworkInterface::as_select())
.load_async::<NetworkInterface>(self.pool_authorized(opctx).await?)
.load_async(self.pool_authorized(opctx).await?)
.await
.map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
}
Expand Down
23 changes: 11 additions & 12 deletions nexus/src/external_api/http_entrypoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@
use super::{
console_api, params, views,
views::{
GlobalImage, IdentityProvider, Image, Organization, Project, Rack,
Role, Silo, Sled, Snapshot, SshKey, User, Vpc, VpcRouter, VpcSubnet,
GlobalImage, IdentityProvider, Image, NetworkInterfaces, Organization,
Project, Rack, Role, Silo, Sled, Snapshot, SshKey, User, Vpc,
VpcRouter, VpcSubnet,
},
};
use crate::authz;
Expand Down Expand Up @@ -1833,12 +1834,10 @@ async fn project_images_delete_image(
}]
async fn instance_network_interfaces_get(
rqctx: Arc<RequestContext<Arc<ServerContext>>>,
query_params: Query<PaginatedByName>,
path_params: Path<InstancePathParam>,
) -> Result<HttpResponseOk<ResultsPage<NetworkInterface>>, HttpError> {
) -> Result<HttpResponseOk<NetworkInterfaces>, HttpError> {
let apictx = rqctx.context();
let nexus = &apictx.nexus;
let query = query_params.into_inner();
let path = path_params.into_inner();
let organization_name = &path.organization_name;
let project_name = &path.project_name;
Expand All @@ -1851,14 +1850,14 @@ async fn instance_network_interfaces_get(
&organization_name,
&project_name,
&instance_name,
&data_page_params_for(&rqctx, &query)?
.map_name(|n| Name::ref_cast(n)),
)
.await?
.into_iter()
.map(|d| d.into())
.collect();
Ok(HttpResponseOk(ScanByName::results_page(&query, interfaces)?))
.await?;
Ok(HttpResponseOk(NetworkInterfaces {
interfaces: interfaces
.into_iter()
.map(|interface| interface.into())
.collect(),
}))
};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}
Expand Down
10 changes: 9 additions & 1 deletion nexus/src/external_api/views.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::db::model;
use api_identity::ObjectIdentity;
use omicron_common::api::external::{
ByteCount, Digest, IdentityMetadata, Ipv4Net, Ipv6Net, Name,
ObjectIdentity, RoleName,
NetworkInterface, ObjectIdentity, RoleName,
};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -419,3 +419,11 @@ impl From<model::SshKey> for SshKey {
}
}
}

// NETWORK INTERFACES

/// Collection of an [`Instance`]'s network interfaces
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
pub struct NetworkInterfaces {
pub interfaces: Vec<NetworkInterface>,
}
19 changes: 17 additions & 2 deletions nexus/test-utils/src/resource_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@ use dropshot::test_util::ClientTestContext;
use dropshot::HttpErrorResponseBody;
use dropshot::Method;
use http::StatusCode;
use omicron_common::api::external::ByteCount;
use omicron_common::api::external::Disk;
use omicron_common::api::external::IdentityMetadataCreateParams;
use omicron_common::api::external::Instance;
use omicron_common::api::external::InstanceCpuCount;
use omicron_common::api::external::{ByteCount, NetworkInterface};
use omicron_nexus::crucible_agent_client::types::State as RegionState;
use omicron_nexus::external_api::params;
use omicron_nexus::external_api::shared;
use omicron_nexus::external_api::shared::IdentityType;
use omicron_nexus::external_api::views::{
Organization, Project, Silo, Vpc, VpcRouter,
NetworkInterfaces, Organization, Project, Silo, Vpc, VpcRouter,
};
use omicron_sled_agent::sim::SledAgent;
use std::sync::Arc;
Expand Down Expand Up @@ -340,6 +340,21 @@ pub async fn project_get(
.expect("failed to parse Project")
}

pub async fn instance_network_interfaces_get(
client: &ClientTestContext,
interfaces_url: &str,
) -> Vec<NetworkInterface> {
let request: NetworkInterfaces =
NexusRequest::object_get(client, interfaces_url)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body()
.unwrap();
request.interfaces
}

pub struct DiskTest {
pub sled_agent: Arc<SledAgent>,
pub zpool_id: Uuid,
Expand Down
63 changes: 45 additions & 18 deletions nexus/tests/integration_tests/instances.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ use http::StatusCode;
use nexus_test_utils::http_testing::AuthnMode;
use nexus_test_utils::http_testing::NexusRequest;
use nexus_test_utils::http_testing::RequestBuilder;
use nexus_test_utils::resource_helpers::create_disk;
use nexus_test_utils::resource_helpers::objects_list_page_authz;
use nexus_test_utils::resource_helpers::DiskTest;
use nexus_test_utils::resource_helpers::{
create_disk, instance_network_interfaces_get,
};
use omicron_common::api::external::ByteCount;
use omicron_common::api::external::Disk;
use omicron_common::api::external::DiskState;
Expand Down Expand Up @@ -940,16 +942,12 @@ async fn test_instance_create_delete_network_interface(
"/organizations/{}/projects/{}/instances/{}/network-interfaces",
ORGANIZATION_NAME, PROJECT_NAME, instance.identity.name,
);
let interfaces = NexusRequest::iter_collection_authn::<NetworkInterface>(
client,
url_interfaces.as_str(),
"",
Some(100),
)
.await
.expect("Failed to get interfaces for instance");

// Get network interface(s) attached to the instance
let interfaces =
instance_network_interfaces_get(client, &url_interfaces).await;
assert!(
interfaces.all_items.is_empty(),
interfaces.is_empty(),
"Expected no network interfaces for instance"
);

Expand Down Expand Up @@ -1034,9 +1032,7 @@ async fn test_instance_create_delete_network_interface(

// Get all interfaces in one request.
let other_interfaces =
objects_list_page_authz::<NetworkInterface>(client, &url_interfaces)
.await
.items;
instance_network_interfaces_get(client, &url_interfaces).await;
for (iface0, iface1) in interfaces.iter().zip(other_interfaces) {
assert_eq!(iface0.identity.id, iface1.identity.id);
assert_eq!(iface0.vpc_id, iface1.vpc_id);
Expand Down Expand Up @@ -1164,7 +1160,7 @@ async fn test_instance_update_network_interfaces(
let instance_params = params::InstanceCreate {
identity: IdentityMetadataCreateParams {
name: Name::try_from(String::from("nic-update-test-inst")).unwrap(),
description: String::from("instance to test updatin nics"),
description: String::from("instance to test updating nics"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_gibibytes_u32(4),
Expand Down Expand Up @@ -1468,7 +1464,7 @@ async fn test_instance_update_network_interfaces(
// The now-secondary interface should be, well, secondary
assert!(
!new_secondary_iface.primary,
"The old primary interface should now be a seconary"
"The old primary interface should now be a secondary"
);

// Nothing else about the old primary should have changed
Expand All @@ -1492,9 +1488,7 @@ async fn test_instance_update_network_interfaces(
.await
.expect("Failed to delete original secondary interface");
let all_interfaces =
objects_list_page_authz::<NetworkInterface>(client, &url_interfaces)
.await
.items;
instance_network_interfaces_get(client, &url_interfaces).await;
assert_eq!(
all_interfaces.len(),
1,
Expand Down Expand Up @@ -2364,6 +2358,39 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) {
.unwrap();
}

// Test getting network interfaces attached to an instance
#[nexus_test]
async fn test_instance_get_network_interfaces(
cptestctx: &ControlPlaneTestContext,
) {
static INSTANCE_NAME: &str = "just-rainsticks";
let client = &cptestctx.external_client;
let path = &format!(
"/organizations/{}/projects/{}/instances/{}/network-interfaces",
ORGANIZATION_NAME, PROJECT_NAME, INSTANCE_NAME
);
create_org_and_project(client).await;

// Create instance with default network interface
let instance = create_instance(
&client,
ORGANIZATION_NAME,
PROJECT_NAME,
INSTANCE_NAME,
)
.await;

// Get network interface(s) attached to the instance
let interface = instance_network_interfaces_get(client, path).await;

assert_eq!(interface[0].identity.name, "net0");
assert_eq!(
interface[0].identity.description,
format!("default primary interface for {}", INSTANCE_NAME)
);
assert_eq!(interface[0].instance_id, instance.identity.id);
}

async fn instance_get(
client: &ClientTestContext,
instance_url: &str,
Expand Down
50 changes: 17 additions & 33 deletions openapi/nexus.json
Original file line number Diff line number Diff line change
Expand Up @@ -2075,36 +2075,6 @@
"summary": "List network interfaces attached to this instance.",
"operationId": "instance_network_interfaces_get",
"parameters": [
{
"in": "query",
"name": "limit",
"description": "Maximum number of items returned by a single call",
"schema": {
"nullable": true,
"type": "integer",
"format": "uint32",
"minimum": 1
},
"style": "form"
},
{
"in": "query",
"name": "page_token",
"description": "Token returned by previous call to retrieve the subsequent page",
"schema": {
"nullable": true,
"type": "string"
},
"style": "form"
},
{
"in": "query",
"name": "sort_by",
"schema": {
"$ref": "#/components/schemas/NameSortMode"
},
"style": "form"
},
{
"in": "path",
"name": "instance_name",
Expand Down Expand Up @@ -2139,7 +2109,7 @@
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/NetworkInterfaceResultsPage"
"$ref": "#/components/schemas/NetworkInterfaces"
}
}
}
Expand All @@ -2150,8 +2120,7 @@
"5XX": {
"$ref": "#/components/responses/Error"
}
},
"x-dropshot-pagination": true
}
},
"post": {
"tags": [
Expand Down Expand Up @@ -7234,6 +7203,21 @@
}
}
},
"NetworkInterfaces": {
"description": "Collection of an [`Instance`]'s network interfaces",
"type": "object",
"properties": {
"interfaces": {
"type": "array",
"items": {
"$ref": "#/components/schemas/NetworkInterface"
}
}
},
"required": [
"interfaces"
]
},
"Organization": {
"description": "Client view of an [`Organization`]",
"type": "object",
Expand Down