move instance migration to internal API (#6311)

Move instance migration from the external API to the internal API per
the determinations in RFD 494. This is almost a purely mechanical change
(I have not, for example, changed any API signatures or return values);
the only unusual bit is one small adjustment to
`Nexus::instance_ensure_registered` to deal with the fact that migration
no longer runs in a context where the caller is acting on behalf of a
user in a silo.

Tests: cargo test; spun up a dev cluster and verified that (1) the
migrate endpoint is gone from the external API, (2) it's present on the
internal API and reachable from the switch zone, and (3) it behaves as
it did before (at least to the extent I could test migration's behavior
on a single-machine dev cluster).

nexus/internal-api/src/lib.rs

@@ -20,15 +20,15 @@ use nexus_types::{
     },
     internal_api::{
         params::{
-            OximeterInfo, RackInitializationRequest, SledAgentInfo,
-            SwitchPutRequest, SwitchPutResponse,
+            InstanceMigrateRequest, OximeterInfo, RackInitializationRequest,
+            SledAgentInfo, SwitchPutRequest, SwitchPutResponse,
         },
         views::{BackgroundTask, DemoSaga, Ipv4NatEntryView, Saga},
     },
 };
 use omicron_common::{
     api::{
-        external::http_pagination::PaginatedById,
+        external::{http_pagination::PaginatedById, Instance},
         internal::nexus::{
             DiskRuntimeState, DownstairsClientStopRequest,
             DownstairsClientStopped, ProducerEndpoint,
@@ -119,6 +119,16 @@ pub trait NexusInternalApi {
         new_runtime_state: TypedBody<SledInstanceState>,
     ) -> Result<HttpResponseUpdatedNoContent, HttpError>;
 
+    #[endpoint {
+        method = POST,
+        path = "/instances/{instance_id}/migrate",
+    }]
+    async fn instance_migrate(
+        rqctx: RequestContext<Self::Context>,
+        path_params: Path<InstancePathParam>,
+        migrate_params: TypedBody<InstanceMigrateRequest>,
+    ) -> Result<HttpResponseOk<Instance>, HttpError>;
+
     /// Report updated state for a disk.
     #[endpoint {
         method = PUT,

nexus/src/app/instance.rs

@@ -191,6 +191,14 @@ enum InstanceStartDisposition {
     AlreadyStarted,
 }
 
+/// The set of API resources needed when ensuring that an instance is registered
+/// on a sled.
+pub(crate) struct InstanceEnsureRegisteredApiResources {
+    pub(crate) authz_silo: nexus_auth::authz::Silo,
+    pub(crate) authz_project: nexus_auth::authz::Project,
+    pub(crate) authz_instance: nexus_auth::authz::Instance,
+}
+
 impl super::Nexus {
     pub fn instance_lookup<'a>(
         &'a self,
@@ -473,14 +481,16 @@ impl super::Nexus {
         Ok(())
     }
 
-    pub(crate) async fn project_instance_migrate(
+    pub(crate) async fn instance_migrate(
         self: &Arc<Self>,
         opctx: &OpContext,
-        instance_lookup: &lookup::Instance<'_>,
-        params: params::InstanceMigrate,
+        id: InstanceUuid,
+        params: nexus_types::internal_api::params::InstanceMigrateRequest,
     ) -> UpdateResult<InstanceAndActiveVmm> {
-        let (.., authz_instance) =
-            instance_lookup.lookup_for(authz::Action::Modify).await?;
+        let (.., authz_instance) = LookupPath::new(&opctx, &self.db_datastore)
+            .instance_id(id.into_untyped_uuid())
+            .lookup_for(authz::Action::Modify)
+            .await?;
 
         let state = self
             .db_datastore
@@ -867,7 +877,11 @@ impl super::Nexus {
     pub(crate) async fn instance_ensure_registered(
         &self,
         opctx: &OpContext,
-        authz_instance: &authz::Instance,
+        InstanceEnsureRegisteredApiResources {
+            authz_silo,
+            authz_project,
+            authz_instance,
+        }: &InstanceEnsureRegisteredApiResources,
         db_instance: &db::model::Instance,
         propolis_id: &PropolisUuid,
         initial_vmm: &db::model::Vmm,
@@ -1067,23 +1081,9 @@ impl super::Nexus {
         let ssh_keys: Vec<String> =
             ssh_keys.map(|ssh_key| ssh_key.public_key).collect();
 
-        // Construct instance metadata used to track its statistics.
-        //
-        // This requires another fetch on the silo and project, to extract their
-        // IDs.
-        let (.., db_project) = self
-            .project_lookup(
-                opctx,
-                params::ProjectSelector {
-                    project: NameOrId::Id(db_instance.project_id),
-                },
-            )?
-            .fetch()
-            .await?;
-        let (_, db_silo) = self.current_silo_lookup(opctx)?.fetch().await?;
         let metadata = sled_agent_client::types::InstanceMetadata {
-            silo_id: db_silo.id(),
-            project_id: db_project.id(),
+            silo_id: authz_silo.id(),
+            project_id: authz_project.id(),
         };
 
         // Ask the sled agent to begin the state change.  Then update the

nexus/src/app/sagas/instance_migrate.rs

@@ -4,15 +4,15 @@
 
 use super::{NexusActionContext, NexusSaga, ACTION_GENERATE_ID};
 use crate::app::instance::{
-    InstanceRegisterReason, InstanceStateChangeError,
-    InstanceStateChangeRequest,
+    InstanceEnsureRegisteredApiResources, InstanceRegisterReason,
+    InstanceStateChangeError, InstanceStateChangeRequest,
 };
 use crate::app::sagas::{
     declare_saga_actions, instance_common::allocate_vmm_ipv6,
 };
-use crate::external_api::params;
 use nexus_db_queries::db::{identity::Resource, lookup::LookupPath};
 use nexus_db_queries::{authn, authz, db};
+use nexus_types::internal_api::params::InstanceMigrateRequest;
 use omicron_uuid_kinds::{GenericUuid, InstanceUuid, PropolisUuid, SledUuid};
 use serde::Deserialize;
 use serde::Serialize;
@@ -30,7 +30,7 @@ pub struct Params {
     pub serialized_authn: authn::saga::Serialized,
     pub instance: db::model::Instance,
     pub src_vmm: db::model::Vmm,
-    pub migrate_params: params::InstanceMigrate,
+    pub migrate_params: InstanceMigrateRequest,
 }
 
 // The migration saga is similar to the instance start saga: get a destination
@@ -401,19 +401,24 @@ async fn sim_ensure_destination_propolis(
           "dst_propolis_id" => %vmm.id,
           "dst_vmm_state" => ?vmm);
 
-    let (.., authz_instance) = LookupPath::new(&opctx, &osagactx.datastore())
-        .instance_id(db_instance.id())
-        .lookup_for(authz::Action::Modify)
-        .await
-        .map_err(ActionError::action_failed)?;
+    let (authz_silo, authz_project, authz_instance) =
+        LookupPath::new(&opctx, &osagactx.datastore())
+            .instance_id(db_instance.id())
+            .lookup_for(authz::Action::Modify)
+            .await
+            .map_err(ActionError::action_failed)?;
 
     let src_propolis_id = PropolisUuid::from_untyped_uuid(params.src_vmm.id);
     let dst_propolis_id = PropolisUuid::from_untyped_uuid(vmm.id);
     osagactx
         .nexus()
         .instance_ensure_registered(
             &opctx,
-            &authz_instance,
+            &InstanceEnsureRegisteredApiResources {
+                authz_silo,
+                authz_project,
+                authz_instance,
+            },
             &db_instance,
             &dst_propolis_id,
             &vmm,
@@ -565,6 +570,7 @@ async fn sim_instance_migrate(
 mod tests {
     use super::*;
     use crate::app::sagas::test_helpers;
+    use crate::external_api::params;
     use dropshot::test_util::ClientTestContext;
     use nexus_test_utils::resource_helpers::{
         create_default_ip_pool, create_project, object_create,
@@ -637,7 +643,7 @@ mod tests {
             serialized_authn: authn::saga::Serialized::for_opctx(&opctx),
             instance: state.instance().clone(),
             src_vmm: vmm.clone(),
-            migrate_params: params::InstanceMigrate {
+            migrate_params: InstanceMigrateRequest {
                 dst_sled_id: dst_sled_id.into_untyped_uuid(),
             },
         };
@@ -706,7 +712,7 @@ mod tests {
                         ),
                         instance: old_instance.clone(),
                         src_vmm: old_vmm.clone(),
-                        migrate_params: params::InstanceMigrate {
+                        migrate_params: InstanceMigrateRequest {
                             dst_sled_id: dst_sled_id.into_untyped_uuid(),
                         },
                     }

nexus/src/app/sagas/instance_start.rs

@@ -10,8 +10,10 @@ use super::{
     instance_common::allocate_vmm_ipv6, NexusActionContext, NexusSaga,
     SagaInitError,
 };
-use crate::app::instance::InstanceRegisterReason;
-use crate::app::instance::InstanceStateChangeError;
+use crate::app::instance::{
+    InstanceEnsureRegisteredApiResources, InstanceRegisterReason,
+    InstanceStateChangeError,
+};
 use crate::app::sagas::declare_saga_actions;
 use chrono::Utc;
 use nexus_db_queries::db::{identity::Resource, lookup::LookupPath};
@@ -502,17 +504,22 @@ async fn sis_ensure_registered(
           "instance_id" => %instance_id,
           "sled_id" => %sled_id);
 
-    let (.., authz_instance) = LookupPath::new(&opctx, &osagactx.datastore())
-        .instance_id(instance_id)
-        .lookup_for(authz::Action::Modify)
-        .await
-        .map_err(ActionError::action_failed)?;
+    let (authz_silo, authz_project, authz_instance) =
+        LookupPath::new(&opctx, &osagactx.datastore())
+            .instance_id(instance_id)
+            .lookup_for(authz::Action::Modify)
+            .await
+            .map_err(ActionError::action_failed)?;
 
     osagactx
         .nexus()
         .instance_ensure_registered(
             &opctx,
-            &authz_instance,
+            &InstanceEnsureRegisteredApiResources {
+                authz_silo,
+                authz_project,
+                authz_instance,
+            },
             &db_instance,
             &propolis_id,
             &vmm_record,

nexus/src/app/sagas/instance_update/mod.rs

@@ -1403,6 +1403,7 @@ mod test {
         create_default_ip_pool, create_project, object_create,
     };
     use nexus_test_utils_macros::nexus_test;
+    use nexus_types::internal_api::params::InstanceMigrateRequest;
     use omicron_common::api::internal::nexus::{
         MigrationRuntimeState, MigrationState, Migrations,
     };
@@ -2358,7 +2359,7 @@ mod test {
                 serialized_authn: authn::saga::Serialized::for_opctx(&opctx),
                 instance: state.instance().clone(),
                 src_vmm: vmm.clone(),
-                migrate_params: params::InstanceMigrate {
+                migrate_params: InstanceMigrateRequest {
                     dst_sled_id: dst_sled_id.into_untyped_uuid(),
                 },
             };

nexus/src/external_api/http_entrypoints.rs

@@ -167,7 +167,6 @@ pub(crate) fn external_api() -> NexusApiDescription {
         api.register(instance_view)?;
         api.register(instance_create)?;
         api.register(instance_delete)?;
-        api.register(instance_migrate)?;
         api.register(instance_reboot)?;
         api.register(instance_start)?;
         api.register(instance_stop)?;
@@ -2866,48 +2865,6 @@ async fn instance_delete(
         .await
 }
 
-// TODO should this be in the public API?
-/// Migrate an instance
-#[endpoint {
-    method = POST,
-    path = "/v1/instances/{instance}/migrate",
-    tags = ["instances"],
-}]
-async fn instance_migrate(
-    rqctx: RequestContext<ApiContext>,
-    query_params: Query<params::OptionalProjectSelector>,
-    path_params: Path<params::InstancePath>,
-    migrate_params: TypedBody<params::InstanceMigrate>,
-) -> Result<HttpResponseOk<Instance>, HttpError> {
-    let apictx = rqctx.context();
-    let nexus = &apictx.context.nexus;
-    let path = path_params.into_inner();
-    let query = query_params.into_inner();
-    let migrate_instance_params = migrate_params.into_inner();
-    let instance_selector = params::InstanceSelector {
-        project: query.project,
-        instance: path.instance,
-    };
-    let handler = async {
-        let opctx = crate::context::op_context_for_external_api(&rqctx).await?;
-        let instance_lookup =
-            nexus.instance_lookup(&opctx, instance_selector)?;
-        let instance = nexus
-            .project_instance_migrate(
-                &opctx,
-                &instance_lookup,
-                migrate_instance_params,
-            )
-            .await?;
-        Ok(HttpResponseOk(instance.into()))
-    };
-    apictx
-        .context
-        .external_latencies
-        .instrument_dropshot_handler(&rqctx, handler)
-        .await
-}
-
 /// Reboot an instance
 #[endpoint {
     method = POST,

nexus/src/internal_api/http_entrypoints.rs

@@ -30,6 +30,7 @@ use nexus_types::external_api::params::UninitializedSledId;
 use nexus_types::external_api::shared::ProbeInfo;
 use nexus_types::external_api::shared::UninitializedSled;
 use nexus_types::external_api::views::SledPolicy;
+use nexus_types::internal_api::params::InstanceMigrateRequest;
 use nexus_types::internal_api::params::SledAgentInfo;
 use nexus_types::internal_api::params::SwitchPutRequest;
 use nexus_types::internal_api::params::SwitchPutResponse;
@@ -42,6 +43,7 @@ use omicron_common::api::external::http_pagination::data_page_params_for;
 use omicron_common::api::external::http_pagination::PaginatedById;
 use omicron_common::api::external::http_pagination::ScanById;
 use omicron_common::api::external::http_pagination::ScanParams;
+use omicron_common::api::external::Instance;
 use omicron_common::api::internal::nexus::DiskRuntimeState;
 use omicron_common::api::internal::nexus::DownstairsClientStopRequest;
 use omicron_common::api::internal::nexus::DownstairsClientStopped;
@@ -190,6 +192,33 @@ impl NexusInternalApi for NexusInternalApiImpl {
             .await
     }
 
+    async fn instance_migrate(
+        rqctx: RequestContext<Self::Context>,
+        path_params: Path<InstancePathParam>,
+        migrate_params: TypedBody<InstanceMigrateRequest>,
+    ) -> Result<HttpResponseOk<Instance>, HttpError> {
+        let apictx = &rqctx.context().context;
+        let nexus = &apictx.nexus;
+        let path = path_params.into_inner();
+        let migrate = migrate_params.into_inner();
+        let handler = async {
+            let opctx =
+                crate::context::op_context_for_internal_api(&rqctx).await;
+            let instance = nexus
+                .instance_migrate(
+                    &opctx,
+                    InstanceUuid::from_untyped_uuid(path.instance_id),
+                    migrate,
+                )
+                .await?;
+            Ok(HttpResponseOk(instance.into()))
+        };
+        apictx
+            .internal_latencies
+            .instrument_dropshot_handler(&rqctx, handler)
+            .await
+    }
+
     async fn cpapi_disks_put(
         rqctx: RequestContext<Self::Context>,
         path_params: Path<DiskPathParam>,

nexus/tests/integration_tests/endpoints.rs

@@ -359,12 +359,6 @@ pub static DEMO_INSTANCE_REBOOT_URL: Lazy<String> = Lazy::new(|| {
         *DEMO_INSTANCE_NAME, *DEMO_PROJECT_SELECTOR
     )
 });
-pub static DEMO_INSTANCE_MIGRATE_URL: Lazy<String> = Lazy::new(|| {
-    format!(
-        "/v1/instances/{}/migrate?{}",
-        *DEMO_INSTANCE_NAME, *DEMO_PROJECT_SELECTOR
-    )
-});
 pub static DEMO_INSTANCE_SERIAL_URL: Lazy<String> = Lazy::new(|| {
     format!(
         "/v1/instances/{}/serial-console?{}",
@@ -1823,18 +1817,6 @@ pub static VERIFY_ENDPOINTS: Lazy<Vec<VerifyEndpoint>> = Lazy::new(|| {
                 AllowedMethod::Post(serde_json::Value::Null)
             ],
         },
-        VerifyEndpoint {
-            url: &DEMO_INSTANCE_MIGRATE_URL,
-            visibility: Visibility::Protected,
-            unprivileged_access: UnprivilegedAccess::None,
-            allowed_methods: vec![
-                AllowedMethod::Post(serde_json::to_value(
-                    params::InstanceMigrate {
-                        dst_sled_id: uuid::Uuid::new_v4(),
-                    }
-                ).unwrap()),
-            ],
-        },
         VerifyEndpoint {
             url: &DEMO_INSTANCE_SERIAL_URL,
             visibility: Visibility::Protected,