From 60bdbb39cb2e88699894f2f84a044f0ad39bbe24 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 11 Jun 2025 08:18:14 +0200 Subject: [PATCH] refactor(gui-client): move change listeners to tunnel service (#8160) At present, listening for DNS server change and network change events is handled in the GUI client. Upon an event, a message is sent to the tunnel service which then applies the new state to `connlib`. We can avoid some of this boilerplate by moving these listeners to the tunnel service as part of the handler. As a result, we get a few improvements: - We don't need to ignore these events if we don't have a session because the lifetime of these listeners is tied to the IPC handler on the service side. - We need fewer IPC messages - We can retry the connection directly from within the tunnel service in case we have no Internet at the time of startup - We can more easily model out the state machine of a connlib session in the tunnel service - On Linux, this means we no longer shell out to `resolvectl` from the GUI process, unifying access to the "resolvers" from the tunnel service - On Windows, we no longer need admin privileges on the GUI client for optimized network-change detection. This now happens in the Tunnel process which already runs as admin. Resolves: #9465 --- rust/bin-shared/src/dns_control.rs | 7 - rust/bin-shared/src/lib.rs | 2 +- rust/gui-client/src-tauri/src/controller.rs | 228 ++----------- .../src-tauri/src/gui/system_tray.rs | 11 - .../src-tauri/src/gui/system_tray/builder.rs | 2 - rust/gui-client/src-tauri/src/service.rs | 314 +++++++++++++----- 6 files changed, 261 insertions(+), 303 deletions(-) diff --git a/rust/bin-shared/src/dns_control.rs b/rust/bin-shared/src/dns_control.rs index ec1e422dc..ef98acb2d 100644 --- a/rust/bin-shared/src/dns_control.rs +++ b/rust/bin-shared/src/dns_control.rs @@ -5,7 +5,6 @@ //! //! On Windows, we use NRPT by default. We can also explicitly not control DNS. -use anyhow::Result; use std::net::IpAddr; #[cfg(target_os = "linux")] @@ -49,9 +48,3 @@ impl DnsController { system_resolvers(self.dns_control_method).unwrap_or_default() } } - -// TODO: Move DNS and network change listening to the Tunnel service, so this won't -// need to be public. -pub fn system_resolvers_for_gui() -> Result> { - system_resolvers(DnsControlMethod::default()) -} diff --git a/rust/bin-shared/src/lib.rs b/rust/bin-shared/src/lib.rs index 0caac00e1..62d25e4cb 100644 --- a/rust/bin-shared/src/lib.rs +++ b/rust/bin-shared/src/lib.rs @@ -53,6 +53,6 @@ pub const BUNDLE_ID: &str = "dev.firezone.client"; /// Mark for Firezone sockets to prevent routing loops on Linux. pub const FIREZONE_MARK: u32 = 0xfd002021; -pub use dns_control::{DnsControlMethod, DnsController, system_resolvers_for_gui}; +pub use dns_control::{DnsControlMethod, DnsController}; pub use network_changes::{new_dns_notifier, new_network_notifier}; pub use tun_device_manager::TunDeviceManager; diff --git a/rust/gui-client/src-tauri/src/controller.rs b/rust/gui-client/src-tauri/src/controller.rs index 3c3d4eef6..56ef2bd82 100644 --- a/rust/gui-client/src-tauri/src/controller.rs +++ b/rust/gui-client/src-tauri/src/controller.rs @@ -10,21 +10,14 @@ use crate::{ }; use anyhow::{Context, Result, anyhow, bail}; use connlib_model::ResourceView; -use firezone_bin_shared::DnsControlMethod; use firezone_logging::FilterReloadHandle; use firezone_telemetry::Telemetry; use futures::{ - SinkExt, Stream, StreamExt, + SinkExt, StreamExt, stream::{self, BoxStream}, }; use secrecy::{ExposeSecret as _, SecretString}; -use std::{ - collections::BTreeSet, - ops::ControlFlow, - path::PathBuf, - task::Poll, - time::{Duration, Instant}, -}; +use std::{collections::BTreeSet, ops::ControlFlow, path::PathBuf, task::Poll, time::Duration}; use tokio::sync::{mpsc, oneshot}; use tokio_stream::wrappers::ReceiverStream; use url::Url; @@ -59,9 +52,6 @@ pub struct Controller { ipc::ServerWrite, )>, >, - - dns_notifier: BoxStream<'static, Result<()>>, - network_notifier: BoxStream<'static, Result<()>>, } pub trait GuiIntegration { @@ -129,12 +119,6 @@ pub enum Failure { pub enum Status { /// Firezone is disconnected. Disconnected, - /// At least one connection request has failed, due to failing to reach the Portal, and we are waiting for a network change before we try again - RetryingConnection { - /// The token to log in to the Portal, for retrying the connection request. - #[debug(skip)] - token: SecretString, - }, Quitting, // The user asked to quit and we're waiting for the tunnel daemon to gracefully disconnect so we can flush telemetry. /// Firezone is ready to use. TunnelReady { @@ -142,18 +126,9 @@ pub enum Status { resources: Vec, }, /// Firezone is signing in to the Portal. - WaitingForPortal { - /// The instant when we sent our most recent connect request. - start_instant: Instant, - /// The token to log in to the Portal, in case we need to retry the connection request. - #[debug(skip)] - token: SecretString, - }, + WaitingForPortal, /// Firezone has connected to the Portal and is raising the tunnel. - WaitingForTunnel { - /// The instant when we sent our most recent connect request. - start_instant: Instant, - }, + WaitingForTunnel, } impl Default for Status { @@ -163,25 +138,11 @@ impl Default for Status { } impl Status { - /// True if we want to hear about DNS and network changes. - fn needs_network_changes(&self) -> bool { - match self { - Status::Disconnected | Status::RetryingConnection { .. } => false, - Status::Quitting => false, - Status::TunnelReady { .. } - | Status::WaitingForPortal { .. } - | Status::WaitingForTunnel { .. } => true, - } - } - /// True if we should react to `OnUpdateResources` fn needs_resource_updates(&self) -> bool { match self { - Status::Disconnected - | Status::RetryingConnection { .. } - | Status::Quitting - | Status::WaitingForPortal { .. } => false, - Status::TunnelReady { .. } | Status::WaitingForTunnel { .. } => true, + Status::Disconnected | Status::Quitting | Status::WaitingForPortal => false, + Status::TunnelReady { .. } | Status::WaitingForTunnel => true, } } @@ -197,8 +158,6 @@ impl Status { } enum EventloopTick { - NetworkChanged(Result<()>), - DnsChanged(Result<()>), IpcMsg(Option>), ControllerRequest(Option), UpdateNotification(Option), @@ -238,9 +197,6 @@ impl Controller { .await .map_err(FailedToReceiveHello)?; - let dns_notifier = new_dns_notifier().await?.boxed(); - let network_notifier = new_network_notifier().await?.boxed(); - let controller = Controller { general_settings, mdm_settings, @@ -257,8 +213,6 @@ impl Controller { status: Default::default(), updates_rx: ReceiverStream::new(updates_rx), uptime: Default::default(), - dns_notifier, - network_notifier, gui_ipc_clients: stream::unfold(gui_ipc, |mut gui_ipc| async move { let result = gui_ipc.next_client_split().await; @@ -298,31 +252,6 @@ impl Controller { loop { match self.tick().await { - EventloopTick::NetworkChanged(Ok(())) => { - if self.status.needs_network_changes() { - tracing::debug!("Internet up/down changed, calling `Session::reset`"); - self.send_ipc(&service::ClientMsg::Reset).await? - } - - self.try_retry_connection().await? - } - EventloopTick::DnsChanged(Ok(())) => { - if self.status.needs_network_changes() { - let resolvers = firezone_bin_shared::system_resolvers_for_gui()?; - tracing::debug!( - ?resolvers, - "New DNS resolvers, calling `Session::set_dns`" - ); - self.send_ipc(&service::ClientMsg::SetDns(resolvers)) - .await?; - } - - self.try_retry_connection().await? - } - EventloopTick::NetworkChanged(Err(e)) | EventloopTick::DnsChanged(Err(e)) => { - return Err(e); - } - EventloopTick::IpcMsg(msg) => { let msg = msg .context("IPC closed")? @@ -375,14 +304,6 @@ impl Controller { async fn tick(&mut self) -> EventloopTick { std::future::poll_fn(|cx| { - if let Poll::Ready(Some(res)) = self.dns_notifier.poll_next_unpin(cx) { - return Poll::Ready(EventloopTick::DnsChanged(res)); - } - - if let Poll::Ready(Some(res)) = self.network_notifier.poll_next_unpin(cx) { - return Poll::Ready(EventloopTick::NetworkChanged(res)); - } - if let Poll::Ready(maybe_ipc) = self.ipc_rx.poll_next_unpin(cx) { return Poll::Ready(EventloopTick::IpcMsg(maybe_ipc)); } @@ -406,12 +327,12 @@ impl Controller { async fn start_session(&mut self, token: SecretString) -> Result<()> { match self.status { - Status::Disconnected | Status::RetryingConnection { .. } => {} + Status::Disconnected => {} Status::Quitting => Err(anyhow!("Can't connect to Firezone, we're quitting"))?, Status::TunnelReady { .. } => Err(anyhow!( "Can't connect to Firezone, we're already connected." ))?, - Status::WaitingForPortal { .. } | Status::WaitingForTunnel { .. } => Err(anyhow!( + Status::WaitingForPortal | Status::WaitingForTunnel => Err(anyhow!( "Can't connect to Firezone, we're already connecting." ))?, } @@ -419,8 +340,6 @@ impl Controller { let api_url = self.api_url().clone(); tracing::info!(api_url = api_url.to_string(), "Starting connlib..."); - // Count the start instant from before we connect - let start_instant = Instant::now(); self.send_ipc(&service::ClientMsg::Connect { api_url: api_url.to_string(), token: token.expose_secret().clone(), @@ -428,10 +347,7 @@ impl Controller { .await?; // Change the status after we begin connecting - self.status = Status::WaitingForPortal { - start_instant, - token, - }; + self.status = Status::WaitingForPortal; let session = self.auth.session().context("Missing session")?; @@ -582,9 +498,7 @@ impl Controller { .set_text(s) .context("Couldn't copy resource URL or other text to clipboard")?, SystemTrayMenu(system_tray::Event::CancelSignIn) => match &self.status { - Status::Disconnected - | Status::RetryingConnection { .. } - | Status::WaitingForPortal { .. } => { + Status::Disconnected | Status::WaitingForPortal => { tracing::info!("Calling `sign_out` to cancel sign-in"); self.sign_out().await?; } @@ -592,7 +506,7 @@ impl Controller { Status::TunnelReady { .. } => tracing::error!( "Can't cancel sign-in, the tunnel is already up. This is a logic error in the code." ), - Status::WaitingForTunnel { .. } => { + Status::WaitingForTunnel => { tracing::debug!( "Connlib is already raising the tunnel, calling `sign_out` anyway" ); @@ -605,9 +519,6 @@ impl Controller { .remove(&resource_id); self.refresh_favorite_resources().await?; } - SystemTrayMenu(system_tray::Event::RetryPortalConnection) => { - self.try_retry_connection().await? - } SystemTrayMenu(system_tray::Event::EnableInternetResource) => { self.general_settings.internet_resource_enabled = Some(true); self.update_disabled_resources().await?; @@ -728,10 +639,20 @@ impl Controller { if !self.status.needs_resource_updates() { return Ok(ControlFlow::Continue(())); } - tracing::debug!(len = resources.len(), "Got new Resources"); - self.status = Status::TunnelReady { resources }; - self.refresh_ui_state(); + // If this is the first time we receive resources, show the notification that we are connected. + if let &Status::WaitingForTunnel = &self.status { + self.integration.show_notification( + "Firezone connected", + "You are now signed in and able to access resources.", + )?; + } + + tracing::debug!(len = resources.len(), "Got new Resources"); + + self.status = Status::TunnelReady { resources }; + + self.refresh_ui_state(); self.update_disabled_resources().await?; } service::ServerMsg::TerminatingGracefully => { @@ -745,20 +666,6 @@ impl Controller { return Ok(ControlFlow::Break(())); } - service::ServerMsg::TunnelReady => { - let Status::WaitingForTunnel { start_instant } = self.status else { - // If we are not waiting for a tunnel, continue. - return Ok(ControlFlow::Continue(())); - }; - - tracing::info!(elapsed = ?start_instant.elapsed(), "Tunnel ready"); - self.status = Status::TunnelReady { resources: vec![] }; - self.integration.show_notification( - "Firezone connected", - "You are now signed in and able to access resources.", - )?; - self.refresh_ui_state(); - } service::ServerMsg::Hello => {} } Ok(ControlFlow::Continue(())) @@ -797,15 +704,8 @@ impl Controller { Ok(()) } - async fn handle_connect_result( - &mut self, - result: Result<(), service::ConnectError>, - ) -> Result<()> { - let Status::WaitingForPortal { - start_instant, - token, - } = &self.status - else { + async fn handle_connect_result(&mut self, result: Result<(), String>) -> Result<()> { + let Status::WaitingForPortal = &self.status else { tracing::debug!(current_state = ?self.status, "Ignoring `ConnectResult`"); return Ok(()); @@ -814,26 +714,11 @@ impl Controller { match result { Ok(()) => { ran_before::set().await?; - self.status = Status::WaitingForTunnel { - start_instant: *start_instant, - }; + self.status = Status::WaitingForTunnel; self.refresh_ui_state(); Ok(()) } - Err(service::ConnectError::Io(error)) => { - // This is typically something like, we don't have Internet access so we can't - // open the PhoenixChannel's WebSocket. - tracing::info!( - error, - "Failed to connect to Firezone Portal, will try again when the network changes" - ); - self.status = Status::RetryingConnection { - token: token.expose_secret().clone().into(), - }; - self.refresh_ui_state(); - Ok(()) - } - Err(service::ConnectError::Other(error)) => { + Err(error) => { // We log this here directly instead of forwarding it because errors hard-abort the event-loop and we still want to be able to export logs and stuff. // See . tracing::error!("Failed to connect to Firezone: {error}"); @@ -921,10 +806,6 @@ impl Controller { system_tray::ConnlibState::Quitting, SessionViewModel::Loading, ), - Status::RetryingConnection { .. } => ( - system_tray::ConnlibState::RetryingConnection, - SessionViewModel::Loading, - ), Status::TunnelReady { resources } => ( system_tray::ConnlibState::SignedIn(system_tray::SignedIn { actor_name: auth_session.actor_name.clone(), @@ -937,11 +818,11 @@ impl Controller { actor_name: auth_session.actor_name.clone(), }, ), - Status::WaitingForPortal { .. } => ( + Status::WaitingForPortal => ( system_tray::ConnlibState::WaitingForPortal, SessionViewModel::Loading, ), - Status::WaitingForTunnel { .. } => ( + Status::WaitingForTunnel => ( system_tray::ConnlibState::WaitingForTunnel, SessionViewModel::Loading, ), @@ -978,31 +859,14 @@ impl Controller { } } - /// If we're in the `RetryingConnection` state, use the token to retry the Portal connection - async fn try_retry_connection(&mut self) -> Result<()> { - let token = match &self.status { - Status::Disconnected - | Status::Quitting - | Status::TunnelReady { .. } - | Status::WaitingForPortal { .. } - | Status::WaitingForTunnel { .. } => return Ok(()), - Status::RetryingConnection { token } => token, - }; - tracing::debug!("Retrying Portal connection..."); - self.start_session(token.expose_secret().clone().into()) - .await?; - Ok(()) - } - /// Deletes the auth token, stops connlib, and refreshes the tray menu async fn sign_out(&mut self) -> Result<()> { match self.status { Status::Quitting => return Ok(()), Status::Disconnected - | Status::RetryingConnection { .. } | Status::TunnelReady { .. } - | Status::WaitingForPortal { .. } - | Status::WaitingForTunnel { .. } => {} + | Status::WaitingForPortal + | Status::WaitingForTunnel => {} } self.auth.sign_out()?; self.status = Status::Disconnected; @@ -1059,34 +923,6 @@ impl Controller { } } -async fn new_dns_notifier() -> Result>> { - let worker = firezone_bin_shared::new_dns_notifier( - tokio::runtime::Handle::current(), - DnsControlMethod::default(), - ) - .await?; - - Ok(stream::try_unfold(worker, |mut worker| async move { - let () = worker.notified().await?; - - Ok(Some(((), worker))) - })) -} - -async fn new_network_notifier() -> Result>> { - let worker = firezone_bin_shared::new_network_notifier( - tokio::runtime::Handle::current(), - DnsControlMethod::default(), - ) - .await?; - - Ok(stream::try_unfold(worker, |mut worker| async move { - let () = worker.notified().await?; - - Ok(Some(((), worker))) - })) -} - async fn receive_hello(ipc_rx: &mut ipc::ClientRead) -> Result<()> { const TIMEOUT: Duration = Duration::from_secs(5); diff --git a/rust/gui-client/src-tauri/src/gui/system_tray.rs b/rust/gui-client/src-tauri/src/gui/system_tray.rs index 5acc73e61..529035cc3 100644 --- a/rust/gui-client/src-tauri/src/gui/system_tray.rs +++ b/rust/gui-client/src-tauri/src/gui/system_tray.rs @@ -114,7 +114,6 @@ impl Tray { let base = match &state.connlib { ConnlibState::Loading | ConnlibState::Quitting - | ConnlibState::RetryingConnection | ConnlibState::WaitingForBrowser | ConnlibState::WaitingForPortal | ConnlibState::WaitingForTunnel => IconBase::Busy, @@ -269,7 +268,6 @@ impl AppState { let quit_text = match &self.connlib { ConnlibState::Loading | ConnlibState::Quitting - | ConnlibState::RetryingConnection | ConnlibState::SignedOut | ConnlibState::WaitingForBrowser | ConnlibState::WaitingForPortal @@ -279,7 +277,6 @@ impl AppState { let menu = match self.connlib { ConnlibState::Loading => Menu::default().disabled("Loading..."), ConnlibState::Quitting => Menu::default().disabled("Quitting..."), - ConnlibState::RetryingConnection => retrying_sign_in("Waiting for Internet access..."), ConnlibState::SignedIn(x) => signed_in(&x), ConnlibState::SignedOut => Menu::default().item(Event::SignIn, "Sign In"), ConnlibState::WaitingForBrowser => signing_in("Waiting for browser..."), @@ -298,7 +295,6 @@ impl AppState { pub enum ConnlibState { Loading, Quitting, - RetryingConnection, SignedIn(SignedIn), SignedOut, WaitingForBrowser, @@ -465,13 +461,6 @@ fn signed_in(signed_in: &SignedIn) -> Menu { menu } -fn retrying_sign_in(waiting_message: &str) -> Menu { - Menu::default() - .disabled(waiting_message) - .item(Event::RetryPortalConnection, "Retry sign-in") - .item(Event::CancelSignIn, "Cancel sign-in") -} - fn signing_in(waiting_message: &str) -> Menu { Menu::default() .disabled(waiting_message) diff --git a/rust/gui-client/src-tauri/src/gui/system_tray/builder.rs b/rust/gui-client/src-tauri/src/gui/system_tray/builder.rs index 63e4f711c..5b51fa6dd 100644 --- a/rust/gui-client/src-tauri/src/gui/system_tray/builder.rs +++ b/rust/gui-client/src-tauri/src/gui/system_tray/builder.rs @@ -50,8 +50,6 @@ pub enum Event { Copy(String), /// Marks this Resource as non-favorite RemoveFavorite(ResourceId), - /// If a Portal connection has failed, try again immediately - RetryPortalConnection, /// Starts the sign-in flow SignIn, /// Signs the user out, without quitting the app diff --git a/rust/gui-client/src-tauri/src/service.rs b/rust/gui-client/src-tauri/src/service.rs index 49f314ee6..0cb11613e 100644 --- a/rust/gui-client/src-tauri/src/service.rs +++ b/rust/gui-client/src-tauri/src/service.rs @@ -16,8 +16,9 @@ use firezone_bin_shared::{ use firezone_logging::{FilterReloadHandle, err_with_src, telemetry_span}; use firezone_telemetry::{Telemetry, analytics}; use futures::{ - Future as _, SinkExt as _, Stream as _, + Future as _, SinkExt as _, Stream, StreamExt, future::poll_fn, + stream::{self, BoxStream}, task::{Context, Poll}, }; use phoenix_channel::{DeviceInfo, LoginUrl, PhoenixChannel, get_user_agent}; @@ -25,7 +26,7 @@ use secrecy::{Secret, SecretString}; use std::{ collections::BTreeSet, io::{self, Write}, - net::IpAddr, + mem, pin::pin, sync::Arc, time::Duration, @@ -58,8 +59,6 @@ pub enum ClientMsg { ApplyLogFilter { directives: String, }, - Reset, - SetDns(Vec), SetDisabledResources(BTreeSet), StartTelemetry { environment: String, @@ -74,7 +73,7 @@ pub enum ServerMsg { Hello, /// The Tunnel service finished clearing its log dir. ClearedLogs(Result<(), String>), - ConnectResult(Result<(), ConnectError>), + ConnectResult(Result<(), String>), DisconnectedGracefully, OnDisconnect { error_msg: String, @@ -87,28 +86,11 @@ pub enum ServerMsg { /// "Firezone is updating, please restart the GUI" instead of an error like, /// "IPC connection closed". TerminatingGracefully, - /// The interface and tunnel are ready for traffic. - TunnelReady, } -// All variants are `String` because almost no error type implements `Serialize` -#[derive(Debug, serde::Deserialize, serde::Serialize, thiserror::Error)] -pub enum ConnectError { - #[error("IO error: {0}")] - Io(String), - #[error("{0}")] - Other(String), -} - -impl From for ConnectError { - fn from(v: io::Error) -> Self { - Self::Io(v.to_string()) - } -} - -impl From for ConnectError { - fn from(v: anyhow::Error) -> Self { - Self::Other(format!("{v:#}")) +impl ServerMsg { + fn connect_result(result: Result<()>) -> Self { + Self::ConnectResult(result.map_err(|e| format!("{e:#}"))) } } @@ -170,16 +152,88 @@ struct Handler<'a> { dns_controller: &'a mut DnsController, ipc_rx: ipc::ServerRead, ipc_tx: ipc::ServerWrite, - last_connlib_start_instant: Option, log_filter_reloader: &'a FilterReloadHandle, - session: Option, + session: Session, telemetry: Telemetry, tun_device: TunDeviceManager, + dns_notifier: BoxStream<'static, Result<()>>, + network_notifier: BoxStream<'static, Result<()>>, } -struct Session { - event_stream: client_shared::EventStream, - connlib: client_shared::Session, +#[derive(Default)] +enum Session { + /// We've launched `connlib` but haven't heard back from it yet. + Creating { + event_stream: client_shared::EventStream, + connlib: client_shared::Session, + started_at: Instant, + }, + Connected { + event_stream: client_shared::EventStream, + connlib: client_shared::Session, + }, + WaitingForNetwork { + api_url: String, + token: SecretString, + }, + #[default] + None, +} + +impl Session { + fn transition_to_connected(&mut self) -> Result<()> { + match mem::take(self) { + Session::Creating { + event_stream, + connlib, + started_at, + } => { + tracing::info!(elapsed = ?started_at.elapsed(), "Tunnel ready"); + + *self = Self::Connected { + event_stream, + connlib, + }; + } + Session::Connected { + event_stream, + connlib, + } => { + *self = Self::Connected { + event_stream, + connlib, + }; + } + Session::WaitingForNetwork { .. } => { + bail!("Invalid state! Cannot transition into `Connected` from `WaitingForNetwork`") + } + Session::None => bail!("No session"), + } + + Ok(()) + } + + fn as_connlib(&self) -> Option<&client_shared::Session> { + match self { + Session::Creating { connlib, .. } => Some(connlib), + Session::Connected { connlib, .. } => Some(connlib), + Session::WaitingForNetwork { .. } => None, + Session::None => None, + } + } + + fn as_event_stream(&mut self) -> Option<&mut client_shared::EventStream> { + match self { + Session::Creating { event_stream, .. } => Some(event_stream), + Session::Connected { event_stream, .. } => Some(event_stream), + Session::WaitingForNetwork { .. } => None, + Session::None => None, + } + } + + fn is_none(&self) -> bool { + matches!(self, Self::None) + } } enum Event { @@ -189,6 +243,8 @@ enum Event { IpcDisconnected, IpcError(anyhow::Error), Terminate, + NetworkChanged(Result<()>), + DnsChanged(Result<()>), } // Open to better names @@ -218,6 +274,8 @@ impl<'a> Handler<'a> { .await .context("Failed to wait for incoming IPC connection from a GUI")?; let tun_device = TunDeviceManager::new(ip_packet::MAX_IP_SIZE, 1)?; + let dns_notifier = new_dns_notifier().await?.boxed(); + let network_notifier = new_network_notifier().await?.boxed(); ipc_tx .send(&ServerMsg::Hello) @@ -229,11 +287,12 @@ impl<'a> Handler<'a> { dns_controller, ipc_rx, ipc_tx, - last_connlib_start_instant: None, log_filter_reloader, - session: None, + session: Session::None, telemetry: Telemetry::default(), tun_device, + dns_notifier, + network_notifier, }) } @@ -282,6 +341,60 @@ impl<'a> Handler<'a> { let _ = self.send_ipc(ServerMsg::TerminatingGracefully).await; break HandlerOk::ServiceTerminating; } + Event::NetworkChanged(Err(e)) => { + tracing::warn!("Error while listening for network change events: {e:#}") + } + Event::DnsChanged(Err(e)) => { + tracing::warn!("Error while listening for DNS change events: {e:#}") + } + Event::NetworkChanged(Ok(())) => match &self.session { + Session::Creating { .. } => { + tracing::debug!("Ignoring network change since we're still signing in"); + } + Session::Connected { connlib, .. } => { + connlib.reset(); + } + Session::WaitingForNetwork { api_url, token } => { + tracing::info!("Attempting to re-connect upon network change"); + + let result = self.try_connect(&api_url.clone(), token.clone()); + + if let Some(e) = result + .as_ref() + .err() + .and_then(|e| e.root_cause().downcast_ref::()) + { + tracing::debug!("Still cannot connect to Firezone: {e}"); + + continue; + } + + let msg = match result { + Ok(session) => { + self.session = session; + + ServerMsg::connect_result(Ok(())) + } + Err(e) => ServerMsg::connect_result(Err(e)), + }; + + let _ = self + .ipc_tx + .send(&msg) + .await + .context("Failed to send `ConnectResult`"); + } + Session::None => continue, + }, + Event::DnsChanged(Ok(())) => { + let Session::Connected { connlib, .. } = &self.session else { + continue; + }; + + let resolvers = self.dns_controller.system_resolvers(); + + connlib.set_dns(resolvers); + } } }; @@ -299,6 +412,15 @@ impl<'a> Handler<'a> { if let Poll::Ready(()) = signals.poll_recv(cx) { return Poll::Ready(Event::Terminate); } + + if let Poll::Ready(Some(result)) = self.network_notifier.poll_next_unpin(cx) { + return Poll::Ready(Event::NetworkChanged(result)); + } + + if let Poll::Ready(Some(result)) = self.dns_notifier.poll_next_unpin(cx) { + return Poll::Ready(Event::DnsChanged(result)); + } + // `FramedRead::next` is cancel-safe. if let Poll::Ready(result) = pin!(&mut self.ipc_rx).poll_next(cx) { return Poll::Ready(match result { @@ -307,21 +429,23 @@ impl<'a> Handler<'a> { None => Event::IpcDisconnected, }); } - if let Some(session) = self.session.as_mut() { - if let Poll::Ready(option) = session.event_stream.poll_next(cx) { + + if let Some(event_stream) = self.session.as_event_stream() { + if let Poll::Ready(option) = event_stream.poll_next(cx) { return Poll::Ready(match option { Some(x) => Event::Connlib(x), None => Event::CallbackChannelClosed, }); } } + Poll::Pending } async fn handle_connlib_event(&mut self, msg: client_shared::Event) -> Result<()> { match msg { client_shared::Event::Disconnected(error) => { - let _ = self.session.take(); + self.session = Session::None; self.dns_controller.deactivate()?; self.send_ipc(ServerMsg::OnDisconnect { error_msg: error.to_string(), @@ -337,15 +461,12 @@ impl<'a> Handler<'a> { ipv4_routes, ipv6_routes, } => { + self.session.transition_to_connected()?; + self.tun_device.set_ips(ipv4, ipv6).await?; self.dns_controller.set_dns(dns, search_domain).await?; - if let Some(instant) = self.last_connlib_start_instant.take() { - tracing::info!(elapsed = ?instant.elapsed(), "Tunnel ready"); - } self.tun_device.set_routes(ipv4_routes, ipv6_routes).await?; self.dns_controller.flush()?; - - self.send_ipc(ServerMsg::TunnelReady).await?; } client_shared::Event::ResourcesUpdated(resources) => { // On every resources update, flush DNS to mitigate @@ -365,16 +486,38 @@ impl<'a> Handler<'a> { .await? } ClientMsg::Connect { api_url, token } => { - // Warning: Connection errors don't bubble to callers of `handle_ipc_msg`. - let token = secrecy::SecretString::from(token); - let result = self.connect_to_firezone(&api_url, token); + let token = SecretString::new(token); - self.send_ipc(ServerMsg::ConnectResult(result)).await? + let result = self.try_connect(&api_url, token.clone()); + + if let Some(e) = result + .as_ref() + .err() + .and_then(|e| e.root_cause().downcast_ref::()) + { + tracing::debug!( + "Encountered IO error when connecting to portal, most likely we don't have Internet: {e}" + ); + self.session = Session::WaitingForNetwork { api_url, token }; + + return Ok(()); + } + + let msg = match result { + Ok(session) => { + self.session = session; + + ServerMsg::connect_result(Ok(())) + } + Err(e) => ServerMsg::connect_result(Err(e)), + }; + + self.send_ipc(msg).await?; } ClientMsg::Disconnect => { - if self.session.take().is_some() { - self.dns_controller.deactivate()?; - } + self.session = Session::None; + self.dns_controller.deactivate()?; + // Always send `DisconnectedGracefully` even if we weren't connected, // so this will be idempotent. self.send_ipc(ServerMsg::DisconnectedGracefully).await?; @@ -390,35 +533,14 @@ impl<'a> Handler<'a> { tracing::warn!(path = %path.display(), %directives, "Failed to write new log directives: {}", err_with_src(&e)); } } - ClientMsg::Reset => { - if self.last_connlib_start_instant.is_some() { - tracing::debug!("Ignoring reset since we're still signing in"); - return Ok(()); - } - let Some(session) = self.session.as_ref() else { - tracing::debug!("Cannot reset if we're signed out"); - return Ok(()); - }; - - session.connlib.reset(); - } - ClientMsg::SetDns(resolvers) => { - let Some(session) = self.session.as_ref() else { - tracing::debug!("Cannot set DNS resolvers if we're signed out"); - return Ok(()); - }; - - tracing::debug!(?resolvers); - session.connlib.set_dns(resolvers); - } ClientMsg::SetDisabledResources(disabled_resources) => { - let Some(session) = self.session.as_ref() else { + let Some(connlib) = self.session.as_connlib() else { // At this point, the GUI has already saved the disabled Resources to disk, so it'll be correct on the next sign-in anyway. tracing::debug!("Cannot set disabled resources if we're signed out"); return Ok(()); }; - session.connlib.set_disabled_resources(disabled_resources); + connlib.set_disabled_resources(disabled_resources); } ClientMsg::StartTelemetry { environment, @@ -439,14 +561,10 @@ impl<'a> Handler<'a> { /// Connects connlib /// - /// Panics if there's no Tokio runtime or if connlib is already connected - /// - /// Throws matchable errors for bad URLs, unable to reach the portal, or unable to create the tunnel device - fn connect_to_firezone( - &mut self, - api_url: &str, - token: SecretString, - ) -> Result<(), ConnectError> { + /// Panics if there's no Tokio runtime or if connlib is already connected. + fn try_connect(&mut self, api_url: &str, token: SecretString) -> Result { + let started_at = Instant::now(); + let _connect_span = telemetry_span!("connect_to_firezone").entered(); assert!(self.session.is_none()); @@ -466,8 +584,6 @@ impl<'a> Handler<'a> { ) .context("Failed to create `LoginUrl`")?; - self.last_connlib_start_instant = Some(Instant::now()); - // Synchronous DNS resolution here let portal = PhoenixChannel::disconnected( Secret::new(url), @@ -480,7 +596,7 @@ impl<'a> Handler<'a> { .build() }, Arc::new(tcp_socket_factory), - )?; // Turn this `io::Error` directly into an `Error` so we can distinguish it from others in the GUI client. + )?; // Read the resolvers before starting connlib, in case connlib's startup interferes. let dns = self.dns_controller.system_resolvers(); @@ -506,13 +622,11 @@ impl<'a> Handler<'a> { }; connlib.set_tun(tun); - let session = Session { + Ok(Session::Creating { event_stream, connlib, - }; - self.session = Some(session); - - Ok(()) + started_at, + }) } async fn send_ipc(&mut self, msg: ServerMsg) -> Result<()> { @@ -590,3 +704,31 @@ pub fn run_smoke_test() -> Result<()> { pub fn run_smoke_test() -> Result<()> { anyhow::bail!("Smoke test is not built for release binaries."); } + +async fn new_dns_notifier() -> Result>> { + let worker = firezone_bin_shared::new_dns_notifier( + tokio::runtime::Handle::current(), + DnsControlMethod::default(), + ) + .await?; + + Ok(stream::try_unfold(worker, |mut worker| async move { + let () = worker.notified().await?; + + Ok(Some(((), worker))) + })) +} + +async fn new_network_notifier() -> Result>> { + let worker = firezone_bin_shared::new_network_notifier( + tokio::runtime::Handle::current(), + DnsControlMethod::default(), + ) + .await?; + + Ok(stream::try_unfold(worker, |mut worker| async move { + let () = worker.notified().await?; + + Ok(Some(((), worker))) + })) +}