From 61c73fd67112cb4991796cabea004a2bc835c983 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 17 Aug 2022 01:36:00 -0400 Subject: [PATCH 1/7] [nexus] Support allocating external IP addresses for internal services --- common/src/sql/dbinit.sql | 17 +- nexus/db-model/src/external_ip.rs | 59 ++++- nexus/db-model/src/schema.rs | 2 +- .../src/db/datastore/instance_external_ip.rs | 15 ++ nexus/src/db/datastore/mod.rs | 6 +- nexus/src/db/queries/external_ip.rs | 246 ++++++++++++++++-- 6 files changed, 296 insertions(+), 49 deletions(-) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 1f9481dcd14..7fdb790e9e1 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -1105,7 +1105,13 @@ CREATE TYPE omicron.public.ip_kind AS ENUM ( * known address that can be moved between instances. Its lifetime is not * fixed to any instance. */ - 'floating' + 'floating', + + /* + * A service IP is an IP address not attached to a project nor an instance. + * It's intended to be used for internal services. + */ + 'service' ); /* @@ -1132,7 +1138,7 @@ CREATE TABLE omicron.public.instance_external_ip ( ip_pool_range_id UUID NOT NULL, /* FK to the `project` table. */ - project_id UUID NOT NULL, + project_id UUID, /* FK to the `instance` table. See the constraints below. */ instance_id UUID, @@ -1162,12 +1168,13 @@ CREATE TABLE omicron.public.instance_external_ip ( ), /* - * Only nullable if this is a floating IP, which may exist not attached - * to any instance. + * Only nullable if this is a floating/service IP, which may exist not + * attached to any instance. */ CONSTRAINT null_non_fip_instance_id CHECK ( (kind != 'floating' AND instance_id IS NOT NULL) OR - (kind = 'floating') + (kind = 'floating') OR + (kind = 'service') ) ); diff --git a/nexus/db-model/src/external_ip.rs b/nexus/db-model/src/external_ip.rs index 969ba50bf80..4d16a74c50b 100644 --- a/nexus/db-model/src/external_ip.rs +++ b/nexus/db-model/src/external_ip.rs @@ -32,6 +32,7 @@ impl_enum_type!( SNat => b"snat" Ephemeral => b"ephemeral" Floating => b"floating" + Service => b"service" ); /// The main model type for external IP addresses for instances. @@ -56,7 +57,7 @@ pub struct InstanceExternalIp { pub time_deleted: Option>, pub ip_pool_id: Uuid, pub ip_pool_range_id: Uuid, - pub project_id: Uuid, + pub project_id: Option, // This is Some(_) for: // - all instance SNAT IPs // - all ephemeral IPs @@ -78,6 +79,18 @@ impl From for sled_agent_client::types::SourceNatConfig { } } +/// Describes where the IP candidates for allocation come from: either +/// from an IP pool, or from a project. +/// +/// This ensures that a source is always specified, and a caller cannot +/// request an external IP allocation without providing at least one of +/// these options. +#[derive(Debug, Clone, Copy)] +pub enum IpSource { + Pool(Uuid), + Project(Uuid), +} + /// An incomplete external IP, used to store state required for issuing the /// database query that selects an available IP and stores the resulting record. #[derive(Debug, Clone)] @@ -87,9 +100,9 @@ pub struct IncompleteInstanceExternalIp { description: Option, time_created: DateTime, kind: IpKind, - project_id: Uuid, + project_id: Option, instance_id: Option, - pool_id: Option, + source: IpSource, } impl IncompleteInstanceExternalIp { @@ -99,15 +112,18 @@ impl IncompleteInstanceExternalIp { instance_id: Uuid, pool_id: Option, ) -> Self { + let source = pool_id + .map(|id| IpSource::Pool(id)) + .unwrap_or_else(|| IpSource::Project(project_id)); Self { id, name: None, description: None, time_created: Utc::now(), kind: IpKind::SNat, - project_id, + project_id: Some(project_id), instance_id: Some(instance_id), - pool_id, + source, } } @@ -117,15 +133,18 @@ impl IncompleteInstanceExternalIp { instance_id: Uuid, pool_id: Option, ) -> Self { + let source = pool_id + .map(|id| IpSource::Pool(id)) + .unwrap_or_else(|| IpSource::Project(project_id)); Self { id, name: None, description: None, time_created: Utc::now(), kind: IpKind::Ephemeral, - project_id, + project_id: Some(project_id), instance_id: Some(instance_id), - pool_id, + source, } } @@ -136,15 +155,31 @@ impl IncompleteInstanceExternalIp { project_id: Uuid, pool_id: Option, ) -> Self { + let source = pool_id + .map(|id| IpSource::Pool(id)) + .unwrap_or_else(|| IpSource::Project(project_id)); Self { id, name: Some(name.clone()), description: Some(description.to_string()), time_created: Utc::now(), kind: IpKind::Floating, - project_id, + project_id: Some(project_id), + instance_id: None, + source, + } + } + + pub fn for_service(id: Uuid, pool_id: Uuid) -> Self { + Self { + id, + name: None, + description: None, + time_created: Utc::now(), + kind: IpKind::Service, + project_id: None, instance_id: None, - pool_id, + source: IpSource::Pool(pool_id), } } @@ -168,7 +203,7 @@ impl IncompleteInstanceExternalIp { &self.kind } - pub fn project_id(&self) -> &Uuid { + pub fn project_id(&self) -> &Option { &self.project_id } @@ -176,8 +211,8 @@ impl IncompleteInstanceExternalIp { &self.instance_id } - pub fn pool_id(&self) -> &Option { - &self.pool_id + pub fn source(&self) -> &IpSource { + &self.source } } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index ae0f355edb7..cfa74483ad5 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -173,7 +173,7 @@ table! { time_deleted -> Nullable, ip_pool_id -> Uuid, ip_pool_range_id -> Uuid, - project_id -> Uuid, + project_id -> Nullable, instance_id -> Nullable, kind -> crate::IpKindEnum, ip -> Inet, diff --git a/nexus/src/db/datastore/instance_external_ip.rs b/nexus/src/db/datastore/instance_external_ip.rs index 66574d215ea..a2f44b6ce26 100644 --- a/nexus/src/db/datastore/instance_external_ip.rs +++ b/nexus/src/db/datastore/instance_external_ip.rs @@ -20,6 +20,7 @@ use crate::db::update_and_check::UpdateStatus; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; use diesel::prelude::*; +use nexus_types::identity::Resource; use omicron_common::api::external::CreateResult; use omicron_common::api::external::Error; use omicron_common::api::external::LookupResult; @@ -101,6 +102,20 @@ impl DataStore { self.allocate_instance_external_ip(opctx, data).await } + /// Allocates an IP address for internal service usage. + pub async fn allocate_service_ip( + &self, + opctx: &OpContext, + ip_id: Uuid, + rack_id: Uuid, + ) -> CreateResult { + let (.., pool) = + self.ip_pools_lookup_by_rack_id(opctx, rack_id).await?; + + let data = IncompleteInstanceExternalIp::for_service(ip_id, pool.id()); + self.allocate_instance_external_ip(opctx, data).await + } + async fn allocate_instance_external_ip( &self, opctx: &OpContext, diff --git a/nexus/src/db/datastore/mod.rs b/nexus/src/db/datastore/mod.rs index 22d72c6a0e8..0c3cba06803 100644 --- a/nexus/src/db/datastore/mod.rs +++ b/nexus/src/db/datastore/mod.rs @@ -1034,7 +1034,7 @@ mod test { time_deleted: None, ip_pool_id: Uuid::new_v4(), ip_pool_range_id: Uuid::new_v4(), - project_id: Uuid::new_v4(), + project_id: Some(Uuid::new_v4()), instance_id: Some(instance_id), kind: IpKind::Ephemeral, ip: ipnetwork::IpNetwork::from(IpAddr::from(Ipv4Addr::new( @@ -1094,7 +1094,7 @@ mod test { time_deleted: None, ip_pool_id: Uuid::new_v4(), ip_pool_range_id: Uuid::new_v4(), - project_id: Uuid::new_v4(), + project_id: Some(Uuid::new_v4()), instance_id: Some(Uuid::new_v4()), kind: IpKind::SNat, ip: ipnetwork::IpNetwork::from(IpAddr::from(Ipv4Addr::new( @@ -1169,7 +1169,7 @@ mod test { time_deleted: None, ip_pool_id: Uuid::new_v4(), ip_pool_range_id: Uuid::new_v4(), - project_id: Uuid::new_v4(), + project_id: Some(Uuid::new_v4()), instance_id: Some(Uuid::new_v4()), kind: IpKind::Floating, ip: addresses.next().unwrap().into(), diff --git a/nexus/src/db/queries/external_ip.rs b/nexus/src/db/queries/external_ip.rs index 7e032e38a65..7b42aa9af95 100644 --- a/nexus/src/db/queries/external_ip.rs +++ b/nexus/src/db/queries/external_ip.rs @@ -9,6 +9,7 @@ use crate::db::model::IncompleteInstanceExternalIp; use crate::db::model::InstanceExternalIp; use crate::db::model::IpKind; use crate::db::model::IpKindEnum; +use crate::db::model::IpSource; use crate::db::model::Name; use crate::db::pool::DbConnection; use crate::db::schema; @@ -122,7 +123,7 @@ const MAX_PORT: i32 = u16::MAX as _; /// (ip, first_port, time_deleted IS NULL) = /// (candidate_ip, candidate_first_port, TRUE) /// WHERE -/// ip IS NULL +/// (ip IS NULL) OR (id = ) /// ORDER BY /// candidate_ip, candidate_first_port /// LIMIT 1 @@ -134,8 +135,8 @@ const MAX_PORT: i32 = u16::MAX as _; /// -- possible on replay of a saga node. /// INSERT INTO /// instance_external_ip -/// (SELECT * FROM external_ip) -/// ON CONFLICT +/// (SELECT * FROM next_external_ip) +/// ON CONFLICT (id) /// DO UPDATE SET /// time_created = excluded.time_created, /// time_modified = excluded.time_modified, @@ -295,7 +296,7 @@ impl NextExternalIp { out.push_sql(", "); // Project ID - out.push_bind_param::(self.ip.project_id())?; + out.push_bind_param::, Option>(self.ip.project_id())?; out.push_sql(" AS "); out.push_identifier(dsl::project_id::NAME)?; out.push_sql(", "); @@ -390,10 +391,14 @@ impl NextExternalIp { // and possibly first port, but since it's been soft-deleted, it's not a // match. In that case, we can get away with _only_ filtering the join // results on the IP from the `instance_external_ip` table being NULL. - out.push_sql(" WHERE "); + out.push_sql(" WHERE ("); out.push_identifier(dsl::ip::NAME)?; + out.push_sql(" IS NULL) OR ("); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(self.ip.id())?; out.push_sql( - " IS NULL \ + ") \ ORDER BY candidate_ip, candidate_first_port \ LIMIT 1", ); @@ -437,18 +442,21 @@ impl NextExternalIp { out.push_sql(") AS candidate_ip FROM "); IP_POOL_RANGE_FROM_CLAUSE.walk_ast(out.reborrow())?; out.push_sql(" WHERE "); - if let Some(ref pool_id) = self.ip.pool_id() { - out.push_identifier(dsl::ip_pool_id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::(pool_id)?; - } else { - out.push_sql("("); - out.push_identifier(dsl::project_id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::(self.ip.project_id())?; - out.push_sql(" OR "); - out.push_identifier(dsl::project_id::NAME)?; - out.push_sql(" IS NULL)"); + match self.ip.source() { + IpSource::Pool(pool_id) => { + out.push_identifier(dsl::ip_pool_id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(pool_id)?; + } + IpSource::Project(project_id) => { + out.push_sql("("); + out.push_identifier(dsl::project_id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(project_id)?; + out.push_sql(" OR "); + out.push_identifier(dsl::project_id::NAME)?; + out.push_sql(" IS NULL)"); + } } out.push_sql(" AND "); out.push_identifier(dsl::time_deleted::NAME)?; @@ -620,6 +628,7 @@ mod tests { use async_bb8_diesel::AsyncRunQueryDsl; use dropshot::test_util::LogContext; use nexus_test_utils::db::test_setup_database; + use nexus_test_utils::RACK_UUID; use omicron_common::api::external::Error; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_test_utils::dev; @@ -641,6 +650,7 @@ mod tests { let logctx = dev::test_setup_log(test_name); let log = logctx.log.new(o!()); let db = test_setup_database(&log).await; + crate::db::datastore::datastore_test(&logctx, &db).await; let cfg = crate::db::Config { url: db.pg_config().clone() }; let pool = Arc::new(crate::db::Pool::new(&cfg)); let db_datastore = @@ -650,22 +660,21 @@ mod tests { Self { logctx, opctx, db, db_datastore } } - async fn create_ip_pool( + async fn create_ip_pool_internal( &self, name: &str, range: IpRange, project_id: Option, + rack_id: Option, ) { - // Create with no org/project name, set project_id manually. - let mut pool = IpPool::new( + let pool = IpPool::new( &IdentityMetadataCreateParams { name: String::from(name).parse().unwrap(), description: format!("ip pool {}", name), }, - /* project_id= */ None, - /* rack_id= */ None, + project_id, + rack_id, ); - pool.project_id = project_id; diesel::insert_into(crate::db::schema::ip_pool::dsl::ip_pool) .values(pool.clone()) @@ -690,6 +699,33 @@ mod tests { .expect("Failed to create IP Pool range"); } + async fn create_rack_ip_pool( + &self, + name: &str, + range: IpRange, + rack_id: Uuid, + ) { + self.create_ip_pool_internal( + name, + range, + /* project_id= */ None, + Some(rack_id), + ) + .await; + } + + async fn create_ip_pool( + &self, + name: &str, + range: IpRange, + project_id: Option, + ) { + self.create_ip_pool_internal( + name, range, project_id, /* rack_id= */ None, + ) + .await; + } + async fn success(mut self) { self.db.cleanup().await.unwrap(); self.logctx.cleanup_successful(); @@ -943,7 +979,7 @@ mod tests { assert_eq!(ip.ip.ip(), second_range.first_address()); assert_eq!(ip.first_port.0, 0); assert_eq!(ip.last_port.0, u16::MAX); - assert_eq!(ip.project_id, instance_project_id); + assert_eq!(ip.project_id.unwrap(), instance_project_id); // Allocating an address on an instance in the same project should get // an address from the first pool. @@ -966,7 +1002,161 @@ mod tests { assert_eq!(ip.ip.ip(), first_range.first_address()); assert_eq!(ip.first_port.0, 0); assert_eq!(ip.last_port.0, u16::MAX); - assert_eq!(ip.project_id, project_id); + assert_eq!(ip.project_id.unwrap(), project_id); + + context.success().await; + } + + #[tokio::test] + async fn test_next_external_ip_for_service() { + let context = + TestContext::new("test_next_external_ip_for_service").await; + + // Create an IP pool without an associated project. + let rack_id = Uuid::parse_str(RACK_UUID).unwrap(); + let ip_range = IpRange::try_from(( + Ipv4Addr::new(10, 0, 0, 1), + Ipv4Addr::new(10, 0, 0, 2), + )) + .unwrap(); + context.create_rack_ip_pool("p0", ip_range, rack_id).await; + + // Allocate an IP address as we would for an external, rack-associated + // service. + let id1 = Uuid::new_v4(); + let ip1 = context + .db_datastore + .allocate_service_ip(&context.opctx, id1, rack_id) + .await + .expect("Failed to allocate service IP address"); + assert_eq!(ip1.kind, IpKind::Service); + assert_eq!(ip1.ip.ip(), IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))); + assert_eq!(ip1.first_port.0, 0); + assert_eq!(ip1.last_port.0, u16::MAX); + assert!(ip1.instance_id.is_none()); + assert!(ip1.project_id.is_none()); + + // Allocate the next (last) IP address + let id2 = Uuid::new_v4(); + let ip2 = context + .db_datastore + .allocate_service_ip(&context.opctx, id2, rack_id) + .await + .expect("Failed to allocate service IP address"); + assert_eq!(ip2.kind, IpKind::Service); + assert_eq!(ip2.ip.ip(), IpAddr::V4(Ipv4Addr::new(10, 0, 0, 2))); + assert_eq!(ip2.first_port.0, 0); + assert_eq!(ip2.last_port.0, u16::MAX); + assert!(ip2.instance_id.is_none()); + assert!(ip2.project_id.is_none()); + + // Once we're out of IP addresses, test that we see the right error. + let id3 = Uuid::new_v4(); + let err = context + .db_datastore + .allocate_service_ip(&context.opctx, id3, rack_id) + .await + .expect_err("Should have failed to allocate after pool exhausted"); + assert_eq!( + err, + Error::InvalidRequest { + message: String::from( + // TODO: The error is a bit misleading; this isn't an IP + // intended for an instance necessarily. + "No external IP addresses available for new instance" + ), + } + ); + + context.success().await; + } + + #[tokio::test] + async fn test_insert_external_ip_for_service_is_idempoent() { + let context = TestContext::new( + "test_insert_external_ip_for_service_is_idempotent", + ) + .await; + + // Create an IP pool without an associated project. + let rack_id = Uuid::parse_str(RACK_UUID).unwrap(); + let ip_range = IpRange::try_from(( + Ipv4Addr::new(10, 0, 0, 1), + Ipv4Addr::new(10, 0, 0, 2), + )) + .unwrap(); + context.create_rack_ip_pool("p0", ip_range, rack_id).await; + + // Allocate an IP address as we would for an external, rack-associated + // service. + let id = Uuid::new_v4(); + let ip = context + .db_datastore + .allocate_service_ip(&context.opctx, id, rack_id) + .await + .expect("Failed to allocate service IP address"); + assert_eq!(ip.kind, IpKind::Service); + assert_eq!(ip.ip.ip(), IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))); + assert_eq!(ip.first_port.0, 0); + assert_eq!(ip.last_port.0, u16::MAX); + assert!(ip.instance_id.is_none()); + assert!(ip.project_id.is_none()); + + let ip_again = context + .db_datastore + .allocate_service_ip(&context.opctx, id, rack_id) + .await + .expect("Failed to allocate service IP address"); + + assert_eq!(ip.id, ip_again.id); + assert_eq!(ip.ip.ip(), ip_again.ip.ip()); + + context.success().await; + } + + // This test is identical to "test_insert_external_ip_is_idempotent", + // but tries to make an idempotent allocation after all addresses in the + // pool have been allocated. + #[tokio::test] + async fn test_insert_external_ip_for_service_is_idempotent_even_when_full() + { + let context = TestContext::new( + "test_insert_external_ip_is_idempotent_even_when_full", + ) + .await; + + // Create an IP pool without an associated project. + let rack_id = Uuid::parse_str(RACK_UUID).unwrap(); + let ip_range = IpRange::try_from(( + Ipv4Addr::new(10, 0, 0, 1), + Ipv4Addr::new(10, 0, 0, 1), + )) + .unwrap(); + context.create_rack_ip_pool("p0", ip_range, rack_id).await; + + // Allocate an IP address as we would for an external, rack-associated + // service. + let id = Uuid::new_v4(); + let ip = context + .db_datastore + .allocate_service_ip(&context.opctx, id, rack_id) + .await + .expect("Failed to allocate service IP address"); + assert_eq!(ip.kind, IpKind::Service); + assert_eq!(ip.ip.ip(), IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))); + assert_eq!(ip.first_port.0, 0); + assert_eq!(ip.last_port.0, u16::MAX); + assert!(ip.instance_id.is_none()); + assert!(ip.project_id.is_none()); + + let ip_again = context + .db_datastore + .allocate_service_ip(&context.opctx, id, rack_id) + .await + .expect("Failed to allocate service IP address"); + + assert_eq!(ip.id, ip_again.id); + assert_eq!(ip.ip.ip(), ip_again.ip.ip()); context.success().await; } @@ -1005,7 +1195,7 @@ mod tests { usize::from(ip.last_port.0), super::NUM_SOURCE_NAT_PORTS - 1 ); - assert_eq!(ip.project_id, project_id); + assert_eq!(ip.project_id.unwrap(), project_id); // Create a new IP, with the _same_ ID, and ensure we get back the same // value. @@ -1079,7 +1269,7 @@ mod tests { assert_eq!(ip.ip.ip(), second_range.first_address()); assert_eq!(ip.first_port.0, 0); assert_eq!(ip.last_port.0, u16::MAX); - assert_eq!(ip.project_id, project_id); + assert_eq!(ip.project_id.unwrap(), project_id); context.success().await; } From 4c25f5b2315847c041340b78c3d26801bb4d17f8 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Aug 2022 13:43:12 -0400 Subject: [PATCH 2/7] de-dup project id from ipsource --- nexus/db-model/src/external_ip.rs | 30 +++++------------------- nexus/src/db/queries/external_ip.rs | 36 ++++++++++++++++++++--------- 2 files changed, 31 insertions(+), 35 deletions(-) diff --git a/nexus/db-model/src/external_ip.rs b/nexus/db-model/src/external_ip.rs index 4d16a74c50b..2571eb7714f 100644 --- a/nexus/db-model/src/external_ip.rs +++ b/nexus/db-model/src/external_ip.rs @@ -87,8 +87,8 @@ impl From for sled_agent_client::types::SourceNatConfig { /// these options. #[derive(Debug, Clone, Copy)] pub enum IpSource { - Pool(Uuid), - Project(Uuid), + Instance { project_id: Uuid, pool_id: Option }, + Service { pool_id: Uuid }, } /// An incomplete external IP, used to store state required for issuing the @@ -100,7 +100,6 @@ pub struct IncompleteInstanceExternalIp { description: Option, time_created: DateTime, kind: IpKind, - project_id: Option, instance_id: Option, source: IpSource, } @@ -112,18 +111,14 @@ impl IncompleteInstanceExternalIp { instance_id: Uuid, pool_id: Option, ) -> Self { - let source = pool_id - .map(|id| IpSource::Pool(id)) - .unwrap_or_else(|| IpSource::Project(project_id)); Self { id, name: None, description: None, time_created: Utc::now(), kind: IpKind::SNat, - project_id: Some(project_id), instance_id: Some(instance_id), - source, + source: IpSource::Instance { project_id, pool_id }, } } @@ -133,18 +128,14 @@ impl IncompleteInstanceExternalIp { instance_id: Uuid, pool_id: Option, ) -> Self { - let source = pool_id - .map(|id| IpSource::Pool(id)) - .unwrap_or_else(|| IpSource::Project(project_id)); Self { id, name: None, description: None, time_created: Utc::now(), kind: IpKind::Ephemeral, - project_id: Some(project_id), instance_id: Some(instance_id), - source, + source: IpSource::Instance { project_id, pool_id }, } } @@ -155,18 +146,14 @@ impl IncompleteInstanceExternalIp { project_id: Uuid, pool_id: Option, ) -> Self { - let source = pool_id - .map(|id| IpSource::Pool(id)) - .unwrap_or_else(|| IpSource::Project(project_id)); Self { id, name: Some(name.clone()), description: Some(description.to_string()), time_created: Utc::now(), kind: IpKind::Floating, - project_id: Some(project_id), instance_id: None, - source, + source: IpSource::Instance { project_id, pool_id }, } } @@ -177,9 +164,8 @@ impl IncompleteInstanceExternalIp { description: None, time_created: Utc::now(), kind: IpKind::Service, - project_id: None, instance_id: None, - source: IpSource::Pool(pool_id), + source: IpSource::Service { pool_id }, } } @@ -203,10 +189,6 @@ impl IncompleteInstanceExternalIp { &self.kind } - pub fn project_id(&self) -> &Option { - &self.project_id - } - pub fn instance_id(&self) -> &Option { &self.instance_id } diff --git a/nexus/src/db/queries/external_ip.rs b/nexus/src/db/queries/external_ip.rs index 7b42aa9af95..916bc62a64a 100644 --- a/nexus/src/db/queries/external_ip.rs +++ b/nexus/src/db/queries/external_ip.rs @@ -296,7 +296,15 @@ impl NextExternalIp { out.push_sql(", "); // Project ID - out.push_bind_param::, Option>(self.ip.project_id())?; + match self.ip.source() { + IpSource::Instance { project_id, .. } => { + out.push_bind_param::(project_id)?; + } + IpSource::Service { .. } => { + out.push_bind_param::, Option>(&None)?; + } + }; + out.push_sql(" AS "); out.push_identifier(dsl::project_id::NAME)?; out.push_sql(", "); @@ -443,20 +451,26 @@ impl NextExternalIp { IP_POOL_RANGE_FROM_CLAUSE.walk_ast(out.reborrow())?; out.push_sql(" WHERE "); match self.ip.source() { - IpSource::Pool(pool_id) => { + IpSource::Instance { project_id, pool_id } => { + if let Some(pool_id) = pool_id { + out.push_identifier(dsl::ip_pool_id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(pool_id)?; + } else { + out.push_sql("("); + out.push_identifier(dsl::project_id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(project_id)?; + out.push_sql(" OR "); + out.push_identifier(dsl::project_id::NAME)?; + out.push_sql(" IS NULL)"); + } + } + IpSource::Service { pool_id } => { out.push_identifier(dsl::ip_pool_id::NAME)?; out.push_sql(" = "); out.push_bind_param::(pool_id)?; } - IpSource::Project(project_id) => { - out.push_sql("("); - out.push_identifier(dsl::project_id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::(project_id)?; - out.push_sql(" OR "); - out.push_identifier(dsl::project_id::NAME)?; - out.push_sql(" IS NULL)"); - } } out.push_sql(" AND "); out.push_identifier(dsl::time_deleted::NAME)?; From ca8cfc42898475301ce4ed9f410bb3b04111a738 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Aug 2022 13:44:15 -0400 Subject: [PATCH 3/7] Remove idempotency comment --- nexus/src/db/datastore/instance_external_ip.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/nexus/src/db/datastore/instance_external_ip.rs b/nexus/src/db/datastore/instance_external_ip.rs index a2f44b6ce26..ca330e9415b 100644 --- a/nexus/src/db/datastore/instance_external_ip.rs +++ b/nexus/src/db/datastore/instance_external_ip.rs @@ -30,7 +30,6 @@ use uuid::Uuid; impl DataStore { /// Create an external IP address for source NAT for an instance. - // TODO-correctness: This should be made idempotent. pub async fn allocate_instance_snat_ip( &self, opctx: &OpContext, From 8e822bac0d20cddd6cf8ddf9b215b8248f57a4b3 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Aug 2022 13:49:45 -0400 Subject: [PATCH 4/7] Add constraint --- common/src/sql/dbinit.sql | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 89c987bf0a0..e639615b815 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -1171,6 +1171,15 @@ CREATE TABLE omicron.public.instance_external_ip ( /* The last port in the allowed range, also inclusive. */ last_port INT4 NOT NULL, + /* + * The project can only be NULL for service IPs. + * Additionally, the project MUST be NULL for service IPs. + */ + CONSTRAINT null_project CHECK( + (kind != 'service' AND project_id IS NOT NULL) OR + (kind = 'service' AND project_id IS NULL) + ), + /* The name must be non-NULL iff this is a floating IP. */ CONSTRAINT null_fip_name CHECK ( (kind != 'floating' AND name IS NULL) OR From 6fc4bb21aeaacb432c12ba6c4fb5d290d5ce448c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Aug 2022 14:15:56 -0400 Subject: [PATCH 5/7] Rename InstanceExternalIp to ExternalIp --- common/src/sql/dbinit.sql | 11 ++-- nexus/db-model/src/external_ip.rs | 21 +++--- nexus/db-model/src/schema.rs | 2 +- nexus/src/app/instance.rs | 5 +- nexus/src/app/sagas/instance_create.rs | 4 +- ...instance_external_ip.rs => external_ip.rs} | 52 +++++++-------- nexus/src/db/datastore/ip_pool.rs | 12 ++-- nexus/src/db/datastore/mod.rs | 65 +++++++++---------- nexus/src/db/queries/external_ip.rs | 41 ++++++------ 9 files changed, 100 insertions(+), 113 deletions(-) rename nexus/src/db/datastore/{instance_external_ip.rs => external_ip.rs} (83%) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index e639615b815..2f9035ca96e 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -1131,9 +1131,10 @@ CREATE TYPE omicron.public.ip_kind AS ENUM ( ); /* - * External IP addresses used for guest instances + * External IP addresses used for guest instances and externally-facing + * services. */ -CREATE TABLE omicron.public.instance_external_ip ( +CREATE TABLE omicron.public.external_ip ( /* Identity metadata */ id UUID PRIMARY KEY, @@ -1207,7 +1208,7 @@ CREATE TABLE omicron.public.instance_external_ip ( * Index used to support quickly looking up children of the IP Pool range table, * when checking for allocated addresses during deletion. */ -CREATE INDEX ON omicron.public.instance_external_ip ( +CREATE INDEX ON omicron.public.external_ip ( ip_pool_id, ip_pool_range_id ) @@ -1220,13 +1221,13 @@ CREATE INDEX ON omicron.public.instance_external_ip ( * pools, _and_ on the fact that the number of ports assigned to each instance * is fixed at compile time. */ -CREATE UNIQUE INDEX ON omicron.public.instance_external_ip ( +CREATE UNIQUE INDEX ON omicron.public.external_ip ( ip, first_port ) WHERE time_deleted IS NULL; -CREATE UNIQUE INDEX ON omicron.public.instance_external_ip ( +CREATE UNIQUE INDEX ON omicron.public.external_ip ( instance_id, id ) diff --git a/nexus/db-model/src/external_ip.rs b/nexus/db-model/src/external_ip.rs index 2571eb7714f..75c51835403 100644 --- a/nexus/db-model/src/external_ip.rs +++ b/nexus/db-model/src/external_ip.rs @@ -6,7 +6,7 @@ //! services. use crate::impl_enum_type; -use crate::schema::instance_external_ip; +use crate::schema::external_ip; use crate::Name; use crate::SqlU16; use chrono::DateTime; @@ -35,7 +35,8 @@ impl_enum_type!( Service => b"service" ); -/// The main model type for external IP addresses for instances. +/// The main model type for external IP addresses for instances +/// and externally-facing services. /// /// This encompasses the three flavors of external IPs: automatic source NAT /// IPs, Ephemeral IPs, and Floating IPs. The first two are similar in that they @@ -45,8 +46,8 @@ impl_enum_type!( /// API at all, and only provide outbound connectivity to instances, not /// inbound. #[derive(Debug, Clone, Selectable, Queryable, Insertable)] -#[diesel(table_name = instance_external_ip)] -pub struct InstanceExternalIp { +#[diesel(table_name = external_ip)] +pub struct ExternalIp { pub id: Uuid, // Only Some(_) for Floating IPs pub name: Option, @@ -69,8 +70,8 @@ pub struct InstanceExternalIp { pub last_port: SqlU16, } -impl From for sled_agent_client::types::SourceNatConfig { - fn from(eip: InstanceExternalIp) -> Self { +impl From for sled_agent_client::types::SourceNatConfig { + fn from(eip: ExternalIp) -> Self { Self { ip: eip.ip.ip(), first_port: eip.first_port.0, @@ -94,7 +95,7 @@ pub enum IpSource { /// An incomplete external IP, used to store state required for issuing the /// database query that selects an available IP and stores the resulting record. #[derive(Debug, Clone)] -pub struct IncompleteInstanceExternalIp { +pub struct IncompleteExternalIp { id: Uuid, name: Option, description: Option, @@ -104,7 +105,7 @@ pub struct IncompleteInstanceExternalIp { source: IpSource, } -impl IncompleteInstanceExternalIp { +impl IncompleteExternalIp { pub fn for_instance_source_nat( id: Uuid, project_id: Uuid, @@ -212,10 +213,10 @@ impl TryFrom for shared::IpKind { } } -impl TryFrom for views::ExternalIp { +impl TryFrom for views::ExternalIp { type Error = Error; - fn try_from(ip: InstanceExternalIp) -> Result { + fn try_from(ip: ExternalIp) -> Result { let kind = ip.kind.try_into()?; Ok(views::ExternalIp { kind, ip: ip.ip.ip() }) } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index dd9dfe3150f..73d91b11a51 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -168,7 +168,7 @@ table! { } table! { - instance_external_ip (id) { + external_ip (id) { id -> Uuid, name -> Nullable, description -> Nullable, diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 7d98e772f82..47f1a68b1d5 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -255,10 +255,7 @@ impl super::Nexus { .await?; // Ignore the count of addresses deleted self.db_datastore - .deallocate_instance_external_ip_by_instance_id( - opctx, - authz_instance.id(), - ) + .deallocate_external_ip_by_instance_id(opctx, authz_instance.id()) .await?; Ok(()) } diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 8005f134e67..42e85b31e30 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -605,7 +605,7 @@ async fn sic_allocate_instance_snat_ip_undo( OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); let ip_id = sagactx.lookup::("snat_ip_id")?; datastore - .deallocate_instance_external_ip(&opctx, ip_id) + .deallocate_external_ip(&opctx, ip_id) .await .map_err(ActionError::action_failed)?; Ok(()) @@ -668,7 +668,7 @@ async fn sic_allocate_instance_external_ip_undo( OpContext::for_saga_action(&sagactx, &saga_params.serialized_authn); let ip_id = repeat_saga_params.new_id; datastore - .deallocate_instance_external_ip(&opctx, ip_id) + .deallocate_external_ip(&opctx, ip_id) .await .map_err(ActionError::action_failed)?; Ok(()) diff --git a/nexus/src/db/datastore/instance_external_ip.rs b/nexus/src/db/datastore/external_ip.rs similarity index 83% rename from nexus/src/db/datastore/instance_external_ip.rs rename to nexus/src/db/datastore/external_ip.rs index ca330e9415b..aa8f41f572f 100644 --- a/nexus/src/db/datastore/instance_external_ip.rs +++ b/nexus/src/db/datastore/external_ip.rs @@ -2,15 +2,15 @@ // 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/. -//! [`DataStore`] methods on [`InstanceExternalIp`]s. +//! [`DataStore`] methods on [`ExternalIp`]s. use super::DataStore; use crate::context::OpContext; use crate::db; use crate::db::error::public_error_from_diesel_pool; use crate::db::error::ErrorHandler; -use crate::db::model::IncompleteInstanceExternalIp; -use crate::db::model::InstanceExternalIp; +use crate::db::model::ExternalIp; +use crate::db::model::IncompleteExternalIp; use crate::db::model::IpKind; use crate::db::model::IpPool; use crate::db::model::Name; @@ -36,14 +36,14 @@ impl DataStore { ip_id: Uuid, project_id: Uuid, instance_id: Uuid, - ) -> CreateResult { - let data = IncompleteInstanceExternalIp::for_instance_source_nat( + ) -> CreateResult { + let data = IncompleteExternalIp::for_instance_source_nat( ip_id, project_id, instance_id, /* pool_id = */ None, ); - self.allocate_instance_external_ip(opctx, data).await + self.allocate_external_ip(opctx, data).await } /// Create an Ephemeral IP address for an instance. @@ -54,7 +54,7 @@ impl DataStore { project_id: Uuid, instance_id: Uuid, pool_name: Option, - ) -> CreateResult { + ) -> CreateResult { let pool_id = if let Some(ref name) = pool_name { // We'd like to add authz checks here, and use the `LookupPath` // methods on the project-scoped view of this resource. It's not @@ -92,13 +92,13 @@ impl DataStore { } else { None }; - let data = IncompleteInstanceExternalIp::for_ephemeral( + let data = IncompleteExternalIp::for_ephemeral( ip_id, project_id, instance_id, pool_id, ); - self.allocate_instance_external_ip(opctx, data).await + self.allocate_external_ip(opctx, data).await } /// Allocates an IP address for internal service usage. @@ -107,19 +107,19 @@ impl DataStore { opctx: &OpContext, ip_id: Uuid, rack_id: Uuid, - ) -> CreateResult { + ) -> CreateResult { let (.., pool) = self.ip_pools_lookup_by_rack_id(opctx, rack_id).await?; - let data = IncompleteInstanceExternalIp::for_service(ip_id, pool.id()); - self.allocate_instance_external_ip(opctx, data).await + let data = IncompleteExternalIp::for_service(ip_id, pool.id()); + self.allocate_external_ip(opctx, data).await } - async fn allocate_instance_external_ip( + async fn allocate_external_ip( &self, opctx: &OpContext, - data: IncompleteInstanceExternalIp, - ) -> CreateResult { + data: IncompleteExternalIp, + ) -> CreateResult { NextExternalIp::new(data) .get_result_async(self.pool_authorized(opctx).await?) .await @@ -146,18 +146,18 @@ impl DataStore { /// - `Ok(false)`: The record was already deleted, such as by a previous /// call /// - `Err(_)`: Any other condition, including a non-existent record. - pub async fn deallocate_instance_external_ip( + pub async fn deallocate_external_ip( &self, opctx: &OpContext, ip_id: Uuid, ) -> Result { - use db::schema::instance_external_ip::dsl; + use db::schema::external_ip::dsl; let now = Utc::now(); - diesel::update(dsl::instance_external_ip) + diesel::update(dsl::external_ip) .filter(dsl::time_deleted.is_null()) .filter(dsl::id.eq(ip_id)) .set(dsl::time_deleted.eq(now)) - .check_if_exists::(ip_id) + .check_if_exists::(ip_id) .execute_and_check(self.pool_authorized(opctx).await?) .await .map(|r| match r.status { @@ -175,14 +175,14 @@ impl DataStore { /// if callers have some invariants they'd like to check. // TODO-correctness: This can't be used for Floating IPs, we'll need a // _detatch_ method for that. - pub async fn deallocate_instance_external_ip_by_instance_id( + pub async fn deallocate_external_ip_by_instance_id( &self, opctx: &OpContext, instance_id: Uuid, ) -> Result { - use db::schema::instance_external_ip::dsl; + use db::schema::external_ip::dsl; let now = Utc::now(); - diesel::update(dsl::instance_external_ip) + diesel::update(dsl::external_ip) .filter(dsl::time_deleted.is_null()) .filter(dsl::instance_id.eq(instance_id)) .filter(dsl::kind.ne(IpKind::Floating)) @@ -197,12 +197,12 @@ impl DataStore { &self, opctx: &OpContext, instance_id: Uuid, - ) -> LookupResult> { - use db::schema::instance_external_ip::dsl; - dsl::instance_external_ip + ) -> LookupResult> { + use db::schema::external_ip::dsl; + dsl::external_ip .filter(dsl::instance_id.eq(instance_id)) .filter(dsl::time_deleted.is_null()) - .select(InstanceExternalIp::as_select()) + .select(ExternalIp::as_select()) .get_results_async(self.pool_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) diff --git a/nexus/src/db/datastore/ip_pool.rs b/nexus/src/db/datastore/ip_pool.rs index 7d3a40ae390..cb229e6fce4 100644 --- a/nexus/src/db/datastore/ip_pool.rs +++ b/nexus/src/db/datastore/ip_pool.rs @@ -331,7 +331,7 @@ impl DataStore { authz_pool: &authz::IpPool, range: &IpRange, ) -> DeleteResult { - use db::schema::instance_external_ip; + use db::schema::external_ip; use db::schema::ip_pool_range::dsl; opctx.authorize(authz::Action::Modify, authz_pool).await?; @@ -370,12 +370,10 @@ impl DataStore { // Find external IPs allocated out of this pool and range. let range_id = range.id; let has_children = diesel::dsl::select(diesel::dsl::exists( - instance_external_ip::table - .filter(instance_external_ip::dsl::ip_pool_id.eq(pool_id)) - .filter( - instance_external_ip::dsl::ip_pool_range_id.eq(range_id), - ) - .filter(instance_external_ip::dsl::time_deleted.is_null()), + external_ip::table + .filter(external_ip::dsl::ip_pool_id.eq(pool_id)) + .filter(external_ip::dsl::ip_pool_range_id.eq(range_id)) + .filter(external_ip::dsl::time_deleted.is_null()), )) .get_result_async::(self.pool_authorized(opctx).await?) .await diff --git a/nexus/src/db/datastore/mod.rs b/nexus/src/db/datastore/mod.rs index 5ed1c1c3e0d..aeef1655741 100644 --- a/nexus/src/db/datastore/mod.rs +++ b/nexus/src/db/datastore/mod.rs @@ -44,10 +44,10 @@ mod console_session; mod dataset; mod device_auth; mod disk; +mod external_ip; mod global_image; mod identity_provider; mod instance; -mod instance_external_ip; mod ip_pool; mod network_interface; mod organization; @@ -233,7 +233,7 @@ mod test { use crate::db::identity::Resource; use crate::db::lookup::LookupPath; use crate::db::model::Dataset; - use crate::db::model::InstanceExternalIp; + use crate::db::model::ExternalIp; use crate::db::model::Rack; use crate::db::model::Region; use crate::db::model::Service; @@ -1011,13 +1011,12 @@ mod test { } #[tokio::test] - async fn test_deallocate_instance_external_ip_by_instance_id_is_idempotent() - { + async fn test_deallocate_external_ip_by_instance_id_is_idempotent() { use crate::db::model::IpKind; - use crate::db::schema::instance_external_ip::dsl; + use crate::db::schema::external_ip::dsl; let logctx = dev::test_setup_log( - "test_deallocate_instance_external_ip_by_instance_id_is_idempotent", + "test_deallocate_external_ip_by_instance_id_is_idempotent", ); let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; @@ -1026,7 +1025,7 @@ mod test { let now = Utc::now(); let instance_id = Uuid::new_v4(); let ips = (0..4) - .map(|i| InstanceExternalIp { + .map(|i| ExternalIp { id: Uuid::new_v4(), name: None, description: None, @@ -1045,7 +1044,7 @@ mod test { last_port: crate::db::model::SqlU16(10), }) .collect::>(); - diesel::insert_into(dsl::instance_external_ip) + diesel::insert_into(dsl::external_ip) .values(ips.clone()) .execute_async(datastore.pool()) .await @@ -1053,7 +1052,7 @@ mod test { // Delete everything, make sure we delete all records we made above let count = datastore - .deallocate_instance_external_ip_by_instance_id(&opctx, instance_id) + .deallocate_external_ip_by_instance_id(&opctx, instance_id) .await .expect("Failed to delete instance external IPs"); assert_eq!( @@ -1064,7 +1063,7 @@ mod test { // Do it again, we should get zero records let count = datastore - .deallocate_instance_external_ip_by_instance_id(&opctx, instance_id) + .deallocate_external_ip_by_instance_id(&opctx, instance_id) .await .expect("Failed to delete instance external IPs"); assert_eq!(count, 0, "Expected to delete zero IPs for the instance"); @@ -1074,19 +1073,18 @@ mod test { } #[tokio::test] - async fn test_deallocate_instance_external_ip_is_idempotent() { + async fn test_deallocate_external_ip_is_idempotent() { use crate::db::model::IpKind; - use crate::db::schema::instance_external_ip::dsl; + use crate::db::schema::external_ip::dsl; - let logctx = dev::test_setup_log( - "test_deallocate_instance_external_ip_is_idempotent", - ); + let logctx = + dev::test_setup_log("test_deallocate_external_ip_is_idempotent"); let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; // Create a record. let now = Utc::now(); - let ip = InstanceExternalIp { + let ip = ExternalIp { id: Uuid::new_v4(), name: None, description: None, @@ -1104,26 +1102,22 @@ mod test { first_port: crate::db::model::SqlU16(0), last_port: crate::db::model::SqlU16(10), }; - diesel::insert_into(dsl::instance_external_ip) + diesel::insert_into(dsl::external_ip) .values(ip.clone()) .execute_async(datastore.pool()) .await .unwrap(); // Delete it twice, make sure we get the right sentinel return values. - let deleted = datastore - .deallocate_instance_external_ip(&opctx, ip.id) - .await - .unwrap(); + let deleted = + datastore.deallocate_external_ip(&opctx, ip.id).await.unwrap(); assert!( deleted, "Got unexpected sentinel value back when \ deleting external IP the first time" ); - let deleted = datastore - .deallocate_instance_external_ip(&opctx, ip.id) - .await - .unwrap(); + let deleted = + datastore.deallocate_external_ip(&opctx, ip.id).await.unwrap(); assert!( !deleted, "Got unexpected sentinel value back when \ @@ -1132,7 +1126,7 @@ mod test { // Deleting a non-existing record fails assert!(datastore - .deallocate_instance_external_ip(&opctx, Uuid::nil()) + .deallocate_external_ip(&opctx, Uuid::nil()) .await .is_err()); @@ -1143,7 +1137,7 @@ mod test { #[tokio::test] async fn test_external_ip_check_constraints() { use crate::db::model::IpKind; - use crate::db::schema::instance_external_ip::dsl; + use crate::db::schema::external_ip::dsl; use async_bb8_diesel::ConnectionError::Query; use async_bb8_diesel::PoolError::Connection; use diesel::result::DatabaseErrorKind::CheckViolation; @@ -1161,7 +1155,7 @@ mod test { ) .unwrap(); let mut addresses = subnet.iter(); - let ip = InstanceExternalIp { + let ip = ExternalIp { id: Uuid::new_v4(), name: None, description: None, @@ -1193,7 +1187,7 @@ mod test { for name in names.iter() { for description in descriptions.iter() { for instance_id in instance_ids.iter() { - let new_ip = InstanceExternalIp { + let new_ip = ExternalIp { id: Uuid::new_v4(), name: name.clone(), description: description.clone(), @@ -1201,7 +1195,7 @@ mod test { instance_id: *instance_id, ..ip }; - let res = diesel::insert_into(dsl::instance_external_ip) + let res = diesel::insert_into(dsl::external_ip) .values(new_ip) .execute_async(datastore.pool()) .await; @@ -1239,7 +1233,7 @@ mod test { for name in names.iter() { for description in descriptions.iter() { for instance_id in instance_ids.iter() { - let new_ip = InstanceExternalIp { + let new_ip = ExternalIp { id: Uuid::new_v4(), name: name.clone(), description: description.clone(), @@ -1248,11 +1242,10 @@ mod test { instance_id: *instance_id, ..ip }; - let res = - diesel::insert_into(dsl::instance_external_ip) - .values(new_ip.clone()) - .execute_async(datastore.pool()) - .await; + let res = diesel::insert_into(dsl::external_ip) + .values(new_ip.clone()) + .execute_async(datastore.pool()) + .await; if name.is_none() && description.is_none() && instance_id.is_some() diff --git a/nexus/src/db/queries/external_ip.rs b/nexus/src/db/queries/external_ip.rs index 916bc62a64a..ba810aa0153 100644 --- a/nexus/src/db/queries/external_ip.rs +++ b/nexus/src/db/queries/external_ip.rs @@ -5,8 +5,8 @@ //! Implementation of queries for operating on external IP addresses from IP //! Pools. -use crate::db::model::IncompleteInstanceExternalIp; -use crate::db::model::InstanceExternalIp; +use crate::db::model::ExternalIp; +use crate::db::model::IncompleteExternalIp; use crate::db::model::IpKind; use crate::db::model::IpKindEnum; use crate::db::model::IpSource; @@ -31,10 +31,9 @@ type FromClause = type IpPoolRangeFromClause = FromClause; const IP_POOL_RANGE_FROM_CLAUSE: IpPoolRangeFromClause = IpPoolRangeFromClause::new(); -type InstanceExternalIpFromClause = - FromClause; -const INSTANCE_EXTERNAL_IP_FROM_CLAUSE: InstanceExternalIpFromClause = - InstanceExternalIpFromClause::new(); +type ExternalIpFromClause = FromClause; +const INSTANCE_EXTERNAL_IP_FROM_CLAUSE: ExternalIpFromClause = + ExternalIpFromClause::new(); // The number of ports available to an instance when doing source NAT. Note // that for static NAT, this value isn't used, and all ports are available. @@ -65,7 +64,7 @@ const MAX_PORT: i32 = u16::MAX as _; /// In general, the query: /// /// - Selects the next available IP address and port range from _any_ IP Pool -/// - Inserts that record into the `instance_external_ip` table +/// - Inserts that record into the `external_ip` table /// - Updates the rcgen and time modified of the parent `ip_pool_range` table /// /// In detail, the query is: @@ -118,7 +117,7 @@ const MAX_PORT: i32 = u16::MAX as _; /// -- Join with existing IPs, selecting the first row from the /// -- address and port sequence subqueryes that has no match. I.e., /// -- is not yet reserved. -/// instance_external_ip +/// external_ip /// ON /// (ip, first_port, time_deleted IS NULL) = /// (candidate_ip, candidate_first_port, TRUE) @@ -134,7 +133,7 @@ const MAX_PORT: i32 = u16::MAX as _; /// -- everything else as it exists in the record. This should only be /// -- possible on replay of a saga node. /// INSERT INTO -/// instance_external_ip +/// external_ip /// (SELECT * FROM next_external_ip) /// ON CONFLICT (id) /// DO UPDATE SET @@ -213,7 +212,7 @@ const MAX_PORT: i32 = u16::MAX as _; /// violates that expectation. #[derive(Debug, Clone)] pub struct NextExternalIp { - ip: IncompleteInstanceExternalIp, + ip: IncompleteExternalIp, // Number of ports reserved per IP address. Only applicable if the IP kind // is snat. n_ports_per_chunk: i32, @@ -225,7 +224,7 @@ pub struct NextExternalIp { } impl NextExternalIp { - pub fn new(ip: IncompleteInstanceExternalIp) -> Self { + pub fn new(ip: IncompleteExternalIp) -> Self { let now = Utc::now(); let n_ports_per_chunk = i32::try_from(NUM_SOURCE_NAT_PORTS).unwrap(); Self { @@ -240,7 +239,7 @@ impl NextExternalIp { &'a self, mut out: AstPass<'_, 'a, Pg>, ) -> QueryResult<()> { - use schema::instance_external_ip::dsl; + use schema::external_ip::dsl; out.push_sql("SELECT "); // NOTE: These columns must be selected in the order in which they @@ -395,10 +394,10 @@ impl NextExternalIp { // the `time_deleted` is null. That means that if the record has been // soft-deleted, it won't be considered a match, and thus both the IP // and first port (in the join result) will be null. Note that there - // _is_ a record in the `instance_external_ip` table that has that IP + // _is_ a record in the `external_ip` table that has that IP // and possibly first port, but since it's been soft-deleted, it's not a // match. In that case, we can get away with _only_ filtering the join - // results on the IP from the `instance_external_ip` table being NULL. + // results on the IP from the `external_ip` table being NULL. out.push_sql(" WHERE ("); out.push_identifier(dsl::ip::NAME)?; out.push_sql(" IS NULL) OR ("); @@ -555,9 +554,7 @@ impl NextExternalIp { out.push_sql(" + 1 WHERE "); out.push_identifier(dsl::id::NAME)?; out.push_sql(" = (SELECT "); - out.push_identifier( - schema::instance_external_ip::ip_pool_range_id::NAME, - )?; + out.push_identifier(schema::external_ip::ip_pool_range_id::NAME)?; out.push_sql(" FROM next_external_ip) AND "); out.push_identifier(dsl::time_deleted::NAME)?; out.push_sql(" IS NULL RETURNING "); @@ -565,11 +562,11 @@ impl NextExternalIp { Ok(()) } - // Push the subquery that updates the actual `instance_external_ip` table + // Push the subquery that updates the actual `external_ip` table // with the candidate record created in the main `next_external_ip` CTE and // returns it. Note that this may just update the timestamps, if a record // with the same primary key is found. - fn push_update_instance_external_ip_subquery<'a>( + fn push_update_external_ip_subquery<'a>( &'a self, mut out: AstPass<'_, 'a, Pg>, ) -> QueryResult<()> { @@ -609,7 +606,7 @@ impl QueryFragment for NextExternalIp { // Push the subquery that potentially inserts this record, or ignores // primary key conflicts (for idempotency). out.push_sql("), external_ip AS ("); - self.push_update_instance_external_ip_subquery(out.reborrow())?; + self.push_update_external_ip_subquery(out.reborrow())?; // Push the subquery that bumps the `rcgen` of the IP Pool range table out.push_sql("), updated_pool_range AS ("); @@ -623,7 +620,7 @@ impl QueryFragment for NextExternalIp { } impl Query for NextExternalIp { - type SqlType = <>::SelectExpression as diesel::Expression>::SqlType; } @@ -857,7 +854,7 @@ mod tests { // Release the first context .db_datastore - .deallocate_instance_external_ip(&context.opctx, ips[0].id) + .deallocate_external_ip(&context.opctx, ips[0].id) .await .expect("Failed to release the first external IP address"); From 92e16e1eaf948d51c8d154274865393d23cfef2c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Aug 2022 14:19:05 -0400 Subject: [PATCH 6/7] clean up error message too --- nexus/src/db/datastore/external_ip.rs | 2 +- nexus/src/db/queries/external_ip.rs | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/nexus/src/db/datastore/external_ip.rs b/nexus/src/db/datastore/external_ip.rs index aa8f41f572f..4c908461924 100644 --- a/nexus/src/db/datastore/external_ip.rs +++ b/nexus/src/db/datastore/external_ip.rs @@ -129,7 +129,7 @@ impl DataStore { use diesel::result::Error::NotFound; match e { Connection(Query(NotFound)) => Error::invalid_request( - "No external IP addresses available for new instance", + "No external IP addresses available", ), _ => public_error_from_diesel_pool(e, ErrorHandler::Server), } diff --git a/nexus/src/db/queries/external_ip.rs b/nexus/src/db/queries/external_ip.rs index ba810aa0153..8dd7fb6f340 100644 --- a/nexus/src/db/queries/external_ip.rs +++ b/nexus/src/db/queries/external_ip.rs @@ -795,9 +795,7 @@ mod tests { assert_eq!( err, Error::InvalidRequest { - message: String::from( - "No external IP addresses available for new instance" - ), + message: String::from("No external IP addresses available"), } ); context.success().await; @@ -1071,11 +1069,7 @@ mod tests { assert_eq!( err, Error::InvalidRequest { - message: String::from( - // TODO: The error is a bit misleading; this isn't an IP - // intended for an instance necessarily. - "No external IP addresses available for new instance" - ), + message: String::from("No external IP addresses available"), } ); From 69fb99d06611f829f06bcc8e518db1c2346317fa Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Aug 2022 15:41:57 -0400 Subject: [PATCH 7/7] constants too --- nexus/src/db/queries/external_ip.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nexus/src/db/queries/external_ip.rs b/nexus/src/db/queries/external_ip.rs index 8dd7fb6f340..24092aa8f27 100644 --- a/nexus/src/db/queries/external_ip.rs +++ b/nexus/src/db/queries/external_ip.rs @@ -32,7 +32,7 @@ type IpPoolRangeFromClause = FromClause; const IP_POOL_RANGE_FROM_CLAUSE: IpPoolRangeFromClause = IpPoolRangeFromClause::new(); type ExternalIpFromClause = FromClause; -const INSTANCE_EXTERNAL_IP_FROM_CLAUSE: ExternalIpFromClause = +const EXTERNAL_IP_FROM_CLAUSE: ExternalIpFromClause = ExternalIpFromClause::new(); // The number of ports available to an instance when doing source NAT. Note @@ -334,7 +334,7 @@ impl NextExternalIp { out.push_sql(") CROSS JOIN ("); self.push_port_sequence_subquery(out.reborrow())?; out.push_sql(") LEFT OUTER JOIN "); - INSTANCE_EXTERNAL_IP_FROM_CLAUSE.walk_ast(out.reborrow())?; + EXTERNAL_IP_FROM_CLAUSE.walk_ast(out.reborrow())?; // The JOIN conditions depend on the IP type. For automatic SNAT IP // addresses, we need to consider existing records with their port @@ -571,7 +571,7 @@ impl NextExternalIp { mut out: AstPass<'_, 'a, Pg>, ) -> QueryResult<()> { out.push_sql("INSERT INTO "); - INSTANCE_EXTERNAL_IP_FROM_CLAUSE.walk_ast(out.reborrow())?; + EXTERNAL_IP_FROM_CLAUSE.walk_ast(out.reborrow())?; out.push_sql( " (SELECT * FROM next_external_ip) \ ON CONFLICT (id) \