[review] check ip_version against pool + sql migration split

Author: zeeshanlakhani

nexus/src/app/instance.rs

@@ -2193,6 +2193,27 @@ impl super::Nexus {
         pool: Option<NameOrId>,
         ip_version: Option<IpVersion>,
     ) -> UpdateResult<views::ExternalIp> {
+        // Validate pool/ip_version compatibility upfront for clear error
+        // communication.
+        //
+        // The saga will look up the pool again, but this ensures the user gets
+        // a direct error before any saga machinery starts, and without changing
+        // the `Ephemeral` type fields.
+        if let (Some(pool_id), Some(requested_version)) = (&pool, ip_version) {
+            let (_, db_pool) = self
+                .ip_pool_lookup(opctx, pool_id)?
+                .fetch_for(authz::Action::CreateChild)
+                .await?;
+            let db_version: IpVersion = db_pool.ip_version.into();
+            if db_version != requested_version {
+                return Err(Error::invalid_request(format!(
+                    "requested IP version ({requested_version}) does not match \
+                     pool '{}' version ({db_version})",
+                    db_pool.name().as_str(),
+                )));
+            }
+        }
+
         let (.., authz_project, authz_instance) =
             instance_lookup.lookup_for(authz::Action::Modify).await?;
 

nexus/src/app/sagas/instance_ip_attach.rs

@@ -95,17 +95,15 @@ async fn siia_begin_attach_ip(
     match &params.create_params {
         // Allocate a new IP address from the target, possibly default, pool
         ExternalIpAttach::Ephemeral { pool, ip_version } => {
-            let pool = if let Some(name_or_id) = pool {
-                Some(
-                    osagactx
-                        .nexus()
-                        .ip_pool_lookup(&opctx, name_or_id)
-                        .map_err(ActionError::action_failed)?
-                        .lookup_for(authz::Action::CreateChild)
-                        .await
-                        .map_err(ActionError::action_failed)?
-                        .0,
-                )
+            let authz_pool = if let Some(name_or_id) = pool {
+                let (authz_pool, _db_pool) = osagactx
+                    .nexus()
+                    .ip_pool_lookup(&opctx, name_or_id)
+                    .map_err(ActionError::action_failed)?
+                    .fetch_for(authz::Action::CreateChild)
+                    .await
+                    .map_err(ActionError::action_failed)?;
+                Some(authz_pool)
             } else {
                 None
             };
@@ -115,7 +113,7 @@ async fn siia_begin_attach_ip(
                     &opctx,
                     Uuid::new_v4(),
                     instance_id,
-                    pool,
+                    authz_pool,
                     ip_version.map(Into::into),
                     false,
                 )

nexus/tests/integration_tests/external_ips.rs

@@ -1548,6 +1548,37 @@ async fn test_ephemeral_ip_ip_version_conflict(
     );
 }
 
+/// Test that specifying both `pool` and IP version where the pool's version
+/// doesn't match the requested version returns an error.
+#[nexus_test]
+async fn test_ephemeral_ip_pool_version_mismatch(
+    cptestctx: &ControlPlaneTestContext,
+) {
+    let client = &cptestctx.external_client;
+
+    // Create default IPv4 pool
+    create_default_ip_pool(&client).await;
+    create_project(&client, PROJECT_NAME).await;
+    instance_for_external_ips(client, INSTANCE_NAMES[0], false, false, &[])
+        .await;
+
+    let url = instance_ephemeral_ip_url(INSTANCE_NAMES[0], PROJECT_NAME);
+
+    // Request IPv6 from the IPv4 pool -> should fail
+    NexusRequest::new(
+        RequestBuilder::new(client, Method::POST, &url)
+            .body(Some(&params::EphemeralIpCreate {
+                pool: Some(NameOrId::Name("default".parse().unwrap())),
+                ip_version: Some(views::IpVersion::V6),
+            }))
+            .expect_status(Some(StatusCode::BAD_REQUEST)),
+    )
+    .authn_as(AuthnMode::PrivilegedUser)
+    .execute()
+    .await
+    .unwrap();
+}
+
 #[nexus_test]
 async fn can_list_instance_snat_ip(cptestctx: &ControlPlaneTestContext) {
     let client = &cptestctx.external_client;

schema/crdb/multiple-default-ip-pools-per-silo/up01.sql

@@ -9,8 +9,3 @@
 
 -- Drop the old single-default constraint
 DROP INDEX IF EXISTS omicron.public.one_default_ip_pool_per_resource;
-
--- Add denormalized columns (nullable initially for backfill)
-ALTER TABLE omicron.public.ip_pool_resource
-    ADD COLUMN IF NOT EXISTS pool_type omicron.public.ip_pool_type,
-    ADD COLUMN IF NOT EXISTS ip_version omicron.public.ip_version;

schema/crdb/multiple-default-ip-pools-per-silo/up02.sql

@@ -1,9 +1,4 @@
--- Backfill pool_type and ip_version from ip_pool table
-SET LOCAL disallow_full_table_scans = off;
-UPDATE omicron.public.ip_pool_resource AS resource
-SET
-    pool_type = pool.pool_type,
-    ip_version = pool.ip_version
-FROM omicron.public.ip_pool AS pool
-WHERE resource.ip_pool_id = pool.id
-  AND (resource.pool_type IS NULL OR resource.ip_version IS NULL);
+-- Add denormalized columns (nullable initially for backfill)
+ALTER TABLE omicron.public.ip_pool_resource
+    ADD COLUMN IF NOT EXISTS pool_type omicron.public.ip_pool_type,
+    ADD COLUMN IF NOT EXISTS ip_version omicron.public.ip_version;

schema/crdb/multiple-default-ip-pools-per-silo/up03.sql

@@ -1,12 +1,9 @@
--- Make columns NOT NULL and create new unique index
-ALTER TABLE omicron.public.ip_pool_resource
-    ALTER COLUMN pool_type SET NOT NULL,
-    ALTER COLUMN ip_version SET NOT NULL;
-
--- One default pool per (resource, pool_type, ip_version) combination
-CREATE UNIQUE INDEX IF NOT EXISTS one_default_ip_pool_per_resource_type_version
-ON omicron.public.ip_pool_resource (
-    resource_id,
-    pool_type,
-    ip_version
-) WHERE is_default = true;
+-- Backfill pool_type and ip_version from ip_pool table
+SET LOCAL disallow_full_table_scans = off;
+UPDATE omicron.public.ip_pool_resource AS resource
+SET
+    pool_type = pool.pool_type,
+    ip_version = pool.ip_version
+FROM omicron.public.ip_pool AS pool
+WHERE resource.ip_pool_id = pool.id
+  AND (resource.pool_type IS NULL OR resource.ip_version IS NULL);

schema/crdb/multiple-default-ip-pools-per-silo/up04.sql

@@ -0,0 +1,4 @@
+-- Make columns NOT NULL after backfill
+ALTER TABLE omicron.public.ip_pool_resource
+    ALTER COLUMN pool_type SET NOT NULL,
+    ALTER COLUMN ip_version SET NOT NULL;

schema/crdb/multiple-default-ip-pools-per-silo/up05.sql

@@ -0,0 +1,7 @@
+-- One default pool per (resource, pool_type, ip_version) combination
+CREATE UNIQUE INDEX IF NOT EXISTS one_default_ip_pool_per_resource_type_version
+ON omicron.public.ip_pool_resource (
+    resource_id,
+    pool_type,
+    ip_version
+) WHERE is_default = true;