Skip to content

Commit

Permalink
[API] Bounds-check that memory for instance is above a minimum, meets…
Browse files Browse the repository at this point in the history 
… a granularity (#1030) (#1157)

- Require the user to select a memory size of at least one gibibyte
    when creating a new instance.
- Create new test to match this requirement.
- Update tests that created instances under one gibibyte.
  • Loading branch information
@ryaeng
ryaeng authored Jun 13, 2022
1 parent c51011a commit bf4a057
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 14 deletions.
27 changes: 27 additions & 0 deletions nexus/src/app/instance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::db::model::Name;
use crate::db::queries::network_interface;
use crate::external_api::params;
use omicron_common::api::external;
use omicron_common::api::external::ByteCount;
use omicron_common::api::external::CreateResult;
use omicron_common::api::external::DataPageParams;
use omicron_common::api::external::DeleteResult;
Expand Down Expand Up @@ -56,6 +57,32 @@ impl super::Nexus {
)));
}

// Reject instances where the memory is not at least
// MIN_MEMORY_SIZE_BYTES
if params.memory.to_bytes() < params::MIN_MEMORY_SIZE_BYTES as u64 {
return Err(Error::InvalidValue {
label: String::from("size"),
message: format!(
"memory must be at least {}",
ByteCount::from(params::MIN_MEMORY_SIZE_BYTES)
),
});
}

// Reject instances where the memory is not divisible by
// MIN_MEMORY_SIZE_BYTES
if (params.memory.to_bytes() % params::MIN_MEMORY_SIZE_BYTES as u64)
!= 0
{
return Err(Error::InvalidValue {
label: String::from("size"),
message: format!(
"memory must be divisible by {}",
ByteCount::from(params::MIN_MEMORY_SIZE_BYTES)
),
});
}

let saga_params = Arc::new(sagas::instance_create::Params {
serialized_authn: authn::saga::Serialized::for_opctx(opctx),
organization_name: organization_name.clone().into(),
Expand Down
2 changes: 2 additions & 0 deletions nexus/src/external_api/params.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,8 @@ pub struct NetworkInterfaceCreate {

// INSTANCES

pub const MIN_MEMORY_SIZE_BYTES: u32 = 1 << 30; // 1 GiB

/// Describes an attachment of a `NetworkInterface` to an `Instance`, at the
/// time the instance is created.
// NOTE: VPC's are an organizing concept for networking resources, not for
Expand Down
2 changes: 1 addition & 1 deletion nexus/test-utils/src/resource_helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ pub async fn create_instance_with_nics(
description: format!("instance {:?}", instance_name),
},
ncpus: InstanceCpuCount(4),
memory: ByteCount::from_mebibytes_u32(256),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("the_host"),
user_data:
b"#cloud-config\nsystem_info:\n default_user:\n name: oxide"
Expand Down
134 changes: 122 additions & 12 deletions nexus/tests/integration_tests/instances.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,20 @@ use nexus_test_utils_macros::nexus_test;
static ORGANIZATION_NAME: &str = "test-org";
static PROJECT_NAME: &str = "springfield-squidport";

fn get_project_url() -> String {
format!("/organizations/{}/projects/{}", ORGANIZATION_NAME, PROJECT_NAME)
}

fn get_instances_url() -> String {
format!("{}/instances", get_project_url())
}

async fn create_org_and_project(client: &ClientTestContext) -> Uuid {
create_organization(&client, ORGANIZATION_NAME).await;
let project = create_project(client, ORGANIZATION_NAME, PROJECT_NAME).await;
project.identity.id
}

#[nexus_test]
async fn test_instances_access_before_create_returns_not_found(
cptestctx: &ControlPlaneTestContext,
Expand Down Expand Up @@ -128,7 +142,7 @@ async fn test_instances_create_reboot_halt(
let InstanceCpuCount(nfoundcpus) = instance.ncpus;
// These particulars are hardcoded in create_instance().
assert_eq!(nfoundcpus, 4);
assert_eq!(instance.memory.to_whole_mebibytes(), 256);
assert_eq!(instance.memory.to_whole_gibibytes(), 1);
assert_eq!(instance.hostname, "the_host");
assert_eq!(instance.runtime.run_state, InstanceState::Starting);

Expand Down Expand Up @@ -565,7 +579,7 @@ async fn test_instance_create_saga_removes_instance_database_record(
description: String::from("instance to test saga unwind"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("inst"),
user_data: vec![],
network_interfaces: interface_params.clone(),
Expand All @@ -587,7 +601,7 @@ async fn test_instance_create_saga_removes_instance_database_record(
description: String::from("instance to test saga unwind 2"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("inst2"),
user_data: vec![],
network_interfaces: interface_params,
Expand Down Expand Up @@ -673,7 +687,7 @@ async fn test_instance_with_single_explicit_ip_address(
description: String::from("instance to test multiple nics"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nic-test"),
user_data: vec![],
network_interfaces: interface_params,
Expand Down Expand Up @@ -790,7 +804,7 @@ async fn test_instance_with_new_custom_network_interfaces(
description: String::from("instance to test multiple nics"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nic-test"),
user_data: vec![],
network_interfaces: interface_params,
Expand Down Expand Up @@ -904,7 +918,7 @@ async fn test_instance_create_delete_network_interface(
description: String::from("instance to test attaching new nic"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nic-test"),
user_data: vec![],
network_interfaces: params::InstanceNetworkInterfaceAttachment::None,
Expand Down Expand Up @@ -1162,7 +1176,7 @@ async fn test_instance_with_multiple_nics_unwinds_completely(
description: String::from("instance to test multiple bad nics"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nic-test"),
user_data: vec![],
network_interfaces: interface_params,
Expand Down Expand Up @@ -1236,7 +1250,7 @@ async fn test_attach_one_disk_to_instance(cptestctx: &ControlPlaneTestContext) {
description: String::from("probably serving data"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nfs"),
user_data: vec![],
network_interfaces: params::InstanceNetworkInterfaceAttachment::Default,
Expand Down Expand Up @@ -1332,7 +1346,7 @@ async fn test_attach_eight_disks_to_instance(
description: String::from("probably serving data"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nfs"),
user_data: vec![],
network_interfaces: params::InstanceNetworkInterfaceAttachment::Default,
Expand Down Expand Up @@ -1438,7 +1452,7 @@ async fn test_cannot_attach_nine_disks_to_instance(
description: String::from("probably serving data"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nfs"),
user_data: vec![],
network_interfaces: params::InstanceNetworkInterfaceAttachment::Default,
Expand Down Expand Up @@ -1565,7 +1579,7 @@ async fn test_cannot_attach_faulted_disks(cptestctx: &ControlPlaneTestContext) {
description: String::from("probably serving data"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nfs"),
user_data: vec![],
network_interfaces: params::InstanceNetworkInterfaceAttachment::Default,
Expand Down Expand Up @@ -1677,7 +1691,7 @@ async fn test_disks_detached_when_instance_destroyed(
description: String::from("probably serving data"),
},
ncpus: InstanceCpuCount::try_from(2).unwrap(),
memory: ByteCount::from_mebibytes_u32(4),
memory: ByteCount::from_gibibytes_u32(1),
hostname: String::from("nfs"),
user_data: vec![],
network_interfaces: params::InstanceNetworkInterfaceAttachment::Default,
Expand Down Expand Up @@ -1772,6 +1786,102 @@ async fn test_disks_detached_when_instance_destroyed(
}
}

// Tests that an instance is rejected if the memory is less than
// MIN_MEMORY_SIZE_BYTES
#[nexus_test]
async fn test_instances_memory_rejected_less_than_min_memory_size(
cptestctx: &ControlPlaneTestContext,
) {
let client = &cptestctx.external_client;
create_org_and_project(client).await;

// Attempt to create the instance, observe a server error.
let instances_url = get_instances_url();
let instance_name = "just-rainsticks";
let instance = params::InstanceCreate {
identity: IdentityMetadataCreateParams {
name: instance_name.parse().unwrap(),
description: format!("instance {:?}", &instance_name),
},
ncpus: InstanceCpuCount(1),
memory: ByteCount::from(params::MIN_MEMORY_SIZE_BYTES / 2),
hostname: String::from("inst"),
user_data:
b"#cloud-config\nsystem_info:\n default_user:\n name: oxide"
.to_vec(),
network_interfaces: params::InstanceNetworkInterfaceAttachment::Default,
disks: vec![],
};

let error = NexusRequest::new(
RequestBuilder::new(client, Method::POST, &instances_url)
.body(Some(&instance))
.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\": memory must be at least {}",
ByteCount::from(params::MIN_MEMORY_SIZE_BYTES)
),
);
}

// Test that an instance is rejected if memory is not divisible by
// MIN_MEMORY_SIZE
#[nexus_test]
async fn test_instances_memory_not_divisible_by_min_memory_size(
cptestctx: &ControlPlaneTestContext,
) {
let client = &cptestctx.external_client;
create_org_and_project(client).await;

// Attempt to create the instance, observe a server error.
let instances_url = get_instances_url();
let instance_name = "just-rainsticks";
let instance = params::InstanceCreate {
identity: IdentityMetadataCreateParams {
name: instance_name.parse().unwrap(),
description: format!("instance {:?}", &instance_name),
},
ncpus: InstanceCpuCount(1),
memory: ByteCount::from(1024 * 1024 * 1024 + 300),
hostname: String::from("inst"),
user_data:
b"#cloud-config\nsystem_info:\n default_user:\n name: oxide"
.to_vec(),
network_interfaces: params::InstanceNetworkInterfaceAttachment::Default,
disks: vec![],
};

let error = NexusRequest::new(
RequestBuilder::new(client, Method::POST, &instances_url)
.body(Some(&instance))
.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\": memory must be divisible by {}",
ByteCount::from(params::MIN_MEMORY_SIZE_BYTES)
),
);
}

async fn instance_get(
client: &ClientTestContext,
instance_url: &str,
Expand Down
2 changes: 1 addition & 1 deletion nexus/tests/integration_tests/subnet_allocation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ async fn create_instance_expect_failure(
description: "".to_string(),
},
ncpus: InstanceCpuCount(1),
memory: ByteCount::from_mebibytes_u32(256),
memory: ByteCount::from_gibibytes_u32(1),
hostname: name.to_string(),
user_data: vec![],
network_interfaces,
Expand Down

0 comments on commit bf4a057

Please sign in to comment.