diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs index 1e40708b3a..1bb8981625 100644 --- a/nexus/db-queries/src/db/datastore/external_ip.rs +++ b/nexus/db-queries/src/db/datastore/external_ip.rs @@ -436,26 +436,19 @@ impl DataStore { &self, opctx: &OpContext, authz_fip: &authz::FloatingIp, - db_fip: &FloatingIp, ) -> DeleteResult { use db::schema::external_ip::dsl; - // Verify this FIP is not attached to any instances/services. - if db_fip.parent_id.is_some() { - return Err(Error::invalid_request( - "Floating IP cannot be deleted while attached to an instance", - )); - } - opctx.authorize(authz::Action::Delete, authz_fip).await?; let now = Utc::now(); - let updated_rows = diesel::update(dsl::external_ip) - .filter(dsl::id.eq(db_fip.id())) + let result = diesel::update(dsl::external_ip) + .filter(dsl::id.eq(authz_fip.id())) .filter(dsl::time_deleted.is_null()) .filter(dsl::parent_id.is_null()) .set(dsl::time_deleted.eq(now)) - .execute_async(&*self.pool_connection_authorized(opctx).await?) + .check_if_exists::(authz_fip.id()) + .execute_and_check(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel( @@ -464,12 +457,15 @@ impl DataStore { ) })?; - if updated_rows == 0 { - return Err(Error::invalid_request( - "deletion failed due to concurrent modification", - )); + match result.status { + // Verify this FIP is not attached to any instances/services. + UpdateStatus::NotUpdatedButExists if result.found.parent_id.is_some() => Err(Error::invalid_request( + "Floating IP cannot be deleted while attached to an instance", + )), + // Only remaining cause of `NotUpdated` is earlier soft-deletion. + // Return success in this case to maintain idempotency. + UpdateStatus::Updated | UpdateStatus::NotUpdatedButExists => Ok(()), } - Ok(()) } /// Attaches a Floating IP address to an instance. @@ -477,18 +473,10 @@ impl DataStore { &self, opctx: &OpContext, authz_fip: &authz::FloatingIp, - db_fip: &FloatingIp, instance_id: Uuid, ) -> UpdateResult { use db::schema::external_ip::dsl; - // Verify this FIP is not attached to any instances/services. - if db_fip.parent_id.is_some() { - return Err(Error::invalid_request( - "Floating IP cannot be attached to one instance while still attached to another", - )); - } - let (.., authz_instance, _db_instance) = LookupPath::new(&opctx, self) .instance_id(instance_id) .fetch_for(authz::Action::Modify) @@ -497,8 +485,10 @@ impl DataStore { opctx.authorize(authz::Action::Modify, authz_fip).await?; opctx.authorize(authz::Action::Modify, &authz_instance).await?; + let fip_id = authz_fip.id(); + let out = diesel::update(dsl::external_ip) - .filter(dsl::id.eq(db_fip.id())) + .filter(dsl::id.eq(fip_id)) .filter(dsl::kind.eq(IpKind::Floating)) .filter(dsl::time_deleted.is_null()) .filter(dsl::parent_id.is_null()) @@ -506,19 +496,23 @@ impl DataStore { dsl::parent_id.eq(Some(instance_id)), dsl::time_modified.eq(Utc::now()), )) - .returning(ExternalIp::as_returning()) - .get_result_async(&*self.pool_connection_authorized(opctx).await?) + .check_if_exists::(fip_id) + .execute_and_check(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel( e, ErrorHandler::NotFoundByResource(authz_fip), ) - }) - .and_then(|r| FloatingIp::try_from(r)) - .map_err(|e| Error::internal_error(&format!("{e}")))?; + })?; - Ok(out) + match (out.status, out.found.parent_id) { + (UpdateStatus::NotUpdatedButExists, Some(_)) => Err(Error::invalid_request( + "Floating IP cannot be attached to one instance while still attached to another", + )), + (UpdateStatus::Updated, _) => Ok(out.found.into()), + _ => unreachable!(), + } } /// Detaches a Floating IP address from an instance. @@ -526,37 +520,22 @@ impl DataStore { &self, opctx: &OpContext, authz_fip: &authz::FloatingIp, - db_fip: &FloatingIp, - target_instance_id: Option, - ) -> UpdateResult<(FloatingIp, Option)> { + instance_id: Uuid, + ) -> UpdateResult { use db::schema::external_ip::dsl; - let Some(instance_id) = db_fip.parent_id else { - return Err(Error::invalid_request( - "Floating IP is not attached to an instance", - )); - }; - - if let Some(target_instance_id) = target_instance_id { - if target_instance_id != instance_id { - return Err(Error::invalid_request( - "Floating IP is not attached to the target instance", - )); - } - } - - let (.., authz_instance, _db_instance) = LookupPath::new(&opctx, self) + let (.., authz_instance) = LookupPath::new(&opctx, self) .instance_id(instance_id) - .fetch_for(authz::Action::Modify) + .lookup_for(authz::Action::Modify) .await?; opctx.authorize(authz::Action::Modify, authz_fip).await?; opctx.authorize(authz::Action::Modify, &authz_instance).await?; - let i = self.instance_fetch_with_vmm(opctx, &authz_instance).await?; + let fip_id = authz_fip.id(); let out = diesel::update(dsl::external_ip) - .filter(dsl::id.eq(db_fip.id())) + .filter(dsl::id.eq(fip_id)) .filter(dsl::kind.eq(IpKind::Floating)) .filter(dsl::time_deleted.is_null()) .filter(dsl::parent_id.eq(instance_id)) @@ -564,18 +543,31 @@ impl DataStore { dsl::parent_id.eq(Option::::None), dsl::time_modified.eq(Utc::now()), )) - .returning(ExternalIp::as_returning()) - .get_result_async(&*self.pool_connection_authorized(opctx).await?) + .check_if_exists::(fip_id) + .execute_and_check(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel( e, ErrorHandler::NotFoundByResource(authz_fip), ) - }) - .and_then(|r| FloatingIp::try_from(r)) - .map_err(|e| Error::internal_error(&format!("{e}")))?; + })?; - Ok((out, i.sled_id())) + match (out.status, out.found.parent_id) { + (UpdateStatus::NotUpdatedButExists, Some(id)) + if id != instance_id => + { + Err(Error::invalid_request( + "Floating IP is not attached to the target instance", + )) + } + (UpdateStatus::NotUpdatedButExists, None) => { + Err(Error::invalid_request( + "Floating IP is not attached to an instance", + )) + } + (UpdateStatus::Updated, _) => Ok(out.found.into()), + _ => unreachable!(), + } } } diff --git a/nexus/src/app/external_ip.rs b/nexus/src/app/external_ip.rs index 404f597288..fba34f767d 100644 --- a/nexus/src/app/external_ip.rs +++ b/nexus/src/app/external_ip.rs @@ -115,9 +115,9 @@ impl super::Nexus { opctx: &OpContext, ip_lookup: lookup::FloatingIp<'_>, ) -> DeleteResult { - let (.., authz_fip, db_fip) = - ip_lookup.fetch_for(authz::Action::Delete).await?; + let (.., authz_fip) = + ip_lookup.lookup_for(authz::Action::Delete).await?; - self.db_datastore.floating_ip_delete(opctx, &authz_fip, &db_fip).await + self.db_datastore.floating_ip_delete(opctx, &authz_fip).await } } diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 89948d4db5..c5e7adcadc 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -634,15 +634,15 @@ async fn sic_allocate_instance_external_ip( // Set the parent of an existing floating IP to the new instance's ID. params::ExternalIpCreate::Floating { ref floating_ip_name } => { let floating_ip_name = db::model::Name(floating_ip_name.clone()); - let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore) + let (.., authz_fip) = LookupPath::new(&opctx, &datastore) .project_id(saga_params.project_id) .floating_ip_name(&floating_ip_name) - .fetch_for(authz::Action::Modify) + .lookup_for(authz::Action::Modify) .await .map_err(ActionError::action_failed)?; datastore - .floating_ip_attach(&opctx, &authz_fip, &db_fip, instance_id) + .floating_ip_attach(&opctx, &authz_fip, instance_id) .await .map_err(ActionError::action_failed)?; } @@ -674,18 +674,17 @@ async fn sic_allocate_instance_external_ip_undo( } params::ExternalIpCreate::Floating { floating_ip_name } => { let floating_ip_name = db::model::Name(floating_ip_name.clone()); - let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore) + let (.., authz_fip) = LookupPath::new(&opctx, &datastore) .project_id(saga_params.project_id) .floating_ip_name(&floating_ip_name) - .fetch_for(authz::Action::Modify) + .lookup_for(authz::Action::Modify) .await?; datastore .floating_ip_detach( &opctx, &authz_fip, - &db_fip, - Some(repeat_saga_params.instance_id), + repeat_saga_params.instance_id, ) .await?; } diff --git a/nexus/src/app/sagas/instance_ip_attach.rs b/nexus/src/app/sagas/instance_ip_attach.rs index b04a02bea9..5d01a4fc02 100644 --- a/nexus/src/app/sagas/instance_ip_attach.rs +++ b/nexus/src/app/sagas/instance_ip_attach.rs @@ -247,23 +247,18 @@ async fn siia_attach_ip( } // Set the parent of an existing floating IP to the new instance's ID. params::ExternalIpCreate::Floating { .. } => { - let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore) + let (.., authz_fip) = LookupPath::new(&opctx, &datastore) .floating_ip_id(new_ip_uuid) - .fetch_for(authz::Action::Modify) + .lookup_for(authz::Action::Modify) .await .map_err(ActionError::action_failed)?; let eip = datastore - .floating_ip_attach( - &opctx, - &authz_fip, - &db_fip, - params.instance.id(), - ) + .floating_ip_attach(&opctx, &authz_fip, params.instance.id()) .await .map_err(ActionError::action_failed)?; - Ok(ExternalIp::Floating(eip.ip.ip(), db_fip.id())) + Ok(ExternalIp::Floating(eip.ip.ip(), authz_fip.id())) } } } @@ -286,18 +281,13 @@ async fn siia_attach_ip_undo( datastore.deallocate_external_ip(&opctx, new_ip_uuid).await?; } params::ExternalIpCreate::Floating { .. } => { - let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore) + let (.., authz_fip) = LookupPath::new(&opctx, &datastore) .floating_ip_id(new_ip_uuid) - .fetch_for(authz::Action::Modify) + .lookup_for(authz::Action::Modify) .await?; datastore - .floating_ip_detach( - &opctx, - &authz_fip, - &db_fip, - Some(params.instance.id()), - ) + .floating_ip_detach(&opctx, &authz_fip, params.instance.id()) .await?; } } diff --git a/nexus/src/app/sagas/instance_ip_detach.rs b/nexus/src/app/sagas/instance_ip_detach.rs index a14d64643f..ac948ad364 100644 --- a/nexus/src/app/sagas/instance_ip_detach.rs +++ b/nexus/src/app/sagas/instance_ip_detach.rs @@ -332,19 +332,14 @@ async fn siid_detach_ip( .map_err(ActionError::action_failed)?; } params::ExternalIpDelete::Floating { .. } => { - let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore) + let (.., authz_fip) = LookupPath::new(&opctx, &datastore) .floating_ip_id(new_ip_uuid) - .fetch_for(authz::Action::Modify) + .lookup_for(authz::Action::Modify) .await .map_err(ActionError::action_failed)?; datastore - .floating_ip_detach( - &opctx, - &authz_fip, - &db_fip, - Some(params.instance.id()), - ) + .floating_ip_detach(&opctx, &authz_fip, params.instance.id()) .await .map_err(ActionError::action_failed)?; } @@ -391,19 +386,14 @@ async fn siid_detach_ip_undo( } // Set the parent of an existing floating IP to the new instance's ID. params::ExternalIpDelete::Floating { .. } => { - let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore) + let (.., authz_fip) = LookupPath::new(&opctx, &datastore) .floating_ip_id(new_ip_uuid) - .fetch_for(authz::Action::Modify) + .lookup_for(authz::Action::Modify) .await .map_err(ActionError::action_failed)?; let _eip = datastore - .floating_ip_attach( - &opctx, - &authz_fip, - &db_fip, - params.instance.id(), - ) + .floating_ip_attach(&opctx, &authz_fip, params.instance.id()) .await .map_err(ActionError::action_failed)?;