diff --git a/nexus/db-model/src/ipv4net.rs b/nexus/db-model/src/ipv4net.rs index c5b45e4064f..8c868b3b647 100644 --- a/nexus/db-model/src/ipv4net.rs +++ b/nexus/db-model/src/ipv4net.rs @@ -10,9 +10,20 @@ use diesel::sql_types; use ipnetwork::IpNetwork; use omicron_common::api::external; use omicron_common::nexus_config::NUM_INITIAL_RESERVED_IP_ADDRESSES; +use serde::Deserialize; +use serde::Serialize; use std::net::Ipv4Addr; -#[derive(Clone, Copy, Debug, PartialEq, AsExpression, FromSqlRow)] +#[derive( + Clone, + Copy, + Debug, + PartialEq, + AsExpression, + FromSqlRow, + Serialize, + Deserialize, +)] #[diesel(sql_type = sql_types::Inet)] pub struct Ipv4Net(pub external::Ipv4Net); diff --git a/nexus/db-model/src/vpc_subnet.rs b/nexus/db-model/src/vpc_subnet.rs index a572f6f535e..2cc74c177bf 100644 --- a/nexus/db-model/src/vpc_subnet.rs +++ b/nexus/db-model/src/vpc_subnet.rs @@ -14,10 +14,21 @@ use nexus_types::external_api::params; use nexus_types::external_api::views; use nexus_types::identity::Resource; use omicron_common::api::external; +use serde::Deserialize; +use serde::Serialize; use std::net::IpAddr; use uuid::Uuid; -#[derive(Queryable, Insertable, Clone, Debug, Selectable, Resource)] +#[derive( + Queryable, + Insertable, + Clone, + Debug, + Selectable, + Resource, + Serialize, + Deserialize, +)] #[diesel(table_name = vpc_subnet)] pub struct VpcSubnet { #[diesel(embed)] diff --git a/nexus/src/app/sagas/vpc_create.rs b/nexus/src/app/sagas/vpc_create.rs index 854cc8cfb37..5a03b4627bf 100644 --- a/nexus/src/app/sagas/vpc_create.rs +++ b/nexus/src/app/sagas/vpc_create.rs @@ -8,13 +8,13 @@ use super::NexusSaga; use super::ACTION_GENERATE_ID; use crate::app::sagas::declare_saga_actions; use crate::context::OpContext; -use crate::db::model::VpcRouterKind; use crate::db::queries::vpc_subnet::SubnetError; use crate::external_api::params; use crate::{authn, authz, db}; use nexus_defaults as defaults; use omicron_common::api::external; use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::LookupType; use omicron_common::api::external::RouteDestination; use omicron_common::api::external::RouteTarget; use omicron_common::api::external::RouterRouteCreateParams; @@ -40,18 +40,23 @@ declare_saga_actions! { vpc_create; VPC_CREATE_VPC -> "vpc" { + svc_create_vpc + - svc_create_vpc_undo } VPC_CREATE_ROUTER -> "router" { + svc_create_router + - svc_create_router_undo } VPC_CREATE_ROUTE -> "route" { + svc_create_route + - svc_create_route_undo } VPC_CREATE_SUBNET -> "subnet" { + svc_create_subnet + - svc_create_subnet_undo } VPC_UPDATE_FIREWALL -> "firewall" { + svc_update_firewall + - svc_update_firewall_undo } VPC_NOTIFY_SLEDS -> "no_result" { + svc_notify_sleds @@ -124,11 +129,6 @@ async fn svc_create_vpc( let vpc_id = sagactx.lookup::("vpc_id")?; let system_router_id = sagactx.lookup::("system_router_id")?; - // TODO: This is both fake and utter nonsense. It should be eventually - // replaced with the proper behavior for creating the default route - // which may not even happen here. Creating the vpc, its system router, - // and that routers default route should all be a part of the same - // transaction. let vpc = db::model::IncompleteVpc::new( vpc_id, params.authz_project.id(), @@ -144,6 +144,21 @@ async fn svc_create_vpc( Ok((authz_vpc, db_vpc)) } +async fn svc_create_vpc_undo( + sagactx: NexusActionContext, +) -> Result<(), anyhow::Error> { + let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; + let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); + let (authz_vpc, db_vpc) = + sagactx.lookup::<(authz::Vpc, db::model::Vpc)>("vpc")?; + osagactx + .datastore() + .project_delete_vpc(&opctx, &db_vpc, &authz_vpc) + .await?; + Ok(()) +} + async fn svc_create_router( sagactx: NexusActionContext, ) -> Result { @@ -162,7 +177,7 @@ async fn svc_create_router( let router = db::model::VpcRouter::new( system_router_id, vpc_id, - VpcRouterKind::System, + db::model::VpcRouterKind::System, params::VpcRouterCreate { identity: IdentityMetadataCreateParams { name: "system".parse().unwrap(), @@ -180,6 +195,18 @@ async fn svc_create_router( Ok(authz_router) } +async fn svc_create_router_undo( + sagactx: NexusActionContext, +) -> Result<(), anyhow::Error> { + let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; + let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); + let authz_router = sagactx.lookup::("router")?; + + osagactx.datastore().vpc_delete_router(&opctx, &authz_router).await?; + Ok(()) +} + async fn svc_create_route( sagactx: NexusActionContext, ) -> Result<(), ActionError> { @@ -214,9 +241,26 @@ async fn svc_create_route( Ok(()) } +async fn svc_create_route_undo( + sagactx: NexusActionContext, +) -> Result<(), anyhow::Error> { + let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; + let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); + let authz_router = sagactx.lookup::("router")?; + let route_id = sagactx.lookup::("default_route_id")?; + let authz_route = authz::RouterRoute::new( + authz_router, + route_id, + LookupType::ById(route_id), + ); + osagactx.datastore().router_delete_route(&opctx, &authz_route).await?; + Ok(()) +} + async fn svc_create_subnet( sagactx: NexusActionContext, -) -> Result<(), ActionError> { +) -> Result<(authz::VpcSubnet, db::model::VpcSubnet), ActionError> { let osagactx = sagactx.user_data(); let params = sagactx.saga_params::()?; let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); @@ -284,8 +328,23 @@ async fn svc_create_subnet( } SubnetError::External(e) => e, }) - .map_err(ActionError::action_failed)?; + .map_err(ActionError::action_failed) +} + +async fn svc_create_subnet_undo( + sagactx: NexusActionContext, +) -> Result<(), anyhow::Error> { + let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; + let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); + + let (authz_subnet, db_subnet) = + sagactx.lookup::<(authz::VpcSubnet, db::model::VpcSubnet)>("subnet")?; + osagactx + .datastore() + .vpc_delete_subnet(&opctx, &db_subnet, &authz_subnet) + .await?; Ok(()) } @@ -315,6 +374,21 @@ async fn svc_update_firewall( Ok(rules) } +async fn svc_update_firewall_undo( + sagactx: NexusActionContext, +) -> Result<(), anyhow::Error> { + let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; + let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); + let (authz_vpc, _) = + sagactx.lookup::<(authz::Vpc, db::model::Vpc)>("vpc")?; + osagactx + .datastore() + .vpc_update_firewall_rules(&opctx, &authz_vpc, vec![]) + .await?; + Ok(()) +} + async fn svc_notify_sleds( sagactx: NexusActionContext, ) -> Result<(), ActionError> { @@ -333,3 +407,323 @@ async fn svc_notify_sleds( Ok(()) } + +#[cfg(test)] +mod test { + use crate::{ + app::saga::create_saga_dag, app::sagas::vpc_create::Params, + app::sagas::vpc_create::SagaVpcCreate, authn::saga::Serialized, authz, + context::OpContext, db::datastore::DataStore, db::lookup::LookupPath, + external_api::params, + }; + use async_bb8_diesel::{AsyncRunQueryDsl, OptionalExtension}; + use diesel::{ExpressionMethods, QueryDsl, SelectableHelper}; + use dropshot::test_util::ClientTestContext; + use nexus_test_utils::resource_helpers::create_organization; + use nexus_test_utils::resource_helpers::create_project; + use nexus_test_utils::resource_helpers::populate_ip_pool; + use nexus_test_utils_macros::nexus_test; + use omicron_common::api::external::IdentityMetadataCreateParams; + use omicron_common::api::external::Name; + use omicron_common::api::external::NameOrId; + use uuid::Uuid; + + type ControlPlaneTestContext = + nexus_test_utils::ControlPlaneTestContext; + + const ORG_NAME: &str = "test-org"; + const PROJECT_NAME: &str = "springfield-squidport"; + + async fn create_org_and_project(client: &ClientTestContext) -> Uuid { + populate_ip_pool(&client, "default", None).await; + create_organization(&client, ORG_NAME).await; + let project = create_project(client, ORG_NAME, PROJECT_NAME).await; + project.identity.id + } + + // Helper for creating VPC create parameters + fn new_test_params( + opctx: &OpContext, + authz_project: authz::Project, + ) -> Params { + Params { + serialized_authn: Serialized::for_opctx(opctx), + vpc_create: params::VpcCreate { + identity: IdentityMetadataCreateParams { + name: "my-vpc".parse().unwrap(), + description: "My VPC".to_string(), + }, + ipv6_prefix: None, + dns_name: "abc".parse().unwrap(), + }, + authz_project, + } + } + + fn test_opctx(cptestctx: &ControlPlaneTestContext) -> OpContext { + OpContext::for_tests( + cptestctx.logctx.log.new(o!()), + cptestctx.server.apictx.nexus.datastore().clone(), + ) + } + + async fn get_authz_project( + cptestctx: &ControlPlaneTestContext, + project_id: Uuid, + action: authz::Action, + ) -> authz::Project { + let nexus = &cptestctx.server.apictx.nexus; + let project_selector = + params::ProjectSelector::new(None, NameOrId::Id(project_id)); + let opctx = test_opctx(&cptestctx); + let (.., authz_project) = nexus + .project_lookup(&opctx, &project_selector) + .expect("Invalid parameters constructing project lookup") + .lookup_for(action) + .await + .expect("Project does not exist"); + authz_project + } + + async fn delete_project_vpc_defaults( + cptestctx: &ControlPlaneTestContext, + project_id: Uuid, + ) { + let opctx = test_opctx(&cptestctx); + let datastore = cptestctx.server.apictx.nexus.datastore(); + let default_name = Name::try_from("default".to_string()).unwrap(); + let system_name = Name::try_from("system".to_string()).unwrap(); + + // Default Subnet + let (.., authz_subnet, subnet) = LookupPath::new(&opctx, &datastore) + .project_id(project_id) + .vpc_name(&default_name.clone().into()) + .vpc_subnet_name(&default_name.clone().into()) + .fetch() + .await + .expect("Failed to fetch default Subnet"); + datastore + .vpc_delete_subnet(&opctx, &subnet, &authz_subnet) + .await + .expect("Failed to delete default Subnet"); + + // Default route + let (.., authz_route, _route) = LookupPath::new(&opctx, &datastore) + .project_id(project_id) + .vpc_name(&default_name.clone().into()) + .vpc_router_name(&system_name.clone().into()) + .router_route_name(&default_name.clone().into()) + .fetch() + .await + .expect("Failed to fetch default route"); + datastore + .router_delete_route(&opctx, &authz_route) + .await + .expect("Failed to delete default route"); + + // System router + let (.., authz_router, _router) = LookupPath::new(&opctx, &datastore) + .project_id(project_id) + .vpc_name(&default_name.clone().into()) + .vpc_router_name(&system_name.into()) + .fetch() + .await + .expect("Failed to fetch system router"); + datastore + .vpc_delete_router(&opctx, &authz_router) + .await + .expect("Failed to delete system router"); + + // Default VPC & Firewall Rules + let (.., authz_vpc, vpc) = LookupPath::new(&opctx, &datastore) + .project_id(project_id) + .vpc_name(&default_name.into()) + .fetch() + .await + .expect("Failed to fetch default VPC"); + datastore + .vpc_delete_all_firewall_rules(&opctx, &authz_vpc) + .await + .expect("Failed to delete all firewall rules for VPC"); + datastore + .project_delete_vpc(&opctx, &vpc, &authz_vpc) + .await + .expect("Failed to delete VPC"); + } + + async fn verify_clean_slate(datastore: &DataStore) { + assert!(no_vpcs_exist(datastore).await); + assert!(no_routers_exist(datastore).await); + assert!(no_routes_exist(datastore).await); + assert!(no_subnets_exist(datastore).await); + assert!(no_firewall_rules_exist(datastore).await); + } + + async fn no_vpcs_exist(datastore: &DataStore) -> bool { + use crate::db::model::Vpc; + use crate::db::schema::vpc::dsl; + + dsl::vpc + .filter(dsl::time_deleted.is_null()) + .select(Vpc::as_select()) + .first_async::(datastore.pool_for_tests().await.unwrap()) + .await + .optional() + .unwrap() + .map(|vpc| { + eprintln!("VPC exists: {vpc:?}"); + }) + .is_none() + } + + async fn no_routers_exist(datastore: &DataStore) -> bool { + use crate::db::model::VpcRouter; + use crate::db::schema::vpc_router::dsl; + + dsl::vpc_router + .filter(dsl::time_deleted.is_null()) + .select(VpcRouter::as_select()) + .first_async::(datastore.pool_for_tests().await.unwrap()) + .await + .optional() + .unwrap() + .map(|router| { + eprintln!("Router exists: {router:?}"); + }) + .is_none() + } + + async fn no_routes_exist(datastore: &DataStore) -> bool { + use crate::db::model::RouterRoute; + use crate::db::schema::router_route::dsl; + + dsl::router_route + .filter(dsl::time_deleted.is_null()) + .select(RouterRoute::as_select()) + .first_async::( + datastore.pool_for_tests().await.unwrap(), + ) + .await + .optional() + .unwrap() + .map(|route| { + eprintln!("Route exists: {route:?}"); + }) + .is_none() + } + + async fn no_subnets_exist(datastore: &DataStore) -> bool { + use crate::db::model::VpcSubnet; + use crate::db::schema::vpc_subnet::dsl; + + dsl::vpc_subnet + .filter(dsl::time_deleted.is_null()) + .select(VpcSubnet::as_select()) + .first_async::(datastore.pool_for_tests().await.unwrap()) + .await + .optional() + .unwrap() + .map(|subnet| { + eprintln!("Subnet exists: {subnet:?}"); + }) + .is_none() + } + + async fn no_firewall_rules_exist(datastore: &DataStore) -> bool { + use crate::db::model::VpcFirewallRule; + use crate::db::schema::vpc_firewall_rule::dsl; + + dsl::vpc_firewall_rule + .filter(dsl::time_deleted.is_null()) + .select(VpcFirewallRule::as_select()) + .first_async::( + datastore.pool_for_tests().await.unwrap(), + ) + .await + .optional() + .unwrap() + .map(|rule| { + eprintln!("Firewall rule exists: {rule:?}"); + }) + .is_none() + } + + #[nexus_test(server = crate::Server)] + async fn test_saga_basic_usage_succeeds( + cptestctx: &ControlPlaneTestContext, + ) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; + let project_id = create_org_and_project(&client).await; + delete_project_vpc_defaults(&cptestctx, project_id).await; + + // Before running the test, confirm we have no records of any VPCs. + verify_clean_slate(nexus.datastore()).await; + + // Build the saga DAG with the provided test parameters + let opctx = test_opctx(&cptestctx); + let authz_project = get_authz_project( + &cptestctx, + project_id, + authz::Action::CreateChild, + ) + .await; + let params = new_test_params(&opctx, authz_project); + let dag = create_saga_dag::(params).unwrap(); + let runnable_saga = nexus.create_runnable_saga(dag).await.unwrap(); + + // Actually run the saga + nexus.run_saga(runnable_saga).await.unwrap(); + } + + #[nexus_test(server = crate::Server)] + async fn test_action_failure_can_unwind( + cptestctx: &ControlPlaneTestContext, + ) { + let log = &cptestctx.logctx.log; + + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; + let project_id = create_org_and_project(&client).await; + delete_project_vpc_defaults(&cptestctx, project_id).await; + + // Build the saga DAG with the provided test parameters + let opctx = test_opctx(&cptestctx); + let authz_project = get_authz_project( + &cptestctx, + project_id, + authz::Action::CreateChild, + ) + .await; + let params = new_test_params(&opctx, authz_project); + let dag = create_saga_dag::(params).unwrap(); + + for node in dag.get_nodes() { + // Create a new saga for this node. + info!( + log, + "Creating new saga which will fail at index {:?}", node.index(); + "node_name" => node.name().as_ref(), + "label" => node.label(), + ); + + let runnable_saga = + nexus.create_runnable_saga(dag.clone()).await.unwrap(); + + // Inject an error instead of running the node. + // + // This should cause the saga to unwind. + nexus + .sec() + .saga_inject_error(runnable_saga.id(), node.index()) + .await + .unwrap(); + nexus + .run_saga(runnable_saga) + .await + .expect_err("Saga should have failed"); + + verify_clean_slate(nexus.datastore()).await; + } + } +} diff --git a/nexus/src/app/vpc_subnet.rs b/nexus/src/app/vpc_subnet.rs index 144e8abf793..7e5d9840de3 100644 --- a/nexus/src/app/vpc_subnet.rs +++ b/nexus/src/app/vpc_subnet.rs @@ -155,7 +155,7 @@ impl super::Nexus { Err(result.unwrap_err().into_external()) } Err(SubnetError::External(e)) => Err(e), - Ok(subnet) => Ok(subnet), + Ok((.., subnet)) => Ok(subnet), } } Some(ipv6_block) => { @@ -178,6 +178,7 @@ impl super::Nexus { self.db_datastore .vpc_create_subnet(opctx, &authz_vpc, subnet) .await + .map(|(.., subnet)| subnet) .map_err(SubnetError::into_external) } } diff --git a/nexus/src/db/datastore/vpc.rs b/nexus/src/db/datastore/vpc.rs index 181743e3383..ba951d7730d 100644 --- a/nexus/src/db/datastore/vpc.rs +++ b/nexus/src/db/datastore/vpc.rs @@ -283,6 +283,7 @@ impl DataStore { .filter(dsl::vpc_id.eq(authz_vpc.id())) .set(dsl::time_deleted.eq(now)); + let rules_is_empty = rules.is_empty(); let insert_new_query = Vpc::insert_resource( authz_vpc.id(), diesel::insert_into(dsl::vpc_firewall_rule).values(rules), @@ -306,6 +307,9 @@ impl DataStore { // The generation count update on the vpc table row will take a // write lock on the row, ensuring that the vpc was not deleted // concurently. + if rules_is_empty { + return Ok(vec![]); + } insert_new_query .insert_and_get_results_async(&conn) .await @@ -379,14 +383,22 @@ impl DataStore { opctx: &OpContext, authz_vpc: &authz::Vpc, subnet: VpcSubnet, - ) -> Result { + ) -> Result<(authz::VpcSubnet, VpcSubnet), SubnetError> { opctx .authorize(authz::Action::CreateChild, authz_vpc) .await .map_err(SubnetError::External)?; assert_eq!(authz_vpc.id(), subnet.vpc_id); - self.vpc_create_subnet_raw(subnet).await + let db_subnet = self.vpc_create_subnet_raw(subnet).await?; + Ok(( + authz::VpcSubnet::new( + authz_vpc.clone(), + db_subnet.id(), + LookupType::ById(db_subnet.id()), + ), + db_subnet, + )) } pub(crate) async fn vpc_create_subnet_raw(