diff --git a/nexus/db-model/src/external_ip.rs b/nexus/db-model/src/external_ip.rs index bbaa8045da..1a755f0396 100644 --- a/nexus/db-model/src/external_ip.rs +++ b/nexus/db-model/src/external_ip.rs @@ -6,7 +6,8 @@ //! services. use crate::impl_enum_type; -use crate::schema::{external_ip, floating_ip}; +use crate::schema::external_ip; +use crate::schema::floating_ip; use crate::Name; use crate::SqlU16; use chrono::DateTime; @@ -20,7 +21,8 @@ use nexus_types::external_api::views; use omicron_common::address::NUM_SOURCE_NAT_PORTS; use omicron_common::api::external::Error; use omicron_common::api::external::IdentityMetadata; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; +use serde::Serialize; use std::convert::TryFrom; use std::net::IpAddr; use uuid::Uuid; diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs index 04549b5845..41e9f657d3 100644 --- a/nexus/db-queries/src/db/datastore/external_ip.rs +++ b/nexus/db-queries/src/db/datastore/external_ip.rs @@ -145,8 +145,6 @@ impl DataStore { ) -> CreateResult { let ip_id = Uuid::new_v4(); - // XXX: mux here to scan *all* project pools in - // current silo for convenience? let pool_id = match params.pool { Some(NameOrId::Name(name)) => { LookupPath::new(opctx, self) @@ -343,8 +341,8 @@ impl DataStore { /// Detach an individual Floating IP address from its parent instance. /// - /// As in `deallocate_external_ip_by_instance_id`, This method returns the - /// number of records deleted, rather than the usual `DeleteResult`. + /// As in `deallocate_external_ip_by_instance_id`, this method returns the + /// number of records altered, rather than an `UpdateResult`. pub async fn detach_floating_ips_by_instance_id( &self, opctx: &OpContext, @@ -386,9 +384,9 @@ impl DataStore { authz_project: &authz::Project, pagparams: &PaginatedBy<'_>, ) -> ListResultVec { - opctx.authorize(authz::Action::ListChildren, authz_project).await?; - use db::schema::floating_ip::dsl; + + opctx.authorize(authz::Action::ListChildren, authz_project).await?; match pagparams { PaginatedBy::Id(pagparams) => { diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index a7a5e8a921..78638e3c77 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -1747,7 +1747,9 @@ mod test { }; db::model::Name(Name::try_from(name).unwrap()) }); - // We do name duplicate checking on the `Some` branch. + + // We do name duplicate checking on the `Some` branch, don't steal the + // name intended for another floating IP. if parent_id.is_none() && modify_name { continue; } @@ -1794,21 +1796,20 @@ mod test { seen_pairs.insert(key); } else if !valid_expression { + // Several permutations are invalid and we want to detect them all. // NOTE: CHECK violation will supersede UNIQUE violation below. - // At least one is not valid, we expect a check violation let err = res.expect_err( "Expected a CHECK violation when inserting a \ - Floating IP record with NULL name and/or description", + Floating IP record with NULL name and/or description, \ + and incorrect project parent relation", ); assert!( matches!(err, DatabaseError(CheckViolation, _)), "Expected a CHECK violation when inserting a \ Floating IP record with NULL name and/or description, \ - and incorrect project parent relation \ - \nGOT: {err:?}", + and incorrect project parent relation", ); } else { - // At least one is not valid, we expect a check violation let err = res.expect_err( "Expected a UNIQUE violation when inserting a \ Floating IP record with existing (name, project_id)", @@ -1816,8 +1817,7 @@ mod test { assert!( matches!(err, DatabaseError(UniqueViolation, _)), "Expected a UNIQUE violation when inserting a \ - Floating IP record with existing (name, project_id) \ - \nGOT: {err:?}", + Floating IP record with existing (name, project_id)", ); } } diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 76e9a0b4ff..0edb2c5ea7 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -925,10 +925,11 @@ impl super::Nexus { .into_iter() .partition(|ip| ip.kind == IpKind::Ephemeral); - if ephemeral_ips.len() > 1 { + if ephemeral_ips.len() > MAX_EPHEMERAL_IPS_PER_INSTANCE { return Err(Error::internal_error( format!( - "Expected at most one ephemeral IP for an instance, found {}", + "Expected at most {} ephemeral IP for an instance, found {}", + MAX_EPHEMERAL_IPS_PER_INSTANCE, ephemeral_ips.len() ) .as_str(), diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index e1c4f273a7..d1644a6380 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -619,8 +619,8 @@ async fn sic_allocate_instance_external_ip( let instance_id = repeat_saga_params.instance_id; let ip_id = repeat_saga_params.new_id; - // Collect the possible pool name for this IP address match ip_params { + // Allocate a new IP address from the target, possibly default, pool params::ExternalIpCreate::Ephemeral { ref pool_name } => { let pool_name = pool_name.as_ref().map(|name| db::model::Name(name.clone())); @@ -634,6 +634,7 @@ async fn sic_allocate_instance_external_ip( .await .map_err(ActionError::action_failed)?; } + // Set the parent of an existing floating IP to the new instance's ID. params::ExternalIpCreate::Floating { ref floating_ip_name } => { let floating_ip_name = db::model::Name(floating_ip_name.clone()); let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index dffff1a472..f54370c32f 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -4,9 +4,8 @@ //! Tests basic instance support in the API -use crate::integration_tests::external_ips::floating_ip_get; -use crate::integration_tests::external_ips::get_floating_ip_by_id_url; - +use super::external_ips::floating_ip_get; +use super::external_ips::get_floating_ip_by_id_url; use super::metrics::{get_latest_silo_metric, get_latest_system_metric}; use camino::Utf8Path; diff --git a/openapi/nexus.json b/openapi/nexus.json index 205d6380d4..f0b66fbcc2 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -10637,7 +10637,7 @@ "properties": { "address": { "nullable": true, - "description": "An IP address to reserve for use as a floating IP. This field is optional if a pool is provided, in which case an address will be automatically chosen from there.", + "description": "An IP address to reserve for use as a floating IP. This field is optional: when not set, an address will be automatically chosen from `pool`. If set, then the IP must be available in the resolved `pool`.", "type": "string", "format": "ip" }, @@ -10649,7 +10649,7 @@ }, "pool": { "nullable": true, - "description": "The parent IP pool that a floating IP is pulled from. If combined with an explicit address, then that address must be available in the pool.", + "description": "The parent IP pool that a floating IP is pulled from. If unset, the default pool is selected.", "allOf": [ { "$ref": "#/components/schemas/NameOrId"