From 9be256925748db116b15fafa2980d1d931c0a9f0 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Fri, 3 Jun 2022 09:49:58 -0400 Subject: [PATCH 1/8] [API] Bounds-check that disks are a minimum size, and use a minimum granularity size (#1029) - Requires the user to select a size of at least one gibibyte when creating a new disk. - Add a test which attempts to create a disk that is 512 mebibytes in size. --- nexus/src/app/disk.rs | 11 +++++++++ nexus/tests/integration_tests/disks.rs | 34 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index 5a0f0ee4117..b5dc2a235be 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -54,6 +54,17 @@ impl super::Nexus { ), }); } + + // Reject disks where the block size isn't at least + // one gibibyte + if params.size.to_whole_gibibytes() < 1 { + return Err(Error::InvalidValue { + label: String::from("size"), + message: String::from( + "total size must be at least one gibibyte", + ), + }); + } } params::DiskSource::Snapshot { snapshot_id: _ } => { // Until we implement snapshots, do not allow disks to be diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 9c8a8910db5..46dc0a283e5 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -732,6 +732,40 @@ async fn test_disk_reject_total_size_not_divisible_by_block_size( .unwrap(); } +//Tests that a disk is rejected if the total size is less than one gibibyte +#[nexus_test] +async fn test_disk_reject_total_size_less_than_one_gibibyte( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + create_org_and_project(client).await; + + let disk_size = ByteCount::from_mebibytes_u32(512); + + // Attempt to allocate the disk, observe a server error. + let disks_url = get_disks_url(); + let new_disk = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: DISK_NAME.parse().unwrap(), + description: String::from("sells rainsticks"), + }, + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512).unwrap(), + }, + size: disk_size, + }; + + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); +} + async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { NexusRequest::object_get(client, disk_url) .authn_as(AuthnMode::PrivilegedUser) From 8d2eb6055a79622fc9081d132e8b70da1637e83f Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Tue, 7 Jun 2022 11:04:51 -0400 Subject: [PATCH 2/8] Add MIN_DISK_SIZE parameter - Add MIN_DISK_SIZE constant. - Modify tests to create disks with a size of MIN_DISK_SIZE / 2. --- nexus/src/external_api/params.rs | 2 ++ nexus/tests/integration_tests/disks.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 59014e2498a..e250eed0612 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -271,6 +271,8 @@ pub struct VpcRouterUpdate { // DISKS +pub const MIN_DISK_SIZE: u32 = 1 << 30; // 1 GiB + #[derive(Copy, Clone, Debug, Deserialize, Serialize)] #[serde(try_from = "u32")] // invoke the try_from validation routine below pub struct BlockSize(pub u32); diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 46dc0a283e5..8a42ac1a61b 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -740,7 +740,7 @@ async fn test_disk_reject_total_size_less_than_one_gibibyte( let client = &cptestctx.external_client; create_org_and_project(client).await; - let disk_size = ByteCount::from_mebibytes_u32(512); + let disk_size = ByteCount::from(params::MIN_DISK_SIZE / 2); // Attempt to allocate the disk, observe a server error. let disks_url = get_disks_url(); From 7f3b58ec70e06427a5013989f496ee27ddd151fb Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Tue, 7 Jun 2022 12:43:02 -0400 Subject: [PATCH 3/8] Add disk size granularity The disk size must be divisible by 1 GiB. --- nexus/src/app/disk.rs | 19 +++++++++--- nexus/tests/integration_tests/disks.rs | 40 ++++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index b5dc2a235be..a88ccc61ae5 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -55,13 +55,24 @@ impl super::Nexus { }); } - // Reject disks where the block size isn't at least - // one gibibyte - if params.size.to_whole_gibibytes() < 1 { + // Reject disks where the size isn't at least MIN_DISK_SIZE + if params.size.to_bytes() < params::MIN_DISK_SIZE as u64 { return Err(Error::InvalidValue { label: String::from("size"), message: String::from( - "total size must be at least one gibibyte", + "total size must be at least 1 GiB", + ), + }); + } + + // Reject disks where the MIN_DISK_SIZE doesn't evenly divide + // the size + if (params.size.to_bytes() % params::MIN_DISK_SIZE as u64) != 0 + { + return Err(Error::InvalidValue { + label: String::from("size"), + message: String::from( + "total size must be a multiple of 1 GiB", ), }); } diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 8a42ac1a61b..bf8c92fc49d 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -688,7 +688,8 @@ async fn test_disk_invalid_block_size_rejected( .unwrap(); } -// Tests that a disk is rejected if the total size isn't divided by the block size +// Tests that a disk is rejected if the total size isn't divided by the +// block size #[nexus_test] async fn test_disk_reject_total_size_not_divisible_by_block_size( cptestctx: &ControlPlaneTestContext, @@ -732,7 +733,7 @@ async fn test_disk_reject_total_size_not_divisible_by_block_size( .unwrap(); } -//Tests that a disk is rejected if the total size is less than one gibibyte +// Tests that a disk is rejected if the total size is less than 1 GiB #[nexus_test] async fn test_disk_reject_total_size_less_than_one_gibibyte( cptestctx: &ControlPlaneTestContext, @@ -766,6 +767,41 @@ async fn test_disk_reject_total_size_less_than_one_gibibyte( .unwrap(); } +// Tests that a disk is rejected if the total size isn't divisible by +// MIN_DISK_SIZE +#[nexus_test] +async fn test_disk_reject_total_size_not_divisible_by_min_disk_size( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + create_org_and_project(client).await; + + let disk_size = ByteCount::from(1024 * 1024 * 1024 + 256); + + // Attempt to allocate the disk, observe a server error. + let disks_url = get_disks_url(); + let new_disk = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: DISK_NAME.parse().unwrap(), + description: String::from("sells rainsticks"), + }, + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512).unwrap(), + }, + size: disk_size, + }; + + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); +} + async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { NexusRequest::object_get(client, disk_url) .authn_as(AuthnMode::PrivilegedUser) From 59488345e6975d49d6b61303ddb3cb91bf206e66 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Tue, 7 Jun 2022 14:39:40 -0400 Subject: [PATCH 4/8] Append _BYTES to MIN_DISK_SIZE --- nexus/src/app/disk.rs | 10 ++++++---- nexus/src/external_api/params.rs | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index a88ccc61ae5..a8ac9940c85 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -55,8 +55,9 @@ impl super::Nexus { }); } - // Reject disks where the size isn't at least MIN_DISK_SIZE - if params.size.to_bytes() < params::MIN_DISK_SIZE as u64 { + // Reject disks where the size isn't at least + // MIN_DISK_SIZE_BYTES_BYTES + if params.size.to_bytes() < params::MIN_DISK_SIZE_BYTES as u64 { return Err(Error::InvalidValue { label: String::from("size"), message: String::from( @@ -65,9 +66,10 @@ impl super::Nexus { }); } - // Reject disks where the MIN_DISK_SIZE doesn't evenly divide + // Reject disks where the MIN_DISK_SIZE_BYTES doesn't evenly divide // the size - if (params.size.to_bytes() % params::MIN_DISK_SIZE as u64) != 0 + if (params.size.to_bytes() % params::MIN_DISK_SIZE_BYTES as u64) + != 0 { return Err(Error::InvalidValue { label: String::from("size"), diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index e250eed0612..f9c52743cc6 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -271,7 +271,7 @@ pub struct VpcRouterUpdate { // DISKS -pub const MIN_DISK_SIZE: u32 = 1 << 30; // 1 GiB +pub const MIN_DISK_SIZE_BYTES: u32 = 1 << 30; // 1 GiB #[derive(Copy, Clone, Debug, Deserialize, Serialize)] #[serde(try_from = "u32")] // invoke the try_from validation routine below From b4ecb7b2fad77750ac0c0e15d870f7390bc1ee22 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Tue, 7 Jun 2022 14:50:14 -0400 Subject: [PATCH 5/8] Add disk minimum & granularity to GlobalImage case for project_create_disk --- nexus/src/app/disk.rs | 26 +++++++++++++++++++++++++- nexus/tests/integration_tests/disks.rs | 2 +- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index a8ac9940c85..2ab692fd16e 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -56,7 +56,7 @@ impl super::Nexus { } // Reject disks where the size isn't at least - // MIN_DISK_SIZE_BYTES_BYTES + // MIN_DISK_SIZE_BYTES if params.size.to_bytes() < params::MIN_DISK_SIZE_BYTES as u64 { return Err(Error::InvalidValue { label: String::from("size"), @@ -129,6 +129,30 @@ impl super::Nexus { ), )); } + + // Reject disks where the size isn't at least + // MIN_DISK_SIZE_BYTES + if params.size.to_bytes() < params::MIN_DISK_SIZE_BYTES as u64 { + return Err(Error::InvalidValue { + label: String::from("size"), + message: String::from( + "total size must be at least 1 GiB", + ), + }); + } + + // Reject disks where the MIN_DISK_SIZE_BYTES doesn't evenly divide + // the size + if (params.size.to_bytes() % params::MIN_DISK_SIZE_BYTES as u64) + != 0 + { + return Err(Error::InvalidValue { + label: String::from("size"), + message: String::from( + "total size must be a multiple of 1 GiB", + ), + }); + } } } diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index bf8c92fc49d..04dadda0027 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -741,7 +741,7 @@ async fn test_disk_reject_total_size_less_than_one_gibibyte( let client = &cptestctx.external_client; create_org_and_project(client).await; - let disk_size = ByteCount::from(params::MIN_DISK_SIZE / 2); + let disk_size = ByteCount::from(params::MIN_DISK_SIZE_BYTES / 2); // Attempt to allocate the disk, observe a server error. let disks_url = get_disks_url(); From cbb0070785b35d246ab3a48291969e9502026274 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Tue, 7 Jun 2022 15:56:24 -0400 Subject: [PATCH 6/8] Change disk size errors to read MIN_DISK_SIZE_BYTES instead of 1 GiB --- nexus/src/app/disk.rs | 8 ++++---- nexus/tests/integration_tests/disks.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index 2ab692fd16e..9357eeb3257 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -61,7 +61,7 @@ impl super::Nexus { return Err(Error::InvalidValue { label: String::from("size"), message: String::from( - "total size must be at least 1 GiB", + "total size must be at least MIN_DISK_SIZE_BYTES", ), }); } @@ -74,7 +74,7 @@ impl super::Nexus { return Err(Error::InvalidValue { label: String::from("size"), message: String::from( - "total size must be a multiple of 1 GiB", + "total size must be a multiple of MIN_DISK_SIZE_BYTES", ), }); } @@ -136,7 +136,7 @@ impl super::Nexus { return Err(Error::InvalidValue { label: String::from("size"), message: String::from( - "total size must be at least 1 GiB", + "total size must be at least MIN_DISK_SIZE_BYTES", ), }); } @@ -149,7 +149,7 @@ impl super::Nexus { return Err(Error::InvalidValue { label: String::from("size"), message: String::from( - "total size must be a multiple of 1 GiB", + "total size must be a multiple of MIN_DISK_SIZE_BYTES", ), }); } diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 04dadda0027..365885afe45 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -733,7 +733,7 @@ async fn test_disk_reject_total_size_not_divisible_by_block_size( .unwrap(); } -// Tests that a disk is rejected if the total size is less than 1 GiB +// Tests that a disk is rejected if the total size is less than MIN_DISK_SIZE #[nexus_test] async fn test_disk_reject_total_size_less_than_one_gibibyte( cptestctx: &ControlPlaneTestContext, @@ -768,7 +768,7 @@ async fn test_disk_reject_total_size_less_than_one_gibibyte( } // Tests that a disk is rejected if the total size isn't divisible by -// MIN_DISK_SIZE +// MIN_DISK_SIZE_BYTES #[nexus_test] async fn test_disk_reject_total_size_not_divisible_by_min_disk_size( cptestctx: &ControlPlaneTestContext, From 2e4838188fd8f9b902a8993988fc4811aa3e38e2 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Thu, 9 Jun 2022 10:29:20 -0400 Subject: [PATCH 7/8] Change MIN_DISK_SIZE error to utilize ByteCount::from --- nexus/src/app/disk.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index 9357eeb3257..dd4aade4df7 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -13,6 +13,7 @@ use crate::db; use crate::db::lookup::LookupPath; use crate::db::model::Name; use crate::external_api::params; +use omicron_common::api::external::ByteCount; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; @@ -60,8 +61,9 @@ impl super::Nexus { if params.size.to_bytes() < params::MIN_DISK_SIZE_BYTES as u64 { return Err(Error::InvalidValue { label: String::from("size"), - message: String::from( - "total size must be at least MIN_DISK_SIZE_BYTES", + message: format!( + "total size must be at least {}", + ByteCount::from(params::MIN_DISK_SIZE_BYTES) ), }); } @@ -73,8 +75,9 @@ impl super::Nexus { { return Err(Error::InvalidValue { label: String::from("size"), - message: String::from( - "total size must be a multiple of MIN_DISK_SIZE_BYTES", + message: format!( + "total size must be a multiple of {}", + ByteCount::from(params::MIN_DISK_SIZE_BYTES) ), }); } @@ -135,8 +138,9 @@ impl super::Nexus { if params.size.to_bytes() < params::MIN_DISK_SIZE_BYTES as u64 { return Err(Error::InvalidValue { label: String::from("size"), - message: String::from( - "total size must be at least MIN_DISK_SIZE_BYTES", + message: format!( + "total size must be at least {}", + ByteCount::from(params::MIN_DISK_SIZE_BYTES) ), }); } @@ -148,8 +152,9 @@ impl super::Nexus { { return Err(Error::InvalidValue { label: String::from("size"), - message: String::from( - "total size must be a multiple of MIN_DISK_SIZE_BYTES", + message: format!( + "total size must be a multiple of {}", + ByteCount::from(params::MIN_DISK_SIZE_BYTES) ), }); } From 10fd3be2ee695ac522699f186e90d9b621988f41 Mon Sep 17 00:00:00 2001 From: "Ryan C. England" Date: Thu, 9 Jun 2022 11:10:36 -0400 Subject: [PATCH 8/8] Expand min_disk_size test coverage to include error's returned. Changed disk granularity test to be a multiple of the block_size. --- nexus/tests/integration_tests/disks.rs | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 365885afe45..e4525d2895b 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -756,7 +756,7 @@ async fn test_disk_reject_total_size_less_than_one_gibibyte( size: disk_size, }; - NexusRequest::new( + let error = NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&new_disk)) .expect_status(Some(StatusCode::BAD_REQUEST)), @@ -764,7 +764,16 @@ async fn test_disk_reject_total_size_less_than_one_gibibyte( .authn_as(AuthnMode::PrivilegedUser) .execute() .await + .unwrap() + .parsed_body::() .unwrap(); + assert_eq!( + error.message, + format!( + "unsupported value for \"size\": total size must be at least {}", + ByteCount::from(params::MIN_DISK_SIZE_BYTES) + ) + ); } // Tests that a disk is rejected if the total size isn't divisible by @@ -776,7 +785,7 @@ async fn test_disk_reject_total_size_not_divisible_by_min_disk_size( let client = &cptestctx.external_client; create_org_and_project(client).await; - let disk_size = ByteCount::from(1024 * 1024 * 1024 + 256); + let disk_size = ByteCount::from(1024 * 1024 * 1024 + 512); // Attempt to allocate the disk, observe a server error. let disks_url = get_disks_url(); @@ -791,7 +800,7 @@ async fn test_disk_reject_total_size_not_divisible_by_min_disk_size( size: disk_size, }; - NexusRequest::new( + let error = NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&new_disk)) .expect_status(Some(StatusCode::BAD_REQUEST)), @@ -799,7 +808,16 @@ async fn test_disk_reject_total_size_not_divisible_by_min_disk_size( .authn_as(AuthnMode::PrivilegedUser) .execute() .await + .unwrap() + .parsed_body::() .unwrap(); + assert_eq!( + error.message, + format!( + "unsupported value for \"size\": total size must be a multiple of {}", + ByteCount::from(params::MIN_DISK_SIZE_BYTES) + ) + ); } async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk {