From ba86368591cba6b1a5a0cf275d53424217af263d Mon Sep 17 00:00:00 2001 From: snow flurry Date: Mon, 19 Aug 2024 12:00:02 -0700 Subject: [PATCH] nzr-api, et al: implement a serializable ApiError This replaces all the API functions that returned Result. Additionally, ToApiResult and Simplify make converting errors to ApiError easier than with String. --- client/src/main.rs | 17 ++- nzr-api/src/error.rs | 208 +++++++++++++++++++++++++++++++++++++ nzr-api/src/lib.rs | 28 ++--- nzr-api/src/mock/client.rs | 6 +- nzr-api/src/mock/mod.rs | 39 ++++--- nzr-api/src/model.rs | 7 +- nzrd/src/cmd/mod.rs | 22 ---- nzrd/src/cmd/net.rs | 41 -------- nzrd/src/cmd/vm.rs | 89 ++++++++++------ nzrd/src/model/mod.rs | 13 ++- nzrd/src/rpc.rs | 147 +++++++++++++++----------- omyacid/src/ctx.rs | 2 +- omyacid/src/main.rs | 1 - 13 files changed, 410 insertions(+), 210 deletions(-) create mode 100644 nzr-api/src/error.rs delete mode 100644 nzrd/src/cmd/net.rs diff --git a/client/src/main.rs b/client/src/main.rs index ba88472..62a964c 100644 --- a/client/src/main.rs +++ b/client/src/main.rs @@ -1,5 +1,6 @@ use clap::{CommandFactory, FromArgMatches, Parser, Subcommand}; use nzr_api::config; +use nzr_api::error::Simplify; use nzr_api::hickory_proto::rr::Name; use nzr_api::model; use nzr_api::net::cidr::CidrV4; @@ -332,9 +333,9 @@ async fn handle_command() -> Result<(), Box> { let mut net = client .get_subnets(nzr_api::default_ctx()) .await - .map_err(|e| e.to_string()) + .simplify() .and_then(|res| { - res?.iter() + res.iter() .find_map(|ent| { if ent.name == args.name { Some(ent.clone()) @@ -342,7 +343,7 @@ async fn handle_command() -> Result<(), Box> { None } }) - .ok_or_else(|| format!("Couldn't find network {}", &args.name)) + .ok_or_else(|| format!("Couldn't find network {}", &args.name).into()) })?; // merge in the new args @@ -365,15 +366,11 @@ async fn handle_command() -> Result<(), Box> { } // run the update - client + let net = client .modify_subnet(nzr_api::default_ctx(), net) .await - .map_err(|err| format!("RPC error: {}", err)) - .and_then(|res| { - res.map(|e| { - println!("Subnet {} updated.", e.name); - }) - })?; + .simplify()?; + println!("Subnet {} updated.", net.name); } NetCmd::Dump { name } => { let subnets = (client.get_subnets(nzr_api::default_ctx()).await?)?; diff --git a/nzr-api/src/error.rs b/nzr-api/src/error.rs new file mode 100644 index 0000000..eee5708 --- /dev/null +++ b/nzr-api/src/error.rs @@ -0,0 +1,208 @@ +use std::fmt; + +use serde::{Deserialize, Serialize}; +use tarpc::client::RpcError; + +#[derive(Clone, Copy, Debug, Serialize, Deserialize)] +pub enum ErrorType { + /// Entity was not found. + NotFound, + /// Error occurred with a database call. + Database, + /// Error occurred in a libvirt call. + VirtError, + /// Error occurred while parsing input. + Parse, + /// An unknown API error occurred. + Other, +} + +impl ErrorType { + fn as_str(&self) -> &'static str { + match self { + ErrorType::NotFound => "Entity not found", + ErrorType::Database => "Database error", + ErrorType::VirtError => "libvirt error", + ErrorType::Parse => "Unable to parse input", + ErrorType::Other => "Unknown API error", + } + } +} + +impl fmt::Display for ErrorType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_str().fmt(f) + } +} + +#[derive(Debug, Serialize, Deserialize)] +pub struct ApiError { + error_type: ErrorType, + message: Option, + inner: Option, +} + +impl fmt::Display for ApiError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.message + .as_deref() + .unwrap_or_else(|| self.error_type.as_str()) + .fmt(f)?; + if let Some(inner) = &self.inner { + write!(f, ": {inner}")?; + } + Ok(()) + } +} + +impl std::error::Error for ApiError {} + +impl ApiError { + pub fn new(error_type: ErrorType, message: impl Into, err: E) -> Self + where + E: std::error::Error, + { + Self { + error_type, + message: Some(message.into()), + inner: Some(err.into()), + } + } + + pub fn err_type(&self) -> ErrorType { + self.error_type + } +} + +impl From for ApiError { + fn from(value: ErrorType) -> Self { + Self { + error_type: value, + message: None, + inner: None, + } + } +} + +impl From for ApiError +where + T: AsRef, +{ + fn from(value: T) -> Self { + let value = value.as_ref(); + Self { + error_type: ErrorType::Other, + message: Some(value.to_owned()), + inner: None, + } + } +} + +#[derive(Serialize, Deserialize)] +struct InnerError { + error_debug: String, + error_message: String, +} + +impl fmt::Debug for InnerError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.error_debug.fmt(f) + } +} + +impl fmt::Display for InnerError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.error_message.fmt(f) + } +} + +impl From for InnerError +where + E: std::error::Error, +{ + fn from(value: E) -> Self { + Self { + error_debug: format!("{value:?}"), + error_message: format!("{value}"), + } + } +} + +pub trait ToApiResult { + /// Converts the result's error type to [`ApiError`] with [`ErrorType::Other`]. + /// + /// [`ApiError`]: nzr-api::error::ApiError + /// [`ErrorType::Other`]: nzr-api::error::ErrorType::Other + fn to_api(self) -> Result; + /// Converts the result's error type to [`ApiError`] with + /// [`ErrorType::Other`], and the given context. + /// + /// [`ApiError`]: nzr-api::error::ApiError + /// [`ErrorType::Other`]: nzr-api::error::ErrorType::Other + fn to_api_with(self, context: impl AsRef) -> Result; + /// Converts the result's error type to [`ApiError`] with the given + /// [`ErrorType`] and context. + /// + /// [`ApiError`]: nzr-api::error::ApiError + /// [`ErrorType`]: nzr-api::error::ErrorType + fn to_api_with_type(self, err_type: ErrorType, context: impl AsRef) + -> Result; + /// Converts the result's error type to [`ApiError`] with the given + /// [`ErrorType`]. + /// + /// [`ApiError`]: nzr-api::error::ApiError + /// [`ErrorType`]: nzr-api::error::ErrorType + fn to_api_type(self, err_type: ErrorType) -> Result; +} + +impl ToApiResult for Result +where + E: std::error::Error + 'static, +{ + fn to_api(self) -> Result { + self.map_err(|e| ApiError { + error_type: ErrorType::Other, + message: None, + inner: Some(e.into()), + }) + } + + fn to_api_with(self, context: impl AsRef) -> Result { + self.map_err(|e| ApiError { + error_type: ErrorType::Other, + message: Some(context.as_ref().to_owned()), + inner: Some(e.into()), + }) + } + + fn to_api_type(self, err_type: ErrorType) -> Result { + self.map_err(|e| ApiError { + error_type: err_type, + message: None, + inner: Some(e.into()), + }) + } + + fn to_api_with_type( + self, + err_type: ErrorType, + context: impl AsRef, + ) -> Result { + self.map_err(|e| ApiError { + error_type: err_type, + message: Some(context.as_ref().to_owned()), + inner: Some(e.into()), + }) + } +} + +pub trait Simplify { + fn simplify(self) -> Result; +} + +impl Simplify for Result, RpcError> { + /// Flattens a Result of `RpcError` and `ApiError` to just `ApiError`. + fn simplify(self) -> Result { + self.to_api_with("RPC Error").and_then(|r| r) + } +} diff --git a/nzr-api/src/lib.rs b/nzr-api/src/lib.rs index 8cc6c43..916bac3 100644 --- a/nzr-api/src/lib.rs +++ b/nzr-api/src/lib.rs @@ -1,9 +1,11 @@ use std::net::Ipv4Addr; +use error::ApiError; use model::{CreateStatus, Instance, SshPubkey, Subnet}; pub mod args; pub mod config; +pub mod error; pub mod event; #[cfg(feature = "mock")] pub mod mock; @@ -24,40 +26,40 @@ pub enum InstanceQuery { #[tarpc::service] pub trait Nazrin { /// Creates a new instance. - async fn new_instance(build_args: args::NewInstance) -> Result; + async fn new_instance(build_args: args::NewInstance) -> Result; /// Poll for the current status of an instance being created. async fn poll_new_instance(task_id: uuid::Uuid) -> Option; /// Deletes an existing instance. /// /// This should involve deleting all related disks and clearing /// the lease information from the subnet data, if any. - async fn delete_instance(name: String) -> Result<(), String>; + async fn delete_instance(name: String) -> Result<(), ApiError>; /// Gets a single instance by the given InstanceQuery. - async fn find_instance(query: InstanceQuery) -> Result, String>; + async fn find_instance(query: InstanceQuery) -> Result, ApiError>; /// Gets a list of existing instances. - async fn get_instances(with_status: bool) -> Result, String>; + async fn get_instances(with_status: bool) -> Result, ApiError>; /// Cleans up unusable entries in the database. - async fn garbage_collect() -> Result<(), String>; + async fn garbage_collect() -> Result<(), ApiError>; /// Creates a new subnet. /// /// Unlike instances, subnets shouldn't perform any changes to the /// interfaces they reference. This should be used primarily for /// ease-of-use and bookkeeping (e.g., assigning dynamic leases). - async fn new_subnet(build_args: Subnet) -> Result; + async fn new_subnet(build_args: Subnet) -> Result; /// Modifies an existing subnet. - async fn modify_subnet(edit_args: Subnet) -> Result; + async fn modify_subnet(edit_args: Subnet) -> Result; /// Gets a list of existing subnets. - async fn get_subnets() -> Result, String>; + async fn get_subnets() -> Result, ApiError>; /// Deletes an existing subnet. - async fn delete_subnet(interface: String) -> Result<(), String>; + async fn delete_subnet(interface: String) -> Result<(), ApiError>; /// Gets the cloud-init user-data for the given instance. - async fn get_instance_userdata(id: i32) -> Result, String>; + async fn get_instance_userdata(id: i32) -> Result, ApiError>; /// Gets all SSH keys stored in the database. - async fn get_ssh_pubkeys() -> Result, String>; + async fn get_ssh_pubkeys() -> Result, ApiError>; /// Adds a new SSH public key to the database. - async fn add_ssh_pubkey(pub_key: String) -> Result; + async fn add_ssh_pubkey(pub_key: String) -> Result; /// Deletes an SSH public key from the database. - async fn delete_ssh_pubkey(id: i32) -> Result<(), String>; + async fn delete_ssh_pubkey(id: i32) -> Result<(), ApiError>; } /// Create a new NazrinClient. diff --git a/nzr-api/src/mock/client.rs b/nzr-api/src/mock/client.rs index e99d35b..8b5f838 100644 --- a/nzr-api/src/mock/client.rs +++ b/nzr-api/src/mock/client.rs @@ -1,20 +1,20 @@ use std::net::Ipv4Addr; -use crate::{args, model, net::cidr::CidrV4}; +use crate::{args, error::ApiError, model, net::cidr::CidrV4}; pub trait NzrClientExt { #[allow(async_fn_in_trait)] async fn new_mock_instance( &mut self, name: impl AsRef, - ) -> Result, crate::RpcError>; + ) -> Result, crate::RpcError>; } impl NzrClientExt for crate::NazrinClient { async fn new_mock_instance( &mut self, name: impl AsRef, - ) -> Result, crate::RpcError> { + ) -> Result, crate::RpcError> { let name = name.as_ref().to_owned(); let subnet = self diff --git a/nzr-api/src/mock/mod.rs b/nzr-api/src/mock/mod.rs index 3381e67..25ee926 100644 --- a/nzr-api/src/mock/mod.rs +++ b/nzr-api/src/mock/mod.rs @@ -10,6 +10,7 @@ use futures::{future, StreamExt}; use tokio::{sync::RwLock, task::JoinHandle}; use crate::{ + error::ApiError, model, net::{cidr::CidrV4, mac::MacAddr}, InstanceQuery, Nazrin, NazrinClient, @@ -62,14 +63,14 @@ impl Nazrin for MockServer { self, _: tarpc::context::Context, build_args: crate::args::NewInstance, - ) -> Result { + ) -> Result { let mut db = self.db.write().await; let Some(net_pos) = db .subnets .iter() .position(|s| s.as_ref().filter(|s| s.name == build_args.subnet).is_some()) else { - return Err("Subnet doesn't exist".to_owned()); + return Err("Subnet doesn't exist".into()); }; let subnet = db.subnets[net_pos].as_ref().unwrap().clone(); let cur_lease = *(db @@ -134,7 +135,11 @@ impl Nazrin for MockServer { } } - async fn delete_instance(self, _: tarpc::context::Context, name: String) -> Result<(), String> { + async fn delete_instance( + self, + _: tarpc::context::Context, + name: String, + ) -> Result<(), ApiError> { let mut db = self.db.write().await; let Some(inst) = db .instances @@ -142,7 +147,7 @@ impl Nazrin for MockServer { .find(|i| i.as_ref().filter(|i| i.name == name).is_some()) .take() else { - return Err("Instance doesn't exist".to_owned()); + return Err("Instance doesn't exist".into()); }; inst.take(); Ok(()) @@ -152,7 +157,7 @@ impl Nazrin for MockServer { self, _: tarpc::context::Context, query: crate::InstanceQuery, - ) -> Result, String> { + ) -> Result, ApiError> { let db = self.db.read().await; let res = { @@ -177,7 +182,7 @@ impl Nazrin for MockServer { self, _: tarpc::context::Context, id: i32, - ) -> Result, String> { + ) -> Result, ApiError> { let db = self.db.read().await; let Some(inst) = db .instances @@ -185,7 +190,7 @@ impl Nazrin for MockServer { .find(|i| i.as_ref().map(|i| i.id == id).is_some()) .and_then(|o| o.as_ref()) else { - return Err("No such instance".to_owned()); + return Err("No such instance".into()); }; Ok(db.ci_userdatas.get(&inst.name).cloned().unwrap_or_default()) } @@ -194,7 +199,7 @@ impl Nazrin for MockServer { self, _: tarpc::context::Context, _with_status: bool, - ) -> Result, String> { + ) -> Result, ApiError> { let db = self.db.read().await; Ok(db .instances @@ -207,7 +212,7 @@ impl Nazrin for MockServer { self, _: tarpc::context::Context, build_args: crate::model::Subnet, - ) -> Result { + ) -> Result { let mut db = self.db.write().await; let subnet = build_args.clone(); db.subnets.push(Some(build_args)); @@ -218,14 +223,14 @@ impl Nazrin for MockServer { self, _: tarpc::context::Context, _edit_args: crate::model::Subnet, - ) -> Result { + ) -> Result { todo!() } async fn get_subnets( self, _: tarpc::context::Context, - ) -> Result, String> { + ) -> Result, ApiError> { let db = self.db.read().await; Ok(db.subnets.iter().filter_map(|net| net.clone()).collect()) } @@ -234,7 +239,7 @@ impl Nazrin for MockServer { self, _: tarpc::context::Context, interface: String, - ) -> Result<(), String> { + ) -> Result<(), ApiError> { let mut db = self.db.write().await; db.instances .iter() @@ -249,20 +254,20 @@ impl Nazrin for MockServer { .iter_mut() .find(|net| net.as_ref().filter(|n| n.name == interface).is_some()) else { - return Err("Subnet doesn't exist".to_owned()); + return Err("Subnet doesn't exist".into()); }; subnet.take(); Ok(()) } - async fn garbage_collect(self, _: tarpc::context::Context) -> Result<(), String> { + async fn garbage_collect(self, _: tarpc::context::Context) -> Result<(), ApiError> { todo!() } async fn get_ssh_pubkeys( self, _: tarpc::context::Context, - ) -> Result, String> { + ) -> Result, ApiError> { let db = self.db.read().await; Ok(db @@ -276,7 +281,7 @@ impl Nazrin for MockServer { self, _: tarpc::context::Context, pub_key: String, - ) -> Result { + ) -> Result { let mut key_model = model::SshPubkey::from_str(&pub_key).map_err(|e| e.to_string())?; let mut db = self.db.write().await; key_model.id = Some(db.ssh_keys.len() as i32); @@ -284,7 +289,7 @@ impl Nazrin for MockServer { Ok(key_model) } - async fn delete_ssh_pubkey(self, _: tarpc::context::Context, id: i32) -> Result<(), String> { + async fn delete_ssh_pubkey(self, _: tarpc::context::Context, id: i32) -> Result<(), ApiError> { let mut db = self.db.write().await; if let Some(key) = db.ssh_keys.get_mut(id as usize) { key.take(); diff --git a/nzr-api/src/model.rs b/nzr-api/src/model.rs index 42a833d..8cac04e 100644 --- a/nzr-api/src/model.rs +++ b/nzr-api/src/model.rs @@ -5,7 +5,10 @@ use serde::{Deserialize, Serialize}; use std::{fmt, net::Ipv4Addr}; use thiserror::Error; -use crate::net::{cidr::CidrV4, mac::MacAddr}; +use crate::{ + error::ApiError, + net::{cidr::CidrV4, mac::MacAddr}, +}; #[derive(Copy, Clone, Debug, Serialize, Deserialize)] #[repr(u32)] @@ -67,7 +70,7 @@ impl fmt::Display for DomainState { pub struct CreateStatus { pub status_text: String, pub completion: f32, - pub result: Option>, + pub result: Option>, } /// Struct representing a VM instance. diff --git a/nzrd/src/cmd/mod.rs b/nzrd/src/cmd/mod.rs index adcdfe1..e44013b 100644 --- a/nzrd/src/cmd/mod.rs +++ b/nzrd/src/cmd/mod.rs @@ -1,23 +1 @@ -pub mod net; pub mod vm; - -use std::fmt; - -#[derive(Debug)] -pub struct CommandError(String); - -impl fmt::Display for CommandError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } -} - -impl std::error::Error for CommandError {} - -macro_rules! cmd_error { - ($($arg:tt)*) => { - Box::new(CommandError(format!($($arg)*))) - }; -} - -pub(crate) use cmd_error; diff --git a/nzrd/src/cmd/net.rs b/nzrd/src/cmd/net.rs deleted file mode 100644 index 5dd966e..0000000 --- a/nzrd/src/cmd/net.rs +++ /dev/null @@ -1,41 +0,0 @@ -use super::*; -use crate::ctx::Context; -use crate::model::tx::Transaction; -use crate::model::Subnet; -use nzr_api::model; - -pub async fn add_subnet( - ctx: &Context, - args: model::Subnet, -) -> Result> { - let subnet = { - let s = Subnet::insert(ctx, args.name, args.data) - .await - .map_err(|er| cmd_error!("Couldn't generate subnet: {}", er))?; - Transaction::begin(ctx, s) - }; - - Ok(subnet.take()) -} - -pub async fn delete_subnet( - ctx: &Context, - name: impl AsRef, -) -> Result<(), Box> { - match Subnet::get_by_name(ctx, name.as_ref()) - .await - .map_err(|er| cmd_error!("Couldn't find subnet: {}", er))? - { - Some(subnet) => { - // TODO: notify clients - - subnet - .delete(ctx) - .await - .map_err(|er| cmd_error!("Couldn't fully delete subnet entry: {}", er)) - } - None => Err(cmd_error!("Subnet not found")), - }?; - - Ok(()) -} diff --git a/nzrd/src/cmd/vm.rs b/nzrd/src/cmd/vm.rs index 2a99785..acb1286 100644 --- a/nzrd/src/cmd/vm.rs +++ b/nzrd/src/cmd/vm.rs @@ -1,3 +1,4 @@ +use nzr_api::error::{ApiError, ErrorType, ToApiResult}; use nzr_api::net::cidr::CidrV4; use nzr_virt::error::DomainError; use nzr_virt::xml::build::DomainBuilder; @@ -5,9 +6,9 @@ use nzr_virt::xml::{self, InfoMap, SerialType, Sysinfo}; use nzr_virt::{datasize, dom, vol}; use tokio::sync::RwLock; -use super::*; use crate::ctrl::vm::Progress; use crate::ctx::Context; +use crate::model::tx::Transaction; use crate::model::{Instance, Subnet}; use nzr_api::net::mac::MacAddr; use nzr_api::{args, model, nzr_event}; @@ -29,23 +30,21 @@ pub async fn new_instance( ctx: Context, prog_task: Arc>, args: &args::NewInstance, -) -> Result<(Instance, dom::Domain), Box> { +) -> Result<(Instance, dom::Domain), ApiError> { progress!(prog_task, 0.0, "Starting..."); // find the subnet corresponding to the interface let subnet = Subnet::get_by_name(&ctx, &args.subnet) .await - .map_err(|er| cmd_error!("Unable to get interface: {}", er))? - .ok_or(cmd_error!( - "Subnet {} wasn't found in database", - &args.subnet - ))?; + .to_api_with("Unable to get interface")? + .ok_or::(format!("Subnet {} wasn't found in database", &args.subnet).into())?; // bail if a domain already exists if let Ok(dom) = ctx.virt.conn.get_instance(&args.name).await { - Err(cmd_error!( + Err(format!( "Domain with name already exists (uuid {})", dom.xml().await.uuid, - )) + ) + .into()) } else { // make sure the base image exists let mut base_image = ctx @@ -54,7 +53,7 @@ pub async fn new_instance( .baseimg .volume(&args.base_image) .await - .map_err(|er| cmd_error!("Couldn't find base image: {}", er))?; + .to_api_with("Couldn't find base image")?; progress!(prog_task, 10.0, "Generating metadata..."); // generate a new lease with a new MAC addr @@ -62,19 +61,23 @@ pub async fn new_instance( let bytes = [VIRT_MAC_OUI, rand::random::<[u8; 3]>().as_ref()].concat(); MacAddr::from_bytes(bytes) } - .map_err(|er| cmd_error!("Unable to create a new MAC address: {}", er))?; + .to_api_with("Unable to create a new MAC address")?; // Get highest host addr + 1 for our new addr let addr = { let addr_num = Instance::all_in_subnet(&ctx, &subnet) - .await? + .await + .to_api_with("Couldn't get instances in subnet")? .into_iter() .max_by(|a, b| a.host_num.cmp(&b.host_num)) .map_or(subnet.start_host, |i| i.host_num + 1); if addr_num > subnet.end_host || addr_num < subnet.start_host { - Err(cmd_error!("Got invalid lease address for instance"))?; + return Err("Got invalid lease address for instance".into()); } - let addr = subnet.network.make_ip(addr_num as u32)?; + let addr = subnet + .network + .make_ip(addr_num as u32) + .to_api_with("Unable to generate instance IP")?; CidrV4::new(addr, subnet.network.cidr()) }; @@ -85,7 +88,12 @@ pub async fn new_instance( }; // generate cloud-init data - let db_inst = Instance::insert(&ctx, &args.name, &subnet, lease.clone(), None).await?; + let db_inst = { + let inst = Instance::insert(&ctx, &args.name, &subnet, lease.clone(), None) + .await + .to_api_type(ErrorType::Database)?; + Transaction::begin(&ctx, inst) + }; progress!(prog_task, 30.0, "Creating instance images..."); // create primary volume from base image @@ -96,7 +104,7 @@ pub async fn new_instance( datasize!((args.disk_sizes.0) GiB), ) .await - .map_err(|er| cmd_error!("Failed to clone base image: {}", er))?; + .to_api_with("Failed to clone base image")?; // and, if it exists: the second volume let sec_vol = match args.disk_sizes.1 { @@ -104,7 +112,11 @@ pub async fn new_instance( let voldata = // TODO: Fix VolType xml::Volume::new(&args.name, xml::VolType::Qcow2, datasize!(sec_size GiB)); - Some(vol::Volume::create(&ctx.virt.pools.secondary, voldata, 0).await?) + Some( + vol::Volume::create(&ctx.virt.pools.secondary, voldata, 0) + .await + .to_api_with("Couldn't create secondary volume")?, + ) } None => None, }; @@ -168,15 +180,20 @@ pub async fn new_instance( .build() }; - let mut virt_dom = ctx.virt.conn.define_instance(dom_xml).await?; + let mut virt_dom = ctx + .virt + .conn + .define_instance(dom_xml) + .await + .to_api_with("Couldn't define libvirt instance")?; // not a fatal error, we can set autostart afterward - if let Err(er) = virt_dom.autostart(true).await { - warn!("Couldn't set autostart for domain: {}", er); + if let Err(err) = virt_dom.autostart(true).await { + warn!("Couldn't set autostart for domain: {err}"); } - if let Err(er) = virt_dom.start().await { - warn!("Domain defined, but couldn't be started! Error: {}", er); + if let Err(err) = virt_dom.start().await { + warn!("Domain defined, but couldn't be started! Error: {err}"); } // set all volumes to persistent to avoid deletion @@ -188,16 +205,19 @@ pub async fn new_instance( progress!(prog_task, 80.0, "Domain created!"); debug!("Domain {} created!", virt_dom.xml().await.name.as_str()); - Ok((db_inst, virt_dom)) + Ok((db_inst.take(), virt_dom)) } } pub async fn delete_instance( ctx: Context, name: String, -) -> Result, Box> { - let Some(inst_db) = Instance::get_by_name(&ctx, &name).await? else { - return Err(cmd_error!("Instance {name} not found")); +) -> Result, ApiError> { + let Some(inst_db) = Instance::get_by_name(&ctx, &name) + .await + .to_api_with_type(ErrorType::Database, "Couldn't find instance")? + else { + return Err(ErrorType::NotFound.into()); }; let api_model = match inst_db.api_model(&ctx).await { Ok(model) => Some(model), @@ -209,16 +229,25 @@ pub async fn delete_instance( // First, destroy the instance match ctx.virt.conn.get_instance(name.clone()).await { Ok(mut inst) => { - inst.stop().await?; - inst.undefine(true).await?; + inst.stop().await.to_api_with("Couldn't stop instance")?; + inst.undefine(true) + .await + .to_api_with("Couldn't undefine instance")?; } Err(DomainError::DomainNotFound) => { warn!("Deleting instance that exists in DB but not libvirt"); } - Err(err) => Err(err)?, + Err(err) => Err(ApiError::new( + nzr_api::error::ErrorType::VirtError, + "Couldn't get instance from libvirt", + err, + ))?, } // Then, delete the DB entity - inst_db.delete(&ctx).await?; + inst_db + .delete(&ctx) + .await + .to_api_with("Couldn't delete from database")?; Ok(api_model) } diff --git a/nzrd/src/model/mod.rs b/nzrd/src/model/mod.rs index 7fcf671..b464279 100644 --- a/nzrd/src/model/mod.rs +++ b/nzrd/src/model/mod.rs @@ -20,12 +20,14 @@ use tx::Transactable; #[derive(Debug, Error)] pub enum ModelError { - #[error("Database error occurred: {0}")] + #[error("{0}")] Db(#[from] diesel::result::Error), - #[error("Unable to get database handle: {0}")] + #[error("Database pool error ({0})")] Pool(#[from] diesel::r2d2::PoolError), #[error("{0}")] Cidr(#[from] cidr::Error), + #[error("Instance belongs to a subnet that has since disappeared")] + NoSubnet, } diesel::table! { @@ -247,10 +249,7 @@ impl Instance { /// Creates an [nzr_api::model::Instance] from the information available in /// the database. - pub async fn api_model( - &self, - ctx: &Context, - ) -> Result> { + pub async fn api_model(&self, ctx: &Context) -> Result { let netid = self.subnet_id; let Some(subnet) = ctx .spawn_db(move |mut db| Subnet::table().find(netid).load::(&mut db)) @@ -258,7 +257,7 @@ impl Instance { .into_iter() .next() else { - todo!("something went horribly wrong"); + return Err(ModelError::NoSubnet); }; Ok(nzr_api::model::Instance { diff --git a/nzrd/src/rpc.rs b/nzrd/src/rpc.rs index 54a112e..0763193 100644 --- a/nzrd/src/rpc.rs +++ b/nzrd/src/rpc.rs @@ -1,4 +1,5 @@ use futures::{future, StreamExt}; +use nzr_api::error::{ApiError, ErrorType, ToApiResult}; use nzr_api::{args, model, nzr_event, InstanceQuery, Nazrin}; use std::str::FromStr; use std::sync::Arc; @@ -36,7 +37,7 @@ impl Nazrin for NzrServer { self, _: tarpc::context::Context, build_args: args::NewInstance, - ) -> Result { + ) -> Result { let progress = Arc::new(RwLock::new(crate::ctrl::vm::Progress { status_text: "Starting...".to_owned(), percentage: 0.0, @@ -44,13 +45,11 @@ impl Nazrin for NzrServer { let prog_task = progress.clone(); let build_task = tokio::spawn(async move { let (inst, dom) = - cmd::vm::new_instance(self.ctx.clone(), prog_task.clone(), &build_args) - .await - .map_err(|e| format!("Instance creation failed: {}", e))?; + cmd::vm::new_instance(self.ctx.clone(), prog_task.clone(), &build_args).await?; let mut api_model = inst .api_model(&self.ctx) .await - .map_err(|e| format!("Couldn't generate API response: {e}"))?; + .to_api_with("Couldn't generate API response")?; match dom.state().await { Ok(state) => { api_model.state = state.into(); @@ -97,7 +96,7 @@ impl Nazrin for NzrServer { Some( task.inner .await - .map_err(|err| format!("Task failed with panic: {}", err)) + .to_api_with("Task failed with panic") .and_then(|res| res), ) } else { @@ -111,10 +110,12 @@ impl Nazrin for NzrServer { }) } - async fn delete_instance(self, _: tarpc::context::Context, name: String) -> Result<(), String> { - let api_model = cmd::vm::delete_instance(self.ctx.clone(), name) - .await - .map_err(|e| format!("Couldn't delete instance: {}", e))?; + async fn delete_instance( + self, + _: tarpc::context::Context, + name: String, + ) -> Result<(), ApiError> { + let api_model = cmd::vm::delete_instance(self.ctx.clone(), name).await?; if let Some(api_model) = api_model { nzr_event!(self.ctx.events, Deleted, api_model); @@ -126,18 +127,16 @@ impl Nazrin for NzrServer { self, _: tarpc::context::Context, query: nzr_api::InstanceQuery, - ) -> Result, String> { + ) -> Result, ApiError> { let res = match query { InstanceQuery::Name(name) => Instance::get_by_name(&self.ctx, name).await, InstanceQuery::MacAddr(addr) => Instance::get_by_mac(&self.ctx, addr).await, InstanceQuery::Ipv4Addr(addr) => Instance::get_by_ip4(&self.ctx, addr).await, } - .map_err(|e| e.to_string())?; + .to_api()?; if let Some(inst) = res { - inst.api_model(&self.ctx) - .await - .map_or_else(|e| Err(e.to_string()), |m| Ok(Some(m))) + inst.api_model(&self.ctx).await.to_api().map(Some) } else { Ok(None) } @@ -147,10 +146,10 @@ impl Nazrin for NzrServer { self, _: tarpc::context::Context, with_status: bool, - ) -> Result, String> { + ) -> Result, ApiError> { let db_models = Instance::all(&self.ctx) .await - .map_err(|e| format!("Unable to get all instances: {e}"))?; + .to_api_type(ErrorType::Database)?; let mut models = Vec::new(); for inst in db_models { let mut api_model = match inst.api_model(&self.ctx).await { @@ -188,12 +187,12 @@ impl Nazrin for NzrServer { self, _: tarpc::context::Context, build_args: model::Subnet, - ) -> Result { - let subnet = cmd::net::add_subnet(&self.ctx, build_args) + ) -> Result { + let subnet = Subnet::insert(&self.ctx, build_args.name, build_args.data) .await - .map_err(|e| e.to_string())? + .to_api_type(ErrorType::Database)? .api_model() - .map_err(|e| e.to_string())?; + .to_api_with("Unable to generate API model")?; // inform event listeners nzr_event!(self.ctx.events, Created, subnet); @@ -204,22 +203,23 @@ impl Nazrin for NzrServer { self, _: tarpc::context::Context, edit_args: model::Subnet, - ) -> Result { + ) -> Result { if let Some(subnet) = Subnet::get_by_name(&self.ctx, &edit_args.name) .await .map_err(|e| e.to_string())? { Err("Modifying subnets not yet supported".into()) } else { - Err(format!("Subnet {} not found", &edit_args.name)) + Err(ErrorType::NotFound.into()) } } - async fn get_subnets(self, _: tarpc::context::Context) -> Result, String> { - Subnet::all(&self.ctx).await.map_or_else( - |e| Err(e.to_string()), - |v| { - Ok(v.into_iter() + async fn get_subnets(self, _: tarpc::context::Context) -> Result, ApiError> { + Subnet::all(&self.ctx) + .await + .to_api_with("Couldn't get list of subnets") + .map(|v| { + v.into_iter() .filter_map(|s| match s.api_model() { Ok(model) => Some(model), Err(err) => { @@ -227,23 +227,43 @@ impl Nazrin for NzrServer { None } }) - .collect()) - }, - ) + .collect() + }) } async fn delete_subnet( self, _: tarpc::context::Context, subnet_name: String, - ) -> Result<(), String> { - cmd::net::delete_subnet(&self.ctx, &subnet_name) + ) -> Result<(), ApiError> { + if let Some(subnet) = Subnet::get_by_name(&self.ctx, subnet_name) .await - .map_err(|e| e.to_string())?; - Ok(()) + .to_api_type(ErrorType::Database)? + { + let api_model = match subnet.api_model() { + Ok(model) => Some(model), + Err(err) => { + tracing::error!("Unable to generate model for clients: {err}"); + None + } + }; + + subnet + .delete(&self.ctx) + .await + .to_api_type(ErrorType::Database)?; + + if let Some(api_model) = api_model { + nzr_event!(&self.ctx.events, Deleted, api_model); + } + + Ok(()) + } else { + Err(ErrorType::NotFound.into()) + } } - async fn garbage_collect(self, _: tarpc::context::Context) -> Result<(), String> { + async fn garbage_collect(self, _: tarpc::context::Context) -> Result<(), ApiError> { cmd::vm::prune_instances(&self.ctx) .await .map_err(|e| e.to_string())?; @@ -254,50 +274,52 @@ impl Nazrin for NzrServer { self, _: tarpc::context::Context, id: i32, - ) -> Result, String> { - let Some(db_model) = Instance::get(&self.ctx, id) + ) -> Result, ApiError> { + if let Some(db_model) = Instance::get(&self.ctx, id) .await - .map_err(|e| e.to_string())? - else { - return Err("Instance doesn't exist".to_owned()); - }; - - Ok(db_model.ci_userdata.unwrap_or_default()) + .to_api_type(ErrorType::Database)? + { + Ok(db_model.ci_userdata.unwrap_or_default()) + } else { + Err(ErrorType::NotFound.into()) + } } async fn get_ssh_pubkeys( self, _: tarpc::context::Context, - ) -> Result, String> { - SshPubkey::all(&self.ctx).await.map_or_else( - |e| Err(e.to_string()), - |k| Ok(k.iter().map(|k| k.api_model()).collect()), - ) + ) -> Result, ApiError> { + SshPubkey::all(&self.ctx) + .await + .to_api_type(ErrorType::Database) + .map(|k| k.iter().map(|k| k.api_model()).collect()) } async fn add_ssh_pubkey( self, _: tarpc::context::Context, pub_key: String, - ) -> Result { - let pubkey = model::SshPubkey::from_str(&pub_key).map_err(|e| e.to_string())?; + ) -> Result { + let pubkey = model::SshPubkey::from_str(&pub_key).to_api_type(ErrorType::Parse)?; SshPubkey::insert(&self.ctx, pubkey.algorithm, pubkey.key_data, pubkey.comment) .await - .map_err(|e| e.to_string()) + .to_api_type(ErrorType::Database) .map(|k| k.api_model()) } - async fn delete_ssh_pubkey(self, _: tarpc::context::Context, id: i32) -> Result<(), String> { - let Some(key) = SshPubkey::get(&self.ctx, id) + async fn delete_ssh_pubkey(self, _: tarpc::context::Context, id: i32) -> Result<(), ApiError> { + if let Some(key) = SshPubkey::get(&self.ctx, id) .await - .map_err(|e| e.to_string())? - else { - return Err("SSH key with ID doesn't exist".into()); - }; - - key.delete(&self.ctx).await.map_err(|e| e.to_string())?; - Ok(()) + .to_api_type(ErrorType::Database)? + { + key.delete(&self.ctx) + .await + .to_api_type(ErrorType::Database)?; + Ok(()) + } else { + Err(ErrorType::NotFound.into()) + } } } @@ -335,7 +357,6 @@ pub async fn serve(ctx: Context) -> Result<(), Box> { debug!("Listening for new connection..."); let (conn, _addr) = listener.accept().await?; let ctx = ctx.clone(); - // hack? tokio::spawn(async move { let framed = codec_builder.new_framed(conn); let transport = tarpc::serde_transport::new(framed, Bincode::default()); @@ -351,6 +372,6 @@ pub async fn serve(ctx: Context) -> Result<(), Box> { } struct InstCreateStatus { - inner: JoinHandle>, + inner: JoinHandle>, progress: Arc>, } diff --git a/omyacid/src/ctx.rs b/omyacid/src/ctx.rs index 1464331..9a0cc0a 100644 --- a/omyacid/src/ctx.rs +++ b/omyacid/src/ctx.rs @@ -57,7 +57,7 @@ impl Context { } pub async fn get_sshkeys(&self) -> Result> { - // TODO: do we cache SSH keys? I don't like the idea of it + // We don't cache SSH keys, so always get from the API server let ssh_keys = self .api_client .get_ssh_pubkeys(nzr_api::default_ctx()) diff --git a/omyacid/src/main.rs b/omyacid/src/main.rs index bdcf0c1..213976a 100644 --- a/omyacid/src/main.rs +++ b/omyacid/src/main.rs @@ -41,7 +41,6 @@ async fn get_meta_data( Ok(Some(inst)) => { let meta = Metadata { inst_name: &inst.name, - // XXX: this is very silly imo ssh_pubkeys: ssh_pubkeys.iter().collect(), };