RFD-322 -- /v1/ Disks API

nexus/src/app/disk.rs

@@ -9,6 +9,7 @@ use crate::authn;
 use crate::authz;
 use crate::context::OpContext;
 use crate::db;
+use crate::db::lookup;
 use crate::db::lookup::LookupPath;
 use crate::db::model::Name;
 use crate::external_api::params;
@@ -20,26 +21,58 @@ use omicron_common::api::external::Error;
 use omicron_common::api::external::InternalContext;
 use omicron_common::api::external::ListResultVec;
 use omicron_common::api::external::LookupResult;
+use omicron_common::api::external::NameOrId;
 use omicron_common::api::internal::nexus::DiskRuntimeState;
+use ref_cast::RefCast;
 use sled_agent_client::Client as SledAgentClient;
 use std::sync::Arc;
 use uuid::Uuid;
 
 impl super::Nexus {
     // Disks
+    pub fn disk_lookup<'a>(
+        &'a self,
+        opctx: &'a OpContext,
+        disk_selector: &'a params::DiskSelector,
+    ) -> LookupResult<lookup::Disk<'a>> {
+        match disk_selector {
+            params::DiskSelector {
+                disk: NameOrId::Id(id),
+                project_selector: None,
+            } => {
+                let disk =
+                    LookupPath::new(opctx, &self.db_datastore).disk_id(*id);
+                Ok(disk)
+            }
+            params::DiskSelector {
+                disk: NameOrId::Name(name),
+                project_selector: Some(project_selector),
+            } => {
+                let disk = self
+                    .project_lookup(opctx, project_selector)?
+                    .disk_name(Name::ref_cast(name));
+                Ok(disk)
+            }
+            params::DiskSelector {
+                disk: NameOrId::Id(_),
+                project_selector: Some(_),
+            } => Err(Error::invalid_request(
+                "when providing disk as an ID, project should not be specified",
+            )),
+            _ => Err(Error::invalid_request(
+                "disk should either be UUID or project should be specified",
+            )),
+        }
+    }
 
     pub async fn project_create_disk(
         self: &Arc<Self>,
         opctx: &OpContext,
-        organization_name: &Name,
-        project_name: &Name,
+        project_lookup: &lookup::Project<'_>,
         params: &params::DiskCreate,
     ) -> CreateResult<db::model::Disk> {
-        let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore)
-            .organization_name(organization_name)
-            .project_name(project_name)
-            .lookup_for(authz::Action::CreateChild)
-            .await?;
+        let (.., authz_project) =
+            project_lookup.lookup_for(authz::Action::CreateChild).await?;
 
         match &params.disk_source {
             params::DiskSource::Blank { block_size } => {
@@ -225,49 +258,30 @@ impl super::Nexus {
         Ok(disk_created)
     }
 
-    pub async fn project_list_disks(
+    pub async fn project_list_disks_by_id(
         &self,
         opctx: &OpContext,
-        organization_name: &Name,
-        project_name: &Name,
-        pagparams: &DataPageParams<'_, Name>,
+        project_lookup: &lookup::Project<'_>,
+        pagparams: &DataPageParams<'_, Uuid>,
     ) -> ListResultVec<db::model::Disk> {
-        let (.., authz_project) = LookupPath::new(opctx, &self.db_datastore)
-            .organization_name(organization_name)
-            .project_name(project_name)
-            .lookup_for(authz::Action::ListChildren)
-            .await?;
+        let (.., authz_project) =
+            project_lookup.lookup_for(authz::Action::ListChildren).await?;
         self.db_datastore
-            .project_list_disks(opctx, &authz_project, pagparams)
+            .project_list_disks_by_id(opctx, &authz_project, pagparams)
             .await
     }
 
-    pub async fn disk_fetch(
-        &self,
-        opctx: &OpContext,
-        organization_name: &Name,
-        project_name: &Name,
-        disk_name: &Name,
-    ) -> LookupResult<db::model::Disk> {
-        let (.., db_disk) = LookupPath::new(opctx, &self.db_datastore)
-            .organization_name(organization_name)
-            .project_name(project_name)
-            .disk_name(disk_name)
-            .fetch()
-            .await?;
-        Ok(db_disk)
-    }
-
-    pub async fn disk_fetch_by_id(
+    pub async fn project_list_disks_by_name(
         &self,
         opctx: &OpContext,
-        disk_id: &Uuid,
-    ) -> LookupResult<db::model::Disk> {
-        let (.., db_disk) = LookupPath::new(opctx, &self.db_datastore)
-            .disk_id(*disk_id)
-            .fetch()
-            .await?;
-        Ok(db_disk)
+        project_lookup: &lookup::Project<'_>,
+        pagparams: &DataPageParams<'_, Name>,
+    ) -> ListResultVec<db::model::Disk> {
+        let (.., authz_project) =
+            project_lookup.lookup_for(authz::Action::ListChildren).await?;
+        self.db_datastore
+            .project_list_disks_by_name(opctx, &authz_project, pagparams)
+            .await
     }
 
     /// Modifies the runtime state of the Disk as requested.  This generally
@@ -372,17 +386,10 @@ impl super::Nexus {
 
     pub async fn project_delete_disk(
         self: &Arc<Self>,
-        opctx: &OpContext,
-        organization_name: &Name,
-        project_name: &Name,
-        disk_name: &Name,
+        disk_lookup: &lookup::Disk<'_>,
     ) -> DeleteResult {
-        let (.., authz_disk) = LookupPath::new(opctx, &self.db_datastore)
-            .organization_name(organization_name)
-            .project_name(project_name)
-            .disk_name(disk_name)
-            .lookup_for(authz::Action::Delete)
-            .await?;
+        let (.., authz_disk) =
+            disk_lookup.lookup_for(authz::Action::Delete).await?;
 
         let saga_params =
             sagas::disk_delete::Params { disk_id: authz_disk.id() };

nexus/src/app/instance.rs

@@ -478,7 +478,7 @@ impl super::Nexus {
         // Gather disk information and turn that into DiskRequests
         let disks = self
             .db_datastore
-            .instance_list_disks(
+            .instance_list_disks_by_name(
                 &opctx,
                 &authz_instance,
                 &DataPageParams {
@@ -699,48 +699,66 @@ impl super::Nexus {
         }
     }
 
-    /// Lists disks attached to the instance.
-    pub async fn instance_list_disks(
+    /// Lists disks attached to the instance by id.
+    pub async fn instance_list_disks_by_id(
         &self,
         opctx: &OpContext,
-        organization_name: &Name,
-        project_name: &Name,
-        instance_name: &Name,
+        instance_lookup: &lookup::Instance<'_>,
+        pagparams: &DataPageParams<'_, Uuid>,
+    ) -> ListResultVec<db::model::Disk> {
+        let (.., authz_instance) =
+            instance_lookup.lookup_for(authz::Action::ListChildren).await?;
+        self.db_datastore
+            .instance_list_disks_by_id(opctx, &authz_instance, pagparams)
+            .await
+    }
+
+    /// Lists disks attached to the instance by name.
+    pub async fn instance_list_disks_by_name(
+        &self,
+        opctx: &OpContext,
+        instance_lookup: &lookup::Instance<'_>,
         pagparams: &DataPageParams<'_, Name>,
     ) -> ListResultVec<db::model::Disk> {
-        let (.., authz_instance) = LookupPath::new(opctx, &self.db_datastore)
-            .organization_name(organization_name)
-            .project_name(project_name)
-            .instance_name(instance_name)
-            .lookup_for(authz::Action::ListChildren)
-            .await?;
+        let (.., authz_instance) =
+            instance_lookup.lookup_for(authz::Action::ListChildren).await?;
         self.db_datastore
-            .instance_list_disks(opctx, &authz_instance, pagparams)
+            .instance_list_disks_by_name(opctx, &authz_instance, pagparams)
             .await
     }
 
     /// Attach a disk to an instance.
     pub async fn instance_attach_disk(
         &self,
         opctx: &OpContext,
-        organization_name: &Name,
-        project_name: &Name,
-        instance_name: &Name,
-        disk_name: &Name,
+        instance_lookup: &lookup::Instance<'_>,
+        disk: NameOrId,
     ) -> UpdateResult<db::model::Disk> {
-        let (.., authz_project, authz_disk, _) =
-            LookupPath::new(opctx, &self.db_datastore)
-                .organization_name(organization_name)
-                .project_name(project_name)
-                .disk_name(disk_name)
-                .fetch()
-                .await?;
-        let (.., authz_instance, _) =
-            LookupPath::new(opctx, &self.db_datastore)
-                .project_id(authz_project.id())
-                .instance_name(instance_name)
-                .fetch()
-                .await?;
+        let (.., authz_project, authz_instance) =
+            instance_lookup.lookup_for(authz::Action::Modify).await?;
+        let (.., authz_project_disk, authz_disk) = self
+            .disk_lookup(
+                opctx,
+                &params::DiskSelector::new(
+                    None,
+                    Some(authz_project.id().into()),
+                    disk,
+                ),
+            )?
+            .lookup_for(authz::Action::Modify)
+            .await?;
+
+        // TODO-v1: Write test to verify this case
+        // Because both instance and disk can be provided by ID it's possible for someone
+        // to specify resources from different projects. The lookups would resolve the resources
+        // (assuming the user had sufficient permissions on both) without verifying the shared hierarchy.
+        // To mitigate that we verify that their parent projects have the same ID.
+        if authz_project.id() != authz_project_disk.id() {
+            return Err(Error::InvalidRequest {
+                message: "disk must be in the same project as the instance"
+                    .to_string(),
+            });
+        }
 
         // TODO(https://github.com/oxidecomputer/omicron/issues/811):
         // Disk attach is only implemented for instances that are not
@@ -771,25 +789,22 @@ impl super::Nexus {
     pub async fn instance_detach_disk(
         &self,
         opctx: &OpContext,
-        organization_name: &Name,
-        project_name: &Name,
-        instance_name: &Name,
-        disk_name: &Name,
+        instance_lookup: &lookup::Instance<'_>,
+        disk: NameOrId,
     ) -> UpdateResult<db::model::Disk> {
-        let (.., authz_project, authz_disk, _) =
-            LookupPath::new(opctx, &self.db_datastore)
-                .organization_name(organization_name)
-                .project_name(project_name)
-                .disk_name(disk_name)
-                .fetch()
-                .await?;
-        let (.., authz_instance, _) =
-            LookupPath::new(opctx, &self.db_datastore)
-                .project_id(authz_project.id())
-                .instance_name(instance_name)
-                .fetch()
-                .await?;
-
+        let (.., authz_project, authz_instance) =
+            instance_lookup.lookup_for(authz::Action::Modify).await?;
+        let (.., authz_disk) = self
+            .disk_lookup(
+                opctx,
+                &params::DiskSelector::new(
+                    None,
+                    Some(authz_project.id().into()),
+                    disk,
+                ),
+            )?
+            .lookup_for(authz::Action::Modify)
+            .await?;
         // TODO(https://github.com/oxidecomputer/omicron/issues/811):
         // Disk detach is only implemented for instances that are not
         // currently running. This operation therefore can operate exclusively

nexus/src/app/sagas/disk_create.rs

@@ -585,7 +585,7 @@ pub(crate) mod test {
     use crate::{
         app::saga::create_saga_dag, app::sagas::disk_create::Params,
         app::sagas::disk_create::SagaDiskCreate, authn::saga::Serialized,
-        context::OpContext, db, db::datastore::DataStore, external_api::params,
+        context::OpContext, db::datastore::DataStore, external_api::params,
     };
     use async_bb8_diesel::{AsyncRunQueryDsl, OptionalExtension};
     use diesel::{ExpressionMethods, QueryDsl, SelectableHelper};
@@ -599,7 +599,6 @@ pub(crate) mod test {
     use omicron_common::api::external::IdentityMetadataCreateParams;
     use omicron_common::api::external::Name;
     use omicron_sled_agent::sim::SledAgent;
-    use ref_cast::RefCast;
     use std::num::NonZeroU32;
     use uuid::Uuid;
 
@@ -863,20 +862,15 @@ pub(crate) mod test {
     async fn destroy_disk(cptestctx: &ControlPlaneTestContext) {
         let nexus = &cptestctx.server.apictx.nexus;
         let opctx = test_opctx(&cptestctx);
+        let disk_selector = params::DiskSelector::new(
+            Some(Name::try_from(ORG_NAME.to_string()).unwrap().into()),
+            Some(Name::try_from(PROJECT_NAME.to_string()).unwrap().into()),
+            Name::try_from(DISK_NAME.to_string()).unwrap().into(),
+        );
+        let disk_lookup = nexus.disk_lookup(&opctx, &disk_selector).unwrap();
 
         nexus
-            .project_delete_disk(
-                &opctx,
-                db::model::Name::ref_cast(
-                    &Name::try_from(ORG_NAME.to_string()).unwrap(),
-                ),
-                db::model::Name::ref_cast(
-                    &Name::try_from(PROJECT_NAME.to_string()).unwrap(),
-                ),
-                db::model::Name::ref_cast(
-                    &Name::try_from(DISK_NAME.to_string()).unwrap(),
-                ),
-            )
+            .project_delete_disk(&disk_lookup)
             .await
             .expect("Failed to delete disk");
     }

nexus/src/app/sagas/disk_delete.rs

@@ -88,16 +88,16 @@ async fn sdd_delete_volume(
 pub(crate) mod test {
     use crate::{
         app::saga::create_saga_dag, app::sagas::disk_delete::Params,
-        app::sagas::disk_delete::SagaDiskDelete, context::OpContext, db,
+        app::sagas::disk_delete::SagaDiskDelete, context::OpContext,
     };
     use dropshot::test_util::ClientTestContext;
     use nexus_test_utils::resource_helpers::create_ip_pool;
     use nexus_test_utils::resource_helpers::create_organization;
     use nexus_test_utils::resource_helpers::create_project;
     use nexus_test_utils::resource_helpers::DiskTest;
     use nexus_test_utils_macros::nexus_test;
+    use nexus_types::external_api::params;
     use omicron_common::api::external::Name;
-    use ref_cast::RefCast;
     use std::num::NonZeroU32;
     use uuid::Uuid;
 
@@ -125,15 +125,17 @@ pub(crate) mod test {
         let nexus = &cptestctx.server.apictx.nexus;
         let opctx = test_opctx(&cptestctx);
 
+        let project_selector = params::ProjectSelector::new(
+            Some(Name::try_from(ORG_NAME.to_string()).unwrap().into()),
+            Name::try_from(PROJECT_NAME.to_string()).unwrap().into(),
+        );
+        let project_lookup =
+            nexus.project_lookup(&opctx, &project_selector).unwrap();
+
         nexus
             .project_create_disk(
                 &opctx,
-                db::model::Name::ref_cast(
-                    &Name::try_from(ORG_NAME.to_string()).unwrap(),
-                ),
-                db::model::Name::ref_cast(
-                    &Name::try_from(PROJECT_NAME.to_string()).unwrap(),
-                ),
+                &project_lookup,
                 &crate::app::sagas::disk_create::test::new_disk_create_params(),
             )
             .await