diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 1947f46a9a6..dc7b684dfc5 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -35,10 +35,7 @@ use crate::context::OpContext; use crate::db::fixed_data::role_assignment_builtin::BUILTIN_ROLE_ASSIGNMENTS; use crate::db::fixed_data::role_builtin::BUILTIN_ROLES; use crate::external_api::params; -use async_bb8_diesel::{ - AsyncConnection, AsyncRunQueryDsl, ConnectionError, ConnectionManager, - PoolError, -}; +use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl, ConnectionManager}; use chrono::Utc; use diesel::pg::Pg; use diesel::prelude::*; @@ -2537,29 +2534,13 @@ impl DataStore { subnet: VpcSubnet, ) -> Result { use db::schema::vpc_subnet::dsl; - let name = subnet.name().clone(); - let values = FilterConflictingVpcSubnetRangesQuery(subnet); + let values = FilterConflictingVpcSubnetRangesQuery(subnet.clone()); diesel::insert_into(dsl::vpc_subnet) .values(values) .returning(VpcSubnet::as_returning()) .get_result_async(self.pool()) .await - .map_err(|err| { - if let PoolError::Connection(ConnectionError::Query( - diesel::result::Error::NotFound, - )) = err - { - SubnetError::OverlappingIpRange - } else { - SubnetError::External(public_error_from_diesel_pool( - err, - ErrorHandler::Conflict( - ResourceType::VpcSubnet, - name.to_string().as_str(), - ), - )) - } - }) + .map_err(|e| SubnetError::from_pool(e, &subnet)) } pub async fn vpc_delete_subnet( diff --git a/nexus/src/db/subnet_allocation.rs b/nexus/src/db/subnet_allocation.rs index 991dc249024..e6ecf5ec02a 100644 --- a/nexus/src/db/subnet_allocation.rs +++ b/nexus/src/db/subnet_allocation.rs @@ -7,6 +7,7 @@ use crate::db; use crate::db::identity::Resource; use crate::db::model::IncompleteNetworkInterface; +use crate::db::model::VpcSubnet; use chrono::{DateTime, Utc}; use diesel::pg::Pg; use diesel::prelude::*; @@ -41,12 +42,166 @@ fn generate_last_address_offset(subnet: &ipnetwork::IpNetwork) -> i64 { } /// Errors related to allocating VPC Subnets. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum SubnetError { - OverlappingIpRange, + /// 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_pool( + e: async_bb8_diesel::PoolError, + subnet: &VpcSubnet, + ) -> Self { + use crate::db::error; + use async_bb8_diesel::ConnectionError; + use async_bb8_diesel::PoolError; + use diesel::result::DatabaseErrorKind; + use diesel::result::Error; + 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 + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError( + DatabaseErrorKind::NotNullViolation, + ref info, + ), + )) if info.message() == IPV4_OVERLAP_ERROR_MESSAGE => { + SubnetError::OverlappingIpRange(subnet.ipv4_block.0 .0.into()) + } + + // Attempt to insert overlapping IPv6 subnet + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError( + DatabaseErrorKind::NotNullViolation, + ref info, + ), + )) if info.message() == IPV6_OVERLAP_ERROR_MESSAGE => { + SubnetError::OverlappingIpRange(subnet.ipv6_block.0 .0.into()) + } + + // Conflicting name for the subnet within a VPC + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError( + DatabaseErrorKind::UniqueViolation, + ref info, + ), + )) if info.constraint_name() == Some(NAME_CONFLICT_CONSTRAINT) => { + SubnetError::External(error::public_error_from_diesel_pool( + 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_pool( + 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. +/// +/// This generates a query that, in full, looks like: +/// +/// ```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( + mut out: AstPass, + vpc_id: &Uuid, + ip: &ipnetwork::IpNetwork, +) -> diesel::QueryResult<()> { + use crate::db::schema::vpc_subnet::dsl; + out.push_sql("SELECT "); + out.push_bind_param::(ip)?; + out.push_sql(" FROM "); + dsl::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. +/// +/// This specifically generates a query that looks like: +/// +/// ```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( + mut out: AstPass, + vpc_id: &Uuid, + ip: &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. @@ -61,9 +216,7 @@ pub enum SubnetError { /// time_created, /// time_modified, /// time_deleted, -/// vpc_id, -/// ipv4_block, -/// ipv6_block +/// vpc_id /// ) AS (VALUES ( /// , /// , @@ -72,24 +225,32 @@ pub enum SubnetError { /// , /// NULL::TIMESTAMPTZ, /// , -/// , -/// -/// )) -/// SELECT * -/// FROM candidate -/// WHERE NOT EXISTS ( -/// SELECT ipv4_block, ipv6_block -/// FROM vpc_subnet -/// WHERE -/// vpc_id = AND -/// time_deleted IS NULL AND -/// ( -/// inet_contains_or_equals(ipv4_block, candidate.ipv4_block) OR -/// inet_contains_or_equals(ipv6_block, candidate.ipv6_block) -/// ) +/// )), +/// 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 +/// ) +/// ) +/// ) +/// ), +/// candidate_ipv6(ipv6_block) AS ( +/// /// ) +/// SELECT * +/// FROM candidate, candidate_ipv4, candidate_ipv6 /// ``` -pub struct FilterConflictingVpcSubnetRangesQuery(pub db::model::VpcSubnet); +pub struct FilterConflictingVpcSubnetRangesQuery(pub VpcSubnet); impl QueryId for FilterConflictingVpcSubnetRangesQuery { type QueryId = (); @@ -100,151 +261,68 @@ impl QueryFragment for FilterConflictingVpcSubnetRangesQuery { fn walk_ast(&self, mut out: AstPass) -> diesel::QueryResult<()> { use db::schema::vpc_subnet::dsl; - // "SELECT * FROM (WITH candidate(" + // Create the base `candidate` from values provided that need no + // verificiation. out.push_sql("SELECT * FROM (WITH candidate("); - - // "id, " out.push_identifier(dsl::id::NAME)?; out.push_sql(", "); - - // "name, " out.push_identifier(dsl::name::NAME)?; out.push_sql(", "); - - // "description, " out.push_identifier(dsl::description::NAME)?; out.push_sql(", "); - - // "time_created, " out.push_identifier(dsl::time_created::NAME)?; out.push_sql(", "); - - // "time_modified, " out.push_identifier(dsl::time_modified::NAME)?; out.push_sql(", "); - - // "time_deleted, " out.push_identifier(dsl::time_deleted::NAME)?; out.push_sql(", "); - - // "vpc_id, " out.push_identifier(dsl::vpc_id::NAME)?; - out.push_sql(", "); - - // "ipv4_block, " - out.push_identifier(dsl::ipv4_block::NAME)?; - out.push_sql(", "); - - // "ipv6_block) AS (VALUES (" - out.push_identifier(dsl::ipv6_block::NAME)?; out.push_sql(") AS (VALUES ("); - - // ", " out.push_bind_param::(&self.0.id())?; out.push_sql(", "); - - // ", " out.push_bind_param::( &self.0.name().to_string(), )?; out.push_sql(", "); - - // ", " out.push_bind_param::(&self.0.description())?; out.push_sql(", "); - - // ", " out.push_bind_param::>( &self.0.time_created(), )?; out.push_sql(", "); - - // ", " out.push_bind_param::>( &self.0.time_modified(), )?; out.push_sql(", "); - - // "NULL::TIMESTAMPTZ, " out.push_sql("NULL::TIMESTAMPTZ, "); - - // ", " out.push_bind_param::(&self.0.vpc_id)?; - out.push_sql(", "); - - // , " - out.push_bind_param::( - &ipnetwork::IpNetwork::from(self.0.ipv4_block.0 .0), - )?; - out.push_sql(", "); + out.push_sql(")), "); - // ")" - out.push_bind_param::( - &ipnetwork::IpNetwork::from(self.0.ipv6_block.0 .0), - )?; - out.push_sql("))"); - - /* - * Possibly filter the candidate row. - * - * This selects everything in the `candidate` CTE, where there is no - * "overlapping" row in the `vpc_subnet` table. Specifically, we search - * that table for rows with: - * - * - The same `vpc_id` - * - Not soft-deleted - * - The IPv4 range overlaps _or_ the IPv6 range overlaps - * - * Those are removed from `candidate`. - */ - - // " SELECT * FROM candidate WHERE NOT EXISTS (" - out.push_sql(" SELECT * FROM candidate WHERE NOT EXISTS ("); - - // " SELECT " - out.push_sql("SELECT "); - - // "ipv4_block, " + // 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(", "); - - // "ipv6_block " - out.push_identifier(dsl::ipv6_block::NAME)?; - - // "FROM vpc_subnet" - out.push_sql(" FROM "); - dsl::vpc_subnet.from_clause().walk_ast(out.reborrow())?; - - // " WHERE " - out.push_sql(" WHERE "); - - // "vpc_id = " - out.push_identifier(dsl::vpc_id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::(&self.0.vpc_id)?; - - // " AND time_deleted IS NULL AND ((" - out.push_sql(" AND "); - out.push_identifier(dsl::time_deleted::NAME)?; - out.push_sql(" IS NULL AND ("); - - // "inet_contains_or_equals(ipv4_block, - out.push_sql("inet_contains_or_equals("); - out.push_identifier(dsl::ipv4_block::NAME)?; - out.push_sql(", "); - out.push_bind_param::( + out.push_sql(") AS ("); + push_null_if_overlapping_ip_range( + out.reborrow(), + &self.0.vpc_id, &ipnetwork::IpNetwork::from(self.0.ipv4_block.0 .0), )?; - // ") OR inet_contains_or_equals(ipv6_block, ))" - out.push_sql(") OR inet_contains_or_equals("); + out.push_sql("), candidate_ipv6("); out.push_identifier(dsl::ipv6_block::NAME)?; - out.push_sql(", "); - out.push_bind_param::( + out.push_sql(") AS ("); + push_null_if_overlapping_ip_range( + out.reborrow(), + &self.0.vpc_id, &ipnetwork::IpNetwork::from(self.0.ipv6_block.0 .0), )?; - out.push_sql("))))"); + out.push_sql(") "); + // Select the entire set of candidate columns. + out.push_sql( + "SELECT * FROM candidate, candidate_ipv4, candidate_ipv6)", + ); Ok(()) } } @@ -1054,13 +1132,12 @@ impl QueryFragment for InsertNetworkInterfaceQueryValues { #[cfg(test)] mod test { - use super::FilterConflictingVpcSubnetRangesQuery; use super::NetworkInterfaceError; use super::SubnetError; use crate::db::model::{ self, IncompleteNetworkInterface, NetworkInterface, VpcSubnet, }; - use diesel::pg::Pg; + use ipnetwork::IpNetwork; use nexus_test_utils::db::test_setup_database; use omicron_common::api::external::{ Error, IdentityMetadataCreateParams, Ipv4Net, Ipv6Net, Name, @@ -1070,46 +1147,6 @@ mod test { use std::sync::Arc; use uuid::Uuid; - #[test] - fn test_filter_conflicting_vpc_subnet_ranges_query_string() { - use crate::db::identity::Resource; - let ipv4_block = Ipv4Net("172.30.0.0/22".parse().unwrap()); - let ipv6_block = Ipv6Net("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 = Uuid::new_v4(); - let subnet_id = Uuid::new_v4(); - let row = - VpcSubnet::new(subnet_id, vpc_id, identity, ipv4_block, ipv6_block); - let query = FilterConflictingVpcSubnetRangesQuery(row.clone()); - let query_str = diesel::debug_query::(&query).to_string(); - let expected_query = format!( - concat!( - "SELECT * FROM (WITH candidate(", - r#""id", "name", "description", "time_created", "time_modified", "#, - r#""time_deleted", "vpc_id", "ipv4_block", "ipv6_block") AS "#, - "(VALUES ($1, $2, $3, $4, $5, NULL::TIMESTAMPTZ, $6, $7, $8)) ", - "SELECT * FROM candidate WHERE NOT EXISTS (", - r#"SELECT "ipv4_block", "ipv6_block" FROM "vpc_subnet" WHERE "#, - r#""vpc_id" = $9 AND "time_deleted" IS NULL AND ("#, - r#"inet_contains_or_equals("ipv4_block", $10) OR inet_contains_or_equals("ipv6_block", $11)))) "#, - r#"-- binds: [{subnet_id}, "{name}", "{description}", {time_created:?}, "#, - r#"{time_modified:?}, {vpc_id}, V4({ipv4_block:?}), V6({ipv6_block:?}), "#, - r#"{vpc_id}, V4({ipv4_block:?}), V6({ipv6_block:?})]"#, - ), - subnet_id = row.id(), - name = row.name(), - description = row.description(), - vpc_id = row.vpc_id, - ipv4_block = row.ipv4_block.0 .0, - ipv6_block = row.ipv6_block.0 .0, - time_created = row.time_created(), - time_modified = row.time_modified(), - ); - assert_eq!(query_str, expected_query); - } - #[tokio::test] async fn test_filter_conflicting_vpc_subnet_ranges_query() { let make_id = @@ -1162,9 +1199,9 @@ mod test { assert!( matches!( db_datastore.vpc_create_subnet_raw(new_row).await, - Err(SubnetError::OverlappingIpRange) + Err(SubnetError::OverlappingIpRange(IpNetwork::V4(_))) ), - "Should not be able to insert new VPC subnet with the same IP ranges" + "Should not be able to insert new VPC subnet with the same IPv4 and IPv6 ranges" ); // We should be able to insert data with the same ranges, if we change @@ -1190,12 +1227,14 @@ mod test { other_ipv4_block, ipv6_block, ); - assert!( - matches!( - db_datastore.vpc_create_subnet_raw(new_row).await, - Err(SubnetError::OverlappingIpRange), - ), - "Should not be able to insert VPC Subnet with overlapping IPv4 range" + let err = db_datastore + .vpc_create_subnet_raw(new_row) + .await + .expect_err("Should not be able to insert VPC Subnet with overlapping IPv6 range"); + assert_eq!( + err, + SubnetError::OverlappingIpRange(IpNetwork::from(ipv6_block.0)), + "SubnetError variant should include the exact IP range that overlaps" ); let new_row = VpcSubnet::new( other_subnet_id, @@ -1204,12 +1243,14 @@ mod test { ipv4_block, other_ipv6_block, ); - assert!( - matches!( - db_datastore.vpc_create_subnet_raw(new_row).await, - Err(SubnetError::OverlappingIpRange), - ), - "Should not be able to insert VPC Subnet with overlapping IPv6 range" + let err = db_datastore + .vpc_create_subnet_raw(new_row) + .await + .expect_err("Should not be able to insert VPC Subnet with overlapping IPv4 range"); + assert_eq!( + err, + SubnetError::OverlappingIpRange(IpNetwork::from(ipv4_block.0)), + "SubnetError variant should include the exact IP range that overlaps" ); // We should get an _external error_ if the IP address ranges are OK, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 232b64bccb5..6b921cd0661 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1822,12 +1822,15 @@ impl Nexus { .vpc_create_subnet(opctx, &authz_vpc, subnet) .await .map_err(|err| match err { - SubnetError::OverlappingIpRange => { + SubnetError::OverlappingIpRange(ip) => { let ipv4_block = &defaults::DEFAULT_VPC_SUBNET_IPV4_BLOCK; - warn!( + error!( self.log, - "failed to create default VPC Subnet, \ - found overlapping IP address ranges"; + concat!( + "failed to create default VPC Subnet, IP address ", + "range '{}' overlaps with existing", + ), + ip; "vpc_id" => ?vpc_id, "subnet_id" => ?default_subnet_id, "ipv4_block" => ?**ipv4_block, @@ -2080,8 +2083,12 @@ impl Nexus { .await; match result { // Allow NUM_RETRIES retries, after the first attempt. - Err(SubnetError::OverlappingIpRange) - if retry <= NUM_RETRIES => + // + // 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() => { debug!( self.log, @@ -2096,7 +2103,9 @@ impl Nexus { } }; match result { - Err(SubnetError::OverlappingIpRange) => { + Err(SubnetError::OverlappingIpRange(ip)) + if ip.is_ipv6() => + { // TODO-monitoring TODO-debugging // // We should maintain a counter for this occurrence, and @@ -2105,7 +2114,7 @@ impl Nexus { // goal here is for us to notice that this is happening // before it becomes a major issue for customers. let vpc_id = authz_vpc.id(); - warn!( + error!( self.log, "failed to generate unique random IPv6 address \ range in {} retries", @@ -2118,6 +2127,10 @@ impl Nexus { for VPC Subnet", )) } + Err(SubnetError::OverlappingIpRange(_)) => { + // Overlapping IPv4 ranges, which is always a client error. + Err(result.unwrap_err().into_external()) + } Err(SubnetError::External(e)) => Err(e), Ok(subnet) => Ok(subnet), } @@ -2142,19 +2155,7 @@ impl Nexus { self.db_datastore .vpc_create_subnet(opctx, &authz_vpc, subnet) .await - .map_err(|err| match err { - SubnetError::OverlappingIpRange => { - external::Error::invalid_request(&format!( - concat!( - "IPv4 block '{}' and/or IPv6 block '{}' ", - "overlaps with existing VPC Subnet ", - "IP address ranges" - ), - params.ipv4_block, ipv6_block, - )) - } - SubnetError::External(e) => e, - }) + .map_err(SubnetError::into_external) } } } diff --git a/nexus/tests/integration_tests/vpc_subnets.rs b/nexus/tests/integration_tests/vpc_subnets.rs index ae507ee54c6..e66192919b0 100644 --- a/nexus/tests/integration_tests/vpc_subnets.rs +++ b/nexus/tests/integration_tests/vpc_subnets.rs @@ -157,6 +157,10 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { ipv4_block, ipv6_block, }; + let expected_error = format!( + "IP address range '{}' conflicts with an existing subnet", + ipv4_block, + ); let error: dropshot::HttpErrorResponseBody = NexusRequest::new( RequestBuilder::new(client, Method::POST, &subnets_url) .expect_status(Some(StatusCode::BAD_REQUEST)) @@ -168,7 +172,7 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { .unwrap() .parsed_body() .unwrap(); - assert!(error.message.starts_with("IPv4 block '")); + assert_eq!(error.message, expected_error); // creating another subnet in the same VPC with the same name, but different // IP address ranges also fails.