-
Notifications
You must be signed in to change notification settings - Fork 63
[nexus] Make project creation unwind safe, add tests #2087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b2a2ba1
f399ff7
66cf0c0
828ed21
2e0a199
533ee76
6abce08
169ed10
29a3ab5
3b62a6a
05cbd9d
f7f5fbe
2f89973
b9fef96
d68c763
9a8504b
79271d2
bfc6048
f9c635a
8c01fcd
666bf64
7b66888
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,6 @@ use super::NexusSaga; | |||||||||
| use crate::app::sagas; | ||||||||||
| use crate::app::sagas::declare_saga_actions; | ||||||||||
| use crate::context::OpContext; | ||||||||||
| use crate::db::lookup::LookupPath; | ||||||||||
| use crate::external_api::params; | ||||||||||
| use crate::{authn, authz, db}; | ||||||||||
| use nexus_defaults as defaults; | ||||||||||
|
|
@@ -33,6 +32,7 @@ declare_saga_actions! { | |||||||||
| project_create; | ||||||||||
| PROJECT_CREATE_RECORD -> "project" { | ||||||||||
| + spc_create_record | ||||||||||
| - spc_create_record_undo | ||||||||||
| } | ||||||||||
| PROJECT_CREATE_VPC_PARAMS -> "vpc_create_params" { | ||||||||||
| + spc_create_vpc_params | ||||||||||
|
|
@@ -74,7 +74,7 @@ impl NexusSaga for SagaProjectCreate { | |||||||||
|
|
||||||||||
| async fn spc_create_record( | ||||||||||
| sagactx: NexusActionContext, | ||||||||||
| ) -> Result<db::model::Project, ActionError> { | ||||||||||
| ) -> Result<(authz::Project, db::model::Project), ActionError> { | ||||||||||
| let osagactx = sagactx.user_data(); | ||||||||||
| let params = sagactx.saga_params::<Params>()?; | ||||||||||
| let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); | ||||||||||
|
|
@@ -88,25 +88,42 @@ async fn spc_create_record( | |||||||||
| .map_err(ActionError::action_failed) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| async fn spc_create_record_undo( | ||||||||||
| sagactx: NexusActionContext, | ||||||||||
| ) -> Result<(), anyhow::Error> { | ||||||||||
| let osagactx = sagactx.user_data(); | ||||||||||
| let params = sagactx.saga_params::<Params>()?; | ||||||||||
| let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); | ||||||||||
|
|
||||||||||
| let (_authz_project, project) = | ||||||||||
| sagactx.lookup::<(authz::Project, db::model::Project)>("project")?; | ||||||||||
|
Comment on lines
+98
to
+99
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Would this work?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're functionally the same; I was just being explicit about "what is the unused value" in this case since deserialization is particularly important on the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, to be pedantic in the context of
Technically in cases (1) and (2), since the bind is anonymous, it also goes out of scope, and However, I don't think So it's not exactly the same, but it's basically the same - I just used this for readability, since the types of values being pulled out of the |
||||||||||
|
|
||||||||||
| let (.., authz_project, project) = | ||||||||||
| db::lookup::LookupPath::new(&opctx, osagactx.datastore()) | ||||||||||
| .project_id(project.id()) | ||||||||||
| .fetch_for(authz::Action::Delete) | ||||||||||
| .await?; | ||||||||||
|
|
||||||||||
| osagactx | ||||||||||
| .datastore() | ||||||||||
| .project_delete(&opctx, &authz_project, &project) | ||||||||||
| .await?; | ||||||||||
| Ok(()) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| async fn spc_create_vpc_params( | ||||||||||
| sagactx: NexusActionContext, | ||||||||||
| ) -> Result<sagas::vpc_create::Params, ActionError> { | ||||||||||
| let osagactx = sagactx.user_data(); | ||||||||||
| let params = sagactx.saga_params::<Params>()?; | ||||||||||
| let opctx = OpContext::for_saga_action(&sagactx, ¶ms.serialized_authn); | ||||||||||
|
|
||||||||||
| let project_id = sagactx.lookup::<db::model::Project>("project")?.id(); | ||||||||||
| let (authz_project, _project) = | ||||||||||
| sagactx.lookup::<(authz::Project, db::model::Project)>("project")?; | ||||||||||
| let ipv6_prefix = Some( | ||||||||||
| defaults::random_vpc_ipv6_prefix() | ||||||||||
| .map_err(ActionError::action_failed)?, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| let (.., authz_project) = LookupPath::new(&opctx, osagactx.datastore()) | ||||||||||
| .project_id(project_id) | ||||||||||
| .lookup_for(authz::Action::CreateChild) | ||||||||||
| .await | ||||||||||
| .map_err(ActionError::action_failed)?; | ||||||||||
|
|
||||||||||
| let vpc_create = params::VpcCreate { | ||||||||||
| identity: IdentityMetadataCreateParams { | ||||||||||
| name: "default".parse().unwrap(), | ||||||||||
|
|
@@ -125,3 +142,167 @@ async fn spc_create_vpc_params( | |||||||||
| }; | ||||||||||
| Ok(saga_params) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| #[cfg(test)] | ||||||||||
| mod test { | ||||||||||
| use crate::{ | ||||||||||
| app::saga::create_saga_dag, app::sagas::project_create::Params, | ||||||||||
| app::sagas::project_create::SagaProjectCreate, authn::saga::Serialized, | ||||||||||
| authz, context::OpContext, db::datastore::DataStore, | ||||||||||
| 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::populate_ip_pool; | ||||||||||
| use nexus_test_utils_macros::nexus_test; | ||||||||||
| use omicron_common::api::external::IdentityMetadataCreateParams; | ||||||||||
| use omicron_common::api::external::NameOrId; | ||||||||||
| use uuid::Uuid; | ||||||||||
|
|
||||||||||
| type ControlPlaneTestContext = | ||||||||||
| nexus_test_utils::ControlPlaneTestContext<crate::Server>; | ||||||||||
|
|
||||||||||
| const ORG_NAME: &str = "test-org"; | ||||||||||
|
|
||||||||||
| async fn create_org(client: &ClientTestContext) -> Uuid { | ||||||||||
| populate_ip_pool(&client, "default", None).await; | ||||||||||
| let org = create_organization(&client, ORG_NAME).await; | ||||||||||
| org.identity.id | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Helper for creating project create parameters | ||||||||||
| fn new_test_params( | ||||||||||
| opctx: &OpContext, | ||||||||||
| authz_org: authz::Organization, | ||||||||||
| ) -> Params { | ||||||||||
| Params { | ||||||||||
| serialized_authn: Serialized::for_opctx(opctx), | ||||||||||
| project_create: params::ProjectCreate { | ||||||||||
| identity: IdentityMetadataCreateParams { | ||||||||||
| name: "my-project".parse().unwrap(), | ||||||||||
| description: "My Project".to_string(), | ||||||||||
| }, | ||||||||||
| }, | ||||||||||
| authz_org, | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| fn test_opctx(cptestctx: &ControlPlaneTestContext) -> OpContext { | ||||||||||
| OpContext::for_tests( | ||||||||||
| cptestctx.logctx.log.new(o!()), | ||||||||||
| cptestctx.server.apictx.nexus.datastore().clone(), | ||||||||||
| ) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| async fn get_authz_org( | ||||||||||
| cptestctx: &ControlPlaneTestContext, | ||||||||||
| org_id: Uuid, | ||||||||||
| action: authz::Action, | ||||||||||
| ) -> authz::Organization { | ||||||||||
| let nexus = &cptestctx.server.apictx.nexus; | ||||||||||
| let org_selector = | ||||||||||
| params::OrganizationSelector { organization: NameOrId::Id(org_id) }; | ||||||||||
| let opctx = test_opctx(&cptestctx); | ||||||||||
| let (.., authz_org) = nexus | ||||||||||
| .organization_lookup(&opctx, &org_selector) | ||||||||||
| .expect("Invalid parameters constructing organization lookup") | ||||||||||
| .lookup_for(action) | ||||||||||
| .await | ||||||||||
| .expect("Organization does not exist"); | ||||||||||
| authz_org | ||||||||||
| } | ||||||||||
|
|
||||||||||
| async fn verify_clean_slate(datastore: &DataStore) { | ||||||||||
| assert!(no_projects_exist(datastore).await); | ||||||||||
| crate::app::sagas::vpc_create::test::verify_clean_slate(datastore) | ||||||||||
| .await; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| async fn no_projects_exist(datastore: &DataStore) -> bool { | ||||||||||
| use crate::db::model::Project; | ||||||||||
| use crate::db::schema::project::dsl; | ||||||||||
|
|
||||||||||
| dsl::project | ||||||||||
| .filter(dsl::time_deleted.is_null()) | ||||||||||
| .select(Project::as_select()) | ||||||||||
| .first_async::<Project>(datastore.pool_for_tests().await.unwrap()) | ||||||||||
| .await | ||||||||||
| .optional() | ||||||||||
| .unwrap() | ||||||||||
| .map(|project| { | ||||||||||
| eprintln!("Project exists: {project:?}"); | ||||||||||
| }) | ||||||||||
| .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 org_id = create_org(&client).await; | ||||||||||
|
|
||||||||||
| // Before running the test, confirm we have no records of any projects. | ||||||||||
| verify_clean_slate(nexus.datastore()).await; | ||||||||||
|
|
||||||||||
| // Build the saga DAG with the provided test parameters | ||||||||||
| let opctx = test_opctx(&cptestctx); | ||||||||||
| let authz_org = | ||||||||||
| get_authz_org(&cptestctx, org_id, authz::Action::CreateChild).await; | ||||||||||
| let params = new_test_params(&opctx, authz_org); | ||||||||||
| let dag = create_saga_dag::<SagaProjectCreate>(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 org_id = create_org(&client).await; | ||||||||||
|
|
||||||||||
| // Build the saga DAG with the provided test parameters | ||||||||||
| let opctx = test_opctx(&cptestctx); | ||||||||||
| let authz_org = | ||||||||||
| get_authz_org(&cptestctx, org_id, authz::Action::CreateChild).await; | ||||||||||
| let params = new_test_params(&opctx, authz_org); | ||||||||||
| let dag = create_saga_dag::<SagaProjectCreate>(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; | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
Uh oh!
There was an error while loading. Please reload this page.