Re-read complete changelog, tidying up.

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;

nexus/db-queries/src/db/datastore/external_ip.rs

@@ -145,8 +145,6 @@ impl DataStore {
     ) -> CreateResult<ExternalIp> {
         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<FloatingIp> {
-        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) => {

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,30 +1796,28 @@ 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)",
                 );
                 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)",
                 );
             }
         }

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(),

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)

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;

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"