github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 10:18:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(gui-client): simplify IPC and how Resources in the menu are updated (#5824)

Browse Source 
The Arc+Notify thing was always overkill, I just thought it was useful
early on. With the IPC change it's easier to just use the existing MPSC
channel

Also removing `TunnelReady` and assuming that the tunnel is ready
whenever connlib sends us the first Resource list

---------

Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
This commit is contained in:
Reactor Scram
2024-07-10 21:37:59 +00:00
committed by GitHub
parent c3380daa75
commit c8c349ac41
8 changed files with 67 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
rust/Cargo.lock generated
View File

@@ -172,12 +172,6 @@ dependencies = [
"x11rb",
]
[[package]]
name = "arc-swap"
version = "1.7.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "69f7f8c3906b62b754cd5326047894316021dcfe5a194c8ea52bdd94934a3457"
[[package]]
name = "array-init"
version = "0.0.4"
@@ -1888,7 +1882,6 @@ version = "1.1.6"
dependencies = [
"anyhow",
"arboard",
"arc-swap",
"atomicwrites",
"chrono",
"clap",

1
rust/gui-client/src-tauri/Cargo.toml
View File

@@ -14,7 +14,6 @@ tauri-build = { version = "1.5", features = [] }
[dependencies]
arboard = { version = "3.4.0", default-features = false }
anyhow = { version = "1.0" }
arc-swap = "1.7.0"
atomicwrites = "0.4.3"
chrono = { workspace = true }
clap = { version = "4.5", features = ["derive", "env"] }

127
rust/gui-client/src-tauri/src/client/gui.rs
View File

@@ -4,20 +4,18 @@
//! The real macOS Client is in `swift/apple`
use crate::client::{
self, about, deep_link,
ipc::{self, CallbackHandler},
logging, network_changes,
self, about, deep_link, ipc, logging, network_changes,
settings::{self, AdvancedSettings},
Failure,
};
use anyhow::{anyhow, bail, Context, Result};
use arc_swap::ArcSwap;
use connlib_client_shared::callbacks::ResourceDescription;
use firezone_headless_client::IpcServerMsg;
use secrecy::{ExposeSecret, SecretString};
use std::{path::PathBuf, str::FromStr, sync::Arc, time::Duration};
use std::{path::PathBuf, str::FromStr, time::Duration};
use system_tray_menu::Event as TrayMenuEvent;
use tauri::{Manager, SystemTray, SystemTrayEvent};
use tokio::sync::{mpsc, oneshot, Notify};
use tokio::sync::{mpsc, oneshot};
use tracing::instrument;
use url::Url;
@@ -410,10 +408,6 @@ pub(crate) enum ControllerRequest {
ApplySettings(AdvancedSettings),
/// Only used for smoke tests
ClearLogs,
Disconnected {
error_msg: String,
is_authentication_error: bool,
},
/// The same as the arguments to `client::logging::export_logs_to`
ExportLogs {
path: PathBuf,
@@ -421,10 +415,10 @@ pub(crate) enum ControllerRequest {
},
Fail(Failure),
GetAdvancedSettings(oneshot::Sender<AdvancedSettings>),
Ipc(IpcServerMsg),
SchemeRequest(SecretString),
SignIn,
SystemTrayMenu(TrayMenuEvent),
TunnelReady,
UpdateAvailable(crate::client::updates::Release),
UpdateNotificationClicked(Url),
}
@@ -435,7 +429,7 @@ enum Status {
/// Firezone is signing in and raising the tunnel.
Connecting,
/// Firezone is ready to use.
TunnelReady,
TunnelReady { resources: Vec<ResourceDescription> },
}
impl Default for Status {
@@ -450,7 +444,7 @@ impl Status {
match self {
Self::Disconnected => false,
Self::Connecting => true,
Self::TunnelReady => true,
Self::TunnelReady { .. } => true,
}
}
}
@@ -464,9 +458,6 @@ struct Controller {
ctlr_tx: CtlrTx,
ipc_client: ipc::Client,
log_filter_reloader: logging::Reloader,
/// Tells us when to wake up and look for a new resource list. Tokio docs say that memory reads and writes are synchronized when notifying, so we don't need an extra mutex on the resources.
notify_controller: Arc<Notify>,
resources: Arc<ArcSwap<Vec<ResourceDescription>>>,
status: Status,
uptime: client::uptime::Tracker,
}
@@ -523,27 +514,6 @@ impl Controller {
Req::ClearLogs => logging::clear_logs_inner()
.await
.context("Failed to clear logs")?,
Req::Disconnected {
error_msg,
is_authentication_error,
} => {
self.sign_out().await?;
if is_authentication_error {
tracing::info!(?error_msg, "Auth error");
os::show_notification(
"Firezone disconnected",
"To access resources, sign in again.",
)?;
} else {
tracing::error!(?error_msg, "Disconnected");
native_dialog::MessageDialog::new()
.set_title("Firezone Error")
.set_text(&error_msg)
.set_type(native_dialog::MessageType::Error)
.show_alert()
.context("Couldn't show Disconnected alert")?;
}
}
Req::ExportLogs { path, stem } => logging::export_logs_to(path, stem)
.await
.context("Failed to export logs to zip")?,
@@ -553,6 +523,9 @@ impl Controller {
Req::GetAdvancedSettings(tx) => {
tx.send(self.advanced_settings.clone()).ok();
}
Req::Ipc(msg) => if let Err(error) = self.handle_ipc(msg).await {
tracing::error!(?error, "Failed to handle IPC message");
}
Req::SchemeRequest(url) => {
if let Err(error) = self.handle_deep_link(&url).await {
tracing::error!(?error, "`handle_deep_link` failed");
@@ -597,7 +570,7 @@ impl Controller {
);
self.sign_out().await?;
}
Status::TunnelReady => tracing::error!("Can't cancel sign-in, the tunnel is already up. This is a logic error in the code."),
Status::TunnelReady{..} => tracing::error!("Can't cancel sign-in, the tunnel is already up. This is a logic error in the code."),
}
}
Req::SystemTrayMenu(TrayMenuEvent::ShowWindow(window)) => {
@@ -623,16 +596,6 @@ impl Controller {
Req::SystemTrayMenu(TrayMenuEvent::Quit) => Err(anyhow!(
"Impossible error: `Quit` should be handled before this"
))?,
Req::TunnelReady => {
if ! matches!(self.status, Status::TunnelReady) {
self.status = Status::TunnelReady;
os::show_notification(
"Firezone connected",
"You are now signed in and able to access resources.",
)?;
}
self.refresh_system_tray_menu()?;
}
Req::UpdateAvailable(release) => {
let title = format!("Firezone {} available for download", release.version);
@@ -650,6 +613,53 @@ impl Controller {
Ok(())
}
async fn handle_ipc(&mut self, msg: IpcServerMsg) -> Result<()> {
match msg {
IpcServerMsg::OnDisconnect {
error_msg,
is_authentication_error,
} => {
self.sign_out().await?;
if is_authentication_error {
tracing::info!(?error_msg, "Auth error");
os::show_notification(
"Firezone disconnected",
"To access resources, sign in again.",
)?;
} else {
tracing::error!(?error_msg, "Disconnected");
native_dialog::MessageDialog::new()
.set_title("Firezone Error")
.set_text(&error_msg)
.set_type(native_dialog::MessageType::Error)
.show_alert()
.context("Couldn't show Disconnected alert")?;
}
}
IpcServerMsg::OnUpdateResources(resources) => {
if self.auth.session().is_none() {
// This could happen if the user cancels the sign-in
// before it completes. This is because the state machine
// between the GUI, the IPC service, and connlib isn't perfectly synced.
tracing::error!("Got `UpdateResources` while signed out");
return Ok(());
}
tracing::debug!(len = resources.len(), "Got new Resources");
if !matches!(self.status, Status::TunnelReady { .. }) {
os::show_notification(
"Firezone connected",
"You are now signed in and able to access resources.",
)?;
}
self.status = Status::TunnelReady { resources };
if let Err(error) = self.refresh_system_tray_menu() {
tracing::error!(?error, "Failed to refresh Resource list");
}
}
}
Ok(())
}
/// Returns a new system tray menu
fn build_system_tray_menu(&self) -> tauri::SystemTrayMenu {
// TODO: Refactor this and the auth module so that "Are we logged in"
@@ -663,8 +673,8 @@ impl Controller {
system_tray_menu::signed_out()
}
Status::Connecting => system_tray_menu::signing_in("Signing In..."),
Status::TunnelReady => {
system_tray_menu::signed_in(&auth_session.actor_name, &self.resources.load())
Status::TunnelReady { resources } => {
system_tray_menu::signed_in(&auth_session.actor_name, resources)
}
}
} else if self.auth.ongoing_request().is_ok() {
@@ -727,14 +737,7 @@ async fn run_controller(
log_filter_reloader: logging::Reloader,
) -> Result<(), Error> {
tracing::info!("Entered `run_controller`");
let notify_controller = Arc::new(Notify::new());
let resources = Default::default();
let callback_handler = CallbackHandler {
ctlr_tx: ctlr_tx.clone(),
notify_controller: Arc::clone(&notify_controller),
resources: Arc::clone(&resources),
};
let ipc_client = ipc::Client::new(callback_handler).await?;
let ipc_client = ipc::Client::new(ctlr_tx.clone()).await?;
let mut controller = Controller {
advanced_settings,
app: app.clone(),
@@ -742,8 +745,6 @@ async fn run_controller(
ctlr_tx,
ipc_client,
log_filter_reloader,
notify_controller, // TODO: Fix cancel-safety
resources,
status: Default::default(),
uptime: Default::default(),
};
@@ -777,12 +778,6 @@ async fn run_controller(
loop {
tokio::select! {
() = controller.notify_controller.notified() => {
tracing::debug!("Controller notified of new resources");
if let Err(error) = controller.refresh_system_tray_menu() {
tracing::error!(?error, "Failed to reload resource list");
}
}
() = com_worker.notified() => {
let new_have_internet = network_changes::check_internet().context("Failed to check for internet")?;
if new_have_internet != have_internet {

51
rust/gui-client/src-tauri/src/client/ipc.rs
View File

@@ -1,47 +1,12 @@
use crate::client::gui::{ControllerRequest, CtlrTx};
use anyhow::{Context as _, Result};
use arc_swap::ArcSwap;
use connlib_client_shared::callbacks::ResourceDescription;
use firezone_headless_client::{
ipc::{self, Error},
IpcClientMsg, IpcServerMsg,
IpcClientMsg,
};
use futures::{SinkExt, StreamExt};
use secrecy::{ExposeSecret, SecretString};
use std::{net::IpAddr, sync::Arc};
use tokio::sync::Notify;
#[derive(Clone)]
pub(crate) struct CallbackHandler {
pub notify_controller: Arc<Notify>,
pub ctlr_tx: CtlrTx,
pub resources: Arc<ArcSwap<Vec<ResourceDescription>>>,
}
// Almost but not quite implements `Callbacks` from connlib.
// Because of the IPC boundary, we can deviate.
impl CallbackHandler {
fn on_disconnect(&self, error_msg: String, is_authentication_error: bool) {
self.ctlr_tx
.try_send(ControllerRequest::Disconnected {
error_msg,
is_authentication_error,
})
.expect("controller channel failed");
}
fn on_tunnel_ready(&self) {
self.ctlr_tx
.try_send(ControllerRequest::TunnelReady)
.expect("controller channel failed");
}
fn on_update_resources(&self, resources: Vec<ResourceDescription>) {
tracing::debug!("on_update_resources");
self.resources.store(resources.into());
self.notify_controller.notify_one();
}
}
use std::net::IpAddr;
pub(crate) struct Client {
task: tokio::task::JoinHandle<Result<()>>,
@@ -57,7 +22,7 @@ impl Drop for Client {
}
impl Client {
pub(crate) async fn new(callback_handler: CallbackHandler) -> Result<Self> {
pub(crate) async fn new(ctlr_tx: CtlrTx) -> Result<Self> {
tracing::info!(
client_pid = std::process::id(),
"Connecting to IPC service..."
@@ -65,15 +30,7 @@ impl Client {
let (mut rx, tx) = ipc::connect_to_service(ipc::ServiceId::Prod).await?;
let task = tokio::task::spawn(async move {
while let Some(msg) = rx.next().await.transpose()? {
match msg {
IpcServerMsg::Ok => {}
IpcServerMsg::OnDisconnect {
error_msg,
is_authentication_error,
} => callback_handler.on_disconnect(error_msg, is_authentication_error),
IpcServerMsg::OnTunnelReady => callback_handler.on_tunnel_ready(),
IpcServerMsg::OnUpdateResources(v) => callback_handler.on_update_resources(v),
}
ctlr_tx.send(ControllerRequest::Ipc(msg)).await?;
}
Ok(())
});

4
rust/headless-client/src/ipc_service.rs
View File

@@ -253,10 +253,6 @@ impl Handler {
InternalServerMsg::OnSetInterfaceConfig { ipv4, ipv6, dns } => {
self.tun_device.set_ips(ipv4, ipv6).await?;
self.dns_controller.set_dns(&dns).await?;
self.ipc_tx
.send(&IpcServerMsg::OnTunnelReady)
.await
.context("Error while sending `OnTunnelReady`")?
}
InternalServerMsg::OnUpdateRoutes { ipv4, ipv6 } => {
self.tun_device.set_routes(ipv4, ipv6).await?

4
rust/headless-client/src/ipc_service/ipc.rs
View File

@@ -207,7 +207,7 @@ mod tests {
while let Some(req) = rx.next().await {
let req = req.expect("Error while reading from IPC client");
ensure!(req == IpcClientMsg::Reconnect);
tx.send(&IpcServerMsg::OnTunnelReady)
tx.send(&IpcServerMsg::OnUpdateResources(vec![]))
.await
.expect("Error while writing to IPC client");
}
@@ -232,7 +232,7 @@ mod tests {
.await
.expect("Should have gotten a reply from the IPC server")
.expect("Error while reading from IPC server");
ensure!(matches!(resp, IpcServerMsg::OnTunnelReady));
ensure!(matches!(resp, IpcServerMsg::OnUpdateResources(_)));
}
}
Ok(())

2
rust/headless-client/src/lib.rs
View File

@@ -92,12 +92,10 @@ enum InternalServerMsg {
/// Messages that we can send to IPC clients
#[derive(Debug, serde::Deserialize, serde::Serialize)]
pub enum IpcServerMsg {
Ok,
OnDisconnect {
error_msg: String,
is_authentication_error: bool,
},
OnTunnelReady,
OnUpdateResources(Vec<callbacks::ResourceDescription>),
}

2
rust/headless-client/src/standalone.rs
View File

@@ -198,8 +198,6 @@ pub fn run_only_headless_client() -> Result<()> {
error_msg,
is_authentication_error: _,
}) => return Err(anyhow!(error_msg).context("Firezone disconnected")),
InternalServerMsg::Ipc(IpcServerMsg::Ok)
| InternalServerMsg::Ipc(IpcServerMsg::OnTunnelReady) => {}
InternalServerMsg::Ipc(IpcServerMsg::OnUpdateResources(_)) => {
// On every resources update, flush DNS to mitigate <https://github.com/firezone/firezone/issues/5052>
dns_controller.flush()?;