[API] Bounds-check that disks are a minimum size, and use a minimum granularity size (#1029) (#1150)

ryaeng · web-flow · commit a05fa49ae852 · 2022-06-09T12:35:37.000-04:00
* [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. * Add MIN_DISK_SIZE parameter - Add MIN_DISK_SIZE constant. - Modify tests to create disks with a size of MIN_DISK_SIZE / 2. * Add disk size granularity The disk size must be divisible by 1 GiB. * Append _BYTES to MIN_DISK_SIZE * Add disk minimum & granularity to GlobalImage case for project_create_disk * Change disk size errors to read MIN_DISK_SIZE_BYTES instead of 1 GiB * Change MIN_DISK_SIZE error to utilize ByteCount::from * Expand min_disk_size test coverage to include error's returned. Changed disk granularity test to be a multiple of the block_size.
diff --git 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;
@@ -54,6 +55,32 @@ 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: format!(
+                            "total size must be at least {}",
+                            ByteCount::from(params::MIN_DISK_SIZE_BYTES)
+                        ),
+                    });
+                }
+
+                // 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: format!(
+                            "total size must be a multiple of {}",
+                            ByteCount::from(params::MIN_DISK_SIZE_BYTES)
+                        ),
+                    });
+                }
             }
             params::DiskSource::Snapshot { snapshot_id: _ } => {
                 // Until we implement snapshots, do not allow disks to be
@@ -105,6 +132,32 @@ 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: format!(
+                            "total size must be at least {}",
+                            ByteCount::from(params::MIN_DISK_SIZE_BYTES)
+                        ),
+                    });
+                }
+
+                // 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: format!(
+                            "total size must be a multiple of {}",
+                            ByteCount::from(params::MIN_DISK_SIZE_BYTES)
+                        ),
+                    });
+                }
             }
         }
 
diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs
@@ -485,6 +485,8 @@ pub struct VpcRouterUpdate {
 
 // DISKS
 
+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
 pub struct BlockSize(pub u32);
diff --git 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,6 +733,93 @@ 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 MIN_DISK_SIZE
+#[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(params::MIN_DISK_SIZE_BYTES / 2);
+
+    // 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,
+    };
+
+    let error = 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()
+    .parsed_body::<dropshot::HttpErrorResponseBody>()
+    .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
+// MIN_DISK_SIZE_BYTES
+#[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 + 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,
+    };
+
+    let error = 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()
+    .parsed_body::<dropshot::HttpErrorResponseBody>()
+    .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 {
     NexusRequest::object_get(client, disk_url)
         .authn_as(AuthnMode::PrivilegedUser)
