From 0057a703ff4962b3d78d6c3f248b67258f79581c Mon Sep 17 00:00:00 2001 From: Brian Schwind Date: Thu, 1 Jun 2023 12:11:31 +0900 Subject: [PATCH] Turn ChangeString into a PeerChange enum (#263) * Turn ChangeString into a PeerChange enum, don't print NAT traversal reattempt as a modification * Remove the ChangeString type * Fix a stupid copy-paste error --- client/src/util.rs | 48 +++++++++++++++++--- shared/src/types.rs | 105 +++++++++++++++++++++++++------------------- 2 files changed, 100 insertions(+), 53 deletions(-) diff --git a/client/src/util.rs b/client/src/util.rs index 72112b1..cfc2ec8 100644 --- a/client/src/util.rs +++ b/client/src/util.rs @@ -3,7 +3,9 @@ use colored::*; use indoc::eprintdoc; use log::{Level, LevelFilter}; use serde::{de::DeserializeOwned, Serialize}; -use shared::{interface_config::ServerInfo, Interface, PeerDiff, INNERNET_PUBKEY_HEADER}; +use shared::{ + interface_config::ServerInfo, Interface, PeerChange, PeerDiff, INNERNET_PUBKEY_HEADER, +}; use std::{ffi::OsStr, io, path::Path, time::Duration}; use ureq::{Agent, AgentBuilder}; @@ -137,13 +139,30 @@ pub fn permissions_helptext(config_dir: &Path, data_dir: &Path, e: &io::Error) { } } +#[derive(Debug, Copy, Clone, PartialEq)] +enum ChangeAction { + Added, + Modified, + Removed, +} + +impl ChangeAction { + fn colored_output(&self) -> ColoredString { + match self { + Self::Added => "added".green(), + Self::Modified => "modified".yellow(), + Self::Removed => "removed".red(), + } + } +} + pub fn print_peer_diff(store: &DataStore, diff: &PeerDiff) { let public_key = diff.public_key().to_base64(); - let text = match (diff.old, diff.new) { - (None, Some(_)) => "added".green(), - (Some(_), Some(_)) => "modified".yellow(), - (Some(_), None) => "removed".red(), + let change_action = match (diff.old, diff.new) { + (None, Some(_)) => ChangeAction::Added, + (Some(_), Some(_)) => ChangeAction::Modified, + (Some(_), None) => ChangeAction::Removed, _ => unreachable!("PeerDiff can't be None -> None"), }; @@ -158,15 +177,30 @@ pub fn print_peer_diff(store: &DataStore, diff: &PeerDiff) { }; let peer_name = peer_hostname.as_deref().unwrap_or("[unknown]"); + if change_action == ChangeAction::Modified + && diff + .changes() + .iter() + .all(|c| *c == PeerChange::NatTraverseReattempt) + { + // If this peer was "modified" but the only change is a NAT Traversal Reattempt, + // don't bother printing this peer. + return; + } + log::info!( " peer {} ({}...) was {}.", peer_name.yellow(), &public_key[..10].dimmed(), - text + change_action.colored_output(), ); for change in diff.changes() { - log::debug!(" {}", change); + if let PeerChange::Endpoint { .. } = change { + log::info!(" {}", change); + } else { + log::debug!(" {}", change); + } } } diff --git a/shared/src/types.rs b/shared/src/types.rs index 7a33bc4..b1e398d 100644 --- a/shared/src/types.rs +++ b/shared/src/types.rs @@ -519,35 +519,55 @@ impl Display for Peer { } } -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct ChangeString { - name: &'static str, - old: Option, - new: Option, +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PeerChange { + AllowedIPs { + old: Vec, + new: Vec, + }, + PersistentKeepalive { + old: Option, + new: Option, + }, + Endpoint { + old: Option, + new: Option, + }, + NatTraverseReattempt, } -impl Display for ChangeString { +impl Display for PeerChange { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!( - f, - "{}: {} => {}", - self.name, - self.old.as_deref().unwrap_or("[none]"), - self.new.as_deref().unwrap_or("[none]") - ) + match self { + Self::AllowedIPs { old, new } => write!(f, "Allowed IPs: {:?} => {:?}", old, new), + Self::PersistentKeepalive { old, new } => write!( + f, + "Persistent Keepalive: {} => {}", + old.display_string(), + new.display_string() + ), + Self::Endpoint { old, new } => write!( + f, + "Endpoint: {} => {}", + old.display_string(), + new.display_string() + ), + Self::NatTraverseReattempt => write!(f, "NAT Traversal Reattempt"), + } } } -impl ChangeString { - pub fn new(name: &'static str, old: Option, new: Option) -> Self - where - T: fmt::Debug, - U: fmt::Debug, - { - Self { - name, - old: old.map(|t| format!("{t:?}")), - new: new.map(|t| format!("{t:?}")), +trait OptionExt { + fn display_string(&self) -> String; +} + +impl OptionExt for Option { + fn display_string(&self) -> String { + match self { + Some(x) => { + format!("{:?}", x) + }, + None => "[none]".to_string(), } } } @@ -559,7 +579,7 @@ pub struct PeerDiff<'a> { pub old: Option<&'a PeerConfig>, pub new: Option<&'a Peer>, builder: PeerConfigBuilder, - changes: Vec, + changes: Vec, } impl<'a> PeerDiff<'a> { @@ -588,14 +608,14 @@ impl<'a> PeerDiff<'a> { self.builder.public_key() } - pub fn changes(&self) -> &[ChangeString] { + pub fn changes(&self) -> &[PeerChange] { &self.changes } fn peer_config_builder( old_info: Option<&PeerInfo>, new: Option<&Peer>, - ) -> Option<(PeerConfigBuilder, Vec)> { + ) -> Option<(PeerConfigBuilder, Vec)> { let old = old_info.map(|p| &p.config); let public_key = match (old, new) { (Some(old), _) => old.public_key.clone(), @@ -622,11 +642,10 @@ impl<'a> PeerDiff<'a> { builder = builder .replace_allowed_ips() .add_allowed_ips(new_allowed_ips); - changes.push(ChangeString::new( - "AllowedIPs", - old.map(|o| &o.allowed_ips[..]), - Some(&new_allowed_ips[0]), - )); + changes.push(PeerChange::AllowedIPs { + old: old.map(|o| o.allowed_ips.clone()).unwrap_or_else(Vec::new), + new: new_allowed_ips.to_vec(), + }); } if old.is_none() @@ -636,11 +655,10 @@ impl<'a> PeerDiff<'a> { Some(interval) => builder.set_persistent_keepalive_interval(interval), None => builder.unset_persistent_keepalive(), }; - changes.push(ChangeString::new( - "PersistentKeepalive", - old.and_then(|p| p.persistent_keepalive_interval), - new.persistent_keepalive_interval, - )); + changes.push(PeerChange::PersistentKeepalive { + old: old.and_then(|p| p.persistent_keepalive_interval), + new: new.persistent_keepalive_interval, + }); } // We won't update the endpoint if there's already a stable connection. @@ -653,20 +671,15 @@ impl<'a> PeerDiff<'a> { if let Some(addr) = resolved { if old.is_none() || matches!(old, Some(old) if old.endpoint != resolved) { builder = builder.set_endpoint(addr); - changes.push(ChangeString::new( - "Endpoint", - old.and_then(|p| p.endpoint), - Some(addr), - )); + changes.push(PeerChange::Endpoint { + old: old.and_then(|p| p.endpoint), + new: Some(addr), + }); endpoint_changed = true; } } if !endpoint_changed && !new.candidates.is_empty() { - changes.push(ChangeString::new( - "Connection status", - "Disconnected".into(), - "NAT traverse reattempt".into(), - )); + changes.push(PeerChange::NatTraverseReattempt) } }