From f038746eab35146f8a29ad85836d7b92cfdcf298 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Thu, 18 Jul 2024 12:45:04 -0700 Subject: [PATCH] Make VPC Subnet insertion query idempotent (#6098) - Fixes #6069 - Modifies the existing query to ignore PK conflicts, making it idempotent, while still detecting overlapping IP ranges from different subnets - Adds regression test for #6069 --- nexus/db-queries/src/db/datastore/mod.rs | 10 +- nexus/db-queries/src/db/datastore/vpc.rs | 26 +- nexus/db-queries/src/db/queries/vpc_subnet.rs | 702 +++++++++--------- nexus/src/app/sagas/vpc_create.rs | 6 +- nexus/src/app/vpc_subnet.rs | 21 +- 5 files changed, 390 insertions(+), 375 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 07b98c0542..2540790477 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -394,9 +394,9 @@ mod test { BlockSize, ConsoleSession, Dataset, DatasetKind, ExternalIp, PhysicalDisk, PhysicalDiskKind, PhysicalDiskPolicy, PhysicalDiskState, Project, Rack, Region, SiloUser, SledBaseboard, SledSystemHardware, - SledUpdate, SshKey, VpcSubnet, Zpool, + SledUpdate, SshKey, Zpool, }; - use crate::db::queries::vpc_subnet::FilterConflictingVpcSubnetRangesQuery; + use crate::db::queries::vpc_subnet::InsertVpcSubnetQuery; use chrono::{Duration, Utc}; use futures::stream; use futures::StreamExt; @@ -1603,11 +1603,7 @@ mod test { "172.30.0.0/22".parse().unwrap(), "fd00::/64".parse().unwrap(), ); - let values = FilterConflictingVpcSubnetRangesQuery::new(subnet); - let query = - diesel::insert_into(db::schema::vpc_subnet::dsl::vpc_subnet) - .values(values) - .returning(VpcSubnet::as_returning()); + let query = InsertVpcSubnetQuery::new(subnet); println!("{}", diesel::debug_query(&query)); let explanation = query.explain_async(&conn).await.unwrap(); assert!( diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index fdb9c82fb5..615ecdac93 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -40,8 +40,8 @@ use crate::db::pagination::paginated; use crate::db::pagination::Paginator; use crate::db::queries::vpc::InsertVpcQuery; use crate::db::queries::vpc::VniSearchIter; -use crate::db::queries::vpc_subnet::FilterConflictingVpcSubnetRangesQuery; -use crate::db::queries::vpc_subnet::SubnetError; +use crate::db::queries::vpc_subnet::InsertVpcSubnetError; +use crate::db::queries::vpc_subnet::InsertVpcSubnetQuery; use crate::transaction_retry::OptionalError; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; @@ -288,7 +288,7 @@ impl DataStore { self.vpc_create_subnet(opctx, &authz_vpc, vpc_subnet.clone()) .await .map(|_| ()) - .map_err(SubnetError::into_external) + .map_err(InsertVpcSubnetError::into_external) .or_else(|e| match e { Error::ObjectAlreadyExists { .. } => Ok(()), _ => Err(e), @@ -809,17 +809,17 @@ impl DataStore { opctx: &OpContext, authz_vpc: &authz::Vpc, subnet: VpcSubnet, - ) -> Result<(authz::VpcSubnet, VpcSubnet), SubnetError> { + ) -> Result<(authz::VpcSubnet, VpcSubnet), InsertVpcSubnetError> { opctx .authorize(authz::Action::CreateChild, authz_vpc) .await - .map_err(SubnetError::External)?; + .map_err(InsertVpcSubnetError::External)?; assert_eq!(authz_vpc.id(), subnet.vpc_id); let db_subnet = self.vpc_create_subnet_raw(subnet).await?; self.vpc_system_router_ensure_subnet_routes(opctx, authz_vpc.id()) .await - .map_err(SubnetError::External)?; + .map_err(InsertVpcSubnetError::External)?; Ok(( authz::VpcSubnet::new( authz_vpc.clone(), @@ -833,20 +833,16 @@ impl DataStore { pub(crate) async fn vpc_create_subnet_raw( &self, subnet: VpcSubnet, - ) -> Result { - use db::schema::vpc_subnet::dsl; - let values = FilterConflictingVpcSubnetRangesQuery::new(subnet.clone()); + ) -> Result { let conn = self .pool_connection_unauthorized() .await - .map_err(SubnetError::External)?; - - diesel::insert_into(dsl::vpc_subnet) - .values(values) - .returning(VpcSubnet::as_returning()) + .map_err(InsertVpcSubnetError::External)?; + let query = InsertVpcSubnetQuery::new(subnet.clone()); + query .get_result_async(&*conn) .await - .map_err(|e| SubnetError::from_diesel(e, &subnet)) + .map_err(|e| InsertVpcSubnetError::from_diesel(e, &subnet)) } pub async fn vpc_delete_subnet( diff --git a/nexus/db-queries/src/db/queries/vpc_subnet.rs b/nexus/db-queries/src/db/queries/vpc_subnet.rs index 72f2771a1e..8cbf4495ca 100644 --- a/nexus/db-queries/src/db/queries/vpc_subnet.rs +++ b/nexus/db-queries/src/db/queries/vpc_subnet.rs @@ -7,407 +7,322 @@ use crate::db; use crate::db::identity::Resource; use crate::db::model::VpcSubnet; -use chrono::{DateTime, Utc}; +use crate::db::schema::vpc_subnet::dsl; +use crate::db::DbConnection; use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; use diesel::result::Error as DieselError; use diesel::sql_types; +use ipnetwork::IpNetwork; use omicron_common::api::external; use ref_cast::RefCast; use uuid::Uuid; -/// Errors related to allocating VPC Subnets. -#[derive(Debug, PartialEq)] -pub enum SubnetError { - /// An IPv4 or IPv6 subnet overlaps with an existing VPC Subnet - OverlappingIpRange(ipnetwork::IpNetwork), - /// An other error - External(external::Error), -} - -impl SubnetError { - /// Construct a `SubnetError` from a Diesel error, catching the desired - /// cases and building useful errors. - pub fn from_diesel(e: DieselError, subnet: &VpcSubnet) -> Self { - use crate::db::error; - use diesel::result::DatabaseErrorKind; - const IPV4_OVERLAP_ERROR_MESSAGE: &str = - r#"null value in column "ipv4_block" violates not-null constraint"#; - const IPV6_OVERLAP_ERROR_MESSAGE: &str = - r#"null value in column "ipv6_block" violates not-null constraint"#; - const NAME_CONFLICT_CONSTRAINT: &str = "vpc_subnet_vpc_id_name_key"; - match e { - // Attempt to insert overlapping IPv4 subnet - DieselError::DatabaseError( - DatabaseErrorKind::NotNullViolation, - ref info, - ) if info.message() == IPV4_OVERLAP_ERROR_MESSAGE => { - SubnetError::OverlappingIpRange(ipnetwork::IpNetwork::V4( - subnet.ipv4_block.0.into(), - )) - } - - // Attempt to insert overlapping IPv6 subnet - DieselError::DatabaseError( - DatabaseErrorKind::NotNullViolation, - ref info, - ) if info.message() == IPV6_OVERLAP_ERROR_MESSAGE => { - SubnetError::OverlappingIpRange(ipnetwork::IpNetwork::V6( - subnet.ipv6_block.0.into(), - )) - } - - // Conflicting name for the subnet within a VPC - DieselError::DatabaseError( - DatabaseErrorKind::UniqueViolation, - ref info, - ) if info.constraint_name() == Some(NAME_CONFLICT_CONSTRAINT) => { - SubnetError::External(error::public_error_from_diesel( - e, - error::ErrorHandler::Conflict( - external::ResourceType::VpcSubnet, - subnet.identity().name.as_str(), - ), - )) - } - - // Any other error at all is a bug - _ => SubnetError::External(error::public_error_from_diesel( - e, - error::ErrorHandler::Server, - )), - } - } - - /// Convert into a public error - pub fn into_external(self) -> external::Error { - match self { - SubnetError::OverlappingIpRange(ip) => { - external::Error::invalid_request( - format!("IP address range '{}' conflicts with an existing subnet", ip).as_str() - ) - }, - SubnetError::External(e) => e, - } - } -} - -/// Generate a subquery that selects any overlapping address ranges of the same -/// type as the input IP subnet. +/// Query used to insert VPC Subnets. /// -/// This generates a query that, in full, looks like: +/// This query is used to idempotently insert a VPC Subnet. The query also looks +/// for any other subnets in the same VPC whose IP address blocks overlap. All +/// Subnets are required to have non-overlapping IP blocks. /// -/// ```sql -/// SELECT -/// -/// FROM -/// vpc_subnet -/// WHERE -/// vpc_id = AND -/// time_deleted IS NULL AND -/// inet_contains_or_equals(ipv*_block, ) -/// LIMIT 1 -/// ``` -/// -/// The input may be either an IPv4 or IPv6 subnet, and the corresponding column -/// is compared against. Note that the exact input IP range is returned on -/// purpose. -fn push_select_overlapping_ip_range<'a>( - mut out: AstPass<'_, 'a, Pg>, - vpc_id: &'a Uuid, - ip: &'a ipnetwork::IpNetwork, -) -> diesel::QueryResult<()> { - use crate::db::schema::vpc_subnet::dsl; - out.push_sql("SELECT "); - out.push_bind_param::(ip)?; - out.push_sql(" FROM "); - VPC_SUBNET_FROM_CLAUSE.walk_ast(out.reborrow())?; - out.push_sql(" WHERE "); - out.push_identifier(dsl::vpc_id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::(vpc_id)?; - out.push_sql(" AND "); - out.push_identifier(dsl::time_deleted::NAME)?; - out.push_sql(" IS NULL AND inet_contains_or_equals("); - if ip.is_ipv4() { - out.push_identifier(dsl::ipv4_block::NAME)?; - } else { - out.push_identifier(dsl::ipv6_block::NAME)?; - } - out.push_sql(", "); - out.push_bind_param::(ip)?; - out.push_sql(")"); - Ok(()) -} - -/// Generate a subquery that returns NULL if there is an overlapping IP address -/// range of any type. +/// Note that this query is idempotent. If a record with the provided primary +/// key already exists, that record is returned exactly from the DB, without any +/// other modification or alteration. If callers care, they can inspect the +/// record to make sure it's what they expected, though that's usually a fraught +/// endeavor. /// -/// This specifically generates a query that looks like: +/// Here is the entire query: /// /// ```sql -/// SELECT NULLIF( -/// , -/// push_select_overlapping_ip_range(, ) -/// ) -/// ``` -/// -/// The `NULLIF` function returns NULL if those two expressions are equal, and -/// the first expression otherwise. That is, this returns NULL if there exists -/// an overlapping IP range already in the VPC Subnet table, and the requested -/// IP range if not. -fn push_null_if_overlapping_ip_range<'a>( - mut out: AstPass<'_, 'a, Pg>, - vpc_id: &'a Uuid, - ip: &'a ipnetwork::IpNetwork, -) -> diesel::QueryResult<()> { - out.push_sql("SELECT NULLIF("); - out.push_bind_param::(ip)?; - out.push_sql(", ("); - push_select_overlapping_ip_range(out.reborrow(), vpc_id, ip)?; - out.push_sql("))"); - Ok(()) -} - -/// Generate a CTE that can be used to insert a VPC Subnet, only if the IP -/// address ranges of that subnet don't overlap with existing Subnets in the -/// same VPC. -/// -/// In particular, this generates a CTE like so: -/// -/// ```sql -/// WITH candidate( -/// id, -/// name, -/// description, -/// time_created, -/// time_modified, -/// time_deleted, -/// vpc_id, -/// rcgen -/// ) AS (VALUES ( -/// , -/// , -/// , -/// , -/// , -/// NULL::TIMESTAMPTZ, -/// , -/// 0 -/// )), -/// candidate_ipv4(ipv4_block) AS ( -/// SELECT( -/// NULLIF( -/// , -/// ( -/// SELECT -/// ipv4_block -/// FROM -/// vpc_subnet -/// WHERE -/// vpc_id = AND -/// time_deleted IS NULL AND -/// inet_contains_or_equals(, ipv4_block) -/// LIMIT 1 +/// WITH +/// -- This CTE generates a casting error if any live records, other than _this_ +/// -- record, have overlapping IP blocks of either family. +/// overlap AS MATERIALIZED ( +/// SELECT +/// -- NOTE: This cast always fails, we just use _how_ it fails to +/// -- learn which IP block overlaps. The filter `id != ` below +/// -- means we're explicitly ignoring an existing, identical record. +/// -- So this cast is only run if there is another record in the same +/// -- VPC with an overlapping subnet, which is exactly the error case +/// -- we're trying to cacth. +/// CAST( +/// IF( +/// inet_contains_or_equals(ipv4_block, ), +/// 'ipv4', +/// 'ipv6' /// ) -/// ) -/// ) -/// ), -/// candidate_ipv6(ipv6_block) AS ( -/// +/// AS BOOL +/// ) +/// FROM +/// vpc_subnet +/// WHERE +/// vpc_id = AND +/// time_deleted IS NULL AND +/// id != AND +/// ( +/// inet_contains_or_equals(ipv4_block, ) OR +/// inet_contains_or_equals(ipv6_block, ) +/// ) +/// ) +/// INSERT INTO +/// vpc_subnet +/// VALUES ( +/// /// ) -/// SELECT * -/// FROM candidate, candidate_ipv4, candidate_ipv6 +/// ON CONFLICT (id) +/// -- We use this "no-op" update to allow us to return the actual row from the +/// -- DB, either the existing or inserted one. +/// DO UPDATE SET id = id +/// RETURNING *; /// ``` -pub struct FilterConflictingVpcSubnetRangesQuery { - // TODO: update with random one if the insertion fails. +#[derive(Clone, Debug)] +pub struct InsertVpcSubnetQuery { + /// The subnet to insert subnet: VpcSubnet, - - // The following fields are derived from the previous field. This begs the - // question: "Why bother storing them at all?" - // - // Diesel's [`diesel::query_builder::ast_pass::AstPass:push_bind_param`] method - // requires that the provided value now live as long as the entire AstPass - // type. By storing these values in the struct, they'll live at least as - // long as the entire call to [`QueryFragment::walk_ast`]. - ipv4_block: ipnetwork::IpNetwork, - ipv6_block: ipnetwork::IpNetwork, + /// Owned values of the IP blocks to check, for inserting in internal pieces + /// of the query. + ipv4_block: IpNetwork, + ipv6_block: IpNetwork, } -impl FilterConflictingVpcSubnetRangesQuery { +impl InsertVpcSubnetQuery { + /// Construct a new query to insert the provided subnet. pub fn new(subnet: VpcSubnet) -> Self { - let ipv4_block = - ipnetwork::Ipv4Network::from(subnet.ipv4_block.0).into(); - let ipv6_block = - ipnetwork::Ipv6Network::from(subnet.ipv6_block.0).into(); + let ipv4_block = IpNetwork::V4(subnet.ipv4_block.0.into()); + let ipv6_block = IpNetwork::V6(subnet.ipv6_block.0.into()); Self { subnet, ipv4_block, ipv6_block } } } -impl QueryId for FilterConflictingVpcSubnetRangesQuery { +impl QueryId for InsertVpcSubnetQuery { type QueryId = (); const HAS_STATIC_QUERY_ID: bool = false; } -impl QueryFragment for FilterConflictingVpcSubnetRangesQuery { +impl QueryFragment for InsertVpcSubnetQuery { fn walk_ast<'a>( &'a self, mut out: AstPass<'_, 'a, Pg>, ) -> diesel::QueryResult<()> { - use db::schema::vpc_subnet::dsl; - - // Create the base `candidate` from values provided that need no - // verificiation. - out.push_sql("SELECT * FROM (WITH candidate("); - out.push_identifier(dsl::id::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::name::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::description::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::time_created::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::time_modified::NAME)?; + out.push_sql("WITH overlap AS MATERIALIZED (SELECT CAST(IF(inet_contains_or_equals("); + out.push_identifier(dsl::ipv4_block::NAME)?; out.push_sql(", "); - out.push_identifier(dsl::time_deleted::NAME)?; + out.push_bind_param::(&self.ipv4_block)?; + out.push_sql("), "); + out.push_bind_param::( + InsertVpcSubnetError::OVERLAPPING_IPV4_BLOCK_SENTINEL, + )?; out.push_sql(", "); + out.push_bind_param::( + InsertVpcSubnetError::OVERLAPPING_IPV6_BLOCK_SENTINEL, + )?; + out.push_sql(") AS BOOL) FROM "); + VPC_SUBNET_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); out.push_identifier(dsl::vpc_id::NAME)?; - out.push_sql(","); - out.push_identifier(dsl::rcgen::NAME)?; - out.push_sql(") AS (VALUES ("); + out.push_sql(" = "); + out.push_bind_param::(&self.subnet.vpc_id)?; + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL AND "); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(" != "); out.push_bind_param::(&self.subnet.identity.id)?; + out.push_sql(" AND (inet_contains_or_equals("); + out.push_identifier(dsl::ipv4_block::NAME)?; out.push_sql(", "); - out.push_bind_param::( - db::model::Name::ref_cast(self.subnet.name()), - )?; + out.push_bind_param::(&self.ipv4_block)?; + out.push_sql(") OR inet_contains_or_equals("); + out.push_identifier(dsl::ipv6_block::NAME)?; out.push_sql(", "); - out.push_bind_param::( + out.push_bind_param::(&self.ipv6_block)?; + + out.push_sql("))) INSERT INTO "); + VPC_SUBNET_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql("VALUES ("); + out.push_bind_param::(&self.subnet.identity.id)?; + out.push_sql(", "); + out.push_bind_param::(db::model::Name::ref_cast( + self.subnet.name(), + ))?; + out.push_sql(", "); + out.push_bind_param::( &self.subnet.identity.description, )?; out.push_sql(", "); - out.push_bind_param::>( + out.push_bind_param::( &self.subnet.identity.time_created, )?; out.push_sql(", "); - out.push_bind_param::>( + out.push_bind_param::( &self.subnet.identity.time_modified, )?; out.push_sql(", "); - out.push_sql("NULL::TIMESTAMPTZ, "); - out.push_bind_param::(&self.subnet.vpc_id)?; - out.push_sql(", 0)), "); - - // Push the candidate IPv4 and IPv6 selection subqueries, which return - // NULL if the corresponding address range overlaps. - out.push_sql("candidate_ipv4("); - out.push_identifier(dsl::ipv4_block::NAME)?; - out.push_sql(") AS ("); - push_null_if_overlapping_ip_range( - out.reborrow(), - &self.subnet.vpc_id, - &self.ipv4_block, + out.push_bind_param::, _>( + &self.subnet.identity.time_deleted, )?; - - out.push_sql("), candidate_ipv6("); - out.push_identifier(dsl::ipv6_block::NAME)?; - out.push_sql(") AS ("); - push_null_if_overlapping_ip_range( - out.reborrow(), - &self.subnet.vpc_id, - &self.ipv6_block, + out.push_sql(", "); + out.push_bind_param::(&self.subnet.vpc_id)?; + out.push_sql(", "); + out.push_bind_param::(&self.subnet.rcgen)?; + out.push_sql(", "); + out.push_bind_param::(&self.ipv4_block)?; + out.push_sql(", "); + out.push_bind_param::(&self.ipv6_block)?; + out.push_sql(", "); + out.push_bind_param::, _>( + &self.subnet.custom_router_id, )?; - out.push_sql(") "); + out.push_sql(") ON CONFLICT ("); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(") DO UPDATE SET "); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.subnet.identity.id)?; + out.push_sql(" RETURNING *"); - // Select the entire set of candidate columns. - out.push_sql( - "SELECT * FROM candidate, candidate_ipv4, candidate_ipv6)", - ); Ok(()) } } -impl Insertable - for FilterConflictingVpcSubnetRangesQuery -{ - type Values = FilterConflictingVpcSubnetRangesQueryValues; +type FromClause = + diesel::internal::table_macro::StaticQueryFragmentInstance; +type VpcSubnetFromClause = FromClause; +const VPC_SUBNET_FROM_CLAUSE: VpcSubnetFromClause = VpcSubnetFromClause::new(); - fn values(self) -> Self::Values { - FilterConflictingVpcSubnetRangesQueryValues(self) - } +impl RunQueryDsl for InsertVpcSubnetQuery {} +impl Query for InsertVpcSubnetQuery { + type SqlType = <>::SelectExpression as diesel::Expression>::SqlType; } -/// Used to allow inserting the result of the -/// `FilterConflictingVpcSubnetRangesQuery`, as in -/// `diesel::insert_into(foo).values(_). Should not be used directly. -pub struct FilterConflictingVpcSubnetRangesQueryValues( - pub FilterConflictingVpcSubnetRangesQuery, -); - -impl QueryId for FilterConflictingVpcSubnetRangesQueryValues { - type QueryId = (); - const HAS_STATIC_QUERY_ID: bool = false; +/// Errors related to inserting VPC Subnets. +#[derive(Debug, PartialEq)] +pub enum InsertVpcSubnetError { + /// The IPv4 or IPv6 subnet overlaps with an existing VPC Subnet + OverlappingIpRange(oxnet::IpNet), + /// Any other error + External(external::Error), } -impl diesel::insertable::CanInsertInSingleQuery - for FilterConflictingVpcSubnetRangesQueryValues -{ - fn rows_to_insert(&self) -> Option { - Some(1) +impl InsertVpcSubnetError { + const OVERLAPPING_IPV4_BLOCK_SENTINEL: &'static str = "ipv4"; + const OVERLAPPING_IPV4_BLOCK_ERROR_MESSAGE: &'static str = + r#"could not parse "ipv4" as type bool: invalid bool value"#; + const OVERLAPPING_IPV6_BLOCK_SENTINEL: &'static str = "ipv6"; + const OVERLAPPING_IPV6_BLOCK_ERROR_MESSAGE: &'static str = + r#"could not parse "ipv6" as type bool: invalid bool value"#; + const NAME_CONFLICT_CONSTRAINT: &'static str = "vpc_subnet_vpc_id_name_key"; + + /// Construct an `InsertError` from a Diesel error, catching the desired + /// cases and building useful errors. + pub fn from_diesel(e: DieselError, subnet: &VpcSubnet) -> Self { + use crate::db::error; + use diesel::result::DatabaseErrorKind; + match e { + // Attempt to insert an overlapping IPv4 subnet + DieselError::DatabaseError( + DatabaseErrorKind::Unknown, + ref info, + ) if info.message() + == Self::OVERLAPPING_IPV4_BLOCK_ERROR_MESSAGE => + { + InsertVpcSubnetError::OverlappingIpRange( + subnet.ipv4_block.0.into(), + ) + } + + // Attempt to insert an overlapping IPv6 subnet + DieselError::DatabaseError( + DatabaseErrorKind::Unknown, + ref info, + ) if info.message() + == Self::OVERLAPPING_IPV6_BLOCK_ERROR_MESSAGE => + { + InsertVpcSubnetError::OverlappingIpRange( + subnet.ipv6_block.0.into(), + ) + } + + // Conflicting name for the subnet within a VPC + DieselError::DatabaseError( + DatabaseErrorKind::UniqueViolation, + ref info, + ) if info.constraint_name() + == Some(Self::NAME_CONFLICT_CONSTRAINT) => + { + InsertVpcSubnetError::External(error::public_error_from_diesel( + e, + error::ErrorHandler::Conflict( + external::ResourceType::VpcSubnet, + subnet.identity().name.as_str(), + ), + )) + } + + // Any other error at all is a bug + _ => InsertVpcSubnetError::External( + error::public_error_from_diesel(e, error::ErrorHandler::Server), + ), + } } -} -impl QueryFragment for FilterConflictingVpcSubnetRangesQueryValues { - fn walk_ast<'a>( - &'a self, - mut out: AstPass<'_, 'a, Pg>, - ) -> diesel::QueryResult<()> { - use db::schema::vpc_subnet::dsl; - out.push_sql("("); - out.push_identifier(dsl::id::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::name::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::description::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::time_created::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::time_modified::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::time_deleted::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::vpc_id::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::rcgen::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::ipv4_block::NAME)?; - out.push_sql(", "); - out.push_identifier(dsl::ipv6_block::NAME)?; - out.push_sql(") "); - self.0.walk_ast(out) + /// Convert into a public error + pub fn into_external(self) -> external::Error { + match self { + InsertVpcSubnetError::OverlappingIpRange(ip) => { + external::Error::invalid_request( + format!( + "IP address range '{}' \ + conflicts with an existing subnet", + ip, + ) + .as_str(), + ) + } + InsertVpcSubnetError::External(e) => e, + } } } -type FromClause = - diesel::internal::table_macro::StaticQueryFragmentInstance; -type VpcSubnetFromClause = FromClause; -const VPC_SUBNET_FROM_CLAUSE: VpcSubnetFromClause = VpcSubnetFromClause::new(); - #[cfg(test)] mod test { - use super::SubnetError; + use super::InsertVpcSubnetError; + use super::InsertVpcSubnetQuery; + use crate::db::explain::ExplainableAsync as _; use crate::db::model::VpcSubnet; - use ipnetwork::IpNetwork; use nexus_test_utils::db::test_setup_database; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Name; use omicron_test_utils::dev; use std::convert::TryInto; use std::sync::Arc; - use uuid::Uuid; #[tokio::test] - async fn test_filter_conflicting_vpc_subnet_ranges_query() { + async fn explain_insert_query() { + let ipv4_block = "172.30.0.0/24".parse().unwrap(); + let ipv6_block = "fd12:3456:7890::/64".parse().unwrap(); + let name = "a-name".to_string().try_into().unwrap(); + let description = "some description".to_string(); + let identity = IdentityMetadataCreateParams { name, description }; + let vpc_id = "d402369d-c9ec-c5ad-9138-9fbee732d53e".parse().unwrap(); + let subnet_id = "093ad2db-769b-e3c2-bc1c-b46e84ce5532".parse().unwrap(); + let row = + VpcSubnet::new(subnet_id, vpc_id, identity, ipv4_block, ipv6_block); + let query = InsertVpcSubnetQuery::new(row); + let logctx = dev::test_setup_log("explain_insert_query"); + let log = logctx.log.new(o!()); + let mut db = test_setup_database(&log).await; + let cfg = crate::db::Config { url: db.pg_config().clone() }; + let pool = Arc::new(crate::db::Pool::new(&logctx.log, &cfg)); + let conn = pool.pool().get().await.unwrap(); + let explain = query.explain_async(&conn).await.unwrap(); + println!("{explain}"); + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn test_insert_vpc_subnet_query() { let make_id = |name: &Name, description: &str| IdentityMetadataCreateParams { name: name.clone(), @@ -427,12 +342,13 @@ mod test { let subnet_id = "093ad2db-769b-e3c2-bc1c-b46e84ce5532".parse().unwrap(); let other_subnet_id = "695debcc-e197-447d-ffb2-976150a7b7cf".parse().unwrap(); + let other_other_subnet_id = + "ddbdc2b7-d22f-40d9-98df-fef5da151e0d".parse().unwrap(); let row = VpcSubnet::new(subnet_id, vpc_id, identity, ipv4_block, ipv6_block); // Setup the test database - let logctx = - dev::test_setup_log("test_filter_conflicting_vpc_subnet_ranges"); + let logctx = dev::test_setup_log("test_insert_vpc_subnet_query"); let log = logctx.log.new(o!()); let mut db = test_setup_database(&log).await; let cfg = crate::db::Config { url: db.pg_config().clone() }; @@ -445,7 +361,10 @@ mod test { // We should be able to insert anything into an empty table. assert!( - matches!(db_datastore.vpc_create_subnet_raw(row).await, Ok(_)), + matches!( + db_datastore.vpc_create_subnet_raw(row.clone()).await, + Ok(_) + ), "Should be able to insert VPC subnet into empty table" ); @@ -460,10 +379,13 @@ mod test { ); assert!( matches!( - db_datastore.vpc_create_subnet_raw(new_row).await, - Err(SubnetError::OverlappingIpRange(IpNetwork::V4(_))) + db_datastore.vpc_create_subnet_raw(new_row.clone()).await, + Err(InsertVpcSubnetError::OverlappingIpRange { .. }), ), - "Should not be able to insert new VPC subnet with the same IPv4 and IPv6 ranges" + "Should not be able to insert new VPC subnet with the \ + same IPv4 and IPv6 ranges,\n\ + first row: {row:?}\n\ + new row: {new_row:?}", ); // We should be able to insert data with the same ranges, if we change @@ -483,7 +405,7 @@ mod test { // We shouldn't be able to insert a subnet if we change only the // IPv4 or IPv6 block. They must _both_ be non-overlapping. let new_row = VpcSubnet::new( - other_subnet_id, + other_other_subnet_id, vpc_id, make_id(&other_name, &description), other_ipv4_block, @@ -495,11 +417,11 @@ mod test { .expect_err("Should not be able to insert VPC Subnet with overlapping IPv6 range"); assert_eq!( err, - SubnetError::OverlappingIpRange(ipnetwork::IpNetwork::from(oxnet::IpNet::from(ipv6_block))), - "SubnetError variant should include the exact IP range that overlaps" + InsertVpcSubnetError::OverlappingIpRange(ipv6_block.into()), + "InsertError variant should indicate an IP block overlaps" ); let new_row = VpcSubnet::new( - other_subnet_id, + other_other_subnet_id, vpc_id, make_id(&other_name, &description), ipv4_block, @@ -511,14 +433,14 @@ mod test { .expect_err("Should not be able to insert VPC Subnet with overlapping IPv4 range"); assert_eq!( err, - SubnetError::OverlappingIpRange(ipnetwork::IpNetwork::from(oxnet::IpNet::from(ipv4_block))), - "SubnetError variant should include the exact IP range that overlaps" + InsertVpcSubnetError::OverlappingIpRange(ipv4_block.into()), + "InsertError variant should indicate an IP block overlaps" ); // We should get an _external error_ if the IP address ranges are OK, // but the name conflicts. let new_row = VpcSubnet::new( - other_subnet_id, + other_other_subnet_id, vpc_id, make_id(&name, &description), other_ipv4_block, @@ -527,7 +449,7 @@ mod test { assert!( matches!( db_datastore.vpc_create_subnet_raw(new_row).await, - Err(SubnetError::External(_)) + Err(InsertVpcSubnetError::External(_)) ), "Should get an error inserting a VPC Subnet with unique IP ranges, but the same name" ); @@ -535,7 +457,7 @@ mod test { // We should be able to insert the row if _both ranges_ are different, // and the name is unique as well. let new_row = VpcSubnet::new( - Uuid::new_v4(), + other_other_subnet_id, vpc_id, make_id(&other_name, &description), other_ipv4_block, @@ -549,4 +471,104 @@ mod test { db.cleanup().await.unwrap(); logctx.cleanup_successful(); } + + // Helper to verify equality of rows, handling timestamp precision. + fn assert_rows_eq(left: &VpcSubnet, right: &VpcSubnet) { + assert_eq!( + left.identity.id, right.identity.id, + "VPC Subnet rows should be equal" + ); + assert_eq!( + left.identity.name, right.identity.name, + "VPC Subnet rows should be equal" + ); + assert_eq!( + left.identity.description, right.identity.description, + "VPC Subnet rows should be equal" + ); + // Timestamps in CRDB have microsecond precision, so ensure we're + // within 1000 nanos. + assert!( + (left.identity.time_modified - right.identity.time_modified) + .num_nanoseconds() + .unwrap() + < 1_000, + "VPC Subnet rows should be equal", + ); + assert!( + (left.identity.time_created - right.identity.time_created) + .num_nanoseconds() + .unwrap() + < 1_000, + "VPC Subnet rows should be equal", + ); + assert_eq!( + left.identity.time_deleted, right.identity.time_deleted, + "VPC Subnet rows should be equal", + ); + assert_eq!( + left.vpc_id, right.vpc_id, + "VPC Subnet rows should be equal" + ); + assert_eq!(left.rcgen, right.rcgen, "VPC Subnet rows should be equal"); + assert_eq!( + left.ipv4_block, right.ipv4_block, + "VPC Subnet rows should be equal" + ); + assert_eq!( + left.ipv6_block, right.ipv6_block, + "VPC Subnet rows should be equal" + ); + assert_eq!( + left.custom_router_id, right.custom_router_id, + "VPC Subnet rows should be equal" + ); + } + + // Regression test for https://github.com/oxidecomputer/omicron/issues/6069. + #[tokio::test] + async fn test_insert_vpc_subnet_query_is_idempotent() { + let ipv4_block = "172.30.0.0/24".parse().unwrap(); + let ipv6_block = "fd12:3456:7890::/64".parse().unwrap(); + let name = "a-name".to_string().try_into().unwrap(); + let description = "some description".to_string(); + let identity = IdentityMetadataCreateParams { name, description }; + let vpc_id = "d402369d-c9ec-c5ad-9138-9fbee732d53e".parse().unwrap(); + let subnet_id = "093ad2db-769b-e3c2-bc1c-b46e84ce5532".parse().unwrap(); + let row = + VpcSubnet::new(subnet_id, vpc_id, identity, ipv4_block, ipv6_block); + + // Setup the test database + let logctx = + dev::test_setup_log("test_insert_vpc_subnet_query_is_idempotent"); + let log = logctx.log.new(o!()); + let mut db = test_setup_database(&log).await; + let cfg = crate::db::Config { url: db.pg_config().clone() }; + let pool = Arc::new(crate::db::Pool::new(&logctx.log, &cfg)); + let db_datastore = Arc::new( + crate::db::DataStore::new(&log, Arc::clone(&pool), None) + .await + .unwrap(), + ); + + // We should be able to insert anything into an empty table. + let inserted = db_datastore + .vpc_create_subnet_raw(row.clone()) + .await + .expect("Should be able to insert VPC subnet into empty table"); + assert_rows_eq(&inserted, &row); + + // We should be able to insert the exact same row again. The IP ranges + // overlap, but the ID is also identical, which should not be an error. + // This is important for saga idempotency. + let inserted = db_datastore + .vpc_create_subnet_raw(row.clone()) + .await + .expect( + "Must be able to insert the exact same VPC subnet more than once", + ); + assert_rows_eq(&inserted, &row); + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } } diff --git a/nexus/src/app/sagas/vpc_create.rs b/nexus/src/app/sagas/vpc_create.rs index a34b25ceb7..832ca64ace 100644 --- a/nexus/src/app/sagas/vpc_create.rs +++ b/nexus/src/app/sagas/vpc_create.rs @@ -8,7 +8,7 @@ use super::NexusSaga; use super::ACTION_GENERATE_ID; use crate::app::sagas::declare_saga_actions; use crate::external_api::params; -use nexus_db_queries::db::queries::vpc_subnet::SubnetError; +use nexus_db_queries::db::queries::vpc_subnet::InsertVpcSubnetError; use nexus_db_queries::{authn, authz, db}; use nexus_defaults as defaults; use omicron_common::api::external; @@ -368,7 +368,7 @@ async fn svc_create_subnet( .vpc_create_subnet(&opctx, &authz_vpc, subnet) .await .map_err(|err| match err { - SubnetError::OverlappingIpRange(ip) => { + InsertVpcSubnetError::OverlappingIpRange(ip) => { let ipv4_block = &defaults::DEFAULT_VPC_SUBNET_IPV4_BLOCK; let log = sagactx.user_data().log(); error!( @@ -388,7 +388,7 @@ async fn svc_create_subnet( found overlapping IP address ranges", ) } - SubnetError::External(e) => e, + InsertVpcSubnetError::External(e) => e, }) .map_err(ActionError::action_failed) } diff --git a/nexus/src/app/vpc_subnet.rs b/nexus/src/app/vpc_subnet.rs index ce0cd423f4..39b9844799 100644 --- a/nexus/src/app/vpc_subnet.rs +++ b/nexus/src/app/vpc_subnet.rs @@ -13,7 +13,7 @@ use nexus_db_queries::db::identity::Resource; use nexus_db_queries::db::lookup; use nexus_db_queries::db::lookup::LookupPath; use nexus_db_queries::db::model::VpcSubnet; -use nexus_db_queries::db::queries::vpc_subnet::SubnetError; +use nexus_db_queries::db::queries::vpc_subnet::InsertVpcSubnetError; use omicron_common::api::external; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::CreateResult; @@ -24,6 +24,7 @@ use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::NameOrId; use omicron_common::api::external::UpdateResult; +use oxnet::IpNet; use uuid::Uuid; impl super::Nexus { @@ -141,9 +142,9 @@ impl super::Nexus { // Note that we only catch IPv6 overlaps. The client // always specifies the IPv4 range, so we fail the // request if that overlaps with an existing range. - Err(SubnetError::OverlappingIpRange(ip)) - if retry <= NUM_RETRIES && ip.is_ipv6() => - { + Err(InsertVpcSubnetError::OverlappingIpRange( + IpNet::V6(_), + )) if retry <= NUM_RETRIES => { debug!( self.log, "autogenerated random IPv6 range overlap"; @@ -157,9 +158,9 @@ impl super::Nexus { } }; match result { - Err(SubnetError::OverlappingIpRange(ip)) - if ip.is_ipv6() => - { + Err(InsertVpcSubnetError::OverlappingIpRange( + IpNet::V6(_), + )) => { // TODO-monitoring TODO-debugging // // We should maintain a counter for this occurrence, and @@ -181,11 +182,11 @@ impl super::Nexus { for VPC Subnet", )) } - Err(SubnetError::OverlappingIpRange(_)) => { + Err(InsertVpcSubnetError::OverlappingIpRange(_)) => { // Overlapping IPv4 ranges, which is always a client error. Err(result.unwrap_err().into_external()) } - Err(SubnetError::External(e)) => Err(e), + Err(InsertVpcSubnetError::External(e)) => Err(e), Ok((.., subnet)) => Ok(subnet), } } @@ -210,7 +211,7 @@ impl super::Nexus { .vpc_create_subnet(opctx, &authz_vpc, subnet) .await .map(|(.., subnet)| subnet) - .map_err(SubnetError::into_external) + .map_err(InsertVpcSubnetError::into_external) } }?;