Check VPC and VPC Subnet for children before deleting (#1483)

* Check VPC and VPC Subnet for children before deleting

- Add rcgen to vpc_subnet table and model
- Add an implementation of `DatastoreCollection` for VPC Subnets, where
  the children are network interfaces. This bumps the `rcgen` whenever a
  new NIC is inserted.
- Check for child NICs before deleting a VPC Subnet, and make the
  deletion conditional on the `rcgen` being the same while doing so.
- Add integration test verifying that VPC Subnets can't be deleted while
  they contain an instance with a NIC in that subnet.
- Ditto for VPCs, checking for VPC Subnets before deleting.

* Remove debugging printlns

Author: bnaecker

common/src/sql/dbinit.sql

@@ -721,7 +721,10 @@ CREATE TABLE omicron.public.vpc (
 
     /* Used to ensure that two requests do not concurrently modify the
        VPC's firewall */
-    firewall_gen INT NOT NULL
+    firewall_gen INT NOT NULL,
+
+    /* Child-resource generation number for VPC Subnets. */
+    subnet_gen INT8 NOT NULL
 );
 
 CREATE UNIQUE INDEX ON omicron.public.vpc (
@@ -745,6 +748,8 @@ CREATE TABLE omicron.public.vpc_subnet (
     /* Indicates that the object has been deleted */
     time_deleted TIMESTAMPTZ,
     vpc_id UUID NOT NULL,
+    /* Child resource creation generation number */
+    rcgen INT8 NOT NULL,
     ipv4_block INET NOT NULL,
     ipv6_block INET NOT NULL
 );

nexus/db-model/src/schema.rs

@@ -439,6 +439,7 @@ table! {
         ipv6_prefix -> Inet,
         dns_name -> Text,
         firewall_gen -> Int8,
+        subnet_gen -> Int8,
     }
 }
 
@@ -451,6 +452,7 @@ table! {
         time_modified -> Timestamptz,
         time_deleted -> Nullable<Timestamptz>,
         vpc_id -> Uuid,
+        rcgen -> Int8,
         ipv4_block -> Inet,
         ipv6_block -> Inet,
     }

nexus/db-model/src/vpc.rs

@@ -2,9 +2,9 @@
 // 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 super::{Generation, Ipv6Net, Name, VpcFirewallRule};
+use super::{Generation, Ipv6Net, Name, VpcFirewallRule, VpcSubnet};
 use crate::collection::DatastoreCollectionConfig;
-use crate::schema::{vpc, vpc_firewall_rule};
+use crate::schema::{vpc, vpc_firewall_rule, vpc_subnet};
 use crate::Vni;
 use chrono::{DateTime, Utc};
 use db_macros::Resource;
@@ -31,6 +31,9 @@ pub struct Vpc {
     /// firewall generation number, used as a child resource generation number
     /// per RFD 192
     pub firewall_gen: Generation,
+
+    /// VPC Subnet generation number
+    pub subnet_gen: Generation,
 }
 
 impl From<Vpc> for views::Vpc {
@@ -58,6 +61,7 @@ pub struct IncompleteVpc {
     pub ipv6_prefix: IpNetwork,
     pub dns_name: Name,
     pub firewall_gen: Generation,
+    pub subnet_gen: Generation,
 }
 
 impl IncompleteVpc {
@@ -92,6 +96,7 @@ impl IncompleteVpc {
             ipv6_prefix,
             dns_name: params.dns_name.into(),
             firewall_gen: Generation::new(),
+            subnet_gen: Generation::new(),
         })
     }
 }
@@ -103,6 +108,13 @@ impl DatastoreCollectionConfig<VpcFirewallRule> for Vpc {
     type CollectionIdColumn = vpc_firewall_rule::dsl::vpc_id;
 }
 
+impl DatastoreCollectionConfig<VpcSubnet> for Vpc {
+    type CollectionId = Uuid;
+    type GenerationNumberColumn = vpc::dsl::subnet_gen;
+    type CollectionTimeDeletedColumn = vpc::dsl::time_deleted;
+    type CollectionIdColumn = vpc_subnet::dsl::vpc_id;
+}
+
 #[derive(AsChangeset)]
 #[diesel(table_name = vpc)]
 pub struct VpcUpdate {

nexus/db-model/src/vpc_subnet.rs

@@ -2,8 +2,12 @@
 // 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 super::Generation;
 use super::{Ipv4Net, Ipv6Net, Name};
+use crate::collection::DatastoreCollectionConfig;
+use crate::schema::network_interface;
 use crate::schema::vpc_subnet;
+use crate::NetworkInterface;
 use chrono::{DateTime, Utc};
 use db_macros::Resource;
 use nexus_types::external_api::params;
@@ -20,6 +24,7 @@ pub struct VpcSubnet {
     pub identity: VpcSubnetIdentity,
 
     pub vpc_id: Uuid,
+    pub rcgen: Generation,
     pub ipv4_block: Ipv4Net,
     pub ipv6_block: Ipv6Net,
 }
@@ -40,6 +45,7 @@ impl VpcSubnet {
         Self {
             identity,
             vpc_id,
+            rcgen: Generation::new(),
             ipv4_block: Ipv4Net(ipv4_block),
             ipv6_block: Ipv6Net(ipv6_block),
         }
@@ -105,3 +111,10 @@ impl From<params::VpcSubnetUpdate> for VpcSubnetUpdate {
         }
     }
 }
+
+impl DatastoreCollectionConfig<NetworkInterface> for VpcSubnet {
+    type CollectionId = Uuid;
+    type GenerationNumberColumn = vpc_subnet::dsl::rcgen;
+    type CollectionTimeDeletedColumn = vpc_subnet::dsl::time_deleted;
+    type CollectionIdColumn = network_interface::dsl::subnet_id;
+}

nexus/src/app/vpc.rs

@@ -261,10 +261,22 @@ impl super::Nexus {
             LookupType::ById(db_vpc.system_router_id),
         );
 
+        // Possibly delete the VPC, then the router and firewall.
+        //
+        // We must delete the VPC first. This will fail if the VPC still
+        // contains at least one subnet, since those are independent containers
+        // that track network interfaces as child resources. If we delete the
+        // router first, it'll succeed even if the VPC contains Subnets, which
+        // means the router is now gone from an otherwise-live subnet.
+        //
+        // This is a good example of need for the original comment:
+        //
         // TODO: This should eventually use a saga to call the
         // networking subsystem to have it clean up the networking resources
+        self.db_datastore
+            .project_delete_vpc(opctx, &db_vpc, &authz_vpc)
+            .await?;
         self.db_datastore.vpc_delete_router(&opctx, &authz_vpc_router).await?;
-        self.db_datastore.project_delete_vpc(opctx, &authz_vpc).await?;
 
         // Delete all firewall rules after deleting the VPC, to ensure no
         // firewall rules get added between rules deletion and VPC deletion.

nexus/src/app/vpc_subnet.rs

@@ -261,14 +261,17 @@ impl super::Nexus {
         vpc_name: &Name,
         subnet_name: &Name,
     ) -> DeleteResult {
-        let (.., authz_subnet) = LookupPath::new(opctx, &self.db_datastore)
-            .organization_name(organization_name)
-            .project_name(project_name)
-            .vpc_name(vpc_name)
-            .vpc_subnet_name(subnet_name)
-            .lookup_for(authz::Action::Delete)
-            .await?;
-        self.db_datastore.vpc_delete_subnet(opctx, &authz_subnet).await
+        let (.., authz_subnet, db_subnet) =
+            LookupPath::new(opctx, &self.db_datastore)
+                .organization_name(organization_name)
+                .project_name(project_name)
+                .vpc_name(vpc_name)
+                .vpc_subnet_name(subnet_name)
+                .fetch_for(authz::Action::Delete)
+                .await?;
+        self.db_datastore
+            .vpc_delete_subnet(opctx, &db_subnet, &authz_subnet)
+            .await
     }
 
     pub async fn subnet_list_network_interfaces(

nexus/src/db/datastore/network_interface.rs

@@ -8,6 +8,8 @@ use super::DataStore;
 use crate::authz;
 use crate::context::OpContext;
 use crate::db;
+use crate::db::collection_insert::AsyncInsertError;
+use crate::db::collection_insert::DatastoreCollection;
 use crate::db::error::public_error_from_diesel_pool;
 use crate::db::error::ErrorHandler;
 use crate::db::error::TransactionError;
@@ -16,6 +18,7 @@ use crate::db::model::Instance;
 use crate::db::model::Name;
 use crate::db::model::NetworkInterface;
 use crate::db::model::NetworkInterfaceUpdate;
+use crate::db::model::VpcSubnet;
 use crate::db::pagination::paginated;
 use crate::db::queries::network_interface;
 use async_bb8_diesel::AsyncConnection;
@@ -27,6 +30,8 @@ use omicron_common::api::external::DataPageParams;
 use omicron_common::api::external::DeleteResult;
 use omicron_common::api::external::Error;
 use omicron_common::api::external::ListResultVec;
+use omicron_common::api::external::LookupType;
+use omicron_common::api::external::ResourceType;
 use omicron_common::api::external::UpdateResult;
 use sled_agent_client::types as sled_client_types;
 
@@ -56,19 +61,31 @@ impl DataStore {
         interface: IncompleteNetworkInterface,
     ) -> Result<NetworkInterface, network_interface::InsertError> {
         use db::schema::network_interface::dsl;
+        let subnet_id = interface.subnet.identity.id;
         let query = network_interface::InsertQuery::new(interface.clone());
-        diesel::insert_into(dsl::network_interface)
-            .values(query)
-            .returning(NetworkInterface::as_returning())
-            .get_result_async(
-                self.pool_authorized(opctx)
-                    .await
-                    .map_err(network_interface::InsertError::External)?,
-            )
-            .await
-            .map_err(|e| {
+        VpcSubnet::insert_resource(
+            subnet_id,
+            diesel::insert_into(dsl::network_interface).values(query),
+        )
+        .insert_and_get_result_async(
+            self.pool_authorized(opctx)
+                .await
+                .map_err(network_interface::InsertError::External)?,
+        )
+        .await
+        .map_err(|e| match e {
+            AsyncInsertError::CollectionNotFound => {
+                network_interface::InsertError::External(
+                    Error::ObjectNotFound {
+                        type_name: ResourceType::VpcSubnet,
+                        lookup_type: LookupType::ById(subnet_id),
+                    },
+                )
+            }
+            AsyncInsertError::DatabaseError(e) => {
                 network_interface::InsertError::from_pool(e, &interface)
-            })
+            }
+        })
     }
 
     /// Delete all network interfaces attached to the given instance.