From 2a46fce574e9b44eb47168cc4ce316d8d3d151cb Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 20 Mar 2024 12:09:20 +1000 Subject: [PATCH] refactor(connlib): remove `Result` return values from callbacks (#4158) Currently, an error returned by `Tunnel::poll_next_event` is only logged. In other words, they are never fatal. This creates a tricky to understand relationship on what kind of errors should be returned from callbacks. Because connlib is used on multiple operating systems, it has no idea how fatal a particular error is. This PR removes all of these `Result` return values with the following consequences: - For Android, we now panic when a callback fails. This is a slight change in behaviour. I believe that previously, any exception thrown by a callback into Android was caught and returned as an error. Now, we panic because in the FFI layer, we don't have any information on how fatal the error is. For non-fatal errors, the Android app should simply not throw an exception. The panics will cause the connlib task to be shut down which triggers an `on_disconnect`. - For Swift, there is no behaviour change. The FFI layer already did not support `Result`s for those callbacks. I don't know how exceptions from Swift are translated across the FFI layer but there is no change to what we had before. - For the Tauri client: - I chose to log errors on ERROR level and continue gracefully for the DNS resolvers. - We panic in case the controller channel is full / closed. That should really never happen in practice though unless we are currently shutting down the app. Resolves: #4064. --- rust/connlib/clients/android/src/lib.rs | 26 ++--- rust/connlib/clients/apple/src/lib.rs | 29 +++--- rust/connlib/clients/shared/src/lib.rs | 11 +-- rust/connlib/shared/src/callbacks.rs | 41 +++----- .../shared/src/callbacks_error_facade.rs | 95 ------------------- rust/connlib/shared/src/error.rs | 12 --- rust/connlib/shared/src/lib.rs | 2 - rust/connlib/tunnel/src/client.rs | 15 +-- rust/connlib/tunnel/src/device_channel.rs | 6 +- .../tunnel/src/device_channel/tun_android.rs | 8 +- .../tunnel/src/device_channel/tun_darwin.rs | 12 +-- .../tunnel/src/device_channel/tun_windows.rs | 4 +- rust/connlib/tunnel/src/lib.rs | 14 +-- rust/gateway/src/main.rs | 4 +- rust/gui-client/src-tauri/src/client/gui.rs | 39 ++++---- rust/linux-client/src/main.rs | 38 ++++---- 16 files changed, 104 insertions(+), 252 deletions(-) delete mode 100644 rust/connlib/shared/src/callbacks_error_facade.rs diff --git a/rust/connlib/clients/android/src/lib.rs b/rust/connlib/clients/android/src/lib.rs index f0156a84a..5f616ad0b 100644 --- a/rust/connlib/clients/android/src/lib.rs +++ b/rust/connlib/clients/android/src/lib.rs @@ -138,14 +138,12 @@ fn init_logging(log_dir: &Path, log_filter: String) -> file_logger::Handle { } impl Callbacks for CallbackHandler { - type Error = CallbackError; - fn on_set_interface_config( &self, tunnel_address_v4: Ipv4Addr, tunnel_address_v6: Ipv6Addr, dns_addresses: Vec, - ) -> Result, Self::Error> { + ) -> Option { self.env(|mut env| { let tunnel_address_v4 = env.new_string(tunnel_address_v4.to_string()) @@ -180,9 +178,10 @@ impl Callbacks for CallbackHandler { .map(Some) .map_err(|source| CallbackError::CallMethodFailed { name, source }) }) + .expect("onSetInterfaceConfig callback failed") } - fn on_tunnel_ready(&self) -> Result<(), Self::Error> { + fn on_tunnel_ready(&self) { self.env(|mut env| { call_method( &mut env, @@ -192,13 +191,14 @@ impl Callbacks for CallbackHandler { &[], ) }) + .expect("onTunnelReady callback failed") } fn on_update_routes( &self, route_list_4: Vec, route_list_6: Vec, - ) -> Result, Self::Error> { + ) -> Option { self.env(|mut env| { let route_list_4 = env .new_string(serde_json::to_string(&route_list_4)?) @@ -224,10 +224,11 @@ impl Callbacks for CallbackHandler { .map(Some) .map_err(|source| CallbackError::CallMethodFailed { name, source }) }) + .expect("onUpdateRoutes callback failed") } #[cfg(target_os = "android")] - fn protect_file_descriptor(&self, file_descriptor: RawFd) -> Result<(), Self::Error> { + fn protect_file_descriptor(&self, file_descriptor: RawFd) { self.env(|mut env| { call_method( &mut env, @@ -237,12 +238,10 @@ impl Callbacks for CallbackHandler { &[JValue::Int(file_descriptor)], ) }) + .expect("protectFileDescriptor callback failed"); } - fn on_update_resources( - &self, - resource_list: Vec, - ) -> Result<(), Self::Error> { + fn on_update_resources(&self, resource_list: Vec) { self.env(|mut env| { let resource_list = env .new_string(serde_json::to_string(&resource_list)?) @@ -258,9 +257,10 @@ impl Callbacks for CallbackHandler { &[JValue::from(&resource_list)], ) }) + .expect("onUpdateResources callback failed") } - fn on_disconnect(&self, error: &Error) -> Result<(), Self::Error> { + fn on_disconnect(&self, error: &Error) { self.env(|mut env| { let error = env .new_string(serde_json::to_string(&error.to_string())?) @@ -276,6 +276,7 @@ impl Callbacks for CallbackHandler { &[JValue::from(&error)], ) }) + .expect("onDisconnect callback failed") } fn roll_log_file(&self) -> Option { @@ -286,7 +287,7 @@ impl Callbacks for CallbackHandler { }) } - fn get_system_default_resolvers(&self) -> Result>, Self::Error> { + fn get_system_default_resolvers(&self) -> Option> { self.env(|mut env| { let name = "getSystemDefaultResolvers"; let addrs = env @@ -297,6 +298,7 @@ impl Callbacks for CallbackHandler { Ok(Some(addrs.iter().filter_map(|v| to_ip(v)).collect())) }) + .expect("getSystemDefaultResolvers callback failed") } } diff --git a/rust/connlib/clients/apple/src/lib.rs b/rust/connlib/clients/apple/src/lib.rs index 28db0ce14..530b6b81e 100644 --- a/rust/connlib/clients/apple/src/lib.rs +++ b/rust/connlib/clients/apple/src/lib.rs @@ -97,57 +97,51 @@ pub struct CallbackHandler { } impl Callbacks for CallbackHandler { - type Error = std::convert::Infallible; - fn on_set_interface_config( &self, tunnel_address_v4: Ipv4Addr, tunnel_address_v6: Ipv6Addr, dns_addresses: Vec, - ) -> Result, Self::Error> { + ) -> Option { self.inner.on_set_interface_config( tunnel_address_v4.to_string(), tunnel_address_v6.to_string(), serde_json::to_string(&dns_addresses) .expect("developer error: a list of ips should always be serializable"), ); - Ok(None) + + None } - fn on_tunnel_ready(&self) -> Result<(), Self::Error> { + fn on_tunnel_ready(&self) { self.inner.on_tunnel_ready(); - Ok(()) } fn on_update_routes( &self, route_list_4: Vec, route_list_6: Vec, - ) -> Result, Self::Error> { + ) -> Option { self.inner.on_update_routes( serde_json::to_string(&route_list_4).unwrap(), serde_json::to_string(&route_list_6).unwrap(), ); - Ok(None) + + None } - fn on_update_resources( - &self, - resource_list: Vec, - ) -> Result<(), Self::Error> { + fn on_update_resources(&self, resource_list: Vec) { self.inner.on_update_resources( serde_json::to_string(&resource_list) .expect("developer error: failed to serialize resource list"), ); - Ok(()) } - fn on_disconnect(&self, error: &Error) -> Result<(), Self::Error> { + fn on_disconnect(&self, error: &Error) { self.inner.on_disconnect(error.to_string()); - Ok(()) } - fn get_system_default_resolvers(&self) -> Result>, Self::Error> { + fn get_system_default_resolvers(&self) -> Option> { let resolvers_json = self.inner.get_system_default_resolvers(); tracing::debug!( "get_system_default_resolvers returned: {:?}", @@ -156,7 +150,8 @@ impl Callbacks for CallbackHandler { let resolvers: Vec = serde_json::from_str(&resolvers_json) .expect("developer error: failed to deserialize resolvers"); - Ok(Some(resolvers)) + + Some(resolvers) } fn roll_log_file(&self) -> Option { diff --git a/rust/connlib/clients/shared/src/lib.rs b/rust/connlib/clients/shared/src/lib.rs index 83d595717..eb62172e5 100644 --- a/rust/connlib/clients/shared/src/lib.rs +++ b/rust/connlib/clients/shared/src/lib.rs @@ -6,7 +6,7 @@ pub use connlib_shared::{ pub use tracing_appender::non_blocking::WorkerGuard; use backoff::ExponentialBackoffBuilder; -use connlib_shared::{get_user_agent, CallbackErrorFacade}; +use connlib_shared::get_user_agent; use firezone_tunnel::ClientTunnel; use phoenix_channel::PhoenixChannel; use std::time::Duration; @@ -41,7 +41,6 @@ impl Session { max_partition_time: Option, handle: tokio::runtime::Handle, ) -> connlib_shared::Result { - let callbacks = CallbackErrorFacade(callbacks); let (tx, rx) = tokio::sync::mpsc::channel(1); let connect_handle = handle.spawn(connect( @@ -127,20 +126,20 @@ where } Ok(Err(e)) => { tracing::error!("connlib failed: {e}"); - let _ = callbacks.on_disconnect(&e); + callbacks.on_disconnect(&e); } Err(e) => match e.try_into_panic() { Ok(panic) => { if let Some(msg) = panic.downcast_ref::<&str>() { - let _ = callbacks.on_disconnect(&Error::Panic(msg.to_string())); + callbacks.on_disconnect(&Error::Panic(msg.to_string())); return; } - let _ = callbacks.on_disconnect(&Error::PanicNonStringPayload); + callbacks.on_disconnect(&Error::PanicNonStringPayload); } Err(_) => { tracing::error!("connlib task was cancelled"); - let _ = callbacks.on_disconnect(&Error::Cancelled); + callbacks.on_disconnect(&Error::Cancelled); } }, } diff --git a/rust/connlib/shared/src/callbacks.rs b/rust/connlib/shared/src/callbacks.rs index cbd619630..7b0e8adb6 100644 --- a/rust/connlib/shared/src/callbacks.rs +++ b/rust/connlib/shared/src/callbacks.rs @@ -1,8 +1,7 @@ use crate::messages::ResourceDescription; use ip_network::{Ipv4Network, Ipv6Network}; use serde::Serialize; -use std::error::Error; -use std::fmt::{Debug, Display}; +use std::fmt::Debug; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::path::PathBuf; @@ -43,47 +42,32 @@ impl From for Cidrv6 { /// Traits that will be used by connlib to callback the client upper layers. pub trait Callbacks: Clone + Send + Sync { - /// Error returned when a callback fails. - type Error: Debug + Display + Error; - /// Called when the tunnel address is set. /// /// This should return a new `fd` if there is one. /// (Only happens on android for now) - fn on_set_interface_config( - &self, - _: Ipv4Addr, - _: Ipv6Addr, - _: Vec, - ) -> Result, Self::Error> { - Ok(None) + fn on_set_interface_config(&self, _: Ipv4Addr, _: Ipv6Addr, _: Vec) -> Option { + None } /// Called when the tunnel is connected. - fn on_tunnel_ready(&self) -> Result<(), Self::Error> { + fn on_tunnel_ready(&self) { tracing::trace!("tunnel_connected"); - Ok(()) } /// Called when the route list changes. - fn on_update_routes( - &self, - _: Vec, - _: Vec, - ) -> Result, Self::Error> { - Ok(None) + fn on_update_routes(&self, _: Vec, _: Vec) -> Option { + None } /// Called when the resource list changes. - fn on_update_resources(&self, _: Vec) -> Result<(), Self::Error> { - Ok(()) - } + fn on_update_resources(&self, _: Vec) {} /// Called when the tunnel is disconnected. /// /// If the tunnel disconnected due to a fatal error, `error` is the error /// that caused the disconnect. - fn on_disconnect(&self, error: &crate::Error) -> Result<(), Self::Error> { + fn on_disconnect(&self, error: &crate::Error) { tracing::error!(error = ?error, "tunnel_disconnected"); // Note that we can't panic here, since we already hooked the panic to this function. std::process::exit(0); @@ -93,16 +77,13 @@ pub trait Callbacks: Clone + Send + Sync { /// /// It's okay for clients to include Firezone's own DNS here, e.g. 100.100.111.1. /// connlib internally filters them out. - fn get_system_default_resolvers(&self) -> Result>, Self::Error> { - Ok(None) + fn get_system_default_resolvers(&self) -> Option> { + None } /// Protects the socket file descriptor from routing loops. #[cfg(target_os = "android")] - fn protect_file_descriptor( - &self, - file_descriptor: std::os::fd::RawFd, - ) -> Result<(), Self::Error>; + fn protect_file_descriptor(&self, file_descriptor: std::os::fd::RawFd); fn roll_log_file(&self) -> Option { None diff --git a/rust/connlib/shared/src/callbacks_error_facade.rs b/rust/connlib/shared/src/callbacks_error_facade.rs deleted file mode 100644 index 1dde305e6..000000000 --- a/rust/connlib/shared/src/callbacks_error_facade.rs +++ /dev/null @@ -1,95 +0,0 @@ -use crate::callbacks::{Cidrv4, Cidrv6}; -use crate::messages::ResourceDescription; -use crate::{Callbacks, Error, Result}; -use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; -use std::path::PathBuf; - -// Avoids having to map types for Windows -type RawFd = i32; - -#[derive(Clone)] -pub struct CallbackErrorFacade(pub CB); - -impl Callbacks for CallbackErrorFacade { - type Error = Error; - - fn on_set_interface_config( - &self, - tunnel_address_v4: Ipv4Addr, - tunnel_address_v6: Ipv6Addr, - dns_addresses: Vec, - ) -> Result> { - let result = self - .0 - .on_set_interface_config(tunnel_address_v4, tunnel_address_v6, dns_addresses) - .map_err(|err| Error::OnSetInterfaceConfigFailed(err.to_string())); - if let Err(err) = result.as_ref() { - tracing::error!(?err); - } - result - } - - fn on_tunnel_ready(&self) -> Result<()> { - let result = self - .0 - .on_tunnel_ready() - .map_err(|err| Error::OnTunnelReadyFailed(err.to_string())); - if let Err(err) = result.as_ref() { - tracing::error!(?err); - } - result - } - - fn on_update_routes( - &self, - routes4: Vec, - routes6: Vec, - ) -> Result> { - let result = self - .0 - .on_update_routes(routes4, routes6) - .map_err(|err| Error::OnUpdateRoutesFailed(err.to_string())); - if let Err(err) = result.as_ref() { - tracing::error!(?err); - } - result - } - - fn on_update_resources(&self, resource_list: Vec) -> Result<()> { - let result = self - .0 - .on_update_resources(resource_list) - .map_err(|err| Error::OnUpdateResourcesFailed(err.to_string())); - if let Err(err) = result.as_ref() { - tracing::error!(?err); - } - result - } - - fn on_disconnect(&self, error: &Error) -> Result<()> { - if let Err(err) = self.0.on_disconnect(error) { - tracing::error!(?err, "`on_disconnect` failed"); - } - // There's nothing we can really do if `on_disconnect` fails. - Ok(()) - } - - fn roll_log_file(&self) -> Option { - self.0.roll_log_file() - } - - fn get_system_default_resolvers( - &self, - ) -> std::result::Result>, Self::Error> { - self.0 - .get_system_default_resolvers() - .map_err(|err| Error::GetSystemDefaultResolverFailed(err.to_string())) - } - - #[cfg(target_os = "android")] - fn protect_file_descriptor(&self, file_descriptor: std::os::fd::RawFd) -> Result<()> { - self.0 - .protect_file_descriptor(file_descriptor) - .map_err(|err| Error::ProtectFileDescriptorFailed(err.to_string())) - } -} diff --git a/rust/connlib/shared/src/error.rs b/rust/connlib/shared/src/error.rs index f7ce8519f..c3ec9f2b1 100644 --- a/rust/connlib/shared/src/error.rs +++ b/rust/connlib/shared/src/error.rs @@ -64,18 +64,6 @@ pub enum ConnlibError { /// Error when reading system's interface #[error("Error while reading system's interface")] IfaceRead(std::io::Error), - #[error("`on_set_interface_config` failed: {0}")] - OnSetInterfaceConfigFailed(String), - #[error("`on_tunnel_ready` failed: {0}")] - OnTunnelReadyFailed(String), - #[error("`on_update_resources` failed: {0}")] - OnUpdateResourcesFailed(String), - #[error("`on_update_routes` failed: {0}")] - OnUpdateRoutesFailed(String), - #[error("`get_system_default_resolvers` failed: {0}")] - GetSystemDefaultResolverFailed(String), - #[error("`protect_file_descriptor` failed: {0}")] - ProtectFileDescriptorFailed(String), /// Glob for errors without a type. #[error("Other error: {0}")] Other(&'static str), diff --git a/rust/connlib/shared/src/lib.rs b/rust/connlib/shared/src/lib.rs index 37ec6fbf6..3f9bd697a 100644 --- a/rust/connlib/shared/src/lib.rs +++ b/rust/connlib/shared/src/lib.rs @@ -4,7 +4,6 @@ //! we are using the same version across our own crates. mod callbacks; -mod callbacks_error_facade; pub mod error; pub mod messages; @@ -22,7 +21,6 @@ pub mod windows; pub use boringtun::x25519::PublicKey; pub use boringtun::x25519::StaticSecret; pub use callbacks::{Callbacks, Cidrv4, Cidrv6}; -pub use callbacks_error_facade::CallbackErrorFacade; pub use error::ConnlibError as Error; pub use error::Result; pub use phoenix_channel::{LoginUrl, LoginUrlError}; diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index e2a0ce5f8..41adbd33f 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -91,7 +91,7 @@ where .insert(resource_description.id(), resource_description.clone()); } - self.update_resource_list()?; + self.update_resource_list(); self.update_routes()?; Ok(()) @@ -115,9 +115,7 @@ where tracing::error!(%id, "Failed to update routes: {err:?}"); } - if let Err(err) = self.update_resource_list() { - tracing::error!("Failed to update resource list: {err:#?}") - } + self.update_resource_list(); let Some(gateway_id) = self.role_state.resources_gateways.remove(&id) else { tracing::debug!("No gateway associated with resource"); @@ -159,7 +157,7 @@ where tracing::debug!("Resource removed") } - fn update_resource_list(&self) -> connlib_shared::Result<()> { + fn update_resource_list(&self) { self.callbacks.on_update_resources( self.role_state .resource_ids @@ -167,8 +165,7 @@ where .sorted() .cloned() .collect_vec(), - )?; - Ok(()) + ); } /// Sets the interface configuration and starts background tasks. @@ -179,8 +176,6 @@ where config.upstream_dns.clone(), self.callbacks .get_system_default_resolvers() - .ok() - .flatten() .unwrap_or_default(), ); @@ -202,7 +197,7 @@ where .set_routes(self.role_state.routes().collect(), &self.callbacks)?; let name = self.io.device_mut().name().to_owned(); - self.callbacks.on_tunnel_ready()?; + self.callbacks.on_tunnel_ready(); tracing::debug!(ip4 = %config.ipv4, ip6 = %config.ipv6, %name, "TUN device initialized"); diff --git a/rust/connlib/tunnel/src/device_channel.rs b/rust/connlib/tunnel/src/device_channel.rs index 340760d0a..80c791172 100644 --- a/rust/connlib/tunnel/src/device_channel.rs +++ b/rust/connlib/tunnel/src/device_channel.rs @@ -68,7 +68,7 @@ impl Device { &mut self, config: &Interface, dns_config: Vec, - callbacks: &impl Callbacks, + callbacks: &impl Callbacks, ) -> Result<(), ConnlibError> { let tun = Tun::new(config, dns_config, callbacks)?; let mtu = ioctl::interface_mtu_by_name(tun.name())?; @@ -88,7 +88,7 @@ impl Device { &mut self, config: &Interface, dns_config: Vec, - _: &impl Callbacks, + _: &impl Callbacks, ) -> Result<(), ConnlibError> { let tun = Tun::new(config, dns_config)?; @@ -189,7 +189,7 @@ impl Device { pub(crate) fn set_routes( &mut self, routes: HashSet, - callbacks: &impl Callbacks, + callbacks: &impl Callbacks, ) -> Result<(), Error> { self.tun_mut()?.set_routes(routes, callbacks)?; Ok(()) diff --git a/rust/connlib/tunnel/src/device_channel/tun_android.rs b/rust/connlib/tunnel/src/device_channel/tun_android.rs index 6c258381e..db37a82c3 100644 --- a/rust/connlib/tunnel/src/device_channel/tun_android.rs +++ b/rust/connlib/tunnel/src/device_channel/tun_android.rs @@ -42,10 +42,10 @@ impl Tun { pub fn new( config: &InterfaceConfig, dns_config: Vec, - callbacks: &impl Callbacks, + callbacks: &impl Callbacks, ) -> Result { let fd = callbacks - .on_set_interface_config(config.ipv4, config.ipv6, dns_config)? + .on_set_interface_config(config.ipv4, config.ipv6, dns_config) .ok_or(Error::NoFd)?; // Safety: File descriptor is open. let name = unsafe { interface_name(fd)? }; @@ -63,13 +63,13 @@ impl Tun { pub fn set_routes( &mut self, routes: HashSet, - callbacks: &impl Callbacks, + callbacks: &impl Callbacks, ) -> Result<()> { let fd = callbacks .on_update_routes( routes.iter().copied().filter_map(ipv4).collect(), routes.iter().copied().filter_map(ipv6).collect(), - )? + ) .ok_or(Error::NoFd)?; // SAFETY: we expect the callback to return a valid file descriptor diff --git a/rust/connlib/tunnel/src/device_channel/tun_darwin.rs b/rust/connlib/tunnel/src/device_channel/tun_darwin.rs index 750bc5818..4f778dfee 100644 --- a/rust/connlib/tunnel/src/device_channel/tun_darwin.rs +++ b/rust/connlib/tunnel/src/device_channel/tun_darwin.rs @@ -73,7 +73,7 @@ impl Tun { pub fn new( config: &InterfaceConfig, dns_config: Vec, - callbacks: &impl Callbacks, + callbacks: &impl Callbacks, ) -> Result { let mut info = ctl_info { ctl_id: 0, @@ -131,7 +131,7 @@ impl Tun { } if addr.sc_id == info.ctl_id { - callbacks.on_set_interface_config(config.ipv4, config.ipv6, dns_config)?; + callbacks.on_set_interface_config(config.ipv4, config.ipv6, dns_config); set_non_blocking(fd)?; @@ -145,16 +145,12 @@ impl Tun { Err(get_last_error()) } - pub fn set_routes( - &self, - routes: HashSet, - callbacks: &impl Callbacks, - ) -> Result<()> { + pub fn set_routes(&self, routes: HashSet, callbacks: &impl Callbacks) -> Result<()> { // This will always be None in macos callbacks.on_update_routes( routes.iter().copied().filter_map(ipv4).collect(), routes.iter().copied().filter_map(ipv6).collect(), - )?; + ); Ok(()) } diff --git a/rust/connlib/tunnel/src/device_channel/tun_windows.rs b/rust/connlib/tunnel/src/device_channel/tun_windows.rs index ffe3fd19e..a058e19da 100644 --- a/rust/connlib/tunnel/src/device_channel/tun_windows.rs +++ b/rust/connlib/tunnel/src/device_channel/tun_windows.rs @@ -1,4 +1,4 @@ -use connlib_shared::{messages::Interface as InterfaceConfig, Callbacks, Error, Result}; +use connlib_shared::{messages::Interface as InterfaceConfig, Callbacks, Result}; use ip_network::IpNetwork; use std::{ collections::HashSet, @@ -160,7 +160,7 @@ impl Tun { pub fn set_routes( &mut self, new_routes: HashSet, - _callbacks: &impl Callbacks, + _callbacks: &impl Callbacks, ) -> Result<()> { for new_route in new_routes.difference(&self.routes) { self.add_route(*new_route)?; diff --git a/rust/connlib/tunnel/src/lib.rs b/rust/connlib/tunnel/src/lib.rs index a4ad0a794..8046d4adc 100644 --- a/rust/connlib/tunnel/src/lib.rs +++ b/rust/connlib/tunnel/src/lib.rs @@ -6,7 +6,7 @@ use boringtun::x25519::StaticSecret; use connlib_shared::{ messages::{ClientId, GatewayId, ResourceId, ReuseConnection}, - CallbackErrorFacade, Callbacks, Result, + Callbacks, Result, }; use io::Io; use std::{ @@ -41,7 +41,7 @@ pub type ClientTunnel = Tunnel; /// Tunnel is a wireguard state machine that uses webrtc's ICE channels instead of UDP sockets to communicate between peers. pub struct Tunnel { - pub callbacks: CallbackErrorFacade, + pub callbacks: CB, /// State that differs per role, i.e. clients vs gateways. role_state: TRoleState, @@ -59,8 +59,6 @@ where CB: Callbacks + 'static, { pub fn new(private_key: StaticSecret, callbacks: CB) -> Result { - let callbacks = CallbackErrorFacade(callbacks); - Ok(Self { io: new_io(&callbacks)?, callbacks, @@ -150,8 +148,6 @@ where CB: Callbacks + 'static, { pub fn new(private_key: StaticSecret, callbacks: CB) -> Result { - let callbacks = CallbackErrorFacade(callbacks); - Ok(Self { io: new_io(&callbacks)?, callbacks, @@ -223,7 +219,7 @@ where } #[cfg_attr(not(target_os = "android"), allow(unused_variables))] -fn new_io(callbacks: &CallbackErrorFacade) -> Result +fn new_io(callbacks: &CB) -> Result where CB: Callbacks, { @@ -233,10 +229,10 @@ where #[cfg(target_os = "android")] { if let Some(ip4_socket) = io.sockets_ref().ip4_socket_fd() { - callbacks.protect_file_descriptor(ip4_socket)?; + callbacks.protect_file_descriptor(ip4_socket); } if let Some(ip6_socket) = io.sockets_ref().ip6_socket_fd() { - callbacks.protect_file_descriptor(ip6_socket)?; + callbacks.protect_file_descriptor(ip6_socket); } } diff --git a/rust/gateway/src/main.rs b/rust/gateway/src/main.rs index ae00a8b9b..ec334ac9a 100644 --- a/rust/gateway/src/main.rs +++ b/rust/gateway/src/main.rs @@ -120,9 +120,7 @@ async fn run(login: LoginUrl, private_key: StaticSecret) -> Result { #[derive(Clone)] struct CallbackHandler; -impl Callbacks for CallbackHandler { - type Error = Infallible; -} +impl Callbacks for CallbackHandler {} #[derive(Parser)] #[command(author, version, about, long_about = None)] diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index ddede987d..de9d34471 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -449,39 +449,38 @@ struct CallbackHandler { resources: Arc>>, } -#[derive(thiserror::Error, Debug)] -enum CallbackError { - #[error("can't send to controller task: {0}")] - SendError(#[from] mpsc::error::TrySendError), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - // Callbacks must all be non-blocking impl connlib_client_shared::Callbacks for CallbackHandler { - type Error = CallbackError; - - fn on_disconnect(&self, error: &connlib_client_shared::Error) -> Result<(), Self::Error> { + fn on_disconnect(&self, error: &connlib_client_shared::Error) { tracing::debug!("on_disconnect {error:?}"); - self.ctlr_tx.try_send(ControllerRequest::Disconnected)?; - Ok(()) + self.ctlr_tx + .try_send(ControllerRequest::Disconnected) + .expect("controller channel failed"); } - fn on_tunnel_ready(&self) -> Result<(), Self::Error> { + fn on_tunnel_ready(&self) { tracing::info!("on_tunnel_ready"); - self.ctlr_tx.try_send(ControllerRequest::TunnelReady)?; - Ok(()) + self.ctlr_tx + .try_send(ControllerRequest::TunnelReady) + .expect("controller channel failed"); } - fn on_update_resources(&self, resources: Vec) -> Result<(), Self::Error> { + fn on_update_resources(&self, resources: Vec) { tracing::debug!("on_update_resources"); self.resources.store(resources.into()); self.notify_controller.notify_one(); - Ok(()) } - fn get_system_default_resolvers(&self) -> Result>, Self::Error> { - Ok(Some(client::resolvers::get()?)) + fn get_system_default_resolvers(&self) -> Option> { + let resolvers = match client::resolvers::get() { + Ok(resolvers) => resolvers, + Err(e) => { + tracing::error!("Failed to get system default resolvers: {e}"); + return None; + } + }; + + Some(resolvers) } fn roll_log_file(&self) -> Option { diff --git a/rust/linux-client/src/main.rs b/rust/linux-client/src/main.rs index 5a393dd66..46a1fc0e7 100644 --- a/rust/linux-client/src/main.rs +++ b/rust/linux-client/src/main.rs @@ -82,32 +82,32 @@ struct CallbackHandler { handle: Option, } -#[derive(Debug, thiserror::Error)] -enum CbError { - #[error(transparent)] - Any(#[from] anyhow::Error), -} - impl Callbacks for CallbackHandler { - // I spent several minutes messing with `anyhow` and couldn't figure out how to make - // it implement `std::error::Error`: - type Error = CbError; - /// May return Firezone's own servers, e.g. `100.100.111.1`. - fn get_system_default_resolvers(&self) -> Result>, Self::Error> { - let default_resolvers = match self.dns_control_method { - None => get_system_default_resolvers_resolv_conf()?, - Some(DnsControlMethod::EtcResolvConf) => get_system_default_resolvers_resolv_conf()?, + fn get_system_default_resolvers(&self) -> Option> { + let maybe_resolvers = match self.dns_control_method { + None => get_system_default_resolvers_resolv_conf(), + Some(DnsControlMethod::EtcResolvConf) => get_system_default_resolvers_resolv_conf(), Some(DnsControlMethod::NetworkManager) => { - get_system_default_resolvers_network_manager()? + get_system_default_resolvers_network_manager() } - Some(DnsControlMethod::Systemd) => get_system_default_resolvers_systemd_resolved()?, + Some(DnsControlMethod::Systemd) => get_system_default_resolvers_systemd_resolved(), }; - tracing::info!(?default_resolvers); - Ok(Some(default_resolvers)) + + let resolvers = match maybe_resolvers { + Ok(resolvers) => resolvers, + Err(e) => { + tracing::error!("Failed to get system default resolvers: {e}"); + return None; + } + }; + + tracing::info!(?resolvers); + + Some(resolvers) } - fn on_disconnect(&self, error: &connlib_client_shared::Error) -> Result<(), Self::Error> { + fn on_disconnect(&self, error: &connlib_client_shared::Error) { tracing::error!("Disconnected: {error}"); std::process::exit(1);