From dcdec48e07767323b6ce145df9a5a83335a17d2c Mon Sep 17 00:00:00 2001
From: Kyle Simpson <kyle@oxide.computer>
Date: Thu, 28 Dec 2023 18:43:42 +0000
Subject: [PATCH] Revalidate consumers of `instance_lookup_external_ips`

---
 nexus/db-queries/src/db/datastore/external_ip.rs |  1 +
 nexus/src/app/external_ip.rs                     |  5 ++++-
 nexus/src/app/instance.rs                        | 10 ++++++++++
 nexus/src/app/sagas/instance_ip_attach.rs        |  4 ++--
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs
index 2364cbf341..3396eb9273 100644
--- a/nexus/db-queries/src/db/datastore/external_ip.rs
+++ b/nexus/db-queries/src/db/datastore/external_ip.rs
@@ -628,6 +628,7 @@ impl DataStore {
     }
 
     /// Fetch all external IP addresses of any kind for the provided instance
+    /// in all attachment states.
     pub async fn instance_lookup_external_ips(
         &self,
         opctx: &OpContext,
diff --git a/nexus/src/app/external_ip.rs b/nexus/src/app/external_ip.rs
index fba34f767d..7f41b7fd20 100644
--- a/nexus/src/app/external_ip.rs
+++ b/nexus/src/app/external_ip.rs
@@ -6,6 +6,7 @@
 
 use crate::external_api::views::ExternalIp;
 use crate::external_api::views::FloatingIp;
+use nexus_db_model::IpAttachState;
 use nexus_db_queries::authz;
 use nexus_db_queries::context::OpContext;
 use nexus_db_queries::db::lookup;
@@ -34,7 +35,9 @@ impl super::Nexus {
             .await?
             .into_iter()
             .filter_map(|ip| {
-                if ip.kind == IpKind::SNat {
+                if ip.kind == IpKind::SNat
+                    || ip.state != IpAttachState::Attached
+                {
                     None
                 } else {
                     Some(ip.try_into().unwrap())
diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs
index 28aa0dcca0..19e41c9dbd 100644
--- a/nexus/src/app/instance.rs
+++ b/nexus/src/app/instance.rs
@@ -17,6 +17,7 @@ use crate::external_api::params;
 use cancel_safe_futures::prelude::*;
 use futures::future::Fuse;
 use futures::{FutureExt, SinkExt, StreamExt};
+use nexus_db_model::IpAttachState;
 use nexus_db_model::IpKind;
 use nexus_db_queries::authn;
 use nexus_db_queries::authz;
@@ -1054,6 +1055,15 @@ impl super::Nexus {
             ));
         }
 
+        // If there are any external IPs not yet fully attached/detached,then
+        // there are attach/detach sagas in progress. That should complete in
+        // its own time, so return a 503 to indicate a possible retry.
+        if external_ips.iter().any(|v| v.state != IpAttachState::Attached) {
+            return Err(Error::unavail(
+                "External IP attach/detach is in progress during instance_ensure_registered"
+            ));
+        }
+
         // Partition remaining external IPs by class: we can have at most
         // one ephemeral ip.
         let (ephemeral_ips, floating_ips): (Vec<_>, Vec<_>) = external_ips
diff --git a/nexus/src/app/sagas/instance_ip_attach.rs b/nexus/src/app/sagas/instance_ip_attach.rs
index 3df445b95e..872d92eabf 100644
--- a/nexus/src/app/sagas/instance_ip_attach.rs
+++ b/nexus/src/app/sagas/instance_ip_attach.rs
@@ -28,8 +28,8 @@ use uuid::Uuid;
 // The main means of access control here is an external IP's `state`.
 // Entering either saga begins with an atomic swap from Attached/Detached
 // to Attaching/Detaching. This prevents concurrent attach/detach on the
-// same EIP, and prevents instance start from executing with an
-// Error::unavail.
+// same EIP, and prevents instance start and migrate from completing with an
+// Error::unavail via instance_ensure_registered and/or DPD.
 //
 // Overlap with stop is handled by treating comms failures with
 // sled-agent as temporary errors and unwinding. For the delete case, we