From b86557139a6055b65d0153e5b2e4383c44c1dd29 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 7 Mar 2024 14:02:21 -0600 Subject: [PATCH 01/13] endpoint for allocated IP count and total capacity for an IP pool --- common/src/address.rs | 34 ++++ nexus/db-model/src/utilization.rs | 18 ++ .../src/db/datastore/utilization.rs | 189 ++++++++++++++++++ nexus/src/app/utilization.rs | 19 ++ nexus/src/external_api/http_entrypoints.rs | 26 +++ nexus/tests/output/nexus_tags.txt | 1 + nexus/types/src/external_api/views.rs | 25 +++ openapi/nexus.json | 56 ++++++ 8 files changed, 368 insertions(+) diff --git a/common/src/address.rs b/common/src/address.rs index 48ec7c46edc..672937e9be5 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -421,6 +421,15 @@ impl IpRange { IpRange::V6(ip6) => IpRangeIter::V6(ip6.iter()), } } + + // Has to be u128 to accommodate IPv6 -- logic around converting back down + // to u32 (if possible) lives in the view layer + pub fn len(&self) -> u128 { + match self { + IpRange::V4(ip4) => u128::from(ip4.len()), + IpRange::V6(ip6) => ip6.len(), + } + } } impl From for IpRange { @@ -508,6 +517,12 @@ impl Ipv4Range { pub fn iter(&self) -> Ipv4RangeIter { Ipv4RangeIter { next: Some(self.first.into()), last: self.last.into() } } + + pub fn len(&self) -> u32 { + let start_num = u32::from(self.first); + let end_num = u32::from(self.last); + end_num - start_num + 1 + } } impl From for Ipv4Range { @@ -564,6 +579,12 @@ impl Ipv6Range { pub fn iter(&self) -> Ipv6RangeIter { Ipv6RangeIter { next: Some(self.first.into()), last: self.last.into() } } + + pub fn len(&self) -> u128 { + let start_num = u128::from(self.first); + let end_num = u128::from(self.last); + end_num - start_num + 1 + } } impl From for Ipv6Range { @@ -781,6 +802,19 @@ mod test { ); } + #[test] + fn test_ip_range_length() { + let lo = Ipv4Addr::new(10, 0, 0, 1); + let hi = Ipv4Addr::new(10, 0, 0, 3); + let range = IpRange::try_from((lo, hi)).unwrap(); + assert_eq!(range.len(), 3); + + let lo = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1); + let hi = Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 1, 3); + let range = IpRange::try_from((lo, hi)).unwrap(); + assert_eq!(range.len(), 2u128.pow(16) + 3); + } + #[test] fn test_ipv6_subnet_deserialize() { let value = json!({ diff --git a/nexus/db-model/src/utilization.rs b/nexus/db-model/src/utilization.rs index b0e6324bc9b..1d1f7196808 100644 --- a/nexus/db-model/src/utilization.rs +++ b/nexus/db-model/src/utilization.rs @@ -55,3 +55,21 @@ impl From for views::Utilization { } } } + +// Not really a DB model, just the result of a datastore function +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct IpPoolUtilization { + // has to be i64 because SQL counts from Diesel are BigInts + pub allocated: i64, + pub total: u128, +} + +impl From for views::IpPoolUtilization { + fn from(util: IpPoolUtilization) -> Self { + Self { + allocated: util.allocated.try_into().ok(), + // if u128 is too big to convert to u32, fall back to None (null) + total: util.total.try_into().ok(), + } + } +} diff --git a/nexus/db-queries/src/db/datastore/utilization.rs b/nexus/db-queries/src/db/datastore/utilization.rs index 826c66043a9..511b59eb217 100644 --- a/nexus/db-queries/src/db/datastore/utilization.rs +++ b/nexus/db-queries/src/db/datastore/utilization.rs @@ -10,6 +10,8 @@ use crate::db::pagination::paginated; use async_bb8_diesel::AsyncRunQueryDsl; use diesel::BoolExpressionMethods; use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; +use nexus_db_model::IpPoolRange; +use omicron_common::address::IpRange; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; @@ -62,4 +64,191 @@ impl DataStore { .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + + pub async fn ip_pool_allocated_count( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + ) -> Result { + opctx.authorize(authz::Action::Read, authz_pool).await?; + + use db::schema::external_ip; + + external_ip::table + .filter(external_ip::ip_pool_id.eq(authz_pool.id())) + .filter(external_ip::time_deleted.is_null()) + .select(external_ip::id) + .count() + // TODO: how much do I have to worry about this being bigger than + // u32? it seems impossible, but do I have to handle it? + .first_async::(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + } + + // TODO: should this just return the vec of ranges and live in ip_pool.rs? + // TODO: generally we never retrieve all of anything, how bad is that + pub async fn ip_pool_total_capacity( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + ) -> Result { + opctx.authorize(authz::Action::Read, authz_pool).await?; + opctx.authorize(authz::Action::ListChildren, authz_pool).await?; + + use db::schema::ip_pool_range; + + let ranges: Vec = ip_pool_range::table + .filter(ip_pool_range::ip_pool_id.eq(authz_pool.id())) + .filter(ip_pool_range::time_deleted.is_null()) + .select(IpPoolRange::as_select()) + .get_results_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource(authz_pool), + ) + })?; + + Ok(ranges.iter().fold(0, |acc, r| acc + IpRange::from(r).len())) + } +} + +#[cfg(test)] +mod test { + use crate::authz; + use crate::db::datastore::test_utils::datastore_test; + use nexus_db_model::{IpPool, IpPoolResource, IpPoolResourceType, Project}; + use nexus_test_utils::db::test_setup_database; + use nexus_types::external_api::params; + use nexus_types::identity::Resource; + use omicron_common::address::{IpRange, Ipv4Range, Ipv6Range}; + use omicron_common::api::external::{ + IdentityMetadataCreateParams, LookupType, + }; + use omicron_test_utils::dev; + + #[tokio::test] + async fn test_ip_utilization() { + let logctx = dev::test_setup_log("test_ip_utilization"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + let authz_silo = opctx.authn.silo_required().unwrap(); + let project = Project::new( + authz_silo.id(), + params::ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "my-project".parse().unwrap(), + description: "".to_string(), + }, + }, + ); + let (.., project) = + datastore.project_create(&opctx, project).await.unwrap(); + + // create an IP pool for the silo, add a range to it, and link it to the silo + let identity = IdentityMetadataCreateParams { + name: "my-pool".parse().unwrap(), + description: "".to_string(), + }; + let pool = datastore + .ip_pool_create(&opctx, IpPool::new(&identity)) + .await + .expect("Failed to create IP pool"); + let authz_pool = authz::IpPool::new( + authz::FLEET, + pool.id(), + LookupType::ById(pool.id()), + ); + + // capacity of zero because there are no ranges + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 0); + + let range = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 5), + ) + .unwrap(), + ); + datastore + .ip_pool_add_range(&opctx, &authz_pool, &range) + .await + .expect("Could not add range"); + + // now it has a capacity of 5 because we added the range + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 5); + + let link = IpPoolResource { + ip_pool_id: pool.id(), + resource_type: IpPoolResourceType::Silo, + resource_id: authz_silo.id(), + is_default: true, + }; + datastore + .ip_pool_link_silo(&opctx, link) + .await + .expect("Could not link pool to silo"); + + let ip_count = datastore + .ip_pool_allocated_count(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(ip_count, 0); + + let identity = IdentityMetadataCreateParams { + name: "my-ip".parse().unwrap(), + description: "".to_string(), + }; + let ip = datastore + .allocate_floating_ip(&opctx, project.id(), identity, None, None) + .await + .expect("Could not allocate floating IP"); + assert_eq!(ip.ip.to_string(), "10.0.0.1/32"); + + let ip_count = datastore + .ip_pool_allocated_count(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(ip_count, 1); + + // allocating one has nothing to do with total capacity + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 5); + + let ipv6_range = IpRange::V6( + Ipv6Range::new( + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 1, 20), + ) + .unwrap(), + ); + datastore + .ip_pool_add_range(&opctx, &authz_pool, &ipv6_range) + .await + .expect("Could not add range"); + + // now test with additional v6 range + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 5 + 11 + 65536); + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } } diff --git a/nexus/src/app/utilization.rs b/nexus/src/app/utilization.rs index 526ebc9470e..88d29dd76ed 100644 --- a/nexus/src/app/utilization.rs +++ b/nexus/src/app/utilization.rs @@ -4,6 +4,7 @@ //! Insights into capacity and utilization +use nexus_db_model::IpPoolUtilization; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; @@ -30,4 +31,22 @@ impl super::Nexus { ) -> ListResultVec { self.db_datastore.silo_utilization_list(opctx, pagparams).await } + + pub async fn ip_pool_utilization_view( + &self, + opctx: &OpContext, + pool_lookup: &lookup::IpPool<'_>, + ) -> Result { + let (.., authz_pool) = + pool_lookup.lookup_for(authz::Action::Read).await?; + let allocated = self + .db_datastore + .ip_pool_allocated_count(opctx, &authz_pool) + .await?; + let total = self + .db_datastore + .ip_pool_total_capacity(opctx, &authz_pool) + .await?; + Ok(IpPoolUtilization { allocated, total }) + } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index aa037e072f6..6551b7c2eeb 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -130,6 +130,7 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(ip_pool_update)?; // Variants for internal services api.register(ip_pool_service_view)?; + api.register(ip_pool_utilization_view)?; // Operator-Accessible IP Pool Range API api.register(ip_pool_range_list)?; @@ -1557,6 +1558,31 @@ async fn ip_pool_update( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// Fetch IP pool +#[endpoint { + method = GET, + path = "/v1/system/ip-pools/{pool}/utilization", + tags = ["system/networking"], +}] +async fn ip_pool_utilization_view( + rqctx: RequestContext>, + path_params: Path, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + let nexus = &apictx.nexus; + let pool_selector = path_params.into_inner().pool; + // We do not prevent the service pool from being fetched by name or ID + // like we do for update, delete, associate. + let pool_lookup = nexus.ip_pool_lookup(&opctx, &pool_selector)?; + let utilization = + nexus.ip_pool_utilization_view(&opctx, &pool_lookup).await?; + Ok(HttpResponseOk(utilization.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + /// List IP pool's linked silos #[endpoint { method = GET, diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 3b954ec6ec6..ba79d75f224 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -164,6 +164,7 @@ ip_pool_silo_list GET /v1/system/ip-pools/{pool}/sil ip_pool_silo_unlink DELETE /v1/system/ip-pools/{pool}/silos/{silo} ip_pool_silo_update PUT /v1/system/ip-pools/{pool}/silos/{silo} ip_pool_update PUT /v1/system/ip-pools/{pool} +ip_pool_utilization_view GET /v1/system/ip-pools/{pool}/utilization ip_pool_view GET /v1/system/ip-pools/{pool} networking_address_lot_block_list GET /v1/system/networking/address-lot/{address_lot}/blocks networking_address_lot_create POST /v1/system/networking/address-lot diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index a3e87b162e9..2d601adba56 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -305,6 +305,31 @@ pub struct IpPool { pub identity: IdentityMetadata, } +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct IpPoolUtilization { + // TODO: correct names + // TODO: discuss why this might be too big for u32, it's a different reason + // from total + pub allocated: Option, + + // Too-detailed explanation for the doc comment: pools containing ipv6 ranges + // can theoretically contain an enormous total number of addresses. (By + // contrast, the number of allocated IPs should always be reasonable.) In + // practice we are unlikely to run into this, but we should have a mechanism + // for dealing with it. Off the top of my head, the obvious thing to do + // would be to make total nullable, and when we convert from the u128 + // total to a u32 (a reasonable largest number we can send back in an API + // response), we simply fall back to None if the conversion fails, and the + // client is supposed to understand that possibility and handle it. Like I + // said, in practice this should be very unlikely except through operator + // error, and it is well understood that networking misconfigurations can + // bork everything. + /// Total will be null if the actual value is too large to convert to a + /// 32-bit integer. This should be unlikely in practice, but it is possible + /// if the pool contains a large IPv6 range. + pub total: Option, +} + /// An IP pool in the context of a silo #[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct SiloIpPool { diff --git a/openapi/nexus.json b/openapi/nexus.json index cacd875accc..0508490782e 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5723,6 +5723,44 @@ } } }, + "/v1/system/ip-pools/{pool}/utilization": { + "get": { + "tags": [ + "system/networking" + ], + "summary": "Fetch IP pool", + "operationId": "ip_pool_utilization_view", + "parameters": [ + { + "in": "path", + "name": "pool", + "description": "Name or ID of the IP pool", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/IpPoolUtilization" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/v1/system/ip-pools-service": { "get": { "tags": [ @@ -13652,6 +13690,24 @@ } } }, + "IpPoolUtilization": { + "type": "object", + "properties": { + "allocated": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 0 + }, + "total": { + "nullable": true, + "description": "Total will be null if the actual value is too large to convert to a 32-bit integer. This should be unlikely in practice, but it is possible if the pool contains a large IPv6 range.", + "type": "integer", + "format": "uint32", + "minimum": 0 + } + } + }, "IpRange": { "oneOf": [ { From da38cd3af7bad4f3676460e07e6a48a818a992b6 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 13 Mar 2024 12:01:35 -0500 Subject: [PATCH 02/13] sprinkle utilization checks everywhere. like if the easter bunny was also a cop --- nexus/test-utils/src/resource_helpers.rs | 26 ++++++++++++ nexus/tests/integration_tests/endpoints.rs | 12 ++++++ nexus/tests/integration_tests/external_ips.rs | 34 +++++++++++++++ nexus/tests/integration_tests/instances.rs | 42 ++++++++++++++++--- 4 files changed, 108 insertions(+), 6 deletions(-) diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 73318430003..e46aaff47ae 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -626,6 +626,32 @@ pub async fn create_router( .unwrap() } +pub async fn assert_ip_pool_utilization( + client: &ClientTestContext, + pool_name: &str, + allocated: u32, + total: u32, +) { + let url = format!("/v1/system/ip-pools/{}/utilization", pool_name); + let utilization: views::IpPoolUtilization = object_get(client, &url).await; + assert_eq!( + utilization.allocated, + Some(allocated), + "IP pool '{}': expected {} IPs allocated, got {:?}", + pool_name, + allocated, + utilization.allocated + ); + assert_eq!( + utilization.total, + Some(total), + "IP pool '{}': expected {} IPs total capacity, got {:?}", + pool_name, + total, + utilization.total + ); +} + /// Grant a role on a resource to a user /// /// * `grant_resource_url`: URL of the resource we're granting the role on diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index f18b2d961d2..7d2ff2596ab 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -656,6 +656,8 @@ pub static DEMO_IP_POOL_PROJ_URL: Lazy = Lazy::new(|| { }); pub static DEMO_IP_POOL_URL: Lazy = Lazy::new(|| format!("/v1/system/ip-pools/{}", *DEMO_IP_POOL_NAME)); +pub static DEMO_IP_POOL_UTILIZATION_URL: Lazy = + Lazy::new(|| format!("{}/utilization", *DEMO_IP_POOL_URL)); pub static DEMO_IP_POOL_UPDATE: Lazy = Lazy::new(|| params::IpPoolUpdate { identity: IdentityMetadataUpdateParams { @@ -1104,6 +1106,16 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { ], }, + // IP pool utilization + VerifyEndpoint { + url: &DEMO_IP_POOL_UTILIZATION_URL, + visibility: Visibility::Protected, + unprivileged_access: UnprivilegedAccess::None, + allowed_methods: vec![ + AllowedMethod::Get, + ], + }, + // IP Pool endpoint (Oxide services) VerifyEndpoint { url: &DEMO_IP_POOL_SERVICE_URL, diff --git a/nexus/tests/integration_tests/external_ips.rs b/nexus/tests/integration_tests/external_ips.rs index 25c74bfd7f5..5d8f3ac8b8f 100644 --- a/nexus/tests/integration_tests/external_ips.rs +++ b/nexus/tests/integration_tests/external_ips.rs @@ -17,6 +17,7 @@ use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; +use nexus_test_utils::resource_helpers::assert_ip_pool_utilization; use nexus_test_utils::resource_helpers::create_default_ip_pool; use nexus_test_utils::resource_helpers::create_floating_ip; use nexus_test_utils::resource_helpers::create_instance_with; @@ -151,6 +152,8 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { // automatically linked to current silo create_default_ip_pool(&client).await; + assert_ip_pool_utilization(client, "default", 0, 65536).await; + let other_pool_range = IpRange::V4( Ipv4Range::new(Ipv4Addr::new(10, 1, 0, 1), Ipv4Addr::new(10, 1, 0, 5)) .unwrap(), @@ -158,6 +161,8 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { // not automatically linked to currently silo. see below create_ip_pool(&client, "other-pool", Some(other_pool_range)).await; + assert_ip_pool_utilization(client, "other-pool", 0, 5).await; + let project = create_project(client, PROJECT_NAME).await; // Create with no chosen IP and fallback to default pool. @@ -175,6 +180,8 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(fip.instance_id, None); assert_eq!(fip.ip, IpAddr::from(Ipv4Addr::new(10, 0, 0, 0))); + assert_ip_pool_utilization(client, "default", 1, 65536).await; + // Create with chosen IP and fallback to default pool. let fip_name = FIP_NAMES[1]; let ip_addr = "10.0.12.34".parse().unwrap(); @@ -191,6 +198,8 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(fip.instance_id, None); assert_eq!(fip.ip, ip_addr); + assert_ip_pool_utilization(client, "default", 2, 65536).await; + // Creating with other-pool fails with 404 until it is linked to the current silo let fip_name = FIP_NAMES[2]; let params = params::FloatingIpCreate { @@ -206,6 +215,8 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { object_create_error(client, &url, ¶ms, StatusCode::NOT_FOUND).await; assert_eq!(error.message, "not found: ip-pool with name \"other-pool\""); + assert_ip_pool_utilization(client, "other-pool", 0, 5).await; + // now link the pool and everything should work with the exact same params let silo_id = DEFAULT_SILO.id(); link_ip_pool(&client, "other-pool", &silo_id, false).await; @@ -217,6 +228,8 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(fip.instance_id, None); assert_eq!(fip.ip, IpAddr::from(Ipv4Addr::new(10, 1, 0, 1))); + assert_ip_pool_utilization(client, "other-pool", 1, 5).await; + // Create with chosen IP from fleet-scoped named pool. let fip_name = FIP_NAMES[3]; let ip_addr = "10.1.0.5".parse().unwrap(); @@ -232,6 +245,8 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(fip.project_id, project.identity.id); assert_eq!(fip.instance_id, None); assert_eq!(fip.ip, ip_addr); + + assert_ip_pool_utilization(client, "other-pool", 2, 5).await; } #[nexus_test] @@ -586,6 +601,8 @@ async fn test_external_ip_live_attach_detach( create_default_ip_pool(&client).await; let project = create_project(client, PROJECT_NAME).await; + assert_ip_pool_utilization(client, "default", 0, 65536).await; + // Create 2 instances, and a floating IP for each instance. // One instance will be started, and one will be stopped. let mut fips = vec![]; @@ -602,6 +619,9 @@ async fn test_external_ip_live_attach_detach( ); } + // 2 floating IPs have been allocated + assert_ip_pool_utilization(client, "default", 2, 65536).await; + let mut instances = vec![]; for (i, start) in [false, true].iter().enumerate() { let instance = instance_for_external_ips( @@ -633,6 +653,10 @@ async fn test_external_ip_live_attach_detach( instances.push(instance); } + // the two instances above were deliberately not given ephemeral IPs, but + // they still always get SNAT IPs, so we went from 2 to 4 + assert_ip_pool_utilization(client, "default", 4, 65536).await; + // Attach a floating IP and ephemeral IP to each instance. let mut recorded_ephs = vec![]; for (instance, fip) in instances.iter().zip(&fips) { @@ -675,6 +699,10 @@ async fn test_external_ip_live_attach_detach( recorded_ephs.push(eph_resp); } + // now 6 because an ephemeral IP was added for each instance. floating IPs + // were attached, but they were already allocated + assert_ip_pool_utilization(client, "default", 6, 65536).await; + // Detach a floating IP and ephemeral IP from each instance. for (instance, fip) in instances.iter().zip(&fips) { let instance_name = instance.identity.name.as_str(); @@ -705,6 +733,9 @@ async fn test_external_ip_live_attach_detach( ); } + // 2 ephemeral go away on detachment but still 2 floating and 2 SNAT + assert_ip_pool_utilization(client, "default", 4, 65536).await; + // Finally, two kind of funny tests. There is special logic in the handler // for the case where the floating IP is specified by name but the instance // by ID and vice versa, so we want to test both combinations. @@ -763,6 +794,9 @@ async fn test_external_ip_live_attach_detach( fetch_instance_external_ips(client, instance_name, PROJECT_NAME).await; assert_eq!(eip_list.len(), 1); assert_eq!(eip_list[0].ip(), fips[1].ip); + + // none of that changed the number of allocated IPs + assert_ip_pool_utilization(client, "default", 4, 65536).await; } #[nexus_test] diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 0df2b830085..5eaecec1a15 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -20,6 +20,7 @@ use nexus_test_interface::NexusServer; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; +use nexus_test_utils::resource_helpers::assert_ip_pool_utilization; use nexus_test_utils::resource_helpers::create_default_ip_pool; use nexus_test_utils::resource_helpers::create_disk; use nexus_test_utils::resource_helpers::create_floating_ip; @@ -3905,10 +3906,14 @@ async fn test_instance_ephemeral_ip_from_correct_pool( create_ip_pool(&client, "pool1", Some(range1)).await; link_ip_pool(&client, "pool1", &DEFAULT_SILO.id(), /*default*/ true).await; + assert_ip_pool_utilization(client, "pool1", 0, 5).await; + // second pool is associated with the silo but not default create_ip_pool(&client, "pool2", Some(range2)).await; link_ip_pool(&client, "pool2", &DEFAULT_SILO.id(), /*default*/ false).await; + assert_ip_pool_utilization(client, "pool2", 0, 5).await; + // Create an instance with pool name blank, expect IP from default pool create_instance_with_pool(client, "pool1-inst", None).await; @@ -3917,6 +3922,10 @@ async fn test_instance_ephemeral_ip_from_correct_pool( ip.ip() >= range1.first_address() && ip.ip() <= range1.last_address(), "Expected ephemeral IP to come from pool1" ); + // 1 ephemeral + 1 snat + assert_ip_pool_utilization(client, "pool1", 2, 5).await; + // pool2 unaffected + assert_ip_pool_utilization(client, "pool2", 0, 5).await; // Create an instance explicitly using the non-default "other-pool". create_instance_with_pool(client, "pool2-inst", Some("pool2")).await; @@ -3926,6 +3935,13 @@ async fn test_instance_ephemeral_ip_from_correct_pool( "Expected ephemeral IP to come from pool2" ); + // added 1 snat because snat comes from default pool. + // https://github.com/oxidecomputer/omicron/issues/5043 is about changing that + assert_ip_pool_utilization(client, "pool1", 3, 5).await; + + // ephemeral IP comes from specified pool + assert_ip_pool_utilization(client, "pool2", 1, 5).await; + // make pool2 default and create instance with default pool. check that it now it comes from pool2 let _: views::IpPoolSiloLink = object_put( client, @@ -3941,6 +3957,11 @@ async fn test_instance_ephemeral_ip_from_correct_pool( "Expected ephemeral IP to come from pool2" ); + // pool1 unchanged + assert_ip_pool_utilization(client, "pool1", 3, 5).await; + // +1 snat (now that pool2 is default) and +1 ephemeral + assert_ip_pool_utilization(client, "pool2", 3, 5).await; + // try to delete association with pool1, but it fails because there is an // instance with an IP from the pool in this silo let pool1_silo_url = @@ -3956,8 +3977,14 @@ async fn test_instance_ephemeral_ip_from_correct_pool( // stop and delete instances with IPs from pool1. perhaps surprisingly, that // includes pool2-inst also because the SNAT IP comes from the default pool // even when different pool is specified for the ephemeral IP - stop_instance(&cptestctx, "pool1-inst").await; - stop_instance(&cptestctx, "pool2-inst").await; + stop_and_delete_instance(&cptestctx, "pool1-inst").await; + stop_and_delete_instance(&cptestctx, "pool2-inst").await; + + // pool1 is down to 0 because it had 1 snat + 1 ephemeral from pool1-inst + // and 1 snat from pool2-inst + assert_ip_pool_utilization(client, "pool1", 0, 5).await; + // pool2 drops one because it had 1 ephemeral from pool2-inst + assert_ip_pool_utilization(client, "pool2", 2, 5).await; // now unlink works object_delete(client, &pool1_silo_url).await; @@ -3992,7 +4019,7 @@ async fn test_instance_ephemeral_ip_from_correct_pool( assert_eq!(error.message, "not found: ip-pool with name \"pool1\""); } -async fn stop_instance( +async fn stop_and_delete_instance( cptestctx: &ControlPlaneTestContext, instance_name: &str, ) { @@ -4146,9 +4173,7 @@ async fn test_instance_attach_several_external_ips( create_ip_pool(&client, "default", Some(default_pool_range)).await; link_ip_pool(&client, "default", &DEFAULT_SILO.id(), true).await; - // this doesn't work as a replacement for the above. figure out why and - // probably delete it - // create_default_ip_pool(&client).await; + assert_ip_pool_utilization(client, "default", 0, 10).await; // Create several floating IPs for the instance, totalling 8 IPs. let mut external_ip_create = @@ -4177,6 +4202,9 @@ async fn test_instance_attach_several_external_ips( ) .await; + // 1 ephemeral + 7 floating + 1 SNAT + assert_ip_pool_utilization(client, "default", 9, 10).await; + // Verify that all external IPs are visible on the instance and have // been allocated in order. let external_ips = @@ -4331,6 +4359,8 @@ async fn test_instance_create_in_silo(cptestctx: &ControlPlaneTestContext) { create_ip_pool(&client, "default", None).await; link_ip_pool(&client, "default", &silo.identity.id, true).await; + assert_ip_pool_utilization(client, "default", 0, 65536).await; + // Create test projects NexusRequest::objects_post( client, From 25d6e2676fbe150eff4f90af5e85ccc1828bc866 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 13 Mar 2024 15:33:40 -0500 Subject: [PATCH 03/13] fix endpoint summary --- nexus/src/external_api/http_entrypoints.rs | 2 +- openapi/nexus.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 6551b7c2eeb..9c8d9cef50a 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1558,7 +1558,7 @@ async fn ip_pool_update( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/// Fetch IP pool +/// Fetch IP pool utilization #[endpoint { method = GET, path = "/v1/system/ip-pools/{pool}/utilization", diff --git a/openapi/nexus.json b/openapi/nexus.json index 0508490782e..d13f32cb935 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5728,7 +5728,7 @@ "tags": [ "system/networking" ], - "summary": "Fetch IP pool", + "summary": "Fetch IP pool utilization", "operationId": "ip_pool_utilization_view", "parameters": [ { From 57dd18c6b7536a802ec3c77da52b1f08a603fa49 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 13 Mar 2024 21:14:31 -0500 Subject: [PATCH 04/13] move datastore functions into ip_pool file --- nexus/db-queries/src/db/datastore/ip_pool.rs | 178 ++++++++++++++++- .../src/db/datastore/utilization.rs | 189 ------------------ 2 files changed, 177 insertions(+), 190 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index a9062e6ee3e..977c6123f11 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -324,6 +324,55 @@ impl DataStore { }) } + pub async fn ip_pool_allocated_count( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + ) -> Result { + opctx.authorize(authz::Action::Read, authz_pool).await?; + + use db::schema::external_ip; + + external_ip::table + .filter(external_ip::ip_pool_id.eq(authz_pool.id())) + .filter(external_ip::time_deleted.is_null()) + .select(external_ip::id) + .count() + // TODO: how much do I have to worry about this being bigger than + // u32? it seems impossible, but do I have to handle it? + .first_async::(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + } + + // TODO: should this just return the vec of ranges? + // TODO: generally we never retrieve all of anything, how bad is that + pub async fn ip_pool_total_capacity( + &self, + opctx: &OpContext, + authz_pool: &authz::IpPool, + ) -> Result { + opctx.authorize(authz::Action::Read, authz_pool).await?; + opctx.authorize(authz::Action::ListChildren, authz_pool).await?; + + use db::schema::ip_pool_range; + + let ranges: Vec = ip_pool_range::table + .filter(ip_pool_range::ip_pool_id.eq(authz_pool.id())) + .filter(ip_pool_range::time_deleted.is_null()) + .select(IpPoolRange::as_select()) + .get_results_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource(authz_pool), + ) + })?; + + Ok(ranges.iter().fold(0, |acc, r| acc + IpRange::from(r).len())) + } + pub async fn ip_pool_silo_list( &self, opctx: &OpContext, @@ -846,10 +895,14 @@ mod test { use crate::authz; use crate::db::datastore::test_utils::datastore_test; - use crate::db::model::{IpPool, IpPoolResource, IpPoolResourceType}; + use crate::db::model::{ + IpPool, IpPoolResource, IpPoolResourceType, Project, + }; use assert_matches::assert_matches; use nexus_test_utils::db::test_setup_database; + use nexus_types::external_api::params; use nexus_types::identity::Resource; + use omicron_common::address::{IpRange, Ipv4Range, Ipv6Range}; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::{ DataPageParams, Error, IdentityMetadataCreateParams, LookupType, @@ -1075,4 +1128,127 @@ mod test { db.cleanup().await.unwrap(); logctx.cleanup_successful(); } + + #[tokio::test] + async fn test_ip_utilization() { + let logctx = dev::test_setup_log("test_ip_utilization"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + let authz_silo = opctx.authn.silo_required().unwrap(); + let project = Project::new( + authz_silo.id(), + params::ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "my-project".parse().unwrap(), + description: "".to_string(), + }, + }, + ); + let (.., project) = + datastore.project_create(&opctx, project).await.unwrap(); + + // create an IP pool for the silo, add a range to it, and link it to the silo + let identity = IdentityMetadataCreateParams { + name: "my-pool".parse().unwrap(), + description: "".to_string(), + }; + let pool = datastore + .ip_pool_create(&opctx, IpPool::new(&identity)) + .await + .expect("Failed to create IP pool"); + let authz_pool = authz::IpPool::new( + authz::FLEET, + pool.id(), + LookupType::ById(pool.id()), + ); + + // capacity of zero because there are no ranges + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 0); + + let range = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 5), + ) + .unwrap(), + ); + datastore + .ip_pool_add_range(&opctx, &authz_pool, &range) + .await + .expect("Could not add range"); + + // now it has a capacity of 5 because we added the range + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 5); + + let link = IpPoolResource { + ip_pool_id: pool.id(), + resource_type: IpPoolResourceType::Silo, + resource_id: authz_silo.id(), + is_default: true, + }; + datastore + .ip_pool_link_silo(&opctx, link) + .await + .expect("Could not link pool to silo"); + + let ip_count = datastore + .ip_pool_allocated_count(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(ip_count, 0); + + let identity = IdentityMetadataCreateParams { + name: "my-ip".parse().unwrap(), + description: "".to_string(), + }; + let ip = datastore + .allocate_floating_ip(&opctx, project.id(), identity, None, None) + .await + .expect("Could not allocate floating IP"); + assert_eq!(ip.ip.to_string(), "10.0.0.1/32"); + + let ip_count = datastore + .ip_pool_allocated_count(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(ip_count, 1); + + // allocating one has nothing to do with total capacity + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 5); + + let ipv6_range = IpRange::V6( + Ipv6Range::new( + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 1, 20), + ) + .unwrap(), + ); + datastore + .ip_pool_add_range(&opctx, &authz_pool, &ipv6_range) + .await + .expect("Could not add range"); + + // now test with additional v6 range + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 5 + 11 + 65536); + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } } diff --git a/nexus/db-queries/src/db/datastore/utilization.rs b/nexus/db-queries/src/db/datastore/utilization.rs index 511b59eb217..826c66043a9 100644 --- a/nexus/db-queries/src/db/datastore/utilization.rs +++ b/nexus/db-queries/src/db/datastore/utilization.rs @@ -10,8 +10,6 @@ use crate::db::pagination::paginated; use async_bb8_diesel::AsyncRunQueryDsl; use diesel::BoolExpressionMethods; use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; -use nexus_db_model::IpPoolRange; -use omicron_common::address::IpRange; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; @@ -64,191 +62,4 @@ impl DataStore { .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } - - pub async fn ip_pool_allocated_count( - &self, - opctx: &OpContext, - authz_pool: &authz::IpPool, - ) -> Result { - opctx.authorize(authz::Action::Read, authz_pool).await?; - - use db::schema::external_ip; - - external_ip::table - .filter(external_ip::ip_pool_id.eq(authz_pool.id())) - .filter(external_ip::time_deleted.is_null()) - .select(external_ip::id) - .count() - // TODO: how much do I have to worry about this being bigger than - // u32? it seems impossible, but do I have to handle it? - .first_async::(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) - } - - // TODO: should this just return the vec of ranges and live in ip_pool.rs? - // TODO: generally we never retrieve all of anything, how bad is that - pub async fn ip_pool_total_capacity( - &self, - opctx: &OpContext, - authz_pool: &authz::IpPool, - ) -> Result { - opctx.authorize(authz::Action::Read, authz_pool).await?; - opctx.authorize(authz::Action::ListChildren, authz_pool).await?; - - use db::schema::ip_pool_range; - - let ranges: Vec = ip_pool_range::table - .filter(ip_pool_range::ip_pool_id.eq(authz_pool.id())) - .filter(ip_pool_range::time_deleted.is_null()) - .select(IpPoolRange::as_select()) - .get_results_async(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| { - public_error_from_diesel( - e, - ErrorHandler::NotFoundByResource(authz_pool), - ) - })?; - - Ok(ranges.iter().fold(0, |acc, r| acc + IpRange::from(r).len())) - } -} - -#[cfg(test)] -mod test { - use crate::authz; - use crate::db::datastore::test_utils::datastore_test; - use nexus_db_model::{IpPool, IpPoolResource, IpPoolResourceType, Project}; - use nexus_test_utils::db::test_setup_database; - use nexus_types::external_api::params; - use nexus_types::identity::Resource; - use omicron_common::address::{IpRange, Ipv4Range, Ipv6Range}; - use omicron_common::api::external::{ - IdentityMetadataCreateParams, LookupType, - }; - use omicron_test_utils::dev; - - #[tokio::test] - async fn test_ip_utilization() { - let logctx = dev::test_setup_log("test_ip_utilization"); - let mut db = test_setup_database(&logctx.log).await; - let (opctx, datastore) = datastore_test(&logctx, &db).await; - - let authz_silo = opctx.authn.silo_required().unwrap(); - let project = Project::new( - authz_silo.id(), - params::ProjectCreate { - identity: IdentityMetadataCreateParams { - name: "my-project".parse().unwrap(), - description: "".to_string(), - }, - }, - ); - let (.., project) = - datastore.project_create(&opctx, project).await.unwrap(); - - // create an IP pool for the silo, add a range to it, and link it to the silo - let identity = IdentityMetadataCreateParams { - name: "my-pool".parse().unwrap(), - description: "".to_string(), - }; - let pool = datastore - .ip_pool_create(&opctx, IpPool::new(&identity)) - .await - .expect("Failed to create IP pool"); - let authz_pool = authz::IpPool::new( - authz::FLEET, - pool.id(), - LookupType::ById(pool.id()), - ); - - // capacity of zero because there are no ranges - let max_ips = datastore - .ip_pool_total_capacity(&opctx, &authz_pool) - .await - .unwrap(); - assert_eq!(max_ips, 0); - - let range = IpRange::V4( - Ipv4Range::new( - std::net::Ipv4Addr::new(10, 0, 0, 1), - std::net::Ipv4Addr::new(10, 0, 0, 5), - ) - .unwrap(), - ); - datastore - .ip_pool_add_range(&opctx, &authz_pool, &range) - .await - .expect("Could not add range"); - - // now it has a capacity of 5 because we added the range - let max_ips = datastore - .ip_pool_total_capacity(&opctx, &authz_pool) - .await - .unwrap(); - assert_eq!(max_ips, 5); - - let link = IpPoolResource { - ip_pool_id: pool.id(), - resource_type: IpPoolResourceType::Silo, - resource_id: authz_silo.id(), - is_default: true, - }; - datastore - .ip_pool_link_silo(&opctx, link) - .await - .expect("Could not link pool to silo"); - - let ip_count = datastore - .ip_pool_allocated_count(&opctx, &authz_pool) - .await - .unwrap(); - assert_eq!(ip_count, 0); - - let identity = IdentityMetadataCreateParams { - name: "my-ip".parse().unwrap(), - description: "".to_string(), - }; - let ip = datastore - .allocate_floating_ip(&opctx, project.id(), identity, None, None) - .await - .expect("Could not allocate floating IP"); - assert_eq!(ip.ip.to_string(), "10.0.0.1/32"); - - let ip_count = datastore - .ip_pool_allocated_count(&opctx, &authz_pool) - .await - .unwrap(); - assert_eq!(ip_count, 1); - - // allocating one has nothing to do with total capacity - let max_ips = datastore - .ip_pool_total_capacity(&opctx, &authz_pool) - .await - .unwrap(); - assert_eq!(max_ips, 5); - - let ipv6_range = IpRange::V6( - Ipv6Range::new( - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 1, 20), - ) - .unwrap(), - ); - datastore - .ip_pool_add_range(&opctx, &authz_pool, &ipv6_range) - .await - .expect("Could not add range"); - - // now test with additional v6 range - let max_ips = datastore - .ip_pool_total_capacity(&opctx, &authz_pool) - .await - .unwrap(); - assert_eq!(max_ips, 5 + 11 + 65536); - - db.cleanup().await.unwrap(); - logctx.cleanup_successful(); - } } From 4629e4062cf29092edf66ec0060e548fb93be1d9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 13 Mar 2024 22:10:08 -0500 Subject: [PATCH 05/13] integration test covering too-big total size we return as None --- nexus/tests/integration_tests/ip_pools.rs | 56 +++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index f1d8825d0e8..a02c094c6a0 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -15,6 +15,7 @@ use nexus_db_queries::db::fixed_data::silo::INTERNAL_SILO_ID; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; +use nexus_test_utils::resource_helpers::assert_ip_pool_utilization; use nexus_test_utils::resource_helpers::create_instance; use nexus_test_utils::resource_helpers::create_ip_pool; use nexus_test_utils::resource_helpers::create_project; @@ -39,6 +40,7 @@ use nexus_types::external_api::shared::IpRange; use nexus_types::external_api::shared::Ipv4Range; use nexus_types::external_api::shared::Ipv6Range; use nexus_types::external_api::shared::SiloIdentityMode; +use nexus_types::external_api::views; use nexus_types::external_api::views::IpPool; use nexus_types::external_api::views::IpPoolRange; use nexus_types::external_api::views::IpPoolSiloLink; @@ -757,6 +759,60 @@ async fn create_pool(client: &ClientTestContext, name: &str) -> IpPool { .unwrap() } +// This is mostly about the threshold where the nullable total field goes +// from defined to null because the size of the pool gets too big for a +// u32. testing allocated is done in a bunch of other places. look for +// assert_ip_pool_utilization calls +#[nexus_test] +async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + + create_pool(client, "p0").await; + + assert_ip_pool_utilization(client, "p0", 0, 0).await; + + let add_url = "/v1/system/ip-pools/p0/ranges/add"; + + // add just 5 addresses to get the party started + let range = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 5), + ) + .unwrap(), + ); + object_create::(client, &add_url, &range).await; + + assert_ip_pool_utilization(client, "p0", 0, 5).await; + + // now lets add the maximum number of addresses we can and still get a total + // in the response. that's FFFF FFFF minus five for the above range + let big_range = IpRange::V6( + Ipv6Range::new( + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1), + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0xffff, 0xfffa), + ) + .unwrap(), + ); + object_create::(client, &add_url, &big_range).await; + + assert_ip_pool_utilization(client, "p0", 0, u32::MAX).await; + + // now adding one more address will take us over the threshold, making total None + let range = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 6), + std::net::Ipv4Addr::new(10, 0, 0, 6), + ) + .unwrap(), + ); + object_create::(client, &add_url, &range).await; + + let utilization: views::IpPoolUtilization = + object_get(client, "/v1/system/ip-pools/p0/utilization").await; + assert_eq!(utilization.total, None); +} + // Data for testing overlapping IP ranges struct TestRange { // A starting IP range that should be inserted correctly From e320f1aa0079d8639e22214d856683bc56b16019 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 13 Mar 2024 22:17:26 -0500 Subject: [PATCH 06/13] silly unit test for conversion to utilization view --- nexus/db-model/src/utilization.rs | 41 +++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/nexus/db-model/src/utilization.rs b/nexus/db-model/src/utilization.rs index 1d1f7196808..9145de9afcb 100644 --- a/nexus/db-model/src/utilization.rs +++ b/nexus/db-model/src/utilization.rs @@ -73,3 +73,44 @@ impl From for views::IpPoolUtilization { } } } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_ip_pool_utilization_to_view() { + let view: views::IpPoolUtilization = + IpPoolUtilization { allocated: 40, total: 100 }.into(); + assert_eq!(view.allocated, Some(40)); + assert_eq!(view.total, Some(100)); + + let view: views::IpPoolUtilization = + IpPoolUtilization { allocated: i64::from(u32::MAX), total: 100 } + .into(); + assert_eq!(view.allocated, Some(u32::MAX)); + assert_eq!(view.total, Some(100)); + + let view: views::IpPoolUtilization = IpPoolUtilization { + allocated: i64::from(u32::MAX) + 1, + total: 100, + } + .into(); + assert_eq!(view.allocated, None); + assert_eq!(view.total, Some(100)); + + let view: views::IpPoolUtilization = + IpPoolUtilization { allocated: 40, total: u128::from(u32::MAX) } + .into(); + assert_eq!(view.allocated, Some(40)); + assert_eq!(view.total, Some(u32::MAX)); + + let view: views::IpPoolUtilization = IpPoolUtilization { + allocated: 40, + total: u128::from(u32::MAX) + 1, + } + .into(); + assert_eq!(view.allocated, Some(40)); + assert_eq!(view.total, None); + } +} From 03a6a58e88403e9c32a721d6e91af7cb01ca5c7f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Mar 2024 12:45:46 -0500 Subject: [PATCH 07/13] serialize u128 as "type": "string", "format": "uint128" instead of nullifying --- nexus/db-model/src/utilization.rs | 16 ++---- nexus/test-utils/src/resource_helpers.rs | 9 +-- nexus/tests/integration_tests/ip_pools.rs | 29 +++------- nexus/types/src/external_api/views.rs | 68 +++++++++++++++++------ openapi/nexus.json | 13 +++-- 5 files changed, 75 insertions(+), 60 deletions(-) diff --git a/nexus/db-model/src/utilization.rs b/nexus/db-model/src/utilization.rs index 9145de9afcb..5a2d6ce45e6 100644 --- a/nexus/db-model/src/utilization.rs +++ b/nexus/db-model/src/utilization.rs @@ -66,11 +66,7 @@ pub struct IpPoolUtilization { impl From for views::IpPoolUtilization { fn from(util: IpPoolUtilization) -> Self { - Self { - allocated: util.allocated.try_into().ok(), - // if u128 is too big to convert to u32, fall back to None (null) - total: util.total.try_into().ok(), - } + Self { allocated: util.allocated.try_into().ok(), total: util.total } } } @@ -83,13 +79,13 @@ mod test { let view: views::IpPoolUtilization = IpPoolUtilization { allocated: 40, total: 100 }.into(); assert_eq!(view.allocated, Some(40)); - assert_eq!(view.total, Some(100)); + assert_eq!(view.total, 100); let view: views::IpPoolUtilization = IpPoolUtilization { allocated: i64::from(u32::MAX), total: 100 } .into(); assert_eq!(view.allocated, Some(u32::MAX)); - assert_eq!(view.total, Some(100)); + assert_eq!(view.total, 100); let view: views::IpPoolUtilization = IpPoolUtilization { allocated: i64::from(u32::MAX) + 1, @@ -97,13 +93,13 @@ mod test { } .into(); assert_eq!(view.allocated, None); - assert_eq!(view.total, Some(100)); + assert_eq!(view.total, 100); let view: views::IpPoolUtilization = IpPoolUtilization { allocated: 40, total: u128::from(u32::MAX) } .into(); assert_eq!(view.allocated, Some(40)); - assert_eq!(view.total, Some(u32::MAX)); + assert_eq!(view.total, u128::from(u32::MAX)); let view: views::IpPoolUtilization = IpPoolUtilization { allocated: 40, @@ -111,6 +107,6 @@ mod test { } .into(); assert_eq!(view.allocated, Some(40)); - assert_eq!(view.total, None); + assert_eq!(view.total, u128::from(u32::MAX) + 1); } } diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index e46aaff47ae..30534d04b23 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -630,7 +630,7 @@ pub async fn assert_ip_pool_utilization( client: &ClientTestContext, pool_name: &str, allocated: u32, - total: u32, + total: u128, ) { let url = format!("/v1/system/ip-pools/{}/utilization", pool_name); let utilization: views::IpPoolUtilization = object_get(client, &url).await; @@ -643,12 +643,9 @@ pub async fn assert_ip_pool_utilization( utilization.allocated ); assert_eq!( - utilization.total, - Some(total), + utilization.total, total, "IP pool '{}': expected {} IPs total capacity, got {:?}", - pool_name, - total, - utilization.total + pool_name, total, utilization.total ); } diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index a02c094c6a0..d8ac77c633c 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -40,7 +40,6 @@ use nexus_types::external_api::shared::IpRange; use nexus_types::external_api::shared::Ipv4Range; use nexus_types::external_api::shared::Ipv6Range; use nexus_types::external_api::shared::SiloIdentityMode; -use nexus_types::external_api::views; use nexus_types::external_api::views::IpPool; use nexus_types::external_api::views::IpPoolRange; use nexus_types::external_api::views::IpPoolSiloLink; @@ -759,9 +758,8 @@ async fn create_pool(client: &ClientTestContext, name: &str) -> IpPool { .unwrap() } -// This is mostly about the threshold where the nullable total field goes -// from defined to null because the size of the pool gets too big for a -// u32. testing allocated is done in a bunch of other places. look for +// This is mostly about testing the total field with huge numbers. +// testing allocated is done in a bunch of other places. look for // assert_ip_pool_utilization calls #[nexus_test] async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { @@ -785,32 +783,19 @@ async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { assert_ip_pool_utilization(client, "p0", 0, 5).await; - // now lets add the maximum number of addresses we can and still get a total - // in the response. that's FFFF FFFF minus five for the above range + // now let's add a gigantic range just for fun let big_range = IpRange::V6( Ipv6Range::new( std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 1), - std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0xffff, 0xfffa), + std::net::Ipv6Addr::new( + 0xfd00, 0, 0, 0, 0xffff, 0xfff, 0xffff, 0xffff, + ), ) .unwrap(), ); object_create::(client, &add_url, &big_range).await; - assert_ip_pool_utilization(client, "p0", 0, u32::MAX).await; - - // now adding one more address will take us over the threshold, making total None - let range = IpRange::V4( - Ipv4Range::new( - std::net::Ipv4Addr::new(10, 0, 0, 6), - std::net::Ipv4Addr::new(10, 0, 0, 6), - ) - .unwrap(), - ); - object_create::(client, &add_url, &range).await; - - let utilization: views::IpPoolUtilization = - object_get(client, "/v1/system/ip-pools/p0/utilization").await; - assert_eq!(utilization.total, None); + assert_ip_pool_utilization(client, "p0", 0, 18446480190918885380).await; } // Data for testing overlapping IP ranges diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index 2d601adba56..7fa415e8c2a 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -310,24 +310,60 @@ pub struct IpPoolUtilization { // TODO: correct names // TODO: discuss why this might be too big for u32, it's a different reason // from total + // TODO: either handle this as a U128String like total or decide that > u32 + // is so unlikely we should just error pub allocated: Option, - // Too-detailed explanation for the doc comment: pools containing ipv6 ranges - // can theoretically contain an enormous total number of addresses. (By - // contrast, the number of allocated IPs should always be reasonable.) In - // practice we are unlikely to run into this, but we should have a mechanism - // for dealing with it. Off the top of my head, the obvious thing to do - // would be to make total nullable, and when we convert from the u128 - // total to a u32 (a reasonable largest number we can send back in an API - // response), we simply fall back to None if the conversion fails, and the - // client is supposed to understand that possibility and handle it. Like I - // said, in practice this should be very unlikely except through operator - // error, and it is well understood that networking misconfigurations can - // bork everything. - /// Total will be null if the actual value is too large to convert to a - /// 32-bit integer. This should be unlikely in practice, but it is possible - /// if the pool contains a large IPv6 range. - pub total: Option, + /// The total number of IP addresses in the pool. + /// + /// An IPv6 range can contain up to 2^128 addresses, so we represent this + /// value in JSON as a string with a custom "uint128" format. + // By contrast, the number of allocated IPs should always be reasonable. + #[serde(with = "U128String")] + pub total: u128, +} + +// Custom struct for serializing/deserializing u128 as a string. The serde +// docs will suggest using a module (or serialize_with and deserialize_with +// functions), but as discussed in the comments on the UserData de/serializer, +// schemars wants this to be a type, so it has to be a struct. +struct U128String; +impl U128String { + pub fn serialize(value: &u128, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_str(&value.to_string()) + } + + pub fn deserialize<'de, D>(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + s.parse().map_err(serde::de::Error::custom) + } +} + +impl JsonSchema for U128String { + fn schema_name() -> String { + "String".to_string() + } + + fn json_schema( + _: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + schemars::schema::SchemaObject { + instance_type: Some(schemars::schema::InstanceType::String.into()), + format: Some("uint128".to_string()), + ..Default::default() + } + .into() + } + + fn is_referenceable() -> bool { + false + } } /// An IP pool in the context of a silo diff --git a/openapi/nexus.json b/openapi/nexus.json index d13f32cb935..b371e2d7164 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -13700,13 +13700,14 @@ "minimum": 0 }, "total": { - "nullable": true, - "description": "Total will be null if the actual value is too large to convert to a 32-bit integer. This should be unlikely in practice, but it is possible if the pool contains a large IPv6 range.", - "type": "integer", - "format": "uint32", - "minimum": 0 + "description": "The total number of IP addresses in the pool.\n\nAn IPv6 range can contain up to 2^128 addresses, so we represent this value in JSON as a string with a custom \"uint128\" format.", + "type": "string", + "format": "uint128" } - } + }, + "required": [ + "total" + ] }, "IpRange": { "oneOf": [ From 7f4052fa1622c736a20a9ec0881c5effaa6136ef Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Mar 2024 15:48:38 -0500 Subject: [PATCH 08/13] make allocated a u128 too --- nexus/db-model/src/utilization.rs | 46 +------------------- nexus/db-queries/src/db/datastore/ip_pool.rs | 21 ++++++--- nexus/test-utils/src/resource_helpers.rs | 9 ++-- nexus/types/src/external_api/views.rs | 28 +++++++----- openapi/nexus.json | 10 ++--- 5 files changed, 43 insertions(+), 71 deletions(-) diff --git a/nexus/db-model/src/utilization.rs b/nexus/db-model/src/utilization.rs index 5a2d6ce45e6..671a15ff25c 100644 --- a/nexus/db-model/src/utilization.rs +++ b/nexus/db-model/src/utilization.rs @@ -59,54 +59,12 @@ impl From for views::Utilization { // Not really a DB model, just the result of a datastore function #[derive(Debug, Clone, Serialize, Deserialize)] pub struct IpPoolUtilization { - // has to be i64 because SQL counts from Diesel are BigInts - pub allocated: i64, + pub allocated: u128, pub total: u128, } impl From for views::IpPoolUtilization { fn from(util: IpPoolUtilization) -> Self { - Self { allocated: util.allocated.try_into().ok(), total: util.total } - } -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn test_ip_pool_utilization_to_view() { - let view: views::IpPoolUtilization = - IpPoolUtilization { allocated: 40, total: 100 }.into(); - assert_eq!(view.allocated, Some(40)); - assert_eq!(view.total, 100); - - let view: views::IpPoolUtilization = - IpPoolUtilization { allocated: i64::from(u32::MAX), total: 100 } - .into(); - assert_eq!(view.allocated, Some(u32::MAX)); - assert_eq!(view.total, 100); - - let view: views::IpPoolUtilization = IpPoolUtilization { - allocated: i64::from(u32::MAX) + 1, - total: 100, - } - .into(); - assert_eq!(view.allocated, None); - assert_eq!(view.total, 100); - - let view: views::IpPoolUtilization = - IpPoolUtilization { allocated: 40, total: u128::from(u32::MAX) } - .into(); - assert_eq!(view.allocated, Some(40)); - assert_eq!(view.total, u128::from(u32::MAX)); - - let view: views::IpPoolUtilization = IpPoolUtilization { - allocated: 40, - total: u128::from(u32::MAX) + 1, - } - .into(); - assert_eq!(view.allocated, Some(40)); - assert_eq!(view.total, u128::from(u32::MAX) + 1); + Self { allocated: util.allocated, total: util.total } } } diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 977c6123f11..03cc2586839 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -328,21 +328,32 @@ impl DataStore { &self, opctx: &OpContext, authz_pool: &authz::IpPool, - ) -> Result { + ) -> Result { opctx.authorize(authz::Action::Read, authz_pool).await?; use db::schema::external_ip; - external_ip::table + let count = external_ip::table .filter(external_ip::ip_pool_id.eq(authz_pool.id())) .filter(external_ip::time_deleted.is_null()) .select(external_ip::id) .count() - // TODO: how much do I have to worry about this being bigger than - // u32? it seems impossible, but do I have to handle it? .first_async::(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + // This is a bit strange but it should be ok. + // a) SQL tends to represent counts as signed integers even though + // they can't really be negative, and Diesel follows that. We + // assume here that it will be a positive i64. + // b) It could clearly be a u64, but the calling code immediate + // converts it to a u128 anyway, so it would be pointless. + Ok(u128::try_from(count).map_err(|_e| { + Error::internal_error(&format!( + "Failed to convert i64 {} SQL count to u64", + count + )) + })?) } // TODO: should this just return the vec of ranges? diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 30534d04b23..d3b5caec480 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -629,18 +629,15 @@ pub async fn create_router( pub async fn assert_ip_pool_utilization( client: &ClientTestContext, pool_name: &str, - allocated: u32, + allocated: u128, total: u128, ) { let url = format!("/v1/system/ip-pools/{}/utilization", pool_name); let utilization: views::IpPoolUtilization = object_get(client, &url).await; assert_eq!( - utilization.allocated, - Some(allocated), + utilization.allocated, allocated, "IP pool '{}': expected {} IPs allocated, got {:?}", - pool_name, - allocated, - utilization.allocated + pool_name, allocated, utilization.allocated ); assert_eq!( utilization.total, total, diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index 7fa415e8c2a..c526552aae7 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -307,18 +307,24 @@ pub struct IpPool { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct IpPoolUtilization { - // TODO: correct names - // TODO: discuss why this might be too big for u32, it's a different reason - // from total - // TODO: either handle this as a U128String like total or decide that > u32 - // is so unlikely we should just error - pub allocated: Option, - - /// The total number of IP addresses in the pool. + // TODO: make sure these names are satisfactory + + // Unlike total, the reason this is bigger than u32 is simply that the + // DB count comes back as an i64, and in theory the count could really + // be bigger than 2^32 = ~4.29B, though in practice that is virtually + // impossible. + /// The number of IPs allocated from the pool /// - /// An IPv6 range can contain up to 2^128 addresses, so we represent this - /// value in JSON as a string with a custom "uint128" format. - // By contrast, the number of allocated IPs should always be reasonable. + /// Like total, this is a numeric string with a custom "uint128" format + /// representing a 128-bit integer, though in practice it is extremely + /// unlikely to be bigger than 32 bits (2^32 = ~4.29 billion). + #[serde(with = "U128String")] + pub allocated: u128, + + /// The total number of IP addresses in the pool, i.e., the sum of the + /// lengths of the ranges. An IPv6 range can contain up to 2^128 addresses, + /// so we represent this value in JSON as a numeric string with a custom + /// "uint128" format. #[serde(with = "U128String")] pub total: u128, } diff --git a/openapi/nexus.json b/openapi/nexus.json index b371e2d7164..d4f0e9bed25 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -13694,18 +13694,18 @@ "type": "object", "properties": { "allocated": { - "nullable": true, - "type": "integer", - "format": "uint32", - "minimum": 0 + "description": "The number of IPs allocated from the pool\n\nLike total, this is a numeric string with a custom \"uint128\" format representing a 128-bit integer, though in practice it is extremely unlikely to be bigger than 32 bits (2^32 = ~4.29 billion).", + "type": "string", + "format": "uint128" }, "total": { - "description": "The total number of IP addresses in the pool.\n\nAn IPv6 range can contain up to 2^128 addresses, so we represent this value in JSON as a string with a custom \"uint128\" format.", + "description": "The total number of IP addresses in the pool\n\nThis is calculated as the sum of the lengths of the ranges in the pool. An IPv6 range can contain up to 2^128 addresses, so we represent this value in JSON as a numeric string with a custom \"uint128\" format.", "type": "string", "format": "uint128" } }, "required": [ + "allocated", "total" ] }, From 759e3e6e594d4f7fd0ec4a06478e9bb9906034d5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Mar 2024 15:55:36 -0500 Subject: [PATCH 09/13] rename "total" to "capacity" everywhere --- nexus/db-model/src/utilization.rs | 4 ++-- nexus/db-queries/src/db/datastore/ip_pool.rs | 23 +++++++++++++++++++- nexus/src/app/utilization.rs | 4 ++-- nexus/test-utils/src/resource_helpers.rs | 6 ++--- nexus/types/src/external_api/views.rs | 2 +- openapi/nexus.json | 6 ++--- 6 files changed, 33 insertions(+), 12 deletions(-) diff --git a/nexus/db-model/src/utilization.rs b/nexus/db-model/src/utilization.rs index 671a15ff25c..8dc3b7c1f7a 100644 --- a/nexus/db-model/src/utilization.rs +++ b/nexus/db-model/src/utilization.rs @@ -60,11 +60,11 @@ impl From for views::Utilization { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct IpPoolUtilization { pub allocated: u128, - pub total: u128, + pub capacity: u128, } impl From for views::IpPoolUtilization { fn from(util: IpPoolUtilization) -> Self { - Self { allocated: util.allocated, total: util.total } + Self { allocated: util.allocated, capacity: util.capacity } } } diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 03cc2586839..bfce025737c 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -1141,7 +1141,7 @@ mod test { } #[tokio::test] - async fn test_ip_utilization() { + async fn test_ip_pool_utilization() { let logctx = dev::test_setup_log("test_ip_utilization"); let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; @@ -1259,6 +1259,27 @@ mod test { .unwrap(); assert_eq!(max_ips, 5 + 11 + 65536); + // add a giant range for fun + let ipv6_range = IpRange::V6( + Ipv6Range::new( + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 1, 21), + std::net::Ipv6Addr::new( + 0xfd00, 0, 0, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, + ), + ) + .unwrap(), + ); + datastore + .ip_pool_add_range(&opctx, &authz_pool, &ipv6_range) + .await + .expect("Could not add range"); + + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips, 1208925819614629174706171); + db.cleanup().await.unwrap(); logctx.cleanup_successful(); } diff --git a/nexus/src/app/utilization.rs b/nexus/src/app/utilization.rs index 88d29dd76ed..05a9063da24 100644 --- a/nexus/src/app/utilization.rs +++ b/nexus/src/app/utilization.rs @@ -43,10 +43,10 @@ impl super::Nexus { .db_datastore .ip_pool_allocated_count(opctx, &authz_pool) .await?; - let total = self + let capacity = self .db_datastore .ip_pool_total_capacity(opctx, &authz_pool) .await?; - Ok(IpPoolUtilization { allocated, total }) + Ok(IpPoolUtilization { allocated, capacity }) } } diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index d3b5caec480..213951793d8 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -630,7 +630,7 @@ pub async fn assert_ip_pool_utilization( client: &ClientTestContext, pool_name: &str, allocated: u128, - total: u128, + capacity: u128, ) { let url = format!("/v1/system/ip-pools/{}/utilization", pool_name); let utilization: views::IpPoolUtilization = object_get(client, &url).await; @@ -640,9 +640,9 @@ pub async fn assert_ip_pool_utilization( pool_name, allocated, utilization.allocated ); assert_eq!( - utilization.total, total, + utilization.capacity, capacity, "IP pool '{}': expected {} IPs total capacity, got {:?}", - pool_name, total, utilization.total + pool_name, capacity, utilization.capacity ); } diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index c526552aae7..dc8e5ea6860 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -326,7 +326,7 @@ pub struct IpPoolUtilization { /// so we represent this value in JSON as a numeric string with a custom /// "uint128" format. #[serde(with = "U128String")] - pub total: u128, + pub capacity: u128, } // Custom struct for serializing/deserializing u128 as a string. The serde diff --git a/openapi/nexus.json b/openapi/nexus.json index d4f0e9bed25..b464023d782 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -13698,15 +13698,15 @@ "type": "string", "format": "uint128" }, - "total": { - "description": "The total number of IP addresses in the pool\n\nThis is calculated as the sum of the lengths of the ranges in the pool. An IPv6 range can contain up to 2^128 addresses, so we represent this value in JSON as a numeric string with a custom \"uint128\" format.", + "capacity": { + "description": "The total number of IP addresses in the pool, i.e., the sum of the lengths of the ranges. An IPv6 range can contain up to 2^128 addresses, so we represent this value in JSON as a numeric string with a custom \"uint128\" format.", "type": "string", "format": "uint128" } }, "required": [ "allocated", - "total" + "capacity" ] }, "IpRange": { From 9860fa873a638b2a5baa490f9f83bbb7365eb0a8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Mar 2024 16:55:14 -0500 Subject: [PATCH 10/13] fix clippy --- common/src/address.rs | 3 +-- nexus/db-queries/src/db/datastore/ip_pool.rs | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/common/src/address.rs b/common/src/address.rs index 672937e9be5..8e128103433 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -422,8 +422,7 @@ impl IpRange { } } - // Has to be u128 to accommodate IPv6 -- logic around converting back down - // to u32 (if possible) lives in the view layer + // Has to be u128 to accommodate IPv6 pub fn len(&self) -> u128 { match self { IpRange::V4(ip4) => u128::from(ip4.len()), diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index bfce025737c..ab41ec6db77 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -348,12 +348,12 @@ impl DataStore { // assume here that it will be a positive i64. // b) It could clearly be a u64, but the calling code immediate // converts it to a u128 anyway, so it would be pointless. - Ok(u128::try_from(count).map_err(|_e| { + u128::try_from(count).map_err(|_e| { Error::internal_error(&format!( "Failed to convert i64 {} SQL count to u64", count )) - })?) + }) } // TODO: should this just return the vec of ranges? From 39edb8ffad89148e40f7be298096e6b7aa5ac899 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 15 Mar 2024 22:16:30 -0500 Subject: [PATCH 11/13] split utilization into separate ipv4 and ipv6 counts --- nexus/db-model/src/utilization.rs | 30 +++++- nexus/db-queries/src/db/datastore/ip_pool.rs | 101 +++++++++++++----- nexus/src/app/utilization.rs | 37 ++++++- nexus/test-utils/src/resource_helpers.rs | 28 +++-- nexus/tests/integration_tests/external_ips.rs | 26 ++--- nexus/tests/integration_tests/instances.rs | 26 ++--- nexus/tests/integration_tests/ip_pools.rs | 7 +- nexus/types/src/external_api/views.rs | 43 +++++--- openapi/nexus.json | 66 ++++++++++-- 9 files changed, 268 insertions(+), 96 deletions(-) diff --git a/nexus/db-model/src/utilization.rs b/nexus/db-model/src/utilization.rs index 8dc3b7c1f7a..e82600d64dc 100644 --- a/nexus/db-model/src/utilization.rs +++ b/nexus/db-model/src/utilization.rs @@ -56,15 +56,39 @@ impl From for views::Utilization { } } -// Not really a DB model, just the result of a datastore function #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct IpPoolUtilization { +pub struct Ipv4Utilization { + pub allocated: u32, + pub capacity: u32, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct Ipv6Utilization { pub allocated: u128, pub capacity: u128, } +// Not really a DB model, just the result of a datastore function +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct IpPoolUtilization { + pub ipv4: Ipv4Utilization, + pub ipv6: Ipv6Utilization, +} + +impl From for views::Ipv4Utilization { + fn from(util: Ipv4Utilization) -> Self { + Self { allocated: util.allocated, capacity: util.capacity } + } +} + +impl From for views::Ipv6Utilization { + fn from(util: Ipv6Utilization) -> Self { + Self { allocated: util.allocated, capacity: util.capacity } + } +} + impl From for views::IpPoolUtilization { fn from(util: IpPoolUtilization) -> Self { - Self { allocated: util.allocated, capacity: util.capacity } + Self { ipv4: util.ipv4.into(), ipv6: util.ipv6.into() } } } diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index ab41ec6db77..fa1f851ee32 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -51,6 +51,16 @@ use omicron_common::api::external::UpdateResult; use ref_cast::RefCast; use uuid::Uuid; +pub struct IpsAllocated { + pub ipv4: i64, + pub ipv6: i64, +} + +pub struct IpsCapacity { + pub ipv4: u32, + pub ipv6: u128, +} + impl DataStore { /// List IP Pools pub async fn ip_pools_list( @@ -328,32 +338,48 @@ impl DataStore { &self, opctx: &OpContext, authz_pool: &authz::IpPool, - ) -> Result { + ) -> Result { opctx.authorize(authz::Action::Read, authz_pool).await?; use db::schema::external_ip; + use diesel::dsl::{count_star, sql}; + use diesel::sql_types::BigInt; - let count = external_ip::table + let counts = external_ip::table .filter(external_ip::ip_pool_id.eq(authz_pool.id())) .filter(external_ip::time_deleted.is_null()) - .select(external_ip::id) - .count() - .first_async::(&*self.pool_connection_authorized(opctx).await?) + // Naturally, what I wanted was something like this: + // + // .group_by(family(external_ip::ip)) + // .select((family(external_ip::ip), count_star())) + // + // but diesel does not like the type, plus diesel doesn't let you do the + // column alias bit. + // + // Also worth noting: while the dsl family() function returns an + // Integer, when I do it with plain sql, if I use Integer and i32, + // I get errors about trying to unpack more than 32 bits. So we use + // BigInt and i64. + .group_by(sql::("ip_family")) + .select((sql::("family(ip) AS ip_family"), count_star())) + .get_results_async::<(i64, i64)>( + &*self.pool_connection_authorized(opctx).await?, + ) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - // This is a bit strange but it should be ok. - // a) SQL tends to represent counts as signed integers even though - // they can't really be negative, and Diesel follows that. We - // assume here that it will be a positive i64. - // b) It could clearly be a u64, but the calling code immediate - // converts it to a u128 anyway, so it would be pointless. - u128::try_from(count).map_err(|_e| { - Error::internal_error(&format!( - "Failed to convert i64 {} SQL count to u64", - count - )) - }) + let mut ipv4 = 0; + let mut ipv6 = 0; + + for &(family, count) in &counts { + match family { + 4 => ipv4 = count, + 6 => ipv6 = count, + _ => (), + } + } + + Ok(IpsAllocated { ipv4, ipv6 }) } // TODO: should this just return the vec of ranges? @@ -362,17 +388,19 @@ impl DataStore { &self, opctx: &OpContext, authz_pool: &authz::IpPool, - ) -> Result { + ) -> Result { opctx.authorize(authz::Action::Read, authz_pool).await?; opctx.authorize(authz::Action::ListChildren, authz_pool).await?; use db::schema::ip_pool_range; - let ranges: Vec = ip_pool_range::table + let ranges = ip_pool_range::table .filter(ip_pool_range::ip_pool_id.eq(authz_pool.id())) .filter(ip_pool_range::time_deleted.is_null()) .select(IpPoolRange::as_select()) - .get_results_async(&*self.pool_connection_authorized(opctx).await?) + .get_results_async::( + &*self.pool_connection_authorized(opctx).await?, + ) .await .map_err(|e| { public_error_from_diesel( @@ -381,7 +409,17 @@ impl DataStore { ) })?; - Ok(ranges.iter().fold(0, |acc, r| acc + IpRange::from(r).len())) + let mut ipv4: u32 = 0; + let mut ipv6: u128 = 0; + + for range in &ranges { + let r = IpRange::from(range); + match r { + IpRange::V4(r) => ipv4 += r.len(), + IpRange::V6(r) => ipv6 += r.len(), + } + } + Ok(IpsCapacity { ipv4, ipv6 }) } pub async fn ip_pool_silo_list( @@ -1179,7 +1217,8 @@ mod test { .ip_pool_total_capacity(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(max_ips, 0); + assert_eq!(max_ips.ipv4, 0); + assert_eq!(max_ips.ipv6, 0); let range = IpRange::V4( Ipv4Range::new( @@ -1198,7 +1237,8 @@ mod test { .ip_pool_total_capacity(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(max_ips, 5); + assert_eq!(max_ips.ipv4, 5); + assert_eq!(max_ips.ipv6, 0); let link = IpPoolResource { ip_pool_id: pool.id(), @@ -1215,7 +1255,8 @@ mod test { .ip_pool_allocated_count(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(ip_count, 0); + assert_eq!(ip_count.ipv4, 0); + assert_eq!(ip_count.ipv6, 0); let identity = IdentityMetadataCreateParams { name: "my-ip".parse().unwrap(), @@ -1231,14 +1272,16 @@ mod test { .ip_pool_allocated_count(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(ip_count, 1); + assert_eq!(ip_count.ipv4, 1); + assert_eq!(ip_count.ipv6, 0); // allocating one has nothing to do with total capacity let max_ips = datastore .ip_pool_total_capacity(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(max_ips, 5); + assert_eq!(max_ips.ipv4, 5); + assert_eq!(max_ips.ipv6, 0); let ipv6_range = IpRange::V6( Ipv6Range::new( @@ -1257,7 +1300,8 @@ mod test { .ip_pool_total_capacity(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(max_ips, 5 + 11 + 65536); + assert_eq!(max_ips.ipv4, 5); + assert_eq!(max_ips.ipv6, 11 + 65536); // add a giant range for fun let ipv6_range = IpRange::V6( @@ -1278,7 +1322,8 @@ mod test { .ip_pool_total_capacity(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(max_ips, 1208925819614629174706171); + assert_eq!(max_ips.ipv4, 5); + assert_eq!(max_ips.ipv6, 1208925819614629174706166); db.cleanup().await.unwrap(); logctx.cleanup_successful(); diff --git a/nexus/src/app/utilization.rs b/nexus/src/app/utilization.rs index 05a9063da24..bd5f5d002ab 100644 --- a/nexus/src/app/utilization.rs +++ b/nexus/src/app/utilization.rs @@ -5,6 +5,8 @@ //! Insights into capacity and utilization use nexus_db_model::IpPoolUtilization; +use nexus_db_model::Ipv4Utilization; +use nexus_db_model::Ipv6Utilization; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; @@ -39,6 +41,7 @@ impl super::Nexus { ) -> Result { let (.., authz_pool) = pool_lookup.lookup_for(authz::Action::Read).await?; + let allocated = self .db_datastore .ip_pool_allocated_count(opctx, &authz_pool) @@ -47,6 +50,38 @@ impl super::Nexus { .db_datastore .ip_pool_total_capacity(opctx, &authz_pool) .await?; - Ok(IpPoolUtilization { allocated, capacity }) + + // we have one query for v4 and v6 allocated and one for both v4 and + // v6 capacity so we can do two queries instead 4, but in the response + // we want them paired by IP version + Ok(IpPoolUtilization { + ipv4: Ipv4Utilization { + // This one less trivial to justify than the u128 conversion + // below because an i64 could obviously be too big for u32. + // In addition to the fact that it is unlikely for anyone to + // allocate 4 billion IPs, we rely on the fact that there can + // only be 2^32 IPv4 addresses, period. + allocated: u32::try_from(allocated.ipv4).map_err(|_e| { + Error::internal_error(&format!( + "Failed to convert i64 {} IPv4 address count to u32", + allocated.ipv4 + )) + })?, + capacity: capacity.ipv4, + }, + ipv6: Ipv6Utilization { + // SQL represents counts as signed integers for historical + // or compatibility reasons even though they can't really be + // negative, and Diesel follows that. We assume here that it + // will be a positive i64. + allocated: u128::try_from(allocated.ipv6).map_err(|_e| { + Error::internal_error(&format!( + "Failed to convert i64 {} IPv6 address count to u128", + allocated.ipv6 + )) + })?, + capacity: capacity.ipv6, + }, + }) } } diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 213951793d8..19631406a33 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -629,20 +629,32 @@ pub async fn create_router( pub async fn assert_ip_pool_utilization( client: &ClientTestContext, pool_name: &str, - allocated: u128, - capacity: u128, + ipv4_allocated: u32, + ipv4_capacity: u32, + ipv6_allocated: u128, + ipv6_capacity: u128, ) { let url = format!("/v1/system/ip-pools/{}/utilization", pool_name); let utilization: views::IpPoolUtilization = object_get(client, &url).await; assert_eq!( - utilization.allocated, allocated, - "IP pool '{}': expected {} IPs allocated, got {:?}", - pool_name, allocated, utilization.allocated + utilization.ipv4.allocated, ipv4_allocated, + "IP pool '{}': expected {} IPv4 allocated, got {:?}", + pool_name, ipv4_allocated, utilization.ipv4.allocated ); assert_eq!( - utilization.capacity, capacity, - "IP pool '{}': expected {} IPs total capacity, got {:?}", - pool_name, capacity, utilization.capacity + utilization.ipv4.capacity, ipv4_capacity, + "IP pool '{}': expected {} IPv4 capacity, got {:?}", + pool_name, ipv4_capacity, utilization.ipv4.capacity + ); + assert_eq!( + utilization.ipv6.allocated, ipv6_allocated, + "IP pool '{}': expected {} IPv6 allocated, got {:?}", + pool_name, ipv6_allocated, utilization.ipv6.allocated + ); + assert_eq!( + utilization.ipv6.capacity, ipv6_capacity, + "IP pool '{}': expected {} IPv6 capacity, got {:?}", + pool_name, ipv6_capacity, utilization.ipv6.capacity ); } diff --git a/nexus/tests/integration_tests/external_ips.rs b/nexus/tests/integration_tests/external_ips.rs index 5d8f3ac8b8f..fa884f8731c 100644 --- a/nexus/tests/integration_tests/external_ips.rs +++ b/nexus/tests/integration_tests/external_ips.rs @@ -152,7 +152,7 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { // automatically linked to current silo create_default_ip_pool(&client).await; - assert_ip_pool_utilization(client, "default", 0, 65536).await; + assert_ip_pool_utilization(client, "default", 0, 65536, 0, 0).await; let other_pool_range = IpRange::V4( Ipv4Range::new(Ipv4Addr::new(10, 1, 0, 1), Ipv4Addr::new(10, 1, 0, 5)) @@ -161,7 +161,7 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { // not automatically linked to currently silo. see below create_ip_pool(&client, "other-pool", Some(other_pool_range)).await; - assert_ip_pool_utilization(client, "other-pool", 0, 5).await; + assert_ip_pool_utilization(client, "other-pool", 0, 5, 0, 0).await; let project = create_project(client, PROJECT_NAME).await; @@ -180,7 +180,7 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(fip.instance_id, None); assert_eq!(fip.ip, IpAddr::from(Ipv4Addr::new(10, 0, 0, 0))); - assert_ip_pool_utilization(client, "default", 1, 65536).await; + assert_ip_pool_utilization(client, "default", 1, 65536, 0, 0).await; // Create with chosen IP and fallback to default pool. let fip_name = FIP_NAMES[1]; @@ -198,7 +198,7 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(fip.instance_id, None); assert_eq!(fip.ip, ip_addr); - assert_ip_pool_utilization(client, "default", 2, 65536).await; + assert_ip_pool_utilization(client, "default", 2, 65536, 0, 0).await; // Creating with other-pool fails with 404 until it is linked to the current silo let fip_name = FIP_NAMES[2]; @@ -215,7 +215,7 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { object_create_error(client, &url, ¶ms, StatusCode::NOT_FOUND).await; assert_eq!(error.message, "not found: ip-pool with name \"other-pool\""); - assert_ip_pool_utilization(client, "other-pool", 0, 5).await; + assert_ip_pool_utilization(client, "other-pool", 0, 5, 0, 0).await; // now link the pool and everything should work with the exact same params let silo_id = DEFAULT_SILO.id(); @@ -228,7 +228,7 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(fip.instance_id, None); assert_eq!(fip.ip, IpAddr::from(Ipv4Addr::new(10, 1, 0, 1))); - assert_ip_pool_utilization(client, "other-pool", 1, 5).await; + assert_ip_pool_utilization(client, "other-pool", 1, 5, 0, 0).await; // Create with chosen IP from fleet-scoped named pool. let fip_name = FIP_NAMES[3]; @@ -246,7 +246,7 @@ async fn test_floating_ip_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(fip.instance_id, None); assert_eq!(fip.ip, ip_addr); - assert_ip_pool_utilization(client, "other-pool", 2, 5).await; + assert_ip_pool_utilization(client, "other-pool", 2, 5, 0, 0).await; } #[nexus_test] @@ -601,7 +601,7 @@ async fn test_external_ip_live_attach_detach( create_default_ip_pool(&client).await; let project = create_project(client, PROJECT_NAME).await; - assert_ip_pool_utilization(client, "default", 0, 65536).await; + assert_ip_pool_utilization(client, "default", 0, 65536, 0, 0).await; // Create 2 instances, and a floating IP for each instance. // One instance will be started, and one will be stopped. @@ -620,7 +620,7 @@ async fn test_external_ip_live_attach_detach( } // 2 floating IPs have been allocated - assert_ip_pool_utilization(client, "default", 2, 65536).await; + assert_ip_pool_utilization(client, "default", 2, 65536, 0, 0).await; let mut instances = vec![]; for (i, start) in [false, true].iter().enumerate() { @@ -655,7 +655,7 @@ async fn test_external_ip_live_attach_detach( // the two instances above were deliberately not given ephemeral IPs, but // they still always get SNAT IPs, so we went from 2 to 4 - assert_ip_pool_utilization(client, "default", 4, 65536).await; + assert_ip_pool_utilization(client, "default", 4, 65536, 0, 0).await; // Attach a floating IP and ephemeral IP to each instance. let mut recorded_ephs = vec![]; @@ -701,7 +701,7 @@ async fn test_external_ip_live_attach_detach( // now 6 because an ephemeral IP was added for each instance. floating IPs // were attached, but they were already allocated - assert_ip_pool_utilization(client, "default", 6, 65536).await; + assert_ip_pool_utilization(client, "default", 6, 65536, 0, 0).await; // Detach a floating IP and ephemeral IP from each instance. for (instance, fip) in instances.iter().zip(&fips) { @@ -734,7 +734,7 @@ async fn test_external_ip_live_attach_detach( } // 2 ephemeral go away on detachment but still 2 floating and 2 SNAT - assert_ip_pool_utilization(client, "default", 4, 65536).await; + assert_ip_pool_utilization(client, "default", 4, 65536, 0, 0).await; // Finally, two kind of funny tests. There is special logic in the handler // for the case where the floating IP is specified by name but the instance @@ -796,7 +796,7 @@ async fn test_external_ip_live_attach_detach( assert_eq!(eip_list[0].ip(), fips[1].ip); // none of that changed the number of allocated IPs - assert_ip_pool_utilization(client, "default", 4, 65536).await; + assert_ip_pool_utilization(client, "default", 4, 65536, 0, 0).await; } #[nexus_test] diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 5eaecec1a15..20b90e8a20f 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -3906,13 +3906,13 @@ async fn test_instance_ephemeral_ip_from_correct_pool( create_ip_pool(&client, "pool1", Some(range1)).await; link_ip_pool(&client, "pool1", &DEFAULT_SILO.id(), /*default*/ true).await; - assert_ip_pool_utilization(client, "pool1", 0, 5).await; + assert_ip_pool_utilization(client, "pool1", 0, 5, 0, 0).await; // second pool is associated with the silo but not default create_ip_pool(&client, "pool2", Some(range2)).await; link_ip_pool(&client, "pool2", &DEFAULT_SILO.id(), /*default*/ false).await; - assert_ip_pool_utilization(client, "pool2", 0, 5).await; + assert_ip_pool_utilization(client, "pool2", 0, 5, 0, 0).await; // Create an instance with pool name blank, expect IP from default pool create_instance_with_pool(client, "pool1-inst", None).await; @@ -3923,9 +3923,9 @@ async fn test_instance_ephemeral_ip_from_correct_pool( "Expected ephemeral IP to come from pool1" ); // 1 ephemeral + 1 snat - assert_ip_pool_utilization(client, "pool1", 2, 5).await; + assert_ip_pool_utilization(client, "pool1", 2, 5, 0, 0).await; // pool2 unaffected - assert_ip_pool_utilization(client, "pool2", 0, 5).await; + assert_ip_pool_utilization(client, "pool2", 0, 5, 0, 0).await; // Create an instance explicitly using the non-default "other-pool". create_instance_with_pool(client, "pool2-inst", Some("pool2")).await; @@ -3937,10 +3937,10 @@ async fn test_instance_ephemeral_ip_from_correct_pool( // added 1 snat because snat comes from default pool. // https://github.com/oxidecomputer/omicron/issues/5043 is about changing that - assert_ip_pool_utilization(client, "pool1", 3, 5).await; + assert_ip_pool_utilization(client, "pool1", 3, 5, 0, 0).await; // ephemeral IP comes from specified pool - assert_ip_pool_utilization(client, "pool2", 1, 5).await; + assert_ip_pool_utilization(client, "pool2", 1, 5, 0, 0).await; // make pool2 default and create instance with default pool. check that it now it comes from pool2 let _: views::IpPoolSiloLink = object_put( @@ -3958,9 +3958,9 @@ async fn test_instance_ephemeral_ip_from_correct_pool( ); // pool1 unchanged - assert_ip_pool_utilization(client, "pool1", 3, 5).await; + assert_ip_pool_utilization(client, "pool1", 3, 5, 0, 0).await; // +1 snat (now that pool2 is default) and +1 ephemeral - assert_ip_pool_utilization(client, "pool2", 3, 5).await; + assert_ip_pool_utilization(client, "pool2", 3, 5, 0, 0).await; // try to delete association with pool1, but it fails because there is an // instance with an IP from the pool in this silo @@ -3982,9 +3982,9 @@ async fn test_instance_ephemeral_ip_from_correct_pool( // pool1 is down to 0 because it had 1 snat + 1 ephemeral from pool1-inst // and 1 snat from pool2-inst - assert_ip_pool_utilization(client, "pool1", 0, 5).await; + assert_ip_pool_utilization(client, "pool1", 0, 5, 0, 0).await; // pool2 drops one because it had 1 ephemeral from pool2-inst - assert_ip_pool_utilization(client, "pool2", 2, 5).await; + assert_ip_pool_utilization(client, "pool2", 2, 5, 0, 0).await; // now unlink works object_delete(client, &pool1_silo_url).await; @@ -4173,7 +4173,7 @@ async fn test_instance_attach_several_external_ips( create_ip_pool(&client, "default", Some(default_pool_range)).await; link_ip_pool(&client, "default", &DEFAULT_SILO.id(), true).await; - assert_ip_pool_utilization(client, "default", 0, 10).await; + assert_ip_pool_utilization(client, "default", 0, 10, 0, 0).await; // Create several floating IPs for the instance, totalling 8 IPs. let mut external_ip_create = @@ -4203,7 +4203,7 @@ async fn test_instance_attach_several_external_ips( .await; // 1 ephemeral + 7 floating + 1 SNAT - assert_ip_pool_utilization(client, "default", 9, 10).await; + assert_ip_pool_utilization(client, "default", 9, 10, 0, 0).await; // Verify that all external IPs are visible on the instance and have // been allocated in order. @@ -4359,7 +4359,7 @@ async fn test_instance_create_in_silo(cptestctx: &ControlPlaneTestContext) { create_ip_pool(&client, "default", None).await; link_ip_pool(&client, "default", &silo.identity.id, true).await; - assert_ip_pool_utilization(client, "default", 0, 65536).await; + assert_ip_pool_utilization(client, "default", 0, 65536, 0, 0).await; // Create test projects NexusRequest::objects_post( diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index d8ac77c633c..c8390e8ce0d 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -767,7 +767,7 @@ async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { create_pool(client, "p0").await; - assert_ip_pool_utilization(client, "p0", 0, 0).await; + assert_ip_pool_utilization(client, "p0", 0, 0, 0, 0).await; let add_url = "/v1/system/ip-pools/p0/ranges/add"; @@ -781,7 +781,7 @@ async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { ); object_create::(client, &add_url, &range).await; - assert_ip_pool_utilization(client, "p0", 0, 5).await; + assert_ip_pool_utilization(client, "p0", 0, 5, 0, 0).await; // now let's add a gigantic range just for fun let big_range = IpRange::V6( @@ -795,7 +795,8 @@ async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { ); object_create::(client, &add_url, &big_range).await; - assert_ip_pool_utilization(client, "p0", 0, 18446480190918885380).await; + assert_ip_pool_utilization(client, "p0", 0, 5, 0, 18446480190918885375) + .await; } // Data for testing overlapping IP ranges diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index dc8e5ea6860..fcea302f725 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -305,30 +305,39 @@ pub struct IpPool { pub identity: IdentityMetadata, } -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct IpPoolUtilization { - // TODO: make sure these names are satisfactory - - // Unlike total, the reason this is bigger than u32 is simply that the - // DB count comes back as an i64, and in theory the count could really - // be bigger than 2^32 = ~4.29B, though in practice that is virtually - // impossible. - /// The number of IPs allocated from the pool - /// - /// Like total, this is a numeric string with a custom "uint128" format - /// representing a 128-bit integer, though in practice it is extremely - /// unlikely to be bigger than 32 bits (2^32 = ~4.29 billion). +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct Ipv4Utilization { + /// The number of IPv4 addresses allocated from this pool + pub allocated: u32, + /// The total number of IPv4 addresses in the pool, i.e., the sum of the + /// lengths of the IPv4 ranges. Unlike IPv6 capacity, can be a 32-bit + /// integer because there are only 2^32 IPv4 addresses. + pub capacity: u32, +} + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct Ipv6Utilization { + /// The number of IPv6 addresses allocated from this pool. A 128-bit integer + /// string to match the capacity field. #[serde(with = "U128String")] pub allocated: u128, - /// The total number of IP addresses in the pool, i.e., the sum of the - /// lengths of the ranges. An IPv6 range can contain up to 2^128 addresses, - /// so we represent this value in JSON as a numeric string with a custom - /// "uint128" format. + /// The total number of IPv6 addresses in the pool, i.e., the sum of the + /// lengths of the IPv6 ranges. An IPv6 range can contain up to 2^128 + /// addresses, so we represent this value in JSON as a numeric string with a + /// custom "uint128" format. #[serde(with = "U128String")] pub capacity: u128, } +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct IpPoolUtilization { + /// Number of allocated and total available IPv4 addresses in pool + pub ipv4: Ipv4Utilization, + /// Number of allocated and total available IPv6 addresses in pool + pub ipv6: Ipv6Utilization, +} + // Custom struct for serializing/deserializing u128 as a string. The serde // docs will suggest using a module (or serialize_with and deserialize_with // functions), but as discussed in the comments on the UserData de/serializer, diff --git a/openapi/nexus.json b/openapi/nexus.json index b464023d782..7516a896a74 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -13693,20 +13693,26 @@ "IpPoolUtilization": { "type": "object", "properties": { - "allocated": { - "description": "The number of IPs allocated from the pool\n\nLike total, this is a numeric string with a custom \"uint128\" format representing a 128-bit integer, though in practice it is extremely unlikely to be bigger than 32 bits (2^32 = ~4.29 billion).", - "type": "string", - "format": "uint128" + "ipv4": { + "description": "Number of allocated and total available IPv4 addresses in pool", + "allOf": [ + { + "$ref": "#/components/schemas/Ipv4Utilization" + } + ] }, - "capacity": { - "description": "The total number of IP addresses in the pool, i.e., the sum of the lengths of the ranges. An IPv6 range can contain up to 2^128 addresses, so we represent this value in JSON as a numeric string with a custom \"uint128\" format.", - "type": "string", - "format": "uint128" + "ipv6": { + "description": "Number of allocated and total available IPv6 addresses in pool", + "allOf": [ + { + "$ref": "#/components/schemas/Ipv6Utilization" + } + ] } }, "required": [ - "allocated", - "capacity" + "ipv4", + "ipv6" ] }, "IpRange": { @@ -13754,6 +13760,27 @@ "last" ] }, + "Ipv4Utilization": { + "type": "object", + "properties": { + "allocated": { + "description": "The number of IPv4 addresses allocated from this pool", + "type": "integer", + "format": "uint32", + "minimum": 0 + }, + "capacity": { + "description": "The total number of IPv4 addresses in the pool, i.e., the sum of the lengths of the IPv4 ranges. Unlike IPv6 capacity, can be a 32-bit integer because there are only 2^32 IPv4 addresses.", + "type": "integer", + "format": "uint32", + "minimum": 0 + } + }, + "required": [ + "allocated", + "capacity" + ] + }, "Ipv6Net": { "example": "fd12:3456::/64", "title": "An IPv6 subnet", @@ -13779,6 +13806,25 @@ "last" ] }, + "Ipv6Utilization": { + "type": "object", + "properties": { + "allocated": { + "description": "The number of IPv6 addresses allocated from this pool. A 128-bit integer string to match the capacity field.", + "type": "string", + "format": "uint128" + }, + "capacity": { + "description": "The total number of IPv6 addresses in the pool, i.e., the sum of the lengths of the IPv6 ranges. An IPv6 range can contain up to 2^128 addresses, so we represent this value in JSON as a numeric string with a custom \"uint128\" format.", + "type": "string", + "format": "uint128" + } + }, + "required": [ + "allocated", + "capacity" + ] + }, "L4PortRange": { "example": "22", "title": "A range of IP ports", From 9657e8d1000f71a5dcfc50faf3b33c131bf65264 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Sat, 16 Mar 2024 16:25:49 -0500 Subject: [PATCH 12/13] nicer and faster way of getting the ipv4 and ipv6 allocated counts --- nexus/db-queries/src/db/datastore/ip_pool.rs | 35 ++++---------------- 1 file changed, 7 insertions(+), 28 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index fa1f851ee32..cd724917672 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -342,43 +342,22 @@ impl DataStore { opctx.authorize(authz::Action::Read, authz_pool).await?; use db::schema::external_ip; - use diesel::dsl::{count_star, sql}; + use diesel::dsl::sql; use diesel::sql_types::BigInt; - let counts = external_ip::table + let (ipv4, ipv6) = external_ip::table .filter(external_ip::ip_pool_id.eq(authz_pool.id())) .filter(external_ip::time_deleted.is_null()) - // Naturally, what I wanted was something like this: - // - // .group_by(family(external_ip::ip)) - // .select((family(external_ip::ip), count_star())) - // - // but diesel does not like the type, plus diesel doesn't let you do the - // column alias bit. - // - // Also worth noting: while the dsl family() function returns an - // Integer, when I do it with plain sql, if I use Integer and i32, - // I get errors about trying to unpack more than 32 bits. So we use - // BigInt and i64. - .group_by(sql::("ip_family")) - .select((sql::("family(ip) AS ip_family"), count_star())) - .get_results_async::<(i64, i64)>( + .select(( + sql::("count(*) FILTER (WHERE family(ip) = 4)"), + sql::("count(*) FILTER (WHERE family(ip) = 6)"), + )) + .first_async::<(i64, i64)>( &*self.pool_connection_authorized(opctx).await?, ) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - let mut ipv4 = 0; - let mut ipv6 = 0; - - for &(family, count) in &counts { - match family { - 4 => ipv4 = count, - 6 => ipv6 = count, - _ => (), - } - } - Ok(IpsAllocated { ipv4, ipv6 }) } From 24763efd589a5e328043cdcfe2d0f43ecdcc986b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 18 Mar 2024 14:45:53 -0500 Subject: [PATCH 13/13] limit range length query to 10000 ranges --- nexus/db-queries/src/db/datastore/ip_pool.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index cd724917672..c0f4e0eea87 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -361,8 +361,6 @@ impl DataStore { Ok(IpsAllocated { ipv4, ipv6 }) } - // TODO: should this just return the vec of ranges? - // TODO: generally we never retrieve all of anything, how bad is that pub async fn ip_pool_total_capacity( &self, opctx: &OpContext, @@ -377,6 +375,15 @@ impl DataStore { .filter(ip_pool_range::ip_pool_id.eq(authz_pool.id())) .filter(ip_pool_range::time_deleted.is_null()) .select(IpPoolRange::as_select()) + // This is a rare unpaginated DB query, which means we are + // vulnerable to a resource exhaustion attack in which someone + // creates a very large number of ranges in order to make this + // query slow. In order to mitigate that, we limit the query to the + // (current) max allowed page size, effectively making this query + // exactly as vulnerable as if it were paginated. If there are more + // than 10,000 ranges in a pool, we will undercount, but I have a + // hard time seeing that as a practical problem. + .limit(10000) .get_results_async::( &*self.pool_connection_authorized(opctx).await?, )