From 2f98c6c0700cd289648e33fabc46837d7b3a8fff Mon Sep 17 00:00:00 2001 From: bnaecker Date: Thu, 5 May 2022 16:57:11 -0700 Subject: [PATCH] Remove support for modifying IP subnets of a VPC Subnet (#1019) * Remove support for modifying IP subnets of a VPC Subnet Previously, the `VpcSubnetUpdate` type allowed users to specify a new IPv4 or IPv6 subnetwork for the VPC Subnet. That is now only supported by creating a new subnet. * Addressing review feedback: test cleanup, some comments on the tests --- nexus/src/db/model/vpc_subnet.rs | 4 - nexus/src/external_api/params.rs | 5 - nexus/src/lib.rs | 2 +- nexus/test-utils/src/resource_helpers.rs | 20 ++- nexus/tests/integration_tests/endpoints.rs | 2 - .../integration_tests/subnet_allocation.rs | 123 ++++++++++++------ nexus/tests/integration_tests/vpc_subnets.rs | 42 ------ openapi/nexus.json | 16 --- 8 files changed, 105 insertions(+), 109 deletions(-) diff --git a/nexus/src/db/model/vpc_subnet.rs b/nexus/src/db/model/vpc_subnet.rs index 3c78d9be45..bbb6f28eea 100644 --- a/nexus/src/db/model/vpc_subnet.rs +++ b/nexus/src/db/model/vpc_subnet.rs @@ -81,8 +81,6 @@ pub struct VpcSubnetUpdate { pub name: Option, pub description: Option, pub time_modified: DateTime, - pub ipv4_block: Option, - pub ipv6_block: Option, } impl From for VpcSubnetUpdate { @@ -91,8 +89,6 @@ impl From for VpcSubnetUpdate { name: params.identity.name.map(Name), description: params.identity.description, time_modified: Utc::now(), - ipv4_block: params.ipv4_block.map(Ipv4Net), - ipv6_block: params.ipv6_block.map(Ipv6Net), } } } diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 5df760fce9..32461aa323 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -251,11 +251,6 @@ pub struct VpcSubnetCreate { pub struct VpcSubnetUpdate { #[serde(flatten)] pub identity: IdentityMetadataUpdateParams, - // TODO-correctness: These need to be removed. Changing these is effectively - // creating a new resource, so we should require explicit - // deletion/recreation by the client. - pub ipv4_block: Option, - pub ipv6_block: Option, } // VPC ROUTERS diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 257db4cdff..a0e2826ee9 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -19,7 +19,7 @@ mod cidata; pub mod config; // Public for testing pub mod context; // Public for documentation examples pub mod db; // Public for documentation examples -mod defaults; +pub mod defaults; // Public for testing pub mod external_api; // Public for testing pub mod internal_api; // Public for testing pub mod nexus; // Public for documentation examples diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 6df0329a19..952e2a454a 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -146,6 +146,23 @@ pub async fn create_instance( organization_name: &str, project_name: &str, instance_name: &str, +) -> Instance { + create_instance_with_nics( + client, + organization_name, + project_name, + instance_name, + ¶ms::InstanceNetworkInterfaceAttachment::Default, + ) + .await +} + +pub async fn create_instance_with_nics( + client: &ClientTestContext, + organization_name: &str, + project_name: &str, + instance_name: &str, + nics: ¶ms::InstanceNetworkInterfaceAttachment, ) -> Instance { let url = format!( "/organizations/{}/projects/{}/instances", @@ -165,8 +182,7 @@ pub async fn create_instance( user_data: b"#cloud-config\nsystem_info:\n default_user:\n name: oxide" .to_vec(), - network_interfaces: - params::InstanceNetworkInterfaceAttachment::Default, + network_interfaces: nics.clone(), disks: vec![], }, ) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 3c3132168b..fab77682cc 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -533,8 +533,6 @@ lazy_static! { name: None, description: Some("different".to_string()) }, - ipv4_block: None, - ipv6_block: None, }).unwrap() ), AllowedMethod::Delete, diff --git a/nexus/tests/integration_tests/subnet_allocation.rs b/nexus/tests/integration_tests/subnet_allocation.rs index 8089660528..4b7a83e8f8 100644 --- a/nexus/tests/integration_tests/subnet_allocation.rs +++ b/nexus/tests/integration_tests/subnet_allocation.rs @@ -5,32 +5,45 @@ //! Tests that subnet allocation will successfully allocate the entire space of a //! subnet and error appropriately when the space is exhausted. +use dropshot::test_util::ClientTestContext; +use dropshot::HttpErrorResponseBody; use http::method::Method; 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_instance; +use nexus_test_utils::resource_helpers::create_instance_with_nics; use nexus_test_utils::resource_helpers::objects_list_page_authz; -use omicron_common::api::external::{ - ByteCount, IdentityMetadataCreateParams, IdentityMetadataUpdateParams, - InstanceCpuCount, Ipv4Net, NetworkInterface, -}; -use omicron_nexus::external_api::params; -use std::net::IpAddr; - -use dropshot::test_util::ClientTestContext; -use dropshot::HttpErrorResponseBody; - use nexus_test_utils::resource_helpers::{create_organization, create_project}; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; +use omicron_common::api::external::{ + ByteCount, IdentityMetadataCreateParams, InstanceCpuCount, Ipv4Net, + NetworkInterface, +}; +use omicron_nexus::defaults::NUM_INITIAL_RESERVED_IP_ADDRESSES; +use omicron_nexus::external_api::params; async fn create_instance_expect_failure( client: &ClientTestContext, url_instances: &String, name: &str, + subnet_name: &str, ) -> HttpErrorResponseBody { + let network_interfaces = + params::InstanceNetworkInterfaceAttachment::Create(vec![ + params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + // We're using the name of the instance purposefully, to + // avoid any naming conflicts on the interface. + name: name.parse().unwrap(), + description: String::from("description"), + }, + vpc_name: "default".parse().unwrap(), + subnet_name: subnet_name.parse().unwrap(), + ip: None, + }, + ]); let new_instance = params::InstanceCreate { identity: IdentityMetadataCreateParams { name: name.parse().unwrap(), @@ -40,7 +53,7 @@ async fn create_instance_expect_failure( memory: ByteCount::from_mebibytes_u32(256), hostname: name.to_string(), user_data: vec![], - network_interfaces: params::InstanceNetworkInterfaceAttachment::Default, + network_interfaces, disks: vec![], }; @@ -72,22 +85,24 @@ async fn test_subnet_allocation(cptestctx: &ControlPlaneTestContext) { organization_name, project_name ); - // Modify the default VPC to have a very small subnet so we don't need to - // issue many requests - let url_subnet = format!( - "/organizations/{}/projects/{}/vpcs/default/subnets/default", + // Create a new, small VPC Subnet, so we don't need to issue many requests + // to test address exhaustion. + let url_subnets = format!( + "/organizations/{}/projects/{}/vpcs/default/subnets", organization_name, project_name ); - let subnet = "192.168.42.0/29".parse().unwrap(); - let subnet_update = params::VpcSubnetUpdate { - identity: IdentityMetadataUpdateParams { - name: Some("default".parse().unwrap()), - description: None, + let subnet_name = "small"; + let subnet = "192.168.42.0/26".parse().unwrap(); + let subnet_create = params::VpcSubnetCreate { + identity: IdentityMetadataCreateParams { + name: subnet_name.parse().unwrap(), + description: String::from("a small subnet"), }, - ipv4_block: Some(Ipv4Net(subnet)), + // Use the minimum subnet size + ipv4_block: Ipv4Net(subnet), ipv6_block: None, }; - NexusRequest::object_put(client, &url_subnet, Some(&subnet_update)) + NexusRequest::objects_post(client, &url_subnets, &Some(&subnet_create)) .authn_as(AuthnMode::PrivilegedUser) .execute() .await @@ -95,30 +110,64 @@ async fn test_subnet_allocation(cptestctx: &ControlPlaneTestContext) { // The valid addresses for allocation in `subnet` are 192.168.42.5 and // 192.168.42.6. The rest are reserved as described in RFD21. - create_instance(client, organization_name, project_name, "i1").await; - create_instance(client, organization_name, project_name, "i2").await; + let nic = params::InstanceNetworkInterfaceAttachment::Create(vec![ + params::NetworkInterfaceCreate { + identity: IdentityMetadataCreateParams { + name: "eth0".parse().unwrap(), + description: String::from("some iface"), + }, + vpc_name: "default".parse().unwrap(), + subnet_name: "small".parse().unwrap(), + ip: None, + }, + ]); + + // Create enough instances to fill the subnet. There should be 58 instances + // total created here. The subnet is a /26, and there are 6 reserved + // addresses. So 2 ** (32 - 26) - 6 = 2 ** 6 - 6 = 64 - 6 = 58. + let n_final_reserved_addresses = 1; + let n_reserved_addresses = + NUM_INITIAL_RESERVED_IP_ADDRESSES + n_final_reserved_addresses; + let subnet_size = subnet.size() as usize - n_reserved_addresses; + for i in 0..subnet_size { + create_instance_with_nics( + client, + organization_name, + project_name, + &format!("i{}", i), + &nic, + ) + .await; + } // This should fail from address exhaustion - let error = - create_instance_expect_failure(client, &url_instances, "i3").await; + let error = create_instance_expect_failure( + client, + &url_instances, + "new-inst", + subnet_name, + ) + .await; assert_eq!(error.message, "No available IP addresses for interface"); // Verify the subnet lists the two addresses as in use - let url_ips = format!("{}/network-interfaces", url_subnet); + let url_ips = format!("{}/{}/network-interfaces", url_subnets, subnet_name); let mut network_interfaces = objects_list_page_authz::(client, &url_ips) .await .items; - assert_eq!(network_interfaces.len(), 2); + assert_eq!(network_interfaces.len(), subnet_size as usize); // Sort by IP address to simplify the checks network_interfaces.sort_by(|a, b| a.ip.cmp(&b.ip)); - assert_eq!( - network_interfaces[0].ip, - "192.168.42.5".parse::().unwrap() - ); - assert_eq!( - network_interfaces[1].ip, - "192.168.42.6".parse::().unwrap() - ); + for (iface, addr) in network_interfaces + .iter() + .zip(subnet.iter().skip(NUM_INITIAL_RESERVED_IP_ADDRESSES as usize)) + { + assert_eq!( + iface.ip, + addr, + "Nexus should provide auto-assigned IP addresses in order within an IP subnet" + ); + } } diff --git a/nexus/tests/integration_tests/vpc_subnets.rs b/nexus/tests/integration_tests/vpc_subnets.rs index a5f8348ca1..906658206f 100644 --- a/nexus/tests/integration_tests/vpc_subnets.rs +++ b/nexus/tests/integration_tests/vpc_subnets.rs @@ -2,7 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use dropshot::test_util::ClientTestContext; use http::method::Method; use http::StatusCode; use nexus_test_utils::http_testing::AuthnMode; @@ -20,7 +19,6 @@ use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::Ipv4Net; use omicron_common::api::external::Ipv6Net; use omicron_nexus::external_api::{params, views::VpcSubnet}; -use serde_json::json; #[nexus_test] async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { @@ -114,24 +112,6 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { assert_eq!(subnet.ipv6_block, ipv6_block.unwrap()); assert!(subnet.ipv6_block.is_vpc_subnet(&vpc.ipv6_prefix)); - // try to update ipv4_block with IPv6 value, should 400 - assert_put_400( - client, - &subnet_url, - &json!({ "ipv4_block": "2001:db8::0/96" }), - "unable to parse body: invalid address: 2001:db8::0", - ) - .await; - - // try to update ipv6_block with IPv4 value, should 400 - assert_put_400( - client, - &subnet_url, - &json!({ "ipv6_block": "10.1.9.32/16" }), - "unable to parse body: invalid address: 10.1.9.32", - ) - .await; - // get subnet, should be the same let same_subnet = NexusRequest::object_get(client, &subnet_url) .authn_as(AuthnMode::PrivilegedUser) @@ -252,8 +232,6 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { name: Some("new-name".parse().unwrap()), description: Some("another description".to_string()), }, - ipv4_block: None, - ipv6_block: None, }; NexusRequest::object_put(client, &subnet_url, Some(&update_params)) .authn_as(AuthnMode::PrivilegedUser) @@ -370,23 +348,3 @@ fn subnets_eq(sn1: &VpcSubnet, sn2: &VpcSubnet) { identity_eq(&sn1.identity, &sn2.identity); assert_eq!(sn1.vpc_id, sn2.vpc_id); } - -async fn assert_put_400( - client: &ClientTestContext, - url: &str, - body: &serde_json::Value, - message: &str, -) { - let error: dropshot::HttpErrorResponseBody = NexusRequest::new( - RequestBuilder::new(client, Method::PUT, &url) - .expect_status(Some(StatusCode::BAD_REQUEST)) - .body(Some(&body)), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); - assert!(error.message.starts_with(message)); -} diff --git a/openapi/nexus.json b/openapi/nexus.json index 72437a40c9..bf3d8d9d00 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -8396,22 +8396,6 @@ "nullable": true, "type": "string" }, - "ipv4_block": { - "nullable": true, - "allOf": [ - { - "$ref": "#/components/schemas/Ipv4Net" - } - ] - }, - "ipv6_block": { - "nullable": true, - "allOf": [ - { - "$ref": "#/components/schemas/Ipv6Net" - } - ] - }, "name": { "nullable": true, "allOf": [