From c563bfe509f7b2105ae33641aaddf042fa8f7110 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Mon, 6 Jun 2022 17:18:35 -0400 Subject: [PATCH 1/5] Instance network interfaces don't need to be paginated (#1009) Removed page params from entrypoint, app, and db calls. --- nexus/src/app/instance.rs | 3 +-- nexus/src/db/datastore.rs | 10 +++++----- nexus/src/external_api/http_entrypoints.rs | 2 -- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 6e3228049c1..e1e77d9e08f 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -692,7 +692,6 @@ impl super::Nexus { organization_name: &Name, project_name: &Name, instance_name: &Name, - pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore) .organization_name(organization_name) @@ -701,7 +700,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 } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 2898d0cbb72..b34496bd760 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1750,16 +1750,16 @@ impl DataStore { &self, opctx: &OpContext, authz_instance: &authz::Instance, - pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { 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())) + .filter(dsl::vpc_id.eq(authz_instance.id())) + .order(dsl::name.asc()) .select(NetworkInterface::as_select()) - .load_async::(self.pool_authorized(opctx).await?) + .load_async(self.pool_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index e3c86717fdc..6d774867b9d 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1704,8 +1704,6 @@ 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() From 42ef4b87b5e5f3d711b59986059de878283db62b Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Tue, 7 Jun 2022 14:27:07 -0400 Subject: [PATCH 2/5] Sort interfaces by slot in ascending order. --- nexus/src/db/datastore.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index b34496bd760..2e4effeee07 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1756,8 +1756,8 @@ impl DataStore { dsl::network_interface .filter(dsl::time_deleted.is_null()) - .filter(dsl::vpc_id.eq(authz_instance.id())) - .order(dsl::name.asc()) + .filter(dsl::instance_id.eq(authz_instance.id())) + .order(dsl::slot.asc()) .select(NetworkInterface::as_select()) .load_async(self.pool_authorized(opctx).await?) .await From 99c01c5e2be868fc6280092317cf7985f8271f60 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Mon, 13 Jun 2022 10:22:00 -0400 Subject: [PATCH 3/5] - Create NetworkInterfaces view - Modify instance_network_interfaces_get to return NetworkInterfaces --- nexus/src/external_api/http_entrypoints.rs | 20 ++++++++++---------- nexus/src/external_api/views.rs | 10 +++++++++- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 6d774867b9d..0ca1c7e412c 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7,8 +7,8 @@ use super::{ console_api, params, views::{ - GlobalImage, Image, Organization, Project, Rack, Role, Silo, Sled, - Snapshot, SshKey, User, Vpc, VpcRouter, VpcSubnet, + GlobalImage, Image, NetworkInterfaces, Organization, Project, Rack, + Role, Silo, Sled, Snapshot, SshKey, User, Vpc, VpcRouter, VpcSubnet, }, }; use crate::authz; @@ -1686,12 +1686,10 @@ async fn project_images_delete_image( }] async fn instance_network_interfaces_get( rqctx: Arc>>, - query_params: Query, path_params: Path, -) -> Result>, HttpError> { +) -> Result, 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; @@ -1705,11 +1703,13 @@ async fn instance_network_interfaces_get( &project_name, &instance_name, ) - .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 } diff --git a/nexus/src/external_api/views.rs b/nexus/src/external_api/views.rs index 2a4805315f9..cb4704ee5d5 100644 --- a/nexus/src/external_api/views.rs +++ b/nexus/src/external_api/views.rs @@ -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}; @@ -345,3 +345,11 @@ impl From for SshKey { } } } + +// NETWORK INTERFACES + +/// Collection of an [`Instance`]'s network interfaces +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct NetworkInterfaces { + pub interfaces: Vec, +} From a7d8d13439e19a0dcd1ee3f2bbea0452221064b7 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Wed, 22 Jun 2022 10:42:17 -0400 Subject: [PATCH 4/5] Solve merge conflict Add test to list interfaces attached to an instance --- nexus/tests/integration_tests/instances.rs | 617 ++++++++++++++++++++- 1 file changed, 605 insertions(+), 12 deletions(-) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 872fb9247d5..54b8dda322f 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -16,6 +16,7 @@ use omicron_common::api::external::ByteCount; use omicron_common::api::external::Disk; use omicron_common::api::external::DiskState; use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::Instance; use omicron_common::api::external::InstanceCpuCount; use omicron_common::api::external::InstanceState; @@ -38,10 +39,25 @@ use nexus_test_utils::resource_helpers::{ }; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; +use omicron_nexus::external_api::views::NetworkInterfaces; static ORGANIZATION_NAME: &str = "test-org"; static PROJECT_NAME: &str = "springfield-squidport"; +fn get_project_url() -> String { + format!("/organizations/{}/projects/{}", ORGANIZATION_NAME, PROJECT_NAME) +} + +fn get_instances_url() -> String { + format!("{}/instances", get_project_url()) +} + +async fn create_org_and_project(client: &ClientTestContext) -> Uuid { + create_organization(&client, ORGANIZATION_NAME).await; + let project = create_project(client, ORGANIZATION_NAME, PROJECT_NAME).await; + project.identity.id +} + #[nexus_test] async fn test_instances_access_before_create_returns_not_found( cptestctx: &ControlPlaneTestContext, @@ -128,7 +144,7 @@ async fn test_instances_create_reboot_halt( let InstanceCpuCount(nfoundcpus) = instance.ncpus; // These particulars are hardcoded in create_instance(). assert_eq!(nfoundcpus, 4); - assert_eq!(instance.memory.to_whole_mebibytes(), 256); + assert_eq!(instance.memory.to_whole_gibibytes(), 1); assert_eq!(instance.hostname, "the_host"); assert_eq!(instance.runtime.run_state, InstanceState::Starting); @@ -565,7 +581,7 @@ async fn test_instance_create_saga_removes_instance_database_record( description: String::from("instance to test saga unwind"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("inst"), user_data: vec![], network_interfaces: interface_params.clone(), @@ -587,7 +603,7 @@ async fn test_instance_create_saga_removes_instance_database_record( description: String::from("instance to test saga unwind 2"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("inst2"), user_data: vec![], network_interfaces: interface_params, @@ -673,7 +689,7 @@ async fn test_instance_with_single_explicit_ip_address( description: String::from("instance to test multiple nics"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nic-test"), user_data: vec![], network_interfaces: interface_params, @@ -790,7 +806,7 @@ async fn test_instance_with_new_custom_network_interfaces( description: String::from("instance to test multiple nics"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nic-test"), user_data: vec![], network_interfaces: interface_params, @@ -904,7 +920,7 @@ async fn test_instance_create_delete_network_interface( description: String::from("instance to test attaching new nic"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nic-test"), user_data: vec![], network_interfaces: params::InstanceNetworkInterfaceAttachment::None, @@ -1112,6 +1128,364 @@ async fn test_instance_create_delete_network_interface( ); } +#[nexus_test] +async fn test_instance_update_network_interfaces( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; + + // Create test organization and project + create_organization(&client, ORGANIZATION_NAME).await; + let url_instances = format!( + "/organizations/{}/projects/{}/instances", + ORGANIZATION_NAME, PROJECT_NAME + ); + let _ = create_project(&client, ORGANIZATION_NAME, PROJECT_NAME).await; + + // Create the VPC Subnet for the secondary interface + let secondary_subnet = params::VpcSubnetCreate { + identity: IdentityMetadataCreateParams { + name: Name::try_from(String::from("secondary")).unwrap(), + description: String::from("A secondary VPC subnet"), + }, + ipv4_block: Ipv4Net("172.31.0.0/24".parse().unwrap()), + ipv6_block: None, + }; + let url_vpc_subnets = format!( + "/organizations/{}/projects/{}/vpcs/{}/subnets", + ORGANIZATION_NAME, PROJECT_NAME, "default", + ); + let _response = + NexusRequest::objects_post(client, &url_vpc_subnets, &secondary_subnet) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create secondary VPC Subnet"); + + // Create an instance with no 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"), + }, + ncpus: InstanceCpuCount::try_from(2).unwrap(), + memory: ByteCount::from_gibibytes_u32(4), + hostname: String::from("nic-test"), + user_data: vec![], + network_interfaces: params::InstanceNetworkInterfaceAttachment::None, + disks: vec![], + }; + let response = + NexusRequest::objects_post(client, &url_instances, &instance_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create instance with two network interfaces"); + let instance = response.parsed_body::().unwrap(); + let url_instance = + format!("{}/{}", url_instances, instance.identity.name.as_str()); + let url_interfaces = format!( + "/organizations/{}/projects/{}/instances/{}/network-interfaces", + ORGANIZATION_NAME, PROJECT_NAME, instance.identity.name, + ); + + // Parameters for each interface to try to modify. + 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()), + }, + params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: "if1".parse().unwrap(), + description: String::from("a new nic"), + }, + vpc_name: "default".parse().unwrap(), + subnet_name: secondary_subnet.identity.name.clone(), + ip: Some("172.31.0.11".parse().unwrap()), + }, + ]; + + // Stop the instance + let instance = + instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + instance_simulate(nexus, &instance.identity.id).await; + + // Create the first interface on the instance. + let primary_iface = NexusRequest::objects_post( + client, + url_interfaces.as_str(), + &if_params[0], + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create network interface on stopped instance") + .parsed_body::() + .unwrap(); + assert_eq!(primary_iface.identity.name, if_params[0].identity.name); + assert_eq!(primary_iface.ip, if_params[0].ip.unwrap()); + assert!(primary_iface.primary, "The first interface should be primary"); + + // Restart the instance, to ensure we can only modify things when it's + // stopped. + let instance = + instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_simulate(nexus, &instance.identity.id).await; + + // We'll change the interface's name and description + let new_name = Name::try_from(String::from("new-if0")).unwrap(); + let new_description = String::from("new description"); + let updates = params::NetworkInterfaceUpdate { + identity: IdentityMetadataUpdateParams { + name: Some(new_name.clone()), + description: Some(new_description.clone()), + }, + make_primary: false, + }; + + // Verify we fail to update the NIC when the instance is running + // + // NOTE: Need to use RequestBuilder manually because `expect_failure` does + // not allow setting the body. + let url_interface = + format!("{}/{}", url_interfaces, primary_iface.identity.name.as_str()); + let builder = + RequestBuilder::new(client, http::Method::PUT, url_interface.as_str()) + .body(Some(&updates)) + .expect_status(Some(http::StatusCode::BAD_REQUEST)); + let err = NexusRequest::new(builder) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Should not be able to update network interface on running instance") + .parsed_body::() + .expect("Failed to parse error response body"); + assert_eq!( + err.message, + "Instance must be stopped to update its network interfaces", + "Expected an InvalidRequest response when modifying an interface on a running instance" + ); + + // Stop the instance again, and now verify that the update works. + let instance = + instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + instance_simulate(nexus, &instance.identity.id).await; + let updated_primary_iface = NexusRequest::object_put( + client, + format!("{}/{}", url_interfaces.as_str(), primary_iface.identity.name) + .as_str(), + Some(&updates), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to update an interface") + .parsed_body::() + .unwrap(); + + // Verify the modifications have taken effect, updating the name, + // description, and modification time. + assert_eq!(updated_primary_iface.identity.name, new_name); + assert_eq!(updated_primary_iface.identity.description, new_description); + assert!(updated_primary_iface.primary); + + // Helper to check that most attributes are unchanged when updating an + // interface, and that the modification time for the new is later than the + // old. + let verify_unchanged_attributes = + |original_iface: &NetworkInterface, new_iface: &NetworkInterface| { + assert_eq!( + original_iface.identity.time_created, + new_iface.identity.time_created + ); + assert!( + original_iface.identity.time_modified + < new_iface.identity.time_modified + ); + assert_eq!(original_iface.ip, new_iface.ip); + assert_eq!(original_iface.mac, new_iface.mac); + assert_eq!(original_iface.subnet_id, new_iface.subnet_id); + assert_eq!(original_iface.vpc_id, new_iface.vpc_id); + assert_eq!(original_iface.instance_id, new_iface.instance_id); + }; + verify_unchanged_attributes(&primary_iface, &updated_primary_iface); + + // Try with the same request again, but this time only changing + // `make_primary`. This should have no effect. + let updates = params::NetworkInterfaceUpdate { + identity: IdentityMetadataUpdateParams { + name: None, + description: None, + }, + make_primary: true, + }; + let updated_primary_iface1 = NexusRequest::object_put( + client, + format!( + "{}/{}", + url_interfaces.as_str(), + updated_primary_iface.identity.name + ) + .as_str(), + Some(&updates), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to update an interface") + .parsed_body::() + .unwrap(); + + // Everything should still be the same, except the modification time. + assert_eq!( + updated_primary_iface.identity.name, + updated_primary_iface1.identity.name + ); + assert_eq!( + updated_primary_iface.identity.description, + updated_primary_iface1.identity.description + ); + assert_eq!(updated_primary_iface.primary, updated_primary_iface1.primary); + verify_unchanged_attributes( + &updated_primary_iface, + &updated_primary_iface1, + ); + + // Add a secondary interface to the instance. We'll use this to check + // behavior related to making a new primary interface for the instance. + // Create the first interface on the instance. + let secondary_iface = NexusRequest::objects_post( + client, + url_interfaces.as_str(), + &if_params[1], + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to create network interface on stopped instance") + .parsed_body::() + .unwrap(); + assert_eq!(secondary_iface.identity.name, if_params[1].identity.name); + assert_eq!(secondary_iface.ip, if_params[1].ip.unwrap()); + assert!( + !secondary_iface.primary, + "Only the first interface should be primary" + ); + + // Restart the instance, and verify that we can't update either interface. + let instance = + instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_simulate(nexus, &instance.identity.id).await; + + for if_name in + [&updated_primary_iface.identity.name, &secondary_iface.identity.name] + { + let url_interface = format!("{}/{}", url_interfaces, if_name.as_str()); + let builder = RequestBuilder::new( + client, + http::Method::PUT, + url_interface.as_str(), + ) + .body(Some(&updates)) + .expect_status(Some(http::StatusCode::BAD_REQUEST)); + let err = NexusRequest::new(builder) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Should not be able to update network interface on running instance") + .parsed_body::() + .expect("Failed to parse error response body"); + assert_eq!( + err.message, + "Instance must be stopped to update its network interfaces", + "Expected an InvalidRequest response when modifying an interface on a running instance" + ); + } + + // Stop the instance again. + let instance = + instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + instance_simulate(nexus, &instance.identity.id).await; + + // Verify that we can set the secondary as the new primary, and that nothing + // else changes about the NICs. + let updates = params::NetworkInterfaceUpdate { + identity: IdentityMetadataUpdateParams { + name: None, + description: None, + }, + make_primary: true, + }; + let new_primary_iface = NexusRequest::object_put( + client, + format!( + "{}/{}", + url_interfaces.as_str(), + secondary_iface.identity.name + ) + .as_str(), + Some(&updates), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to update an interface") + .parsed_body::() + .unwrap(); + + // It should now be the primary and have an updated modification time + assert!(new_primary_iface.primary, "Failed to set the new primary"); + + // Nothing else about the new primary should have changed + assert_eq!(new_primary_iface.identity.name, secondary_iface.identity.name); + assert_eq!( + new_primary_iface.identity.description, + secondary_iface.identity.description + ); + verify_unchanged_attributes(&secondary_iface, &new_primary_iface); + + // Get the newly-made secondary interface to test + let new_secondary_iface = NexusRequest::object_get( + client, + format!( + "{}/{}", + url_interfaces.as_str(), + updated_primary_iface.identity.name + ) + .as_str(), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to get the old primary / new secondary interface") + .parsed_body::() + .unwrap(); + + // The now-secondary interface should be, well, secondary + assert!( + !new_secondary_iface.primary, + "The old primary interface should now be a seconary" + ); + + // Nothing else about the old primary should have changed + assert_eq!( + new_secondary_iface.identity.name, + updated_primary_iface.identity.name + ); + assert_eq!( + new_secondary_iface.identity.description, + updated_primary_iface.identity.description + ); + verify_unchanged_attributes(&updated_primary_iface, &new_secondary_iface); +} + /// This test specifically creates two NICs, the second of which will fail the /// saga on purpose, since its IP address is the same. This is to verify that /// the initial NIC is also deleted. @@ -1162,7 +1536,7 @@ async fn test_instance_with_multiple_nics_unwinds_completely( description: String::from("instance to test multiple bad nics"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nic-test"), user_data: vec![], network_interfaces: interface_params, @@ -1236,7 +1610,7 @@ async fn test_attach_one_disk_to_instance(cptestctx: &ControlPlaneTestContext) { description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nfs"), user_data: vec![], network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, @@ -1332,7 +1706,7 @@ async fn test_attach_eight_disks_to_instance( description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nfs"), user_data: vec![], network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, @@ -1438,7 +1812,7 @@ async fn test_cannot_attach_nine_disks_to_instance( description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nfs"), user_data: vec![], network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, @@ -1565,7 +1939,7 @@ async fn test_cannot_attach_faulted_disks(cptestctx: &ControlPlaneTestContext) { description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nfs"), user_data: vec![], network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, @@ -1677,7 +2051,7 @@ async fn test_disks_detached_when_instance_destroyed( description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), - memory: ByteCount::from_mebibytes_u32(4), + memory: ByteCount::from_gibibytes_u32(4), hostname: String::from("nfs"), user_data: vec![], network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, @@ -1772,6 +2146,225 @@ async fn test_disks_detached_when_instance_destroyed( } } +// Tests that an instance is rejected if the memory is less than +// MIN_MEMORY_SIZE_BYTES +#[nexus_test] +async fn test_instances_memory_rejected_less_than_min_memory_size( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + create_org_and_project(client).await; + + // Attempt to create the instance, observe a server error. + let instances_url = get_instances_url(); + let instance_name = "just-rainsticks"; + let instance = params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: instance_name.parse().unwrap(), + description: format!("instance {:?}", &instance_name), + }, + ncpus: InstanceCpuCount(1), + memory: ByteCount::from(params::MIN_MEMORY_SIZE_BYTES / 2), + hostname: String::from("inst"), + user_data: + b"#cloud-config\nsystem_info:\n default_user:\n name: oxide" + .to_vec(), + network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, + disks: vec![], + }; + + let error = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &instances_url) + .body(Some(&instance)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(); + + assert_eq!( + error.message, + format!( + "unsupported value for \"size\": memory must be at least {}", + ByteCount::from(params::MIN_MEMORY_SIZE_BYTES) + ), + ); +} + +// Test that an instance is rejected if memory is not divisible by +// MIN_MEMORY_SIZE +#[nexus_test] +async fn test_instances_memory_not_divisible_by_min_memory_size( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + create_org_and_project(client).await; + + // Attempt to create the instance, observe a server error. + let instances_url = get_instances_url(); + let instance_name = "just-rainsticks"; + let instance = params::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: instance_name.parse().unwrap(), + description: format!("instance {:?}", &instance_name), + }, + ncpus: InstanceCpuCount(1), + memory: ByteCount::from(1024 * 1024 * 1024 + 300), + hostname: String::from("inst"), + user_data: + b"#cloud-config\nsystem_info:\n default_user:\n name: oxide" + .to_vec(), + network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, + disks: vec![], + }; + + let error = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &instances_url) + .body(Some(&instance)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(); + + assert_eq!( + error.message, + format!( + "unsupported value for \"size\": memory must be divisible by {}", + ByteCount::from(params::MIN_MEMORY_SIZE_BYTES) + ), + ); +} + +#[nexus_test] +async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let apictx = &cptestctx.server.apictx; + let nexus = &apictx.nexus; + + // Create a project that we'll use for testing. + create_organization(&client, ORGANIZATION_NAME).await; + let url_instances = format!( + "/organizations/{}/projects/{}/instances", + ORGANIZATION_NAME, PROJECT_NAME + ); + let _ = create_project(&client, ORGANIZATION_NAME, PROJECT_NAME).await; + + // Make sure we get a 404 if we try to access the serial console before creation. + let instance_serial_url = + format!("{}/kris-picks/serial?from_start=0", url_instances); + let error: HttpErrorResponseBody = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + &instance_serial_url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!(error.message, "not found: instance with name \"kris-picks\""); + + // Create an instance. + let instance_url = format!("{}/kris-picks", url_instances); + let instance = + create_instance(client, ORGANIZATION_NAME, PROJECT_NAME, "kris-picks") + .await; + + // Now, simulate completion of instance boot and check the state reported. + instance_simulate(nexus, &instance.identity.id).await; + let instance_next = instance_get(&client, &instance_url).await; + identity_eq(&instance.identity, &instance_next.identity); + assert_eq!(instance_next.runtime.run_state, InstanceState::Running); + assert!( + instance_next.runtime.time_run_state_updated + > instance.runtime.time_run_state_updated + ); + + let serial_data: params::InstanceSerialConsoleData = + NexusRequest::object_get(client, &instance_serial_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make request") + .parsed_body() + .unwrap(); + + // FIXME: this is not necessarily going to always be the sled-agent-sim! + // when it's reasonable to boot arbitrary images, perhaps we simply use one + // that outputs something predictable like this + let expected = "This is simulated serial console output for ".as_bytes(); + assert_eq!(&serial_data.data[..expected.len()], expected); + + // Request a halt and verify both the immediate state and the finished state. + let instance = instance_next; + let instance_next = + instance_post(&client, &instance_url, InstanceOp::Stop).await; + assert_eq!(instance_next.runtime.run_state, InstanceState::Stopping); + assert!( + instance_next.runtime.time_run_state_updated + > instance.runtime.time_run_state_updated + ); + + let instance = instance_next; + instance_simulate(nexus, &instance.identity.id).await; + let instance_next = instance_get(&client, &instance_url).await; + assert_eq!(instance_next.runtime.run_state, InstanceState::Stopped); + assert!( + instance_next.runtime.time_run_state_updated + > instance.runtime.time_run_state_updated + ); + + // Delete the instance. + NexusRequest::object_delete(client, &instance_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); +} + +// Test that listing the network interfaces attached to an instance do not +// accept pagparams +#[nexus_test] +async fn test_instance_list_network_interfaces_declines_pagparams( + cptestctx: &ControlPlaneTestContext +) { + let client = &cptestctx.external_client; + static INSTANCE_NAME: &str = "just-rainsticks"; + 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 request: NetworkInterfaces = NexusRequest::object_get(client, path) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + + assert_eq!(true, true); +} + async fn instance_get( client: &ClientTestContext, instance_url: &str, From cb7afca2559032d6b2024fa05db886044f43b09b Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Thu, 23 Jun 2022 10:21:00 -0400 Subject: [PATCH 5/5] - Create helper function to get network interfaces - Create test for instance_network_interfaces_get - Replace object_get with helper function across tests - Fix typos --- nexus/test-utils/src/resource_helpers.rs | 19 ++++++- nexus/tests/integration_tests/instances.rs | 63 +++++++++------------- openapi/nexus.json | 50 ++++++----------- 3 files changed, 60 insertions(+), 72 deletions(-) diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 9aeafee445f..985e72b4325 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -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; @@ -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 { + 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, pub zpool_id: Uuid, diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 3f6d74b4c4c..33e2f0b2c95 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -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; @@ -39,7 +41,6 @@ use nexus_test_utils::resource_helpers::{ }; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; -use omicron_nexus::external_api::views::NetworkInterfaces; static ORGANIZATION_NAME: &str = "test-org"; static PROJECT_NAME: &str = "springfield-squidport"; @@ -941,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::( - 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" ); @@ -1035,9 +1032,7 @@ async fn test_instance_create_delete_network_interface( // Get all interfaces in one request. let other_interfaces = - objects_list_page_authz::(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); @@ -1165,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), @@ -1469,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 @@ -1493,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::(client, &url_interfaces) - .await - .items; + instance_network_interfaces_get(client, &url_interfaces).await; assert_eq!( all_interfaces.len(), 1, @@ -2365,15 +2358,13 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { .unwrap(); } -<<<<<<< HEAD -// Test that listing the network interfaces attached to an instance do not -// accept pagparams +// Test getting network interfaces attached to an instance #[nexus_test] -async fn test_instance_list_network_interfaces_declines_pagparams( - cptestctx: &ControlPlaneTestContext +async fn test_instance_get_network_interfaces( + cptestctx: &ControlPlaneTestContext, ) { - let client = &cptestctx.external_client; 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 @@ -2385,23 +2376,21 @@ async fn test_instance_list_network_interfaces_declines_pagparams( &client, ORGANIZATION_NAME, PROJECT_NAME, - INSTANCE_NAME - ).await; + INSTANCE_NAME, + ) + .await; // Get network interface(s) attached to the instance - let request: NetworkInterfaces = NexusRequest::object_get(client, path) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); + let interface = instance_network_interfaces_get(client, path).await; - assert_eq!(true, true); + 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); } -======= ->>>>>>> main async fn instance_get( client: &ClientTestContext, instance_url: &str, diff --git a/openapi/nexus.json b/openapi/nexus.json index 3a9c4e71060..d209003689f 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -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", @@ -2139,7 +2109,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/NetworkInterfaceResultsPage" + "$ref": "#/components/schemas/NetworkInterfaces" } } } @@ -2150,8 +2120,7 @@ "5XX": { "$ref": "#/components/responses/Error" } - }, - "x-dropshot-pagination": true + } }, "post": { "tags": [ @@ -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",