From 8f3f6d6735a2e86a35c281df67d3f75ec65e1da7 Mon Sep 17 00:00:00 2001 From: Adrian Benavides Date: Mon, 17 Feb 2025 08:33:24 +0100 Subject: [PATCH] refactor(rust): add log format for user-facing terminal logs --- .../src/logs/logging_configuration.rs | 14 ++- .../ockam_api/src/logs/logging_options.rs | 37 ++++++++ .../rust/ockam/ockam_api/src/logs/setup.rs | 8 +- .../ockam_api/src/nodes/service/relay.rs | 10 +- .../service/tcp_inlets/session_replacer.rs | 6 +- .../ockam/ockam_api/src/ui/terminal/mod.rs | 45 ++++----- .../ockam_api/src/ui/terminal/notification.rs | 2 +- .../ockam/ockam_api/src/ui/terminal/term.rs | 9 +- .../rust/ockam/ockam_app_lib/src/log.rs | 13 ++- .../rust/ockam/ockam_command/src/command.rs | 67 ++++++++----- .../ockam/ockam_command/src/entry_point.rs | 12 ++- .../ockam/ockam_command/src/node/create.rs | 22 ++++- .../ockam/ockam_command/src/subcommand.rs | 8 +- .../ockam/ockam_command/src/value_parsers.rs | 6 +- .../src/portal/addresses.rs | 28 +++--- .../src/portal/inlet_listener.rs | 6 +- .../src/portal/portal_worker.rs | 94 +++++++++---------- 17 files changed, 239 insertions(+), 148 deletions(-) diff --git a/implementations/rust/ockam/ockam_api/src/logs/logging_configuration.rs b/implementations/rust/ockam/ockam_api/src/logs/logging_configuration.rs index 3f07cf823ae..a4354ae7861 100644 --- a/implementations/rust/ockam/ockam_api/src/logs/logging_configuration.rs +++ b/implementations/rust/ockam/ockam_api/src/logs/logging_configuration.rs @@ -7,7 +7,7 @@ use std::path::PathBuf; use tracing_core::Level; use tracing_subscriber::EnvFilter; -use super::{Colored, LoggingEnabled}; +use super::{Colored, LoggingEnabled, OckamUserLogFormat}; use crate::logs::LogFormat; /// List of all the configuration parameters relevant for configuring the logs @@ -26,9 +26,9 @@ pub struct LoggingConfiguration { /// This parameter specifies if the log output is colored (typically in terminals supporting it) colored: Colored, /// Director where log files must be created. - /// If no directory is defined then log messages appear on the console + /// If no directory is defined, then log messages appear on the console log_dir: Option, - /// List of create for which we want to keep log messages + /// List of crates for which we want to keep log messages crates: Option>, } @@ -207,18 +207,20 @@ pub fn logging_configuration( level_and_crates: LogLevelWithCratesFilter, log_dir: Option, colored: Colored, + default_log_format: LogFormat, + enabled: LoggingEnabled, ) -> ockam_core::Result { let enabled = if level_and_crates.explicit_verbose_flag { LoggingEnabled::On } else { - logging_enabled()? + enabled }; Ok(LoggingConfiguration::new( enabled, level_and_crates.level, log_max_size_bytes()?, log_max_files()?, - log_format()?, + get_env_with_default(OCKAM_LOG_FORMAT, default_log_format)?, colored, log_dir, level_and_crates.crates_filter.clone(), @@ -353,6 +355,7 @@ impl CratesFilter { CratesFilter::Basic => Some(vec![ "ockam_api::ui::terminal".to_string(), "ockam_command".to_string(), + OckamUserLogFormat::TARGET.to_string(), ]), CratesFilter::Core => Some(vec![ "ockam".to_string(), @@ -363,6 +366,7 @@ impl CratesFilter { "ockam_transport_tcp".to_string(), "ockam_api".to_string(), "ockam_command".to_string(), + OckamUserLogFormat::TARGET.to_string(), ]), CratesFilter::Selected(list) => Some(list.clone()), } diff --git a/implementations/rust/ockam/ockam_api/src/logs/logging_options.rs b/implementations/rust/ockam/ockam_api/src/logs/logging_options.rs index dea02f8157c..fe4394a97f2 100644 --- a/implementations/rust/ockam/ockam_api/src/logs/logging_options.rs +++ b/implementations/rust/ockam/ockam_api/src/logs/logging_options.rs @@ -1,3 +1,4 @@ +use crate::fmt_log; use nu_ansi_term::{Color, Style}; use ockam_core::env::FromString; use std::fmt::{Debug, Display, Formatter}; @@ -43,6 +44,7 @@ pub enum Colored { #[derive(Clone, Debug, PartialEq, Eq)] pub enum LogFormat { Default, + User, Pretty, Json, } @@ -50,6 +52,7 @@ pub enum LogFormat { impl FromString for LogFormat { fn from_string(s: &str) -> ockam_core::Result { match s { + "user" => Ok(LogFormat::User), "pretty" => Ok(LogFormat::Pretty), "json" => Ok(LogFormat::Json), _ => Ok(LogFormat::Default), @@ -61,6 +64,7 @@ impl Display for LogFormat { fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { match self { LogFormat::Default => write!(f, "default"), + LogFormat::User => write!(f, "user"), LogFormat::Pretty => write!(f, "pretty"), LogFormat::Json => write!(f, "json"), } @@ -174,6 +178,39 @@ where } } +#[derive(Default)] +pub struct OckamUserLogFormat {} + +impl OckamUserLogFormat { + pub const TARGET: &'static str = "ockam_command::user"; + + pub fn new() -> Self { + Self {} + } +} + +impl FormatEvent for OckamUserLogFormat +where + S: Subscriber + for<'a> LookupSpan<'a>, + N: for<'a> FormatFields<'a> + 'static, +{ + fn format_event( + &self, + ctx: &FmtContext<'_, S, N>, + mut writer: Writer<'_>, + event: &Event<'_>, + ) -> std::fmt::Result { + // Padding + writer.write_str(&fmt_log!(""))?; + + // Event + ctx.format_fields(writer.by_ref(), event)?; + writer.write_char(' ')?; + + writeln!(writer) + } +} + struct FmtLevel<'a> { level: &'a Level, ansi: bool, diff --git a/implementations/rust/ockam/ockam_api/src/logs/setup.rs b/implementations/rust/ockam/ockam_api/src/logs/setup.rs index 1bbb855e6ed..11db57fb430 100644 --- a/implementations/rust/ockam/ockam_api/src/logs/setup.rs +++ b/implementations/rust/ockam/ockam_api/src/logs/setup.rs @@ -5,7 +5,7 @@ use crate::logs::secure_client_service::SecureClientService; use crate::logs::tracing_guard::TracingGuard; use crate::logs::{ ExportingConfiguration, LoggingConfiguration, OckamLogExporter, OckamLogFormat, - TelemetryEndpoint, + OckamUserLogFormat, TelemetryEndpoint, }; use crate::logs::{LogFormat, OckamSpanExporter}; use gethostname::gethostname; @@ -131,6 +131,9 @@ impl LoggingTracing { LogFormat::Default => layers .with(appender.event_format(OckamLogFormat::new())) .try_init(), + LogFormat::User => layers + .with(appender.event_format(OckamUserLogFormat::new())) + .try_init(), }; result.expect("Failed to initialize tracing subscriber"); @@ -152,6 +155,9 @@ impl LoggingTracing { LogFormat::Default => layers .with(appender.event_format(OckamLogFormat::new())) .try_init(), + LogFormat::User => layers + .with(appender.event_format(OckamUserLogFormat::new())) + .try_init(), }; result.expect("Failed to initialize tracing subscriber"); }; diff --git a/implementations/rust/ockam/ockam_api/src/nodes/service/relay.rs b/implementations/rust/ockam/ockam_api/src/nodes/service/relay.rs index bddea502770..caaa28a26cf 100644 --- a/implementations/rust/ockam/ockam_api/src/nodes/service/relay.rs +++ b/implementations/rust/ockam/ockam_api/src/nodes/service/relay.rs @@ -162,6 +162,7 @@ impl NodeManager { node_manager: Arc::downgrade(self), context: ctx.try_clone()?, addr: address.clone(), + alias: alias.clone(), relay_address: relay_address.clone(), connection: None, relay_worker_address: None, @@ -288,6 +289,7 @@ impl InMemoryNode { struct RelaySessionReplacer { node_manager: Weak, context: Context, + alias: String, relay_address: Option, // current status @@ -379,7 +381,9 @@ impl SessionReplacer for RelaySessionReplacer { if let Some(node_manager) = self.node_manager.upgrade() { node_manager.cli_state.notify_message( fmt_warn!( - "The Node lost the connection to the Relay at {}\n", + "The Node {} lost the connection to the Relay {} listening at {}\n", + color_primary(&node_manager.node_name), + color_primary(&self.alias), color_primary(&self.addr) ) + &fmt_info!("Attempting to reconnect...\n"), ); @@ -389,7 +393,9 @@ impl SessionReplacer for RelaySessionReplacer { async fn on_session_replaced(&self) { if let Some(node_manager) = self.node_manager.upgrade() { node_manager.cli_state.notify_message(fmt_ok!( - "The Node has restored the connection to the Relay at {}\n", + "The Node {} has restored the connection to the Relay {} listening at {}\n", + color_primary(&node_manager.node_name), + color_primary(&self.alias), color_primary(&self.addr) )); } diff --git a/implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer.rs b/implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer.rs index fceec112df0..c118405b292 100644 --- a/implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer.rs +++ b/implementations/rust/ockam/ockam_api/src/nodes/service/tcp_inlets/session_replacer.rs @@ -302,7 +302,8 @@ impl SessionReplacer for InletSessionReplacer { if let Some(node_manager) = self.node_manager.upgrade() { node_manager.cli_state.notify_message( fmt_warn!( - "The TCP Inlet at {} lost the connection to the TCP Outlet at {}\n", + "The TCP Inlet {} listening at {} lost the connection to the TCP Outlet at {}\n", + color_primary(&self.resource.resource_name), color_primary(&self.listen_addr), color_primary(&self.outlet_addr) ) + &fmt_info!("Attempting to reconnect...\n"), @@ -313,7 +314,8 @@ impl SessionReplacer for InletSessionReplacer { async fn on_session_replaced(&self) { if let Some(node_manager) = self.node_manager.upgrade() { node_manager.cli_state.notify_message(fmt_ok!( - "The TCP Inlet at {} has restored the connection to the TCP Outlet at {}\n", + "The TCP Inlet {} listening at {} has restored the connection to the TCP Outlet at {}\n", + color_primary(&self.resource.resource_name), color_primary(&self.listen_addr), color_primary(&self.outlet_addr) )); diff --git a/implementations/rust/ockam/ockam_api/src/ui/terminal/mod.rs b/implementations/rust/ockam/ockam_api/src/ui/terminal/mod.rs index 38048a763c5..b631568f9ee 100644 --- a/implementations/rust/ockam/ockam_api/src/ui/terminal/mod.rs +++ b/implementations/rust/ockam/ockam_api/src/ui/terminal/mod.rs @@ -31,8 +31,7 @@ use tracing::warn; pub struct Terminal { stdout: T, stderr: T, - logging_enabled: bool, - logging_goes_to_file: bool, + logging_options: LoggingOptions, quiet: bool, no_input: bool, output_format: OutputFormat, @@ -47,7 +46,7 @@ impl Terminal { } fn log_msg(&self, msg: &str) { - if !self.logging_enabled { + if !self.logging_options.enabled { return; } for line in msg.lines() { @@ -72,6 +71,13 @@ impl Terminal { } } +#[derive(Clone, Debug)] +pub struct LoggingOptions { + pub enabled: bool, + pub logging_to_file: bool, + pub with_user_format: bool, +} + /// A small wrapper around the `Write` trait, enriched with CLI /// attributes to facilitate output handling. #[derive(Clone, Debug)] @@ -109,8 +115,7 @@ pub trait TerminalWriter: Clone { impl Terminal { #[allow(clippy::too_many_arguments)] pub fn new( - logging_enabled: bool, - logging_goes_to_file: bool, + logging_options: LoggingOptions, quiet: bool, no_color: bool, no_input: bool, @@ -125,8 +130,7 @@ impl Terminal { Self { stdout, stderr, - logging_enabled, - logging_goes_to_file, + logging_options, quiet, no_input, output_format, @@ -136,18 +140,6 @@ impl Terminal { } } - pub fn quiet(logging_enabled: bool, logging_goes_to_file: bool) -> Self { - Self::new( - logging_enabled, - logging_goes_to_file, - true, - false, - false, - OutputFormat::Plain, - OutputBranding::default(), - ) - } - /// Prompt the user for a confirmation. pub fn confirm(&self, msg: impl AsRef) -> Result { if !self.can_ask_for_user_input() { @@ -245,8 +237,8 @@ impl Terminal { } /// Return true if log messages are emitted to the console - fn logging_to_console_only(&self) -> bool { - self.logging_enabled && !self.logging_goes_to_file + fn logging_to_console(&self) -> bool { + self.logging_options.enabled && !self.logging_options.logging_to_file } /// Return true if we can write to stderr @@ -254,7 +246,7 @@ impl Terminal { /// - all the messages are logged to the console /// - or quiet is true fn can_write_to_stderr(&self) -> bool { - !self.logging_to_console_only() && !self.is_quiet() + self.logging_options.with_user_format || (!self.logging_to_console() && !self.is_quiet()) } pub fn write(&self, msg: impl AsRef) -> Result<()> { @@ -312,8 +304,7 @@ impl Terminal { Terminal { stdout: self.stdout, stderr: self.stderr, - logging_enabled: self.logging_enabled, - logging_goes_to_file: self.logging_goes_to_file, + logging_options: self.logging_options, quiet: self.quiet, no_input: self.no_input, output_format: self.output_format, @@ -354,14 +345,14 @@ impl Terminal { } /// Return true if log messages are emitted to the console - fn logging_to_console_only(&self) -> bool { - self.logging_enabled && !self.logging_goes_to_file + fn logging_to_console(&self) -> bool { + self.logging_options.enabled && !self.logging_options.logging_to_file } /// Return true if we can write to stdout /// We can write to stdout unless all the messages are logged to the console fn can_write_to_stdout(&self) -> bool { - !self.logging_to_console_only() + self.logging_options.with_user_format || !self.logging_to_console() } pub fn write_line(mut self) -> Result<()> { diff --git a/implementations/rust/ockam/ockam_api/src/ui/terminal/notification.rs b/implementations/rust/ockam/ockam_api/src/ui/terminal/notification.rs index 968151b9e56..5762df9d7c8 100644 --- a/implementations/rust/ockam/ockam_api/src/ui/terminal/notification.rs +++ b/implementations/rust/ockam/ockam_api/src/ui/terminal/notification.rs @@ -78,7 +78,7 @@ impl NotificationHandler { let stop = Arc::new(AtomicBool::new(false)); let _self = NotificationHandler { rx: cli_state.subscribe_to_notifications(), - terminal: terminal.clone(), + terminal, progress_bar: None, stop: stop.clone(), }; diff --git a/implementations/rust/ockam/ockam_api/src/ui/terminal/term.rs b/implementations/rust/ockam/ockam_api/src/ui/terminal/term.rs index c9d0720bbde..0556cf41511 100644 --- a/implementations/rust/ockam/ockam_api/src/ui/terminal/term.rs +++ b/implementations/rust/ockam/ockam_api/src/ui/terminal/term.rs @@ -61,13 +61,16 @@ mod tests { use dialoguer::console::Term; use crate::output::{OutputBranding, OutputFormat}; - use crate::terminal::{Terminal, TerminalStream}; + use crate::terminal::{LoggingOptions, Terminal, TerminalStream}; #[test] fn test_write() { let sut: Terminal> = Terminal::new( - false, - false, + LoggingOptions { + enabled: false, + logging_to_file: false, + with_user_format: false, + }, false, false, false, diff --git a/implementations/rust/ockam/ockam_app_lib/src/log.rs b/implementations/rust/ockam/ockam_app_lib/src/log.rs index d0cb5ecc2dc..5a1c949c5ad 100644 --- a/implementations/rust/ockam/ockam_app_lib/src/log.rs +++ b/implementations/rust/ockam/ockam_app_lib/src/log.rs @@ -1,7 +1,7 @@ use crate::state::{AppState, NODE_NAME}; use ockam_api::logs::{ - logging_configuration, Colored, ExportingConfiguration, LogLevelWithCratesFilter, - LoggingTracing, + logging_configuration, logging_enabled, Colored, ExportingConfiguration, LogFormat, + LogLevelWithCratesFilter, LoggingTracing, }; use ockam_core::TryClone; use ockam_node::Context; @@ -30,7 +30,14 @@ impl AppState { .unwrap() .add_crates(vec!["ockam_app_lib"]); let tracing_guard = LoggingTracing::setup( - &logging_configuration(level_and_crates, Some(node_dir), Colored::Off).unwrap(), + &logging_configuration( + level_and_crates, + Some(node_dir), + Colored::Off, + LogFormat::Default, + logging_enabled().unwrap(), + ) + .unwrap(), &ExportingConfiguration::foreground(&state, ctx) .await .unwrap(), diff --git a/implementations/rust/ockam/ockam_command/src/command.rs b/implementations/rust/ockam/ockam_command/src/command.rs index 0905500cb35..2e55692567a 100644 --- a/implementations/rust/ockam/ockam_command/src/command.rs +++ b/implementations/rust/ockam/ockam_command/src/command.rs @@ -12,10 +12,11 @@ use console::Term; use miette::{miette, IntoDiagnostic}; use ockam_api::colors::color_primary; use ockam_api::logs::{ - is_exporting_set, logging_configuration, Colored, ExportingConfiguration, - LogLevelWithCratesFilter, LoggingConfiguration, LoggingTracing, TracingGuard, + is_exporting_set, logging_configuration, logging_enabled, Colored, CratesFilter, + ExportingConfiguration, LogFormat, LogLevelWithCratesFilter, LoggingConfiguration, + LoggingEnabled, LoggingTracing, OckamUserLogFormat, TracingGuard, }; -use ockam_api::terminal::Terminal; +use ockam_api::terminal::{LoggingOptions, Terminal}; use ockam_api::{fmt_err, fmt_log, fmt_ok, fmt_warn, CliState}; use ockam_core::OCKAM_TRACER_NAME; use ockam_node::Context; @@ -127,24 +128,39 @@ impl OckamCommand { /// Create the logging configuration, depending on the command to execute fn make_logging_configuration(&self, is_tty: bool) -> miette::Result { - let log_path = self.subcommand.log_path(); if self.subcommand.is_background_node() { - Ok(LoggingConfiguration::background(log_path).into_diagnostic()?) + Ok(LoggingConfiguration::background(self.subcommand.log_path()).into_diagnostic()?) } else { - let level_and_crates = LogLevelWithCratesFilter::from_verbose(self.global_args.verbose) - .into_diagnostic()?; - let log_path = - if level_and_crates.explicit_verbose_flag || self.subcommand.is_foreground_node() { - None - } else { - Some(CliState::command_log_path(self.subcommand.name().as_str())?) - }; + let verbose = self.global_args.verbose; + let mut level_and_crates = + LogLevelWithCratesFilter::from_verbose(verbose).into_diagnostic()?; + let mut log_path = if level_and_crates.explicit_verbose_flag { + None + } else { + Some(CliState::command_log_path(self.subcommand.name().as_str())?) + }; + let mut logging_enabled = logging_enabled()?; + let mut default_log_format = LogFormat::Default; + if self.subcommand.is_foreground_node() && verbose == 0 { + log_path = None; + logging_enabled = LoggingEnabled::On; + level_and_crates.crates_filter = + CratesFilter::Selected(vec![OckamUserLogFormat::TARGET.to_string()]); + default_log_format = LogFormat::User; + } let colored = if !self.global_args.no_color && is_tty && log_path.is_none() { Colored::On } else { Colored::Off }; - Ok(logging_configuration(level_and_crates, log_path, colored).into_diagnostic()?) + Ok(logging_configuration( + level_and_crates, + log_path, + colored, + default_log_format, + logging_enabled, + ) + .into_diagnostic()?) } } @@ -192,14 +208,7 @@ impl OckamCommand { let logging_configuration = self.make_logging_configuration(Term::stdout().is_term())?; - let (exporting_configuration, tracing_guard, cli_state) = if !is_exporting_set()? { - // Allows to have logging enabled before initializing CliState - let exporting_configuration = ExportingConfiguration::off().into_diagnostic()?; - let tracing_guard = - self.setup_logging_tracing(&logging_configuration, &exporting_configuration, ctx); - - (exporting_configuration, tracing_guard, None) - } else { + let (exporting_configuration, tracing_guard, cli_state) = if is_exporting_set()? { let cli_state = self.init_cli_state(in_memory).await; let exporting_configuration = self.make_exporting_configuration(&cli_state, ctx).await?; @@ -208,6 +217,13 @@ impl OckamCommand { let cli_state = cli_state.set_tracing_enabled(exporting_configuration.is_enabled()); (exporting_configuration, tracing_guard, Some(cli_state)) + } else { + // Allows having logging enabled before initializing CliState + let exporting_configuration = ExportingConfiguration::off().into_diagnostic()?; + let tracing_guard = + self.setup_logging_tracing(&logging_configuration, &exporting_configuration, ctx); + + (exporting_configuration, tracing_guard, None) }; info!("Tracing initialized"); @@ -245,8 +261,11 @@ impl OckamCommand { }; let terminal = Terminal::new( - logging_configuration.is_enabled(), - logging_configuration.log_dir().is_some(), + LoggingOptions { + enabled: logging_configuration.is_enabled(), + logging_to_file: logging_configuration.log_dir().is_some(), + with_user_format: logging_configuration.format() == LogFormat::User, + }, self.global_args.quiet, self.global_args.no_color, self.global_args.no_input, diff --git a/implementations/rust/ockam/ockam_command/src/entry_point.rs b/implementations/rust/ockam/ockam_command/src/entry_point.rs index 6e61eddfec6..a23149f395b 100644 --- a/implementations/rust/ockam/ockam_command/src/entry_point.rs +++ b/implementations/rust/ockam/ockam_command/src/entry_point.rs @@ -10,8 +10,8 @@ use crate::{ }; use ockam_api::cli_state::{CliState, CliStateMode}; use ockam_api::logs::{ - logging_configuration, Colored, ExportingConfiguration, LogLevelWithCratesFilter, - LoggingTracing, + logging_configuration, logging_enabled, Colored, ExportingConfiguration, LogFormat, + LogLevelWithCratesFilter, LoggingTracing, }; use ockam_api::output::Output; use ockam_node::{Context, NodeBuilder}; @@ -79,7 +79,13 @@ async fn handle_invalid_command( .join(" "); let cli_state = CliState::create(CliStateMode::InMemory).await?; let level_and_crates = LogLevelWithCratesFilter::new().into_diagnostic()?; - let logging_configuration = logging_configuration(level_and_crates, None, Colored::On); + let logging_configuration = logging_configuration( + level_and_crates, + None, + Colored::On, + LogFormat::Default, + logging_enabled()?, + ); let _guard = LoggingTracing::setup( &logging_configuration.into_diagnostic()?, &ExportingConfiguration::foreground(&cli_state, ctx) diff --git a/implementations/rust/ockam/ockam_command/src/node/create.rs b/implementations/rust/ockam/ockam_command/src/node/create.rs index 65d7fb89f5f..2881ee24990 100644 --- a/implementations/rust/ockam/ockam_command/src/node/create.rs +++ b/implementations/rust/ockam/ockam_command/src/node/create.rs @@ -300,6 +300,12 @@ impl CreateCommand { // if no config is used and the name arg is the default node name, set a random name else if self.config_args.configuration.is_none() && self.name == DEFAULT_NODE_NAME { self.name = random_name(); + if let Ok(default_node) = opts.state.get_default_node().await { + if !default_node.is_running() { + // The default node was stopped, so we can reuse the name + self.name = default_node.name(); + } + } } if self.http_server { @@ -393,7 +399,7 @@ mod tests { use crate::run::parser::resource::utils::parse_cmd_from_args; use crate::GlobalArgs; use ockam_api::output::{OutputBranding, OutputFormat}; - use ockam_api::terminal::Terminal; + use ockam_api::terminal::{LoggingOptions, Terminal}; use ockam_api::CliState; #[test] @@ -523,8 +529,11 @@ mod tests { let opts = CommandGlobalOpts { state: CliState::test().await.unwrap(), terminal: Terminal::new( - false, - false, + LoggingOptions { + enabled: false, + logging_to_file: false, + with_user_format: false, + }, false, true, false, @@ -581,8 +590,11 @@ mod tests { let opts = CommandGlobalOpts { state: CliState::test().await.unwrap(), terminal: Terminal::new( - false, - false, + LoggingOptions { + enabled: false, + logging_to_file: false, + with_user_format: false, + }, false, true, false, diff --git a/implementations/rust/ockam/ockam_command/src/subcommand.rs b/implementations/rust/ockam/ockam_command/src/subcommand.rs index bbaebe1d58a..e6caeed19de 100644 --- a/implementations/rust/ockam/ockam_command/src/subcommand.rs +++ b/implementations/rust/ockam/ockam_command/src/subcommand.rs @@ -286,7 +286,9 @@ impl OckamSubcommand { match self { OckamSubcommand::Node(cmd) => match &cmd.subcommand { NodeSubcommand::Create(cmd) => { - if cmd.foreground_args.child_process || !cmd.foreground_args.foreground { + let is_spawned_node = cmd.foreground_args.child_process; + let no_foreground_flag = !cmd.foreground_args.foreground; + if is_spawned_node || no_foreground_flag { CliState::default_node_dir(&cmd.name).ok() } else { None @@ -297,7 +299,9 @@ impl OckamSubcommand { OckamSubcommand::Authority(cmd) => match &cmd.subcommand { AuthoritySubcommand::Create(cmd) => { - if cmd.child_process || !cmd.foreground { + let is_spawned_node = cmd.child_process; + let no_foreground_flag = !cmd.foreground; + if is_spawned_node || no_foreground_flag { CliState::default_node_dir(&cmd.node_name()).ok() } else { None diff --git a/implementations/rust/ockam/ockam_command/src/value_parsers.rs b/implementations/rust/ockam/ockam_command/src/value_parsers.rs index 38c1eed6265..f4f7e264e06 100644 --- a/implementations/rust/ockam/ockam_command/src/value_parsers.rs +++ b/implementations/rust/ockam/ockam_command/src/value_parsers.rs @@ -5,7 +5,7 @@ use miette::{miette, Context, IntoDiagnostic}; use ockam_api::cli_state::{EnrollmentTicket, ExportedEnrollmentTicket, LegacyEnrollmentTicket}; use ockam_api::colors::color_primary; use std::str::FromStr; -use tracing::{trace, warn}; +use tracing::{debug, trace}; use url::Url; /// Parse a single key-value pair @@ -65,7 +65,7 @@ pub(crate) async fn read_contents_from_string_or_path_or_url( value: &str, ) -> miette::Result { read_contents_from_path_or_url(value).await.or_else(|err| { - warn!(%value, %err, "Couldn't parse value as a path or URL. Returning plain value to be processed as inline contents"); + debug!(%value, %err, "Couldn't parse value as a path or URL. Returning plain value to be processed as inline contents"); Ok(value.to_string()) }) } @@ -89,7 +89,7 @@ pub(crate) async fn read_contents_from_path_or_url(value: &str) -> miette::Resul .into_diagnostic() .context("Failed to read contents from file") } else { - warn!(%value, "Couldn't parse value as a path or URL"); + debug!(%value, "Couldn't parse value as a path or URL"); Err(miette!( "Couldn't parse value {} as a path or URL", color_primary(value) diff --git a/implementations/rust/ockam/ockam_transport_tcp/src/portal/addresses.rs b/implementations/rust/ockam/ockam_transport_tcp/src/portal/addresses.rs index 8ed4040faa1..d4babea3679 100644 --- a/implementations/rust/ockam/ockam_transport_tcp/src/portal/addresses.rs +++ b/implementations/rust/ockam/ockam_transport_tcp/src/portal/addresses.rs @@ -1,13 +1,18 @@ use core::fmt::Display; use ockam_core::Address; +use ockam_transport_core::HostnamePort; /// Enumerate all portal types #[derive(Debug, Eq, PartialEq, Clone)] pub(crate) enum PortalType { - Inlet, + Inlet { + listener_address: HostnamePort, + }, Outlet, #[allow(unused)] - PrivilegedInlet, + PrivilegedInlet { + listener_address: HostnamePort, + }, #[allow(unused)] PrivilegedOutlet, } @@ -15,28 +20,17 @@ pub(crate) enum PortalType { impl PortalType { pub fn str(&self) -> &'static str { match self { - PortalType::Inlet | PortalType::PrivilegedInlet => "inlet", + PortalType::Inlet { .. } | PortalType::PrivilegedInlet { .. } => "inlet", PortalType::Outlet | PortalType::PrivilegedOutlet => "outlet", } } pub fn is_privileged(&self) -> bool { match self { - PortalType::Inlet | PortalType::Outlet => false, - PortalType::PrivilegedInlet | PortalType::PrivilegedOutlet => true, + PortalType::Inlet { .. } | PortalType::Outlet => false, + PortalType::PrivilegedInlet { .. } | PortalType::PrivilegedOutlet => true, } } - - pub fn is_inlet(&self) -> bool { - match self { - PortalType::Inlet | PortalType::PrivilegedInlet => true, - PortalType::Outlet | PortalType::PrivilegedOutlet => false, - } - } - - pub fn is_outlet(&self) -> bool { - !self.is_inlet() - } } impl Display for PortalType { @@ -47,6 +41,7 @@ impl Display for PortalType { #[derive(Clone, Debug)] pub(crate) struct Addresses { + pub(crate) portal_type: PortalType, /// Used to receive messages from the corresponding receiver `receiver_internal` Address pub(crate) sender_internal: Address, /// Used to receive messages from the other side's Receiver @@ -83,6 +78,7 @@ impl Addresses { )); Self { + portal_type, sender_internal, sender_remote, receiver_internal, diff --git a/implementations/rust/ockam/ockam_transport_tcp/src/portal/inlet_listener.rs b/implementations/rust/ockam/ockam_transport_tcp/src/portal/inlet_listener.rs index 51c9cc4c407..a184191bcd1 100644 --- a/implementations/rust/ockam/ockam_transport_tcp/src/portal/inlet_listener.rs +++ b/implementations/rust/ockam/ockam_transport_tcp/src/portal/inlet_listener.rs @@ -182,7 +182,11 @@ impl Processor for TcpInletListenProcessor { .set_nodelay(!self.options.enable_nagle) .map_err(TransportError::from)?; - let addresses = Addresses::generate(PortalType::Inlet); + let addresses = Addresses::generate(PortalType::Inlet { + listener_address: HostnamePort::from( + self.inner.local_addr().map_err(TransportError::from)?, + ), + }); let inlet_shared_state = self.inlet_shared_state.read().unwrap().clone(); diff --git a/implementations/rust/ockam/ockam_transport_tcp/src/portal/portal_worker.rs b/implementations/rust/ockam/ockam_transport_tcp/src/portal/portal_worker.rs index d93edf3e910..44a5fb50197 100644 --- a/implementations/rust/ockam/ockam_transport_tcp/src/portal/portal_worker.rs +++ b/implementations/rust/ockam/ockam_transport_tcp/src/portal/portal_worker.rs @@ -57,7 +57,6 @@ pub(crate) struct TcpPortalWorker { addresses: Addresses, remote_route: Option, is_disconnecting: bool, - portal_type: PortalType, last_received_packet_counter: u16, outgoing_access_control: Arc, is_tls: bool, @@ -206,12 +205,7 @@ impl TcpPortalWorker { handshake_mode: HandshakeMode, enable_nagle: bool, ) -> Result<()> { - let portal_type = if streams.is_some() { - PortalType::Inlet - } else { - PortalType::Outlet - }; - debug!(%portal_type, sender_remote=%addresses.sender_remote, %is_tls, "creating portal worker"); + debug!(%addresses.portal_type, sender_remote=%addresses.sender_remote, %is_tls, "creating portal worker"); let (rx, tx) = match streams { // A TcpStream is provided in case of an inlet @@ -232,7 +226,6 @@ impl TcpPortalWorker { addresses: addresses.clone(), remote_route, is_disconnecting: false, - portal_type, last_received_packet_counter: u16::MAX, is_tls, outgoing_access_control: outgoing_access_control.clone(), @@ -354,12 +347,12 @@ impl TcpPortalWorker { .is_err() { debug!( - portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, + portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "error notifying the other side of portal that the connection is dropped", ); } else { debug!( - portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, + portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "notified the other side of portal that the connection is dropped", ); } @@ -369,11 +362,11 @@ impl TcpPortalWorker { fn stop_receiver(&self, ctx: &Context) { match ctx.stop_address(&self.addresses.receiver_remote) { Ok(_) => { - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "stopped receiver due to connection drop"); } Err(_) => { - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "error stopping receiver due to connection drop"); } } @@ -431,7 +424,7 @@ impl TcpPortalWorker { } } - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "stopped due to connection drop"); Ok(()) @@ -447,7 +440,7 @@ impl TcpPortalWorker { ) .await?; - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, "sent ping"); + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "sent ping"); if self.skip_handshake() { self.remote_route = Some(ping_route.clone()); @@ -461,12 +454,12 @@ impl TcpPortalWorker { async fn connect(&mut self) -> Result<()> { if self.is_tls { - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, "connect to {} via TLS", &self.hostname_port); + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "connect to {} via TLS", &self.hostname_port); let (rx, tx) = connect_tls(&self.hostname_port, self.enable_nagle).await?; self.write_half = Some(WriteHalfWithTls(tx)); self.read_half = Some(ReadHalfWithTls(rx)); } else { - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, "connect to {}", self.hostname_port); + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "connect to {}", self.hostname_port); let (rx, tx) = connect(&self.hostname_port, self.enable_nagle, None).await?; self.write_half = Some(WriteHalfNoTls(tx)); self.read_half = Some(ReadHalfNoTls(rx)); @@ -496,7 +489,7 @@ impl TcpPortalWorker { self.start_receiver(ctx, pong_route.clone())?; - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, "sent pong"); + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "sent pong"); self.remote_route = Some(pong_route); Ok(State::Initialized) @@ -529,27 +522,25 @@ impl Worker for TcpPortalWorker { self.registry .add_portal_worker(&self.addresses.sender_remote); - if self.portal_type.is_inlet() { - info!( - "The TCP Inlet at {} connected to {}", - &self.hostname_port, - self.their_identifier - .as_ref() - .map(|i| i.to_string()) - .unwrap_or_else(|| "unknown".to_string()), - ); - } else if self.portal_type.is_outlet() { - info!( - "The TCP Outlet at {} received connection from {}", - &self.hostname_port, - self.their_identifier - .as_ref() - .map(|i| i.to_string()) - .unwrap_or_else(|| "unknown".to_string()), - ); + let (portal_type, listener_address) = match &self.addresses.portal_type { + PortalType::Inlet { listener_address } + | PortalType::PrivilegedInlet { listener_address } => ("TCP Inlet", listener_address), + PortalType::Outlet | PortalType::PrivilegedOutlet => { + ("TCP Outlet", &self.hostname_port) + } + }; + info! { + target: "ockam_command::user", + "The {} listening at {} connected to {}", + portal_type, + listener_address, + self.their_identifier + .as_ref() + .map(|i| i.to_string()) + .unwrap_or_else(|| "unknown".to_string()), } - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "tcp portal worker initialized" ); @@ -622,7 +613,7 @@ impl Worker for TcpPortalWorker { self.handle_receive_pong(ctx, return_route) } State::Initialized => { - trace!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, + trace!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "received {} tcp packet", if remote_packet { "remote" } else { "internal " }, ); @@ -659,7 +650,7 @@ impl TcpPortalWorker { #[instrument(skip_all)] fn handle_receive_pong(&mut self, ctx: &Context, return_route: Route) -> Result<()> { self.start_receiver(ctx, return_route.clone())?; - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, "received pong"); + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "received pong"); self.remote_route = Some(return_route); self.state = State::Initialized; Ok(()) @@ -667,21 +658,24 @@ impl TcpPortalWorker { #[instrument(skip_all)] async fn handle_disconnect(&mut self, ctx: &Context) -> Result<()> { - let portal_type_str = if self.portal_type.is_inlet() { - "TCP Inlet" - } else { - "TCP Outlet" + let (portal_type, listener_address) = match &self.addresses.portal_type { + PortalType::Inlet { listener_address } + | PortalType::PrivilegedInlet { listener_address } => ("TCP Inlet", listener_address), + PortalType::Outlet | PortalType::PrivilegedOutlet => { + ("TCP Outlet", &self.hostname_port) + } }; - info!( - "The {} at {} was disconnected from to {}", - portal_type_str, - &self.hostname_port, + info! { + target: "ockam_command::user", + "The {} listening at {} was disconnected from {}", + portal_type, + listener_address, self.their_identifier .as_ref() .map(|i| i.to_string()) .unwrap_or_else(|| "unknown".to_string()), - ); - debug!(portal_type = %self.portal_type, sender_internal = %self.addresses.sender_internal, + } + debug!(portal_type = %self.addresses.portal_type, sender_internal = %self.addresses.sender_internal, "tcp stream was dropped"); self.start_disconnection(ctx, DisconnectionReason::FailedRx) .await @@ -707,7 +701,7 @@ impl TcpPortalWorker { WriteHalfWithTls(tx) => tx.write_all(payload).await, }; if let Err(err) = result { - warn!(portal_type = %self.portal_type, %err, + warn!(portal_type = %self.addresses.portal_type, %err, "failed to send message to peer {} with error", self.hostname_port ); @@ -732,7 +726,7 @@ impl TcpPortalWorker { }; if packet_counter != expected_counter { - warn!(portal_type = %self.portal_type, + warn!(portal_type = %self.addresses.portal_type, "Received packet with counter {} while expecting {}, disconnecting", packet_counter, expected_counter );