Fix unique constraints for images (#3254)

Co-authored-by: David Crespo <[email protected]>
oxidecomputer · Jul 6, 2023 · c2de480 · c2de480
1 parent c3e856e
commit c2de480
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 228 deletions.
diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql
@@ -1047,12 +1047,22 @@ FROM
 WHERE 
     project_id IS NULL;
 
+/* Index for silo images */
+CREATE UNIQUE INDEX on omicron.public.image (
+    silo_id,
+    name
+) WHERE
+    time_deleted is NULL AND
+    project_id is NULL;
+
+/* Index for project images */
 CREATE UNIQUE INDEX on omicron.public.image (
     silo_id,
     project_id,
     name
 ) WHERE
-    time_deleted is NULL;
+    time_deleted is NULL AND
+    project_id is NOT NULL;
 
 CREATE TYPE omicron.public.snapshot_state AS ENUM (
   'creating',

diff --git a/nexus/db-queries/src/db/datastore/image.rs b/nexus/db-queries/src/db/datastore/image.rs
@@ -1,14 +1,3 @@
-use chrono::Utc;
-use diesel::prelude::*;
-use nexus_db_model::Name;
-use nexus_types::identity::Resource;
-use omicron_common::api::external::http_pagination::PaginatedBy;
-use omicron_common::api::external::CreateResult;
-use omicron_common::api::external::ListResultVec;
-use omicron_common::api::external::ResourceType;
-use omicron_common::api::external::UpdateResult;
-use ref_cast::RefCast;
-
 use crate::authz;
 use crate::authz::ApiResource;
 use crate::context::OpContext;
@@ -23,8 +12,17 @@ use crate::db::model::ProjectImage;
 use crate::db::model::Silo;
 use crate::db::model::SiloImage;
 use crate::db::pagination::paginated;
-
 use async_bb8_diesel::AsyncRunQueryDsl;
+use chrono::Utc;
+use diesel::prelude::*;
+use nexus_db_model::Name;
+use nexus_types::identity::Resource;
+use omicron_common::api::external::http_pagination::PaginatedBy;
+use omicron_common::api::external::CreateResult;
+use omicron_common::api::external::ListResultVec;
+use omicron_common::api::external::ResourceType;
+use omicron_common::api::external::UpdateResult;
+use ref_cast::RefCast;
 use uuid::Uuid;
 
 use super::DataStore;
@@ -33,59 +31,31 @@ impl DataStore {
     pub async fn project_image_list(
         &self,
         opctx: &OpContext,
-        authz_silo: &authz::Silo,
         authz_project: &authz::Project,
-        include_silo_images: bool,
         pagparams: &PaginatedBy<'_>,
     ) -> ListResultVec<Image> {
         opctx.authorize(authz::Action::ListChildren, authz_project).await?;
 
-        use db::schema::image::dsl;
         use db::schema::project_image::dsl as project_dsl;
-        match include_silo_images {
-            true => match pagparams {
-                PaginatedBy::Id(pagparams) => {
-                    paginated(dsl::image, dsl::id, &pagparams)
-                }
-                PaginatedBy::Name(pagparams) => paginated(
-                    dsl::image,
-                    dsl::name,
-                    &pagparams.map_name(|n| Name::ref_cast(n)),
-                ),
-            }
-            .filter(dsl::time_deleted.is_null())
-            .filter(dsl::silo_id.eq(authz_silo.id()))
-            .filter(
-                dsl::project_id
-                    .is_null()
-                    .or(dsl::project_id.eq(authz_project.id())),
-            )
-            .select(Image::as_select())
-            .load_async::<Image>(self.pool_authorized(opctx).await?)
-            .await
-            .map_err(|e| {
-                public_error_from_diesel_pool(e, ErrorHandler::Server)
-            }),
-            false => match pagparams {
-                PaginatedBy::Id(pagparams) => paginated(
-                    project_dsl::project_image,
-                    project_dsl::id,
-                    &pagparams,
-                ),
-                PaginatedBy::Name(pagparams) => paginated(
-                    project_dsl::project_image,
-                    project_dsl::name,
-                    &pagparams.map_name(|n| Name::ref_cast(n)),
-                ),
-            }
-            .filter(project_dsl::time_deleted.is_null())
-            .filter(project_dsl::project_id.eq(authz_project.id()))
-            .select(ProjectImage::as_select())
-            .load_async::<ProjectImage>(self.pool_authorized(opctx).await?)
-            .await
-            .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
-            .map(|v| v.into_iter().map(|v| v.into()).collect()),
+        match pagparams {
+            PaginatedBy::Id(pagparams) => paginated(
+                project_dsl::project_image,
+                project_dsl::id,
+                &pagparams,
+            ),
+            PaginatedBy::Name(pagparams) => paginated(
+                project_dsl::project_image,
+                project_dsl::name,
+                &pagparams.map_name(|n| Name::ref_cast(n)),
+            ),
         }
+        .filter(project_dsl::time_deleted.is_null())
+        .filter(project_dsl::project_id.eq(authz_project.id()))
+        .select(ProjectImage::as_select())
+        .load_async::<ProjectImage>(self.pool_authorized(opctx).await?)
+        .await
+        .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
+        .map(|v| v.into_iter().map(|v| v.into()).collect())
     }
 
     pub async fn silo_image_list(
@@ -197,12 +167,13 @@ impl DataStore {
         opctx: &OpContext,
         authz_silo: &authz::Silo,
         authz_project_image: &authz::ProjectImage,
+        project_image: &ProjectImage,
     ) -> UpdateResult<Image> {
         opctx.authorize(authz::Action::CreateChild, authz_silo).await?;
         opctx.authorize(authz::Action::Modify, authz_project_image).await?;
 
         use db::schema::image::dsl;
-        let image: Image = diesel::update(dsl::image)
+        let image = diesel::update(dsl::image)
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::id.eq(authz_project_image.id()))
             .set((
@@ -215,9 +186,13 @@ impl DataStore {
             .map_err(|e| {
                 public_error_from_diesel_pool(
                     e,
-                    ErrorHandler::NotFoundByResource(authz_project_image),
+                    ErrorHandler::Conflict(
+                        ResourceType::SiloImage,
+                        project_image.name().as_str(),
+                    ),
                 )
             })?;
+
         Ok(image)
     }
 
@@ -226,6 +201,7 @@ impl DataStore {
         opctx: &OpContext,
         authz_silo_image: &authz::SiloImage,
         authz_project: &authz::Project,
+        silo_image: &SiloImage,
     ) -> UpdateResult<Image> {
         opctx.authorize(authz::Action::Modify, authz_silo_image).await?;
         opctx.authorize(authz::Action::CreateChild, authz_project).await?;
@@ -244,7 +220,10 @@ impl DataStore {
             .map_err(|e| {
                 public_error_from_diesel_pool(
                     e,
-                    ErrorHandler::NotFoundByResource(authz_silo_image),
+                    ErrorHandler::Conflict(
+                        ResourceType::ProjectImage,
+                        silo_image.name().as_str(),
+                    ),
                 )
             })?;
         Ok(image)

diff --git a/nexus/src/app/image.rs b/nexus/src/app/image.rs
@@ -328,21 +328,14 @@ impl super::Nexus {
         &self,
         opctx: &OpContext,
         parent_lookup: &ImageParentLookup<'_>,
-        include_silo_images: bool,
         pagparams: &PaginatedBy<'_>,
     ) -> ListResultVec<db::model::Image> {
         match parent_lookup {
             ImageParentLookup::Project(project) => {
-                let (authz_silo, authz_project) =
+                let (.., authz_project) =
                     project.lookup_for(authz::Action::ListChildren).await?;
                 self.db_datastore
-                    .project_image_list(
-                        opctx,
-                        &authz_silo,
-                        &authz_project,
-                        include_silo_images,
-                        pagparams,
-                    )
+                    .project_image_list(opctx, &authz_project, pagparams)
                     .await
             }
             ImageParentLookup::Silo(silo) => {
@@ -385,8 +378,8 @@ impl super::Nexus {
     ) -> UpdateResult<db::model::Image> {
         match image_lookup {
             ImageLookup::ProjectImage(lookup) => {
-                let (authz_silo, _, authz_project_image) =
-                    lookup.lookup_for(authz::Action::Modify).await?;
+                let (authz_silo, _, authz_project_image, project_image) =
+                    lookup.fetch_for(authz::Action::Modify).await?;
                 opctx
                     .authorize(authz::Action::CreateChild, &authz_silo)
                     .await?;
@@ -395,6 +388,7 @@ impl super::Nexus {
                         opctx,
                         &authz_silo,
                         &authz_project_image,
+                        &project_image,
                     )
                     .await
             }
@@ -413,12 +407,17 @@ impl super::Nexus {
     ) -> UpdateResult<db::model::Image> {
         match image_lookup {
             ImageLookup::SiloImage(lookup) => {
-                let (_, authz_silo_image) =
-                    lookup.lookup_for(authz::Action::Modify).await?;
+                let (_, authz_silo_image, silo_image) =
+                    lookup.fetch_for(authz::Action::Modify).await?;
                 let (_, authz_project) =
                     project_lookup.lookup_for(authz::Action::Modify).await?;
                 self.db_datastore
-                    .silo_image_demote(opctx, &authz_silo_image, &authz_project)
+                    .silo_image_demote(
+                        opctx,
+                        &authz_silo_image,
+                        &authz_project,
+                        &silo_image,
+                    )
                     .await
             }
             ImageLookup::ProjectImage(_) => Err(Error::InvalidRequest {

diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs
@@ -2799,7 +2799,7 @@ async fn networking_switch_port_clear_settings(
 }]
 async fn image_list(
     rqctx: RequestContext<Arc<ServerContext>>,
-    query_params: Query<PaginatedByNameOrId<params::ImageListSelector>>,
+    query_params: Query<PaginatedByNameOrId<params::OptionalProjectSelector>>,
 ) -> Result<HttpResponseOk<ResultsPage<Image>>, HttpError> {
     let apictx = rqctx.context();
     let handler = async {
@@ -2823,12 +2823,7 @@ async fn image_list(
             }
         };
         let images = nexus
-            .image_list(
-                &opctx,
-                &parent_lookup,
-                scan_params.selector.include_silo_images.unwrap_or(false),
-                &paginated_by,
-            )
+            .image_list(&opctx, &parent_lookup, &paginated_by)
             .await?
             .into_iter()
             .map(|d| d.into())