Skip to content

Commit

Permalink
clean up mess of ip pool test helpers. now pristine-ish
Browse files Browse the repository at this point in the history
  • Loading branch information
@david-crespo
david-crespo committed Dec 6, 2023
1 parent 1b71818 commit 75c0290
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 123 deletions.
20 changes: 8 additions & 12 deletions nexus/src/app/sagas/disk_create.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -836,10 +836,8 @@ pub(crate) mod test {
use diesel::{
ExpressionMethods, OptionalExtension, QueryDsl, SelectableHelper,
};
use dropshot::test_util::ClientTestContext;
use nexus_db_queries::context::OpContext;
use nexus_db_queries::{authn::saga::Serialized, db::datastore::DataStore};
use nexus_test_utils::resource_helpers::create_ip_pool;
use nexus_test_utils::resource_helpers::create_project;
use nexus_test_utils::resource_helpers::DiskTest;
use nexus_test_utils_macros::nexus_test;
Expand All @@ -855,12 +853,6 @@ pub(crate) mod test {
const DISK_NAME: &str = "my-disk";
const PROJECT_NAME: &str = "springfield-squidport";

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

pub fn new_disk_create_params() -> params::DiskCreate {
params::DiskCreate {
identity: IdentityMetadataCreateParams {
Expand Down Expand Up @@ -898,7 +890,8 @@ pub(crate) mod test {

let client = &cptestctx.external_client;
let nexus = &cptestctx.server.apictx().nexus;
let project_id = create_org_and_project(&client).await;
let project_id =
create_project(&client, PROJECT_NAME).await.identity.id;

// Build the saga DAG with the provided test parameters
let opctx = test_opctx(cptestctx);
Expand Down Expand Up @@ -1069,7 +1062,8 @@ pub(crate) mod test {

let client = &cptestctx.external_client;
let nexus = &cptestctx.server.apictx().nexus;
let project_id = create_org_and_project(&client).await;
let project_id =
create_project(&client, PROJECT_NAME).await.identity.id;
let opctx = test_opctx(cptestctx);

crate::app::sagas::test_helpers::action_failure_can_unwind::<
Expand Down Expand Up @@ -1098,7 +1092,8 @@ pub(crate) mod test {

let client = &cptestctx.external_client;
let nexus = &cptestctx.server.apictx.nexus;
let project_id = create_org_and_project(&client).await;
let project_id =
create_project(&client, PROJECT_NAME).await.identity.id;
let opctx = test_opctx(&cptestctx);

crate::app::sagas::test_helpers::action_failure_can_unwind_idempotently::<
Expand Down Expand Up @@ -1138,7 +1133,8 @@ pub(crate) mod test {

let client = &cptestctx.external_client;
let nexus = &cptestctx.server.apictx.nexus;
let project_id = create_org_and_project(&client).await;
let project_id =
create_project(&client, PROJECT_NAME).await.identity.id;

// Build the saga DAG with the provided test parameters
let opctx = test_opctx(&cptestctx);
Expand Down
13 changes: 2 additions & 11 deletions nexus/src/app/sagas/disk_delete.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,29 +171,20 @@ pub(crate) mod test {
app::saga::create_saga_dag, app::sagas::disk_delete::Params,
app::sagas::disk_delete::SagaDiskDelete,
};
use dropshot::test_util::ClientTestContext;
use nexus_db_model::Disk;
use nexus_db_queries::authn::saga::Serialized;
use nexus_db_queries::context::OpContext;
use nexus_test_utils::resource_helpers::create_ip_pool;
use nexus_test_utils::resource_helpers::create_project;
use nexus_test_utils::resource_helpers::DiskTest;
use nexus_test_utils_macros::nexus_test;
use nexus_types::external_api::params;
use omicron_common::api::external::Name;
use uuid::Uuid;

type ControlPlaneTestContext =
nexus_test_utils::ControlPlaneTestContext<crate::Server>;

const PROJECT_NAME: &str = "springfield-squidport";

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

pub fn test_opctx(cptestctx: &ControlPlaneTestContext) -> OpContext {
OpContext::for_tests(
cptestctx.logctx.log.new(o!()),
Expand Down Expand Up @@ -229,7 +220,7 @@ pub(crate) mod test {

let client = &cptestctx.external_client;
let nexus = &cptestctx.server.apictx.nexus;
let project_id = create_org_and_project(&client).await;
let project_id = create_project(client, PROJECT_NAME).await.identity.id;
let disk = create_disk(&cptestctx).await;

// Build the saga DAG with the provided test parameters
Expand All @@ -255,7 +246,7 @@ pub(crate) mod test {

let client = &cptestctx.external_client;
let nexus = &cptestctx.server.apictx.nexus;
let project_id = create_org_and_project(&client).await;
let project_id = create_project(client, PROJECT_NAME).await.identity.id;
let disk = create_disk(&cptestctx).await;

// Build the saga DAG with the provided test parameters
Expand Down
30 changes: 0 additions & 30 deletions nexus/src/app/sagas/snapshot_create.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1554,7 +1554,6 @@ mod test {

use crate::app::saga::create_saga_dag;
use crate::app::sagas::test_helpers;
use crate::external_api::shared::IpRange;
use async_bb8_diesel::AsyncRunQueryDsl;
use diesel::{
ExpressionMethods, OptionalExtension, QueryDsl, SelectableHelper,
Expand All @@ -1568,7 +1567,6 @@ mod test {
use nexus_test_utils::resource_helpers::create_project;
use nexus_test_utils::resource_helpers::delete_disk;
use nexus_test_utils::resource_helpers::object_create;
use nexus_test_utils::resource_helpers::populate_ip_pool;
use nexus_test_utils::resource_helpers::DiskTest;
use nexus_test_utils_macros::nexus_test;
use nexus_types::external_api::params::InstanceDiskAttachment;
Expand All @@ -1580,7 +1578,6 @@ mod test {
use omicron_common::api::external::NameOrId;
use sled_agent_client::types::CrucibleOpts;
use sled_agent_client::TestInterfaces as SledAgentTestInterfaces;
use std::net::Ipv4Addr;
use std::str::FromStr;

#[test]
Expand Down Expand Up @@ -2042,22 +2039,6 @@ mod test {
// before the first attempt to run the saga recreates it.
delete_disk(client, PROJECT_NAME, DISK_NAME).await;

// The no-pantry variant of the test needs to see the disk attached to
// an instance. Set up an IP pool so that instances can be created
// against it.
if !use_the_pantry {
populate_ip_pool(
&client,
"default",
IpRange::try_from((
Ipv4Addr::new(10, 1, 0, 0),
Ipv4Addr::new(10, 1, 255, 255),
))
.unwrap(),
)
.await;
}

crate::app::sagas::test_helpers::action_failure_can_unwind::<
SagaSnapshotCreate,
_,
Expand Down Expand Up @@ -2352,17 +2333,6 @@ mod test {
assert!(output.is_err());

// Attach the disk to an instance, then rerun the saga
populate_ip_pool(
&client,
"default",
IpRange::try_from((
Ipv4Addr::new(10, 1, 0, 0),
Ipv4Addr::new(10, 1, 255, 255),
))
.unwrap(),
)
.await;

let instance_state = setup_test_instance(
cptestctx,
client,
Expand Down
46 changes: 20 additions & 26 deletions nexus/test-utils/src/resource_helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,6 @@ pub async fn object_delete(client: &ClientTestContext, path: &str) {
});
}

pub async fn populate_ip_pool(
client: &ClientTestContext,
pool_name: &str,
ip_range: IpRange,
) -> IpPoolRange {
let url = format!("/v1/system/ip-pools/{}/ranges/add", pool_name);
object_create(client, &url, &ip_range).await
}

/// Create an IP pool with a single range for testing.
///
/// The IP range may be specified if it's important for testing the behavior
Expand All @@ -123,9 +114,6 @@ pub async fn create_ip_pool(
client: &ClientTestContext,
pool_name: &str,
ip_range: Option<IpRange>,
// TODO: could change this to is_default -- always associate with default
// silo, let caller decide whether it's the default
silo_link: Option<params::IpPoolSiloLink>,
) -> (IpPool, IpPoolRange) {
let pool = object_create(
client,
Expand All @@ -139,14 +127,6 @@ pub async fn create_ip_pool(
)
.await;

if let Some(silo_link) = silo_link {
let url = format!("/v1/system/ip-pools/{pool_name}/silos");
object_create::<params::IpPoolSiloLink, views::IpPoolSilo>(
client, &url, &silo_link,
)
.await;
}

let ip_range = ip_range.unwrap_or_else(|| {
use std::net::Ipv4Addr;
IpRange::try_from((
Expand All @@ -155,18 +135,32 @@ pub async fn create_ip_pool(
))
.unwrap()
});
let range = populate_ip_pool(client, pool_name, ip_range).await;
let url = format!("/v1/system/ip-pools/{}/ranges/add", pool_name);
let range = object_create(client, &url, &ip_range).await;
(pool, range)
}

pub async fn link_ip_pool(
client: &ClientTestContext,
pool_name: &str,
silo_id: &Uuid,
is_default: bool,
) {
let link =
params::IpPoolSiloLink { silo: NameOrId::Id(*silo_id), is_default };
let url = format!("/v1/system/ip-pools/{pool_name}/silos");
object_create::<params::IpPoolSiloLink, views::IpPoolSilo>(
client, &url, &link,
)
.await;
}

/// What you want for any test that is not testing IP logic specifically
pub async fn create_default_ip_pool(
client: &ClientTestContext,
) -> views::IpPool {
let link = params::IpPoolSiloLink {
silo: NameOrId::Id(DEFAULT_SILO.id()),
is_default: true,
};
let (pool, ..) = create_ip_pool(&client, "default", None, Some(link)).await;
let (pool, ..) = create_ip_pool(&client, "default", None).await;
link_ip_pool(&client, "default", &DEFAULT_SILO.id(), true).await;
pool
}

Expand Down
55 changes: 11 additions & 44 deletions nexus/tests/integration_tests/instances.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ use nexus_test_utils::resource_helpers::create_ip_pool;
use nexus_test_utils::resource_helpers::create_local_user;
use nexus_test_utils::resource_helpers::create_silo;
use nexus_test_utils::resource_helpers::grant_iam;
use nexus_test_utils::resource_helpers::link_ip_pool;
use nexus_test_utils::resource_helpers::object_create;
use nexus_test_utils::resource_helpers::objects_list_page_authz;
use nexus_test_utils::resource_helpers::populate_ip_pool;
use nexus_test_utils::resource_helpers::DiskTest;
use nexus_test_utils::start_sled_agent;
use nexus_types::external_api::shared::IpKind;
Expand Down Expand Up @@ -3575,14 +3575,12 @@ async fn test_instance_ephemeral_ip_from_correct_pool(
);

// make first pool the default for the priv user's silo
dbg!(DEFAULT_SILO.id());
let silo = NameOrId::Id(DEFAULT_SILO.id());
let link = params::IpPoolSiloLink { silo: silo.clone(), is_default: true };
create_ip_pool(&client, "pool1", Some(range1), Some(link)).await;
create_ip_pool(&client, "pool1", Some(range1)).await;
link_ip_pool(&client, "pool1", &DEFAULT_SILO.id(), /*default*/ true).await;

// second pool is associated with the silo but not default
let link = params::IpPoolSiloLink { silo: silo.clone(), is_default: false };
create_ip_pool(&client, "pool2", Some(range2), Some(link)).await;
create_ip_pool(&client, "pool2", Some(range2)).await;
link_ip_pool(&client, "pool2", &DEFAULT_SILO.id(), /*default*/ false).await;

// Create an instance with pool name blank, expect IP from default pool
create_instance_with_pool(client, "pool1-inst", None).await;
Expand All @@ -3605,7 +3603,7 @@ async fn test_instance_ephemeral_ip_from_correct_pool(
let _: views::IpPoolSilo = object_create(
client,
"/v1/system/ip-pools/pool2/make-default",
&params::SiloSelector { silo: silo.clone() },
&params::SiloSelector { silo: NameOrId::Id(DEFAULT_SILO.id()) },
)
.await;

Expand All @@ -3627,37 +3625,9 @@ async fn test_instance_ephemeral_ip_from_orphan_pool(

let _ = create_project(&client, PROJECT_NAME).await;

// have to give default pool a range or snat IP allocation fails before it
// can get to failing on ephemeral IP allocation
let default_pool_range = IpRange::V4(
Ipv4Range::new(
std::net::Ipv4Addr::new(10, 0, 0, 1),
std::net::Ipv4Addr::new(10, 0, 0, 5),
)
.unwrap(),
);

// make first pool the default for the priv user's silo
let link = params::IpPoolSiloLink {
silo: NameOrId::Id(DEFAULT_SILO.id()),
is_default: true,
};
create_ip_pool(&client, "default", Some(default_pool_range), Some(link))
.await;

// don't use create_ip_pool because it automatically associates with a pool
let pool_name = "orphan-pool";
let _: views::IpPool = object_create(
client,
"/v1/system/ip-pools",
&params::IpPoolCreate {
identity: IdentityMetadataCreateParams {
name: String::from(pool_name).parse().unwrap(),
description: String::from("an ip pool"),
},
},
)
.await;
create_ip_pool(&client, "default", None).await;
link_ip_pool(&client, "default", &DEFAULT_SILO.id(), true).await;

let orphan_pool_range = IpRange::V4(
Ipv4Range::new(
Expand All @@ -3666,7 +3636,7 @@ async fn test_instance_ephemeral_ip_from_orphan_pool(
)
.unwrap(),
);
populate_ip_pool(client, pool_name, orphan_pool_range).await;
create_ip_pool(&client, "orphan-pool", Some(orphan_pool_range)).await;

// this should 404
let instance_name = "orphan-pool-inst";
Expand Down Expand Up @@ -3794,11 +3764,8 @@ async fn test_instance_create_in_silo(cptestctx: &ControlPlaneTestContext) {
.await;

// can't use create_default_ip_pool because we need to link to the silo we just made
let link = params::IpPoolSiloLink {
silo: NameOrId::Id(silo.identity.id),
is_default: true,
};
create_ip_pool(&client, "default", None, Some(link)).await;
create_ip_pool(&client, "default", None).await;
link_ip_pool(&client, "default", &silo.identity.id, true).await;

// Create test projects
NexusRequest::objects_post(
Expand Down

0 comments on commit 75c0290

Please sign in to comment.