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

@@ -81,8 +81,6 @@ pub struct VpcSubnetUpdate {
     pub name: Option<Name>,
     pub description: Option<String>,
     pub time_modified: DateTime<Utc>,
-    pub ipv4_block: Option<Ipv4Net>,
-    pub ipv6_block: Option<Ipv6Net>,
 }
 
 impl From<params::VpcSubnetUpdate> for VpcSubnetUpdate {
@@ -91,8 +89,6 @@ impl From<params::VpcSubnetUpdate> 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),
         }
     }
 }

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<Ipv4Net>,
-    pub ipv6_block: Option<Ipv6Net>,
 }
 
 // VPC ROUTERS

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

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,
+        &params::InstanceNetworkInterfaceAttachment::Default,
+    )
+    .await
+}
+
+pub async fn create_instance_with_nics(
+    client: &ClientTestContext,
+    organization_name: &str,
+    project_name: &str,
+    instance_name: &str,
+    nics: &params::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![],
         },
     )

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,

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,53 +85,89 @@ 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
         .unwrap();
 
     // 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::<NetworkInterface>(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::<IpAddr>().unwrap()
-    );
-    assert_eq!(
-        network_interfaces[1].ip,
-        "192.168.42.6".parse::<IpAddr>().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"
+        );
+    }
 }

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));
-}

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": [