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.

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/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.

nexus/src/db/datastore/vpc.rs

@@ -11,6 +11,7 @@ use crate::db;
 use crate::db::collection_insert::AsyncInsertError;
 use crate::db::collection_insert::DatastoreCollection;
 use crate::db::collection_insert::SyncInsertError;
+use crate::db::error::diesel_pool_result_optional;
 use crate::db::error::public_error_from_diesel_pool;
 use crate::db::error::ErrorHandler;
 use crate::db::error::TransactionError;
@@ -43,6 +44,7 @@ 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 uuid::Uuid;
 
 impl DataStore {
     pub async fn project_list_vpcs(
@@ -128,33 +130,80 @@ impl DataStore {
     pub async fn project_delete_vpc(
         &self,
         opctx: &OpContext,
+        db_vpc: &Vpc,
         authz_vpc: &authz::Vpc,
     ) -> DeleteResult {
         opctx.authorize(authz::Action::Delete, authz_vpc).await?;
 
         use db::schema::vpc::dsl;
+        use db::schema::vpc_subnet;
 
         // Note that we don't ensure the firewall rules are empty here, because
         // we allow deleting VPCs with firewall rules present. Inserting new
         // rules is serialized with respect to the deletion by the row lock
         // associated with the VPC row, since we use the collection insert CTE
         // pattern to add firewall rules.
 
+        // We _do_ need to check for the existence of subnets. VPC Subnets
+        // cannot be deleted while there are network interfaces in them
+        // (associations between an instance and a VPC Subnet). Because VPC
+        // Subnets are themselves containers for resources that we don't want to
+        // auto-delete (now, anyway), we've got to check there aren't any. We
+        // _might_ be able to make this a check for NICs, rather than subnets,
+        // but we can't have NICs be a child of both tables at this point, and
+        // we need to prevent VPC Subnets from being deleted while they have
+        // NICs in them as well.
+        println!("ABOUT TO SEARCH FOR SUBNETS");
+        if diesel_pool_result_optional(
+            vpc_subnet::dsl::vpc_subnet
+                .filter(vpc_subnet::dsl::vpc_id.eq(authz_vpc.id()))
+                .filter(vpc_subnet::dsl::time_deleted.is_null())
+                .select(vpc_subnet::dsl::id)
+                .limit(1)
+                .first_async::<Uuid>(self.pool_authorized(opctx).await?)
+                .await,
+        )
+        .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))?
+        .is_some()
+        {
+            println!("FOUND SOME SUBNETS");
+            return Err(Error::InvalidRequest {
+                message: String::from(
+                    "VPC cannot be deleted while VPC Subnets exist",
+                ),
+            });
+        }
+        println!("FOUND NO SUBNETS");
+
+        // Delete the VPC, conditional on the subnet_gen not having changed.
         let now = Utc::now();
-        diesel::update(dsl::vpc)
+        println!("ABOUT TO UPDATE");
+        let updated_rows = diesel::update(dsl::vpc)
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::id.eq(authz_vpc.id()))
+            .filter(dsl::subnet_gen.eq(db_vpc.subnet_gen))
             .set(dsl::time_deleted.eq(now))
-            .returning(Vpc::as_returning())
-            .get_result_async(self.pool_authorized(opctx).await?)
+            .execute_async(self.pool_authorized(opctx).await?)
             .await
             .map_err(|e| {
+                println!("FAILED TO UPDATE");
                 public_error_from_diesel_pool(
                     e,
                     ErrorHandler::NotFoundByResource(authz_vpc),
                 )
             })?;
-        Ok(())
+        println!("FINISHED UPDATE");
+        if updated_rows == 0 {
+            println!("NO ROWS UPDATED");
+            Err(Error::InvalidRequest {
+                message: String::from(
+                    "deletion failed to to concurrent modification",
+                ),
+            })
+        } else {
+            println!("SOME ROWS UPDATED");
+            Ok(())
+        }
     }
 
     pub async fn vpc_list_firewall_rules(
@@ -328,26 +377,59 @@ impl DataStore {
     pub async fn vpc_delete_subnet(
         &self,
         opctx: &OpContext,
+        db_subnet: &VpcSubnet,
         authz_subnet: &authz::VpcSubnet,
     ) -> DeleteResult {
         opctx.authorize(authz::Action::Delete, authz_subnet).await?;
 
+        use db::schema::network_interface;
         use db::schema::vpc_subnet::dsl;
+
+        // Verify there are no child network interfaces in this VPC Subnet
+        if diesel_pool_result_optional(
+            network_interface::dsl::network_interface
+                .filter(network_interface::dsl::subnet_id.eq(authz_subnet.id()))
+                .filter(network_interface::dsl::time_deleted.is_null())
+                .select(network_interface::dsl::id)
+                .limit(1)
+                .first_async::<Uuid>(self.pool_authorized(opctx).await?)
+                .await,
+        )
+        .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))?
+        .is_some()
+        {
+            return Err(Error::InvalidRequest {
+                message: String::from(
+                    "VPC Subnet cannot be deleted while instances \
+                    with network interfaces in the subnet exist",
+                ),
+            });
+        }
+
+        // Delete the subnet, conditional on the rcgen not having changed.
         let now = Utc::now();
-        diesel::update(dsl::vpc_subnet)
+        let updated_rows = diesel::update(dsl::vpc_subnet)
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::id.eq(authz_subnet.id()))
+            .filter(dsl::rcgen.eq(db_subnet.rcgen))
             .set(dsl::time_deleted.eq(now))
-            .returning(VpcSubnet::as_returning())
-            .get_result_async(self.pool_authorized(opctx).await?)
+            .execute_async(self.pool_authorized(opctx).await?)
             .await
             .map_err(|e| {
                 public_error_from_diesel_pool(
                     e,
                     ErrorHandler::NotFoundByResource(authz_subnet),
                 )
             })?;
-        Ok(())
+        if updated_rows == 0 {
+            return Err(Error::InvalidRequest {
+                message: String::from(
+                    "deletion failed to to concurrent modification",
+                ),
+            });
+        } else {
+            Ok(())
+        }
     }
 
     pub async fn vpc_update_subnet(
@@ -463,8 +545,7 @@ impl DataStore {
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::id.eq(authz_router.id()))
             .set(dsl::time_deleted.eq(now))
-            .returning(VpcRouter::as_returning())
-            .get_result_async(self.pool())
+            .execute_async(self.pool())
             .await
             .map_err(|e| {
                 public_error_from_diesel_pool(

nexus/src/db/model/vpc.rs

@@ -2,11 +2,11 @@
 // 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::db::collection_insert::DatastoreCollection;
 use crate::db::identity::Resource;
 use crate::db::model::Vni;
-use crate::db::schema::{vpc, vpc_firewall_rule};
+use crate::db::schema::{vpc, vpc_firewall_rule, vpc_subnet};
 use crate::defaults;
 use crate::external_api::params;
 use chrono::{DateTime, Utc};
@@ -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 DatastoreCollection<VpcFirewallRule> for Vpc {
     type CollectionIdColumn = vpc_firewall_rule::dsl::vpc_id;
 }
 
+impl DatastoreCollection<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 {