diff --git a/nexus/db-queries/src/db/datastore/image.rs b/nexus/db-queries/src/db/datastore/image.rs index e44da013cd..759b523010 100644 --- a/nexus/db-queries/src/db/datastore/image.rs +++ b/nexus/db-queries/src/db/datastore/image.rs @@ -19,6 +19,7 @@ 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::DeleteResult; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; @@ -232,4 +233,40 @@ impl DataStore { })?; Ok(image) } + + pub async fn silo_image_delete( + &self, + opctx: &OpContext, + authz_image: &authz::SiloImage, + image: SiloImage, + ) -> DeleteResult { + opctx.authorize(authz::Action::Delete, authz_image).await?; + self.image_delete(opctx, image.into()).await + } + + pub async fn project_image_delete( + &self, + opctx: &OpContext, + authz_image: &authz::ProjectImage, + image: ProjectImage, + ) -> DeleteResult { + opctx.authorize(authz::Action::Delete, authz_image).await?; + self.image_delete(opctx, image.into()).await + } + + async fn image_delete( + &self, + opctx: &OpContext, + image: Image, + ) -> DeleteResult { + use db::schema::image::dsl; + diesel::update(dsl::image) + .filter(dsl::id.eq(image.id())) + .set(dsl::time_deleted.eq(Utc::now())) + .execute_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Ok(()) + } } diff --git a/nexus/db-queries/src/db/datastore/volume.rs b/nexus/db-queries/src/db/datastore/volume.rs index 38e3875036..f9f982213f 100644 --- a/nexus/db-queries/src/db/datastore/volume.rs +++ b/nexus/db-queries/src/db/datastore/volume.rs @@ -14,6 +14,7 @@ use crate::db::model::Dataset; use crate::db::model::Region; use crate::db::model::RegionSnapshot; use crate::db::model::Volume; +use anyhow::bail; use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; @@ -391,6 +392,16 @@ impl DataStore { opts, gen, } => { + if !opts.read_only { + // Only one volume can "own" a Region, and that volume's + // UUID is recorded in the region table accordingly. It is + // an error to make a copy of a volume construction request + // that references non-read-only Regions. + bail!( + "only one Volume can reference a Region non-read-only!" + ); + } + let mut opts = opts.clone(); opts.id = Uuid::new_v4(); @@ -416,7 +427,10 @@ impl DataStore { /// Checkout a copy of the Volume from the database using `volume_checkout`, /// then randomize the UUIDs in the construction request. Because this is a /// new volume, it is immediately passed to `volume_create` so that the - /// accounting for Crucible resources stays correct. + /// accounting for Crucible resources stays correct. This is only valid for + /// Volumes that reference regions read-only - it's important for accounting + /// purposes that each region in this volume construction request is + /// returned by `read_only_resources_associated_with_volume`. pub async fn volume_checkout_randomize_ids( &self, volume_id: Uuid, @@ -469,6 +483,8 @@ impl DataStore { .eq(0) // Despite the SQL specifying that this column is NOT NULL, // this null check is required for this function to work! + // It's possible that the left join of region_snapshot above + // could join zero rows, making this null. .or(dsl::volume_references.is_null()), ) // where the volume has already been soft-deleted diff --git a/nexus/src/app/image.rs b/nexus/src/app/image.rs index ac51773b05..8fa9308c1d 100644 --- a/nexus/src/app/image.rs +++ b/nexus/src/app/image.rs @@ -4,8 +4,8 @@ //! Images (both project and silo scoped) -use super::Unimpl; use crate::external_api::params; +use nexus_db_queries::authn; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; @@ -27,6 +27,8 @@ use std::str::FromStr; use std::sync::Arc; use uuid::Uuid; +use super::sagas; + impl super::Nexus { pub(crate) async fn image_lookup<'a>( &'a self, @@ -356,26 +358,33 @@ impl super::Nexus { } } - // TODO-MVP: Implement pub(crate) async fn image_delete( self: &Arc, opctx: &OpContext, image_lookup: &ImageLookup<'_>, ) -> DeleteResult { - match image_lookup { + let image_param: sagas::image_delete::ImageParam = match image_lookup { ImageLookup::ProjectImage(lookup) => { - lookup.lookup_for(authz::Action::Delete).await?; + let (_, _, authz_image, image) = + lookup.fetch_for(authz::Action::Delete).await?; + sagas::image_delete::ImageParam::Project { authz_image, image } } ImageLookup::SiloImage(lookup) => { - lookup.lookup_for(authz::Action::Delete).await?; + let (_, authz_image, image) = + lookup.fetch_for(authz::Action::Delete).await?; + sagas::image_delete::ImageParam::Silo { authz_image, image } } }; - let error = Error::InternalError { - internal_message: "Endpoint not implemented".to_string(), + + let saga_params = sagas::image_delete::Params { + serialized_authn: authn::saga::Serialized::for_opctx(opctx), + image_param, }; - Err(self - .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) - .await) + + self.execute_saga::(saga_params) + .await?; + + Ok(()) } /// Converts a project scoped image into a silo scoped image diff --git a/nexus/src/app/sagas/image_delete.rs b/nexus/src/app/sagas/image_delete.rs new file mode 100644 index 0000000000..1d88ff17af --- /dev/null +++ b/nexus/src/app/sagas/image_delete.rs @@ -0,0 +1,124 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use super::{ActionRegistry, NexusActionContext, NexusSaga}; +use crate::app::sagas; +use crate::app::sagas::declare_saga_actions; +use nexus_db_queries::{authn, authz, db}; +use serde::Deserialize; +use serde::Serialize; +use steno::ActionError; +use steno::Node; +use uuid::Uuid; + +#[derive(Debug, Deserialize, Serialize)] +pub(crate) enum ImageParam { + Project { authz_image: authz::ProjectImage, image: db::model::ProjectImage }, + + Silo { authz_image: authz::SiloImage, image: db::model::SiloImage }, +} + +impl ImageParam { + fn volume_id(&self) -> Uuid { + match self { + ImageParam::Project { image, .. } => image.volume_id, + + ImageParam::Silo { image, .. } => image.volume_id, + } + } +} + +#[derive(Debug, Deserialize, Serialize)] +pub(crate) struct Params { + pub serialized_authn: authn::saga::Serialized, + pub image_param: ImageParam, +} + +declare_saga_actions! { + image_delete; + DELETE_IMAGE_RECORD -> "no_result1" { + + sid_delete_image_record + } +} + +#[derive(Debug)] +pub(crate) struct SagaImageDelete; +impl NexusSaga for SagaImageDelete { + const NAME: &'static str = "image-delete"; + type Params = Params; + + fn register_actions(registry: &mut ActionRegistry) { + image_delete_register_actions(registry); + } + + fn make_saga_dag( + params: &Self::Params, + mut builder: steno::DagBuilder, + ) -> Result { + builder.append(delete_image_record_action()); + + const DELETE_VOLUME_PARAMS: &'static str = "delete_volume_params"; + + let volume_delete_params = sagas::volume_delete::Params { + serialized_authn: params.serialized_authn.clone(), + volume_id: params.image_param.volume_id(), + }; + builder.append(Node::constant( + DELETE_VOLUME_PARAMS, + serde_json::to_value(&volume_delete_params).map_err(|e| { + super::SagaInitError::SerializeError( + String::from("volume_id"), + e, + ) + })?, + )); + + let make_volume_delete_dag = || { + let subsaga_builder = steno::DagBuilder::new(steno::SagaName::new( + sagas::volume_delete::SagaVolumeDelete::NAME, + )); + sagas::volume_delete::create_dag(subsaga_builder) + }; + builder.append(steno::Node::subsaga( + "delete_volume", + make_volume_delete_dag()?, + DELETE_VOLUME_PARAMS, + )); + + Ok(builder.build()?) + } +} + +// image delete saga: action implementations + +async fn sid_delete_image_record( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; + let opctx = crate::context::op_context_for_saga_action( + &sagactx, + ¶ms.serialized_authn, + ); + + match params.image_param { + ImageParam::Project { authz_image, image } => { + osagactx + .datastore() + .project_image_delete(&opctx, &authz_image, image) + .await + .map_err(ActionError::action_failed)?; + } + + ImageParam::Silo { authz_image, image } => { + osagactx + .datastore() + .silo_image_delete(&opctx, &authz_image, image) + .await + .map_err(ActionError::action_failed)?; + } + } + + Ok(()) +} diff --git a/nexus/src/app/sagas/mod.rs b/nexus/src/app/sagas/mod.rs index 88778e3573..0ae17c7237 100644 --- a/nexus/src/app/sagas/mod.rs +++ b/nexus/src/app/sagas/mod.rs @@ -22,6 +22,7 @@ use uuid::Uuid; pub mod disk_create; pub mod disk_delete; pub mod finalize_disk; +pub mod image_delete; pub mod import_blocks_from_url; mod instance_common; pub mod instance_create; @@ -167,6 +168,9 @@ fn make_action_registry() -> ActionRegistry { &mut registry, ); ::register_actions(&mut registry); + ::register_actions( + &mut registry, + ); #[cfg(test)] ::register_actions(&mut registry); diff --git a/nexus/tests/integration_tests/images.rs b/nexus/tests/integration_tests/images.rs index 84a8a1f50f..c3db9e8f13 100644 --- a/nexus/tests/integration_tests/images.rs +++ b/nexus/tests/integration_tests/images.rs @@ -7,15 +7,21 @@ use dropshot::ResultsPage; use http::method::Method; use http::StatusCode; +use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO; +use nexus_db_queries::db::fixed_data::silo_user::USER_TEST_UNPRIVILEGED; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::resource_helpers::create_project; +use nexus_test_utils::resource_helpers::grant_iam; use nexus_test_utils::resource_helpers::DiskTest; use nexus_test_utils_macros::nexus_test; -use omicron_common::api::external::Disk; - +use nexus_types::external_api::shared::ProjectRole; +use nexus_types::external_api::shared::SiloRole; use nexus_types::external_api::{params, views}; +use nexus_types::identity::Asset; +use nexus_types::identity::Resource; +use omicron_common::api::external::Disk; use omicron_common::api::external::{ByteCount, IdentityMetadataCreateParams}; use httptest::{matchers::*, responders::*, Expectation, ServerBuilder}; @@ -709,3 +715,121 @@ async fn test_image_from_other_project_snapshot_fails( .unwrap(); assert_eq!(error.message, "snapshot does not belong to this project"); } + +#[nexus_test] +async fn test_image_deletion_permissions(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + DiskTest::new(&cptestctx).await; + + // Create a project + + create_project(client, PROJECT_NAME).await; + + // Grant the unprivileged user viewer on the silo and admin on that project + + let silo_url = format!("/v1/system/silos/{}", DEFAULT_SILO.id()); + grant_iam( + client, + &silo_url, + SiloRole::Viewer, + USER_TEST_UNPRIVILEGED.id(), + AuthnMode::PrivilegedUser, + ) + .await; + + let project_url = format!("/v1/projects/{}", PROJECT_NAME); + grant_iam( + client, + &project_url, + ProjectRole::Admin, + USER_TEST_UNPRIVILEGED.id(), + AuthnMode::PrivilegedUser, + ) + .await; + + // Create an image in the default silo using the privileged user + + let server = ServerBuilder::new().run().unwrap(); + server.expect( + Expectation::matching(request::method_path("HEAD", "/image.raw")) + .times(1..) + .respond_with( + status_code(200).append_header( + "Content-Length", + format!("{}", 4096 * 1000), + ), + ), + ); + + let silo_images_url = "/v1/images"; + let images_url = get_project_images_url(PROJECT_NAME); + + let image_create_params = get_image_create(params::ImageSource::Url { + url: server.url("/image.raw").to_string(), + block_size: BLOCK_SIZE, + }); + + let image = + NexusRequest::objects_post(client, &images_url, &image_create_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute_and_parse_unwrap::() + .await; + + let image_id = image.identity.id; + + // promote the image to the silo + + let promote_url = format!("/v1/images/{}/promote", image_id); + 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"); + + // the unprivileged user should not be able to delete that image + + let image_url = format!("/v1/images/{}", image_id); + NexusRequest::new( + RequestBuilder::new(client, http::Method::DELETE, &image_url) + .expect_status(Some(http::StatusCode::FORBIDDEN)), + ) + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .expect("should not be able to delete silo image as unpriv user!"); + + // Demote that image + + let demote_url = + format!("/v1/images/{}/demote?project={}", image_id, PROJECT_NAME); + 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; + + // now the unpriviledged user should be able to delete that image + + let image_url = format!("/v1/images/{}", image_id); + NexusRequest::new( + RequestBuilder::new(client, http::Method::DELETE, &image_url) + .expect_status(Some(http::StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .expect("should be able to delete project image as unpriv user!"); +} diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index f974e85dc4..24b2721a1d 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -245,32 +245,24 @@ async fn test_project_deletion_with_image(cptestctx: &ControlPlaneTestContext) { delete_project_expect_fail(&url, &client).await, ); - // TODO: finish test once image delete is implemented. Image create works - // and project delete with image fails as expected, but image delete is not - // implemented yet, so we can't show that project delete works after image - // delete. let image_url = format!("/v1/images/{}", image.identity.id); - NexusRequest::expect_failure_with_body( - client, - StatusCode::INTERNAL_SERVER_ERROR, - Method::DELETE, - &image_url, - &image_create_params, + NexusRequest::object_delete(&client, &image_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to delete image"); + + // Expect that trying to GET the image results in a 404 + NexusRequest::new( + RequestBuilder::new(&client, http::Method::GET, &image_url) + .expect_status(Some(http::StatusCode::NOT_FOUND)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() .await - .unwrap(); + .expect("GET of a deleted image did not return 404"); - // TODO: delete the image - // NexusRequest::object_delete(&client, &image_url) - // .authn_as(AuthnMode::PrivilegedUser) - // .execute() - // .await - // .expect("failed to delete image"); - - // TODO: now delete project works - // delete_project(&url, &client).await; + delete_project(&url, &client).await; } #[nexus_test] diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index e263593def..f5935676a7 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -1103,8 +1103,6 @@ async fn test_create_image_from_snapshot_delete( assert!(!disk_test.crucible_resources_deleted().await); // Delete the image - // TODO-unimplemented - /* let image_url = "/v1/images/debian-11"; NexusRequest::object_delete(client, &image_url) .authn_as(AuthnMode::PrivilegedUser) @@ -1114,7 +1112,213 @@ async fn test_create_image_from_snapshot_delete( // Assert everything was cleaned up assert!(disk_test.crucible_resources_deleted().await); - */ +} + +enum DeleteImageTestParam { + Image, + Disk, + Snapshot, +} + +async fn delete_image_test( + cptestctx: &ControlPlaneTestContext, + order: &[DeleteImageTestParam], +) { + // 1. Create a blank disk + // 2. Take a snapshot of that disk + // 3. Create an image from that snapshot + // 4. Delete each of these items in some order + + let disk_test = DiskTest::new(&cptestctx).await; + + let client = &cptestctx.external_client; + populate_ip_pool(&client, "default", None).await; + create_org_and_project(client).await; + + let disks_url = get_disks_url(); + + // Create a blank disk + + let disk_size = ByteCount::from_gibibytes_u32(2); + let base_disk_name: Name = "base-disk".parse().unwrap(); + let base_disk = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: base_disk_name.clone(), + description: String::from("all your base disk are belong to us"), + }, + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512).unwrap(), + }, + size: disk_size, + }; + + let _base_disk: Disk = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&base_disk)) + .expect_status(Some(StatusCode::CREATED)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + + // Issue snapshot request + let snapshots_url = format!("/v1/snapshots?project={}", PROJECT_NAME); + + let snapshot: views::Snapshot = object_create( + client, + &snapshots_url, + ¶ms::SnapshotCreate { + identity: IdentityMetadataCreateParams { + name: "a-snapshot".parse().unwrap(), + description: String::from("you are on the way to destruction"), + }, + disk: base_disk_name.clone().into(), + }, + ) + .await; + + // Create an image from the snapshot + let image_create_params = params::ImageCreate { + identity: IdentityMetadataCreateParams { + name: "debian-11".parse().unwrap(), + description: String::from( + "you have no chance to survive make your time", + ), + }, + source: params::ImageSource::Snapshot { id: snapshot.identity.id }, + os: "debian".parse().unwrap(), + version: "12".into(), + }; + + let _image: views::Image = + NexusRequest::objects_post(client, "/v1/images", &image_create_params) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + + assert_eq!(order.len(), 3); + for item in order { + // Still some crucible resources + assert!(!disk_test.crucible_resources_deleted().await); + + match item { + DeleteImageTestParam::Image => { + let image_url = "/v1/images/debian-11"; + NexusRequest::object_delete(client, &image_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to delete image"); + } + + DeleteImageTestParam::Disk => { + NexusRequest::object_delete(client, &get_disk_url("base-disk")) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to delete disk"); + } + + DeleteImageTestParam::Snapshot => { + let snapshot_url = get_snapshot_url("a-snapshot"); + NexusRequest::object_delete(client, &snapshot_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to delete snapshot"); + } + } + } + + // Assert everything was cleaned up + assert!(disk_test.crucible_resources_deleted().await); +} + +// Make sure that whatever order disks, images, and snapshots are deleted, the +// Crucible resource accounting that Nexus does is correct. + +#[nexus_test] +async fn test_delete_image_order_1(cptestctx: &ControlPlaneTestContext) { + delete_image_test( + cptestctx, + &[ + DeleteImageTestParam::Disk, + DeleteImageTestParam::Image, + DeleteImageTestParam::Snapshot, + ], + ) + .await; +} + +#[nexus_test] +async fn test_delete_image_order_2(cptestctx: &ControlPlaneTestContext) { + delete_image_test( + cptestctx, + &[ + DeleteImageTestParam::Disk, + DeleteImageTestParam::Snapshot, + DeleteImageTestParam::Image, + ], + ) + .await; +} + +#[nexus_test] +async fn test_delete_image_order_3(cptestctx: &ControlPlaneTestContext) { + delete_image_test( + cptestctx, + &[ + DeleteImageTestParam::Image, + DeleteImageTestParam::Disk, + DeleteImageTestParam::Snapshot, + ], + ) + .await; +} + +#[nexus_test] +async fn test_delete_image_order_4(cptestctx: &ControlPlaneTestContext) { + delete_image_test( + cptestctx, + &[ + DeleteImageTestParam::Image, + DeleteImageTestParam::Snapshot, + DeleteImageTestParam::Disk, + ], + ) + .await; +} + +#[nexus_test] +async fn test_delete_image_order_5(cptestctx: &ControlPlaneTestContext) { + delete_image_test( + cptestctx, + &[ + DeleteImageTestParam::Snapshot, + DeleteImageTestParam::Disk, + DeleteImageTestParam::Image, + ], + ) + .await; +} + +#[nexus_test] +async fn test_delete_image_order_6(cptestctx: &ControlPlaneTestContext) { + delete_image_test( + cptestctx, + &[ + DeleteImageTestParam::Snapshot, + DeleteImageTestParam::Image, + DeleteImageTestParam::Disk, + ], + ) + .await; } // A test function to create a volume with the provided read only parent. @@ -1814,6 +2018,44 @@ async fn test_volume_checkout_updates_sparse_mid_multiple_gen( volume_match_gen(new_vol, vec![Some(8), None, Some(10)]); } +#[nexus_test] +async fn test_volume_checkout_randomize_ids_only_read_only( + cptestctx: &ControlPlaneTestContext, +) { + // Verify that a volume_checkout_randomize_ids will not work for + // non-read-only Regions + let nexus = &cptestctx.server.apictx().nexus; + let datastore = nexus.datastore(); + let volume_id = Uuid::new_v4(); + let block_size = 512; + + // Create three sub_vols. + let subvol_one = create_region(block_size, 7, Uuid::new_v4()); + let subvol_two = create_region(block_size, 7, Uuid::new_v4()); + let subvol_three = create_region(block_size, 7, Uuid::new_v4()); + + // Make the volume with our three sub_volumes + let volume_construction_request = VolumeConstructionRequest::Volume { + id: volume_id, + block_size, + sub_volumes: vec![subvol_one, subvol_two, subvol_three], + read_only_parent: None, + }; + + // Insert the volume into the database. + datastore + .volume_create(nexus_db_model::Volume::new( + volume_id, + serde_json::to_string(&volume_construction_request).unwrap(), + )) + .await + .unwrap(); + + // volume_checkout_randomize_ids should fail + let r = datastore.volume_checkout_randomize_ids(volume_id).await; + assert!(r.is_err()); +} + /// Test that the Crucible agent's port reuse does not confuse /// `decrease_crucible_resource_count_and_soft_delete_volume`, due to the /// `[ipv6]:port` targets being reused.