From f81f8b2ed733a15ab249263f95560624bcc4c2ae Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 2 Dec 2024 23:28:43 +0000 Subject: [PATCH] fix(gui-client): don't share log-directives via file system (#7445) At present, the GUI client shares the current log-directives with the IPC service via the file system. Supposedly, this has been done to allow the IPC service to start back up with the same log filter as before. This behaviour appears to be buggy though as we are receiving a fair number of error reports where this file is not writable. Instead of relying on the file system to communicate, we send the current log-directives to the IPC service as soon as we start up. The IPC service then uses the file system as a cache that log string and re-apply it on the next startup. This way, no two programs need to read / write the same file. The IPC service runs with higher privileges, so this should resolve the permission errors we are seeing in Sentry. --- rust/gui-client/src-common/src/controller.rs | 7 +++- rust/gui-client/src-common/src/settings.rs | 19 ++-------- rust/headless-client/src/ipc_service.rs | 37 +++++++++++++------ rust/headless-client/src/known_dirs.rs | 10 +++-- rust/headless-client/src/known_dirs/linux.rs | 6 +-- .../headless-client/src/known_dirs/windows.rs | 6 +-- 6 files changed, 45 insertions(+), 40 deletions(-) diff --git a/rust/gui-client/src-common/src/controller.rs b/rust/gui-client/src-common/src/controller.rs index 0fcf7bda1..31ec2e998 100644 --- a/rust/gui-client/src-common/src/controller.rs +++ b/rust/gui-client/src-common/src/controller.rs @@ -386,16 +386,19 @@ impl<'a, I: GuiIntegration> Controller<'a, I> { async fn handle_request(&mut self, req: ControllerRequest) -> Result<(), Error> { match req { Req::ApplySettings(settings) => { - let filter = firezone_logging::try_filter(&self.advanced_settings.log_filter) + let filter = firezone_logging::try_filter(&settings.log_filter) .context("Couldn't parse new log filter directives")?; self.advanced_settings = *settings; + self.log_filter_reloader .reload(filter) .context("Couldn't reload log filter")?; - self.ipc_client.send_msg(&IpcClientMsg::ReloadLogFilter).await?; + self.ipc_client.send_msg(&IpcClientMsg::ApplyLogFilter { directives: self.advanced_settings.log_filter.clone() }).await?; + tracing::debug!( "Applied new settings. Log level will take effect immediately." ); + // Refresh the menu in case the favorites were reset. self.refresh_system_tray_menu()?; } diff --git a/rust/gui-client/src-common/src/settings.rs b/rust/gui-client/src-common/src/settings.rs index 2cbfcff13..924daed28 100644 --- a/rust/gui-client/src-common/src/settings.rs +++ b/rust/gui-client/src-common/src/settings.rs @@ -2,12 +2,10 @@ //! advanced settings and code for manipulating diagnostic logs. use anyhow::{Context as _, Result}; -use atomicwrites::{AtomicFile, OverwriteBehavior}; use connlib_model::ResourceId; use firezone_headless_client::known_dirs; -use firezone_logging::std_dyn_err; use serde::{Deserialize, Serialize}; -use std::{collections::HashSet, io::Write, path::PathBuf}; +use std::{collections::HashSet, path::PathBuf}; use url::Url; #[derive(Clone, Deserialize, Serialize)] @@ -65,21 +63,12 @@ pub async fn save(settings: &AdvancedSettings) -> Result<()> { let dir = path .parent() .context("settings path should have a parent")?; + tokio::fs::create_dir_all(dir).await?; tokio::fs::write(&path, serde_json::to_string(settings)?).await?; - // Don't create the dir for the log filter file, that's the IPC service's job. - // If it isn't there for some reason yet, just log an error and move on. - let log_filter_path = known_dirs::ipc_log_filter().context("`ipc_log_filter` failed")?; - let f = AtomicFile::new(&log_filter_path, OverwriteBehavior::AllowOverwrite); - // Note: Blocking file write in async function - if let Err(error) = f.write(|f| f.write_all(settings.log_filter.as_bytes())) { - tracing::error!( - error = std_dyn_err(&error), - ?log_filter_path, - "Couldn't write log filter file for IPC service" - ); - } + tracing::debug!(?path, "Saved settings"); + Ok(()) } diff --git a/rust/headless-client/src/ipc_service.rs b/rust/headless-client/src/ipc_service.rs index f5de4195f..105b0ddd0 100644 --- a/rust/headless-client/src/ipc_service.rs +++ b/rust/headless-client/src/ipc_service.rs @@ -3,13 +3,14 @@ use crate::{ ConnlibMsg, LogFilterReloader, }; use anyhow::{bail, Context as _, Result}; +use atomicwrites::{AtomicFile, OverwriteBehavior}; use clap::Parser; use connlib_model::ResourceView; use firezone_bin_shared::{ platform::{tcp_socket_factory, udp_socket_factory, DnsControlMethod}, TunDeviceManager, TOKEN_ENV_KEY, }; -use firezone_logging::{anyhow_dyn_err, sentry_layer, telemetry_span}; +use firezone_logging::{anyhow_dyn_err, sentry_layer, std_dyn_err, telemetry_span}; use firezone_telemetry::Telemetry; use futures::{ future::poll_fn, @@ -20,9 +21,15 @@ use phoenix_channel::LoginUrl; use secrecy::SecretString; use serde::{Deserialize, Serialize}; use std::{ - collections::BTreeSet, io, net::IpAddr, path::PathBuf, pin::pin, sync::Arc, time::Duration, + collections::BTreeSet, + io::{self, Write}, + net::IpAddr, + path::PathBuf, + pin::pin, + sync::Arc, + time::Duration, }; -use tokio::{sync::mpsc, task::spawn_blocking, time::Instant}; +use tokio::{sync::mpsc, time::Instant}; use tracing_subscriber::{layer::SubscriberExt, reload, EnvFilter, Layer, Registry}; use url::Url; @@ -83,7 +90,9 @@ pub enum ClientMsg { token: String, }, Disconnect, - ReloadLogFilter, + ApplyLogFilter { + directives: String, + }, Reset, SetDns(Vec), SetDisabledResources(BTreeSet), @@ -498,9 +507,16 @@ impl<'a> Handler<'a> { .await .context("Failed to send `DisconnectedGracefully`")?; } - ClientMsg::ReloadLogFilter => { - let filter = spawn_blocking(get_log_filter).await??; - self.log_filter_reloader.reload(filter)?; + ClientMsg::ApplyLogFilter { directives } => { + self.log_filter_reloader.reload(directives.clone())?; + + let path = known_dirs::ipc_log_filter()?; + + if let Err(e) = AtomicFile::new(&path, OverwriteBehavior::AllowOverwrite) + .write(|f| f.write_all(directives.as_bytes())) + { + tracing::warn!(path = %path.display(), %directives, error = std_dyn_err(&e), "Failed to write new log directives"); + } } ClientMsg::Reset => { if self.last_connlib_start_instant.is_some() { @@ -673,11 +689,8 @@ pub(crate) fn get_log_filter() -> Result { return Ok(filter); } - if let Ok(filter) = std::fs::read_to_string( - known_dirs::ipc_log_filter() - .context("Failed to compute directory for log filter config file")?, - ) - .map(|s| s.trim().to_string()) + if let Ok(filter) = + std::fs::read_to_string(known_dirs::ipc_log_filter()?).map(|s| s.trim().to_string()) { return Ok(filter); } diff --git a/rust/headless-client/src/known_dirs.rs b/rust/headless-client/src/known_dirs.rs index a01676570..d10751c0d 100644 --- a/rust/headless-client/src/known_dirs.rs +++ b/rust/headless-client/src/known_dirs.rs @@ -7,9 +7,11 @@ //! //! I wanted the ProgramData folder on Windows, which `dirs` alone doesn't provide. -pub use platform::{ipc_service_config, ipc_service_logs, logs, runtime, session, settings}; +use anyhow::{Context as _, Result}; use std::path::PathBuf; +pub use platform::{ipc_service_config, ipc_service_logs, logs, runtime, session, settings}; + #[cfg(target_os = "linux")] #[path = "known_dirs/linux.rs"] pub mod platform; @@ -22,8 +24,10 @@ pub mod platform; #[path = "known_dirs/windows.rs"] pub mod platform; -pub fn ipc_log_filter() -> Option { - Some(ipc_service_config()?.join("log-filter")) +pub fn ipc_log_filter() -> Result { + Ok(ipc_service_config() + .context("Failed to compute `ipc_service_config` directory")? + .join("log-filter")) } #[cfg(test)] diff --git a/rust/headless-client/src/known_dirs/linux.rs b/rust/headless-client/src/known_dirs/linux.rs index 546b34d53..c2a2dd88b 100644 --- a/rust/headless-client/src/known_dirs/linux.rs +++ b/rust/headless-client/src/known_dirs/linux.rs @@ -1,11 +1,9 @@ use firezone_bin_shared::BUNDLE_ID; use std::path::PathBuf; -/// Path for IPC service config that either the IPC service or GUI can write +/// Path for IPC service config that the IPC service can write /// -/// e.g. the device ID should only be written by the IPC service, and -/// the log filter should only be written by the GUI. No file should be written -/// by both programs. All writes should use `atomicwrites`. +/// All writes should use `atomicwrites`. /// /// On Linux, `/var/lib/$BUNDLE_ID/config/firezone-id` /// diff --git a/rust/headless-client/src/known_dirs/windows.rs b/rust/headless-client/src/known_dirs/windows.rs index 1154ab76b..a82592c3c 100644 --- a/rust/headless-client/src/known_dirs/windows.rs +++ b/rust/headless-client/src/known_dirs/windows.rs @@ -2,11 +2,9 @@ use firezone_bin_shared::{platform::app_local_data_dir, BUNDLE_ID}; use known_folders::{get_known_folder_path, KnownFolder}; use std::path::PathBuf; -/// Path for IPC service config that either the IPC service or GUI can write +/// Path for IPC service config that the IPC service can write /// -/// e.g. the device ID should only be written by the IPC service, and -/// the log filter should only be written by the GUI. No file should be written -/// by both programs. All writes should use `atomicwrites`. +/// All writes should use `atomicwrites`. /// /// On Windows, `C:/ProgramData/$BUNDLE_ID/config` pub fn ipc_service_config() -> Option {