From c2de4805acfd0c20bef9d87afacac9f8e821ad87 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 6 Jul 2023 18:02:06 -0400 Subject: [PATCH] Fix unique constraints for images (#3254) Co-authored-by: David Crespo --- common/src/sql/dbinit.sql | 12 +- nexus/db-queries/src/db/datastore/image.rs | 101 +++++++--------- nexus/src/app/image.rs | 27 ++--- nexus/src/external_api/http_entrypoints.rs | 9 +- nexus/tests/integration_tests/images.rs | 133 +++++++-------------- nexus/types/src/external_api/params.rs | 45 ------- openapi/nexus.json | 9 -- 7 files changed, 108 insertions(+), 228 deletions(-) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 0ee95b4310..9498ac8b8f 100644 --- 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 index e5b7c3fb74..17bdb6fae0 100644 --- 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 { 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::(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::(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::(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 { 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 { 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 index c51178f9a3..6ee9a18d5e 100644 --- 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 { 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 { 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 { 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 index ae2240affd..b5c37b4234 100644 --- 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>, - query_params: Query>, + query_params: Query>, ) -> Result>, 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()) diff --git a/nexus/tests/integration_tests/images.rs b/nexus/tests/integration_tests/images.rs index e1071ae73b..4effa3ab2e 100644 --- a/nexus/tests/integration_tests/images.rs +++ b/nexus/tests/integration_tests/images.rs @@ -24,18 +24,12 @@ type ControlPlaneTestContext = const PROJECT_NAME: &str = "myproj"; -const PROJECT_NAME_2: &str = "myproj2"; - const BLOCK_SIZE: params::BlockSize = params::BlockSize(512); fn get_project_images_url(project_name: &str) -> String { format!("/v1/images?project={}", project_name) } -fn get_project_images_with_silo_images_url(project_name: &str) -> String { - format!("/v1/images?project={}&include_silo_images=true", project_name) -} - fn get_image_create(source: params::ImageSource) -> params::ImageCreate { params::ImageCreate { identity: IdentityMetadataCreateParams { @@ -67,9 +61,7 @@ async fn test_image_create(cptestctx: &ControlPlaneTestContext) { ), ); - let silo_images_url = "/v1/images"; - let project_images_url = get_project_images_url(PROJECT_NAME); - let images_url = get_project_images_with_silo_images_url(PROJECT_NAME); + let images_url = get_project_images_url(PROJECT_NAME); // Before project exists, image list 404s NexusRequest::expect_failure( @@ -84,7 +76,7 @@ async fn test_image_create(cptestctx: &ControlPlaneTestContext) { .expect("Expected 404"); // create the project, now we expect an empty list - let project = create_project(client, PROJECT_NAME).await; + let _project = create_project(client, PROJECT_NAME).await; let images = NexusRequest::object_get(client, &images_url) .authn_as(AuthnMode::PrivilegedUser) @@ -114,86 +106,6 @@ async fn test_image_create(cptestctx: &ControlPlaneTestContext) { assert_eq!(images.len(), 1); assert_eq!(images[0].identity.name, "alpine-edge"); - - // create another project, which is empty until we promote the image to global - create_project(client, PROJECT_NAME_2).await; - - let project_2_images_url = - get_project_images_with_silo_images_url(PROJECT_NAME_2); - - let images = NexusRequest::object_get(client, &project_2_images_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::>() - .await - .items; - assert_eq!(images.len(), 0); - - // promote the image to the silo - let promote_url = format!( - "/v1/images/{}/promote?project={}", - "alpine-edge", PROJECT_NAME - ); - NexusRequest::new( - RequestBuilder::new(client, http::Method::POST, &promote_url) - .expect_status(Some(http::StatusCode::ACCEPTED)), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::() - .await; - - let silo_images = NexusRequest::object_get(client, &silo_images_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::>() - .await - .items; - - assert_eq!(silo_images.len(), 1); - assert_eq!(silo_images[0].identity.name, "alpine-edge"); - - // Ensure original project images is empty - let images = NexusRequest::object_get(client, &project_images_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::>() - .await - .items; - assert_eq!(images.len(), 0); - - // Ensure project 2 images with silos lists the promoted image - let images = NexusRequest::object_get(client, &project_2_images_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::>() - .await - .items; - assert_eq!(images.len(), 1); - - // demote the image back to the project - let demote_url = format!( - "/v1/images/{}/demote?project={}", - silo_images[0].identity.id, project.identity.id - ); - NexusRequest::new( - RequestBuilder::new(client, http::Method::POST, &demote_url) - .expect_status(Some(http::StatusCode::ACCEPTED)), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::() - .await; - - // Ensure there are no more silo images now that it has been demoted - let silo_images = NexusRequest::object_get(client, &silo_images_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::>() - .await - .items; - assert_eq!(silo_images.len(), 0); - - // Ensure image has returned to the project it was demoted to - let images = NexusRequest::object_get(client, &project_images_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::>() - .await - .items; - assert_eq!(images.len(), 1); } #[nexus_test] @@ -521,7 +433,7 @@ async fn test_make_disk_from_image_too_small( } #[nexus_test] -async fn test_image_access(cptestctx: &ControlPlaneTestContext) { +async fn test_image_promotion(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; DiskTest::new(&cptestctx).await; @@ -598,6 +510,15 @@ async fn test_image_access(cptestctx: &ControlPlaneTestContext) { assert_eq!(silo_images.len(), 1); assert_eq!(silo_images[0].identity.name, "alpine-edge"); + // Ensure there are no more project images + let project_images = NexusRequest::object_get(client, &images_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute_and_parse_unwrap::>() + .await + .items; + + assert_eq!(project_images.len(), 0); + let silo_image_url = format!("/v1/images/{}", image_id); let silo_image = NexusRequest::object_get(client, &silo_image_url) .authn_as(AuthnMode::PrivilegedUser) @@ -605,4 +526,34 @@ async fn test_image_access(cptestctx: &ControlPlaneTestContext) { .await; assert_eq!(silo_image.identity.id, image_id); + + // Create another project image with the same name + NexusRequest::objects_post(client, &images_url, &image_create_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute_and_parse_unwrap::() + .await; + + // Ensure project image was created + let project_images = NexusRequest::object_get(client, &images_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute_and_parse_unwrap::>() + .await + .items; + + assert_eq!(project_images.len(), 1); + assert_eq!(project_images[0].identity.name, "alpine-edge"); + + let image_id = project_images[0].identity.id; + let promote_url = format!("/v1/images/{}/promote", image_id); + + NexusRequest::new( + RequestBuilder::new(client, http::Method::POST, &promote_url) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("unexpected success") + .parsed_body::() + .unwrap(); } diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 99bbb850b1..7f9da6c942 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -145,51 +145,6 @@ pub struct SnapshotSelector { pub snapshot: NameOrId, } -/// A specialized selector for image list, it contains an extra field to indicate -/// if silo scoped images should be included when listing project images. -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] -pub struct ImageListSelector { - /// Name or ID of the project - pub project: Option, - /// Flag used to indicate if silo scoped images should be included when - /// listing project images. Only valid when `project` is provided. - #[serde(default)] - #[serde(deserialize_with = "deserialize_optional_bool_from_string")] - pub include_silo_images: Option, -} - -// Unfortunately `include_silo_images` can't used the default `Deserialize` -// derive given the selector that uses it is embedded via `serde(flatten)` which -// causes it to attempt to deserialize all flattened values a string. Similar workarounds -// have been implemented here: https://github.com/oxidecomputer/omicron/blob/efb03b501d7febe961cc8793b4d72e8542d28eab/gateway/src/http_entrypoints.rs#L443 -fn deserialize_optional_bool_from_string<'de, D>( - deserializer: D, -) -> Result, D::Error> -where - D: serde::Deserializer<'de>, -{ - use serde::de::Unexpected; - - #[derive(Debug, Deserialize)] - #[serde(untagged)] - enum StringOrOptionalBool { - String(String), - OptionalBool(Option), - } - - match StringOrOptionalBool::deserialize(deserializer)? { - StringOrOptionalBool::String(s) => match s.as_str() { - "true" => Ok(Some(true)), - "false" => Ok(None), - "" => Ok(None), - _ => { - Err(de::Error::invalid_type(Unexpected::Str(&s), &"a boolean")) - } - }, - StringOrOptionalBool::OptionalBool(b) => Ok(b), - } -} - #[derive(Deserialize, JsonSchema)] pub struct ImageSelector { /// Name or ID of the project, only required if `image` is provided as a `Name` diff --git a/openapi/nexus.json b/openapi/nexus.json index 947a757dec..b27b5907c2 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -960,15 +960,6 @@ "description": "List images which are global or scoped to the specified project. The images are returned sorted by creation date, with the most recent images appearing first.", "operationId": "image_list", "parameters": [ - { - "in": "query", - "name": "include_silo_images", - "description": "Flag used to indicate if silo scoped images should be included when listing project images. Only valid when `project` is provided.", - "schema": { - "nullable": true, - "type": "boolean" - } - }, { "in": "query", "name": "limit",