From 13b08512c9ff15ca1422cf3ab8eb678423f32c58 Mon Sep 17 00:00:00 2001 From: Chris O'Neil Date: Thu, 14 Mar 2024 23:22:41 +0000 Subject: [PATCH] feat: support a port range on the `add` command Previously we would not allow a custom port to be specified if more than one service was being added. However, without autonat, people still need to open ports manually on their router, so the node services they add will need to match this port range. Without the ability to specify a range, they'd need to add the services one by one. --- sn_node_manager/src/add_services/config.rs | 28 ++- sn_node_manager/src/add_services/mod.rs | 44 +++- sn_node_manager/src/add_services/tests.rs | 272 ++++++++++++++++++++- sn_node_manager/src/bin/cli/main.rs | 17 +- sn_node_manager/src/cmd/node.rs | 7 +- 5 files changed, 340 insertions(+), 28 deletions(-) diff --git a/sn_node_manager/src/add_services/config.rs b/sn_node_manager/src/add_services/config.rs index 1f426208fb..805f0a9b66 100644 --- a/sn_node_manager/src/add_services/config.rs +++ b/sn_node_manager/src/add_services/config.rs @@ -6,15 +6,39 @@ // KIND, either express or implied. Please review the Licences for the specific language governing // permissions and limitations relating to use of the SAFE Network Software. -use color_eyre::Result; +use color_eyre::{eyre::eyre, Result}; use libp2p::Multiaddr; use service_manager::{ServiceInstallCtx, ServiceLabel}; use std::{ ffi::OsString, net::{Ipv4Addr, SocketAddr}, path::PathBuf, + str::FromStr, }; +#[derive(Clone, Debug)] +pub enum PortRange { + Single(u16), + Range(u16, u16), +} + +pub fn parse_port_range(s: &str) -> Result { + if let Ok(port) = u16::from_str(s) { + Ok(PortRange::Single(port)) + } else { + let parts: Vec<&str> = s.split('-').collect(); + if parts.len() != 2 { + return Err(eyre!("Port range must be in the format 'start-end'")); + } + let start = parts[0].parse::()?; + let end = parts[1].parse::()?; + if start >= end { + return Err(eyre!("End port must be greater than start port")); + } + Ok(PortRange::Range(start, end)) + } +} + #[derive(Debug, PartialEq)] pub struct InstallNodeServiceCtxBuilder { pub data_dir_path: PathBuf, @@ -82,7 +106,7 @@ pub struct AddNodeServiceOptions { pub env_variables: Option>, pub genesis: bool, pub local: bool, - pub node_port: Option, + pub node_port: Option, pub rpc_address: Option, pub safenode_bin_path: PathBuf, pub safenode_dir_path: PathBuf, diff --git a/sn_node_manager/src/add_services/mod.rs b/sn_node_manager/src/add_services/mod.rs index 482b686df7..0efc0e5618 100644 --- a/sn_node_manager/src/add_services/mod.rs +++ b/sn_node_manager/src/add_services/mod.rs @@ -11,7 +11,7 @@ mod tests; use self::config::{ AddDaemonServiceOptions, AddFaucetServiceOptions, AddNodeServiceOptions, - InstallFaucetServiceCtxBuilder, InstallNodeServiceCtxBuilder, + InstallFaucetServiceCtxBuilder, InstallNodeServiceCtxBuilder, PortRange, }; use crate::{config::create_owned_dir, VerbosityLevel, DAEMON_SERVICE_NAME}; use color_eyre::{eyre::eyre, Help, Result}; @@ -51,12 +51,25 @@ pub async fn add_node( } } - if options.count.is_some() && options.node_port.is_some() { - let count = options.count.unwrap(); - if count > 1 { - return Err(eyre!( - "Custom node port can only be used when adding a single service" - )); + if let Some(ref port_range) = options.node_port { + match port_range { + PortRange::Single(_) => { + let count = options.count.unwrap_or(1); + if count != 1 { + return Err(eyre!( + "The number of services to add ({count}) does not match the number of ports (1)" + )); + } + } + PortRange::Range(start, end) => { + let port_count = end - start + 1; + let service_count = options.count.unwrap_or(1); + if port_count != service_count { + return Err(eyre!( + "The number of services to add ({service_count}) does not match the number of ports ({port_count})" + )); + } + } } } @@ -99,6 +112,15 @@ pub async fn add_node( let target_node_count = current_node_count + options.count.unwrap_or(1); let mut node_number = current_node_count + 1; + let mut port_number = if let Some(port) = options.node_port { + match port { + PortRange::Single(val) => Some(val), + PortRange::Range(start, _) => Some(start), + } + } else { + None + }; + while node_number <= target_node_count { let rpc_free_port = service_control.get_available_port()?; let rpc_socket_addr = if let Some(addr) = options.rpc_address { @@ -127,7 +149,7 @@ pub async fn add_node( local: options.local, log_dir_path: service_log_dir_path.clone(), name: service_name.clone(), - node_port: options.node_port, + node_port: port_number, rpc_socket_addr, safenode_path: service_safenode_path.clone(), service_user: options.user.clone(), @@ -171,6 +193,12 @@ pub async fn add_node( } node_number += 1; + port_number = if let Some(port) = port_number { + let incremented_port = port + 1; + Some(incremented_port) + } else { + None + }; } std::fs::remove_file(options.safenode_bin_path)?; diff --git a/sn_node_manager/src/add_services/tests.rs b/sn_node_manager/src/add_services/tests.rs index 804c4ac482..c2db11af99 100644 --- a/sn_node_manager/src/add_services/tests.rs +++ b/sn_node_manager/src/add_services/tests.rs @@ -11,7 +11,7 @@ use crate::{ add_daemon, add_faucet, add_node, config::{ AddDaemonServiceOptions, AddFaucetServiceOptions, AddNodeServiceOptions, - InstallNodeServiceCtxBuilder, + InstallNodeServiceCtxBuilder, PortRange, }, }, VerbosityLevel, @@ -918,7 +918,7 @@ async fn add_node_should_use_custom_ports_for_one_service() -> Result<()> { service_data_dir_path: node_data_dir.to_path_buf(), service_log_dir_path: node_logs_dir.to_path_buf(), bootstrap_peers: vec![], - node_port: Some(custom_port), + node_port: Some(PortRange::Single(custom_port)), rpc_address: Some(Ipv4Addr::new(127, 0, 0, 1)), url: None, user: get_username(), @@ -958,11 +958,12 @@ async fn add_node_should_use_custom_ports_for_one_service() -> Result<()> { } #[tokio::test] -async fn add_node_should_return_error_if_custom_port_is_used_and_more_than_one_service_is_used( -) -> Result<()> { +async fn add_node_should_use_a_custom_port_range() -> Result<()> { let tmp_data_dir = assert_fs::TempDir::new()?; let node_reg_path = tmp_data_dir.child("node_reg.json"); + let mut mock_service_control = MockServiceControl::new(); + let mut node_registry = NodeRegistry { faucet: None, save_path: node_reg_path.to_path_buf(), @@ -977,21 +978,270 @@ async fn add_node_should_return_error_if_custom_port_is_used_and_more_than_one_s node_data_dir.create_dir_all()?; let node_logs_dir = temp_dir.child("logs"); node_logs_dir.create_dir_all()?; + let safenode_download_path = temp_dir.child(SAFENODE_FILE_NAME); + safenode_download_path.write_binary(b"fake safenode bin")?; - let custom_port = 12000; + let mut seq = Sequence::new(); - let result = add_node( + // First service + mock_service_control + .expect_get_available_port() + .times(1) + .returning(|| Ok(15000)) + .in_sequence(&mut seq); + mock_service_control + .expect_install() + .times(1) + .with(eq(ServiceInstallCtx { + args: vec![ + OsString::from("--rpc"), + OsString::from("127.0.0.1:15000"), + OsString::from("--root-dir"), + OsString::from( + node_data_dir + .to_path_buf() + .join("safenode1") + .to_string_lossy() + .to_string(), + ), + OsString::from("--log-output-dest"), + OsString::from( + node_logs_dir + .to_path_buf() + .join("safenode1") + .to_string_lossy() + .to_string(), + ), + OsString::from("--port"), + OsString::from("12000"), + ], + contents: None, + environment: None, + label: "safenode1".parse()?, + program: node_data_dir + .to_path_buf() + .join("safenode1") + .join(SAFENODE_FILE_NAME), + username: Some(get_username()), + working_directory: None, + })) + .returning(|_| Ok(())) + .in_sequence(&mut seq); + + // Second service + mock_service_control + .expect_get_available_port() + .times(1) + .returning(|| Ok(15001)) + .in_sequence(&mut seq); + mock_service_control + .expect_install() + .times(1) + .with(eq(ServiceInstallCtx { + args: vec![ + OsString::from("--rpc"), + OsString::from("127.0.0.1:15001"), + OsString::from("--root-dir"), + OsString::from( + node_data_dir + .to_path_buf() + .join("safenode2") + .to_string_lossy() + .to_string(), + ), + OsString::from("--log-output-dest"), + OsString::from( + node_logs_dir + .to_path_buf() + .join("safenode2") + .to_string_lossy() + .to_string(), + ), + OsString::from("--port"), + OsString::from("12001"), + ], + contents: None, + environment: None, + label: "safenode2".parse()?, + program: node_data_dir + .to_path_buf() + .join("safenode2") + .join(SAFENODE_FILE_NAME), + username: Some(get_username()), + working_directory: None, + })) + .returning(|_| Ok(())) + .in_sequence(&mut seq); + + // Third service + mock_service_control + .expect_get_available_port() + .times(1) + .returning(|| Ok(15002)) + .in_sequence(&mut seq); + mock_service_control + .expect_install() + .times(1) + .with(eq(ServiceInstallCtx { + args: vec![ + OsString::from("--rpc"), + OsString::from("127.0.0.1:15002"), + OsString::from("--root-dir"), + OsString::from( + node_data_dir + .to_path_buf() + .join("safenode3") + .to_string_lossy() + .to_string(), + ), + OsString::from("--log-output-dest"), + OsString::from( + node_logs_dir + .to_path_buf() + .join("safenode3") + .to_string_lossy() + .to_string(), + ), + OsString::from("--port"), + OsString::from("12002"), + ], + contents: None, + environment: None, + label: "safenode3".parse()?, + program: node_data_dir + .to_path_buf() + .join("safenode3") + .join(SAFENODE_FILE_NAME), + username: Some(get_username()), + working_directory: None, + })) + .returning(|_| Ok(())) + .in_sequence(&mut seq); + + add_node( AddNodeServiceOptions { - local: true, + local: false, genesis: false, count: Some(3), - safenode_bin_path: PathBuf::new(), + safenode_bin_path: safenode_download_path.to_path_buf(), safenode_dir_path: temp_dir.to_path_buf(), service_data_dir_path: node_data_dir.to_path_buf(), service_log_dir_path: node_logs_dir.to_path_buf(), bootstrap_peers: vec![], - node_port: Some(custom_port), - rpc_address: None, + node_port: Some(PortRange::Range(12000, 12002)), + rpc_address: Some(Ipv4Addr::new(127, 0, 0, 1)), + url: None, + user: get_username(), + version: latest_version.to_string(), + env_variables: None, + }, + &mut node_registry, + &mock_service_control, + VerbosityLevel::Normal, + ) + .await?; + + safenode_download_path.assert(predicate::path::missing()); + node_data_dir.assert(predicate::path::is_dir()); + node_logs_dir.assert(predicate::path::is_dir()); + assert_eq!(node_registry.nodes.len(), 3); + + Ok(()) +} + +#[tokio::test] +async fn add_node_should_return_an_error_if_port_and_node_count_do_not_match() -> Result<()> { + let tmp_data_dir = assert_fs::TempDir::new()?; + let node_reg_path = tmp_data_dir.child("node_reg.json"); + + let mut node_registry = NodeRegistry { + faucet: None, + save_path: node_reg_path.to_path_buf(), + nodes: vec![], + bootstrap_peers: vec![], + environment_variables: None, + daemon: None, + }; + let latest_version = "0.96.4"; + let temp_dir = assert_fs::TempDir::new()?; + let node_data_dir = temp_dir.child("data"); + node_data_dir.create_dir_all()?; + let node_logs_dir = temp_dir.child("logs"); + node_logs_dir.create_dir_all()?; + let safenode_download_path = temp_dir.child(SAFENODE_FILE_NAME); + safenode_download_path.write_binary(b"fake safenode bin")?; + + let result = add_node( + AddNodeServiceOptions { + local: false, + genesis: false, + count: Some(2), + safenode_bin_path: safenode_download_path.to_path_buf(), + safenode_dir_path: temp_dir.to_path_buf(), + service_data_dir_path: node_data_dir.to_path_buf(), + service_log_dir_path: node_logs_dir.to_path_buf(), + bootstrap_peers: vec![], + node_port: Some(PortRange::Range(12000, 12002)), + rpc_address: Some(Ipv4Addr::new(127, 0, 0, 1)), + url: None, + user: get_username(), + version: latest_version.to_string(), + env_variables: None, + }, + &mut node_registry, + &MockServiceControl::new(), + VerbosityLevel::Normal, + ) + .await; + + match result { + Ok(_) => panic!("This test should result in an error"), + Err(e) => { + assert_eq!( + format!("The number of services to add (2) does not match the number of ports (3)"), + e.to_string() + ) + } + } + + Ok(()) +} + +#[tokio::test] +async fn add_node_should_return_an_error_if_multiple_services_are_specified_with_a_single_port( +) -> Result<()> { + let tmp_data_dir = assert_fs::TempDir::new()?; + let node_reg_path = tmp_data_dir.child("node_reg.json"); + + let mut node_registry = NodeRegistry { + faucet: None, + save_path: node_reg_path.to_path_buf(), + nodes: vec![], + bootstrap_peers: vec![], + environment_variables: None, + daemon: None, + }; + let latest_version = "0.96.4"; + let temp_dir = assert_fs::TempDir::new()?; + let node_data_dir = temp_dir.child("data"); + node_data_dir.create_dir_all()?; + let node_logs_dir = temp_dir.child("logs"); + node_logs_dir.create_dir_all()?; + let safenode_download_path = temp_dir.child(SAFENODE_FILE_NAME); + safenode_download_path.write_binary(b"fake safenode bin")?; + + let result = add_node( + AddNodeServiceOptions { + local: false, + genesis: false, + count: Some(2), + safenode_bin_path: safenode_download_path.to_path_buf(), + safenode_dir_path: temp_dir.to_path_buf(), + service_data_dir_path: node_data_dir.to_path_buf(), + service_log_dir_path: node_logs_dir.to_path_buf(), + bootstrap_peers: vec![], + node_port: Some(PortRange::Single(12000)), + rpc_address: Some(Ipv4Addr::new(127, 0, 0, 1)), url: None, user: get_username(), version: latest_version.to_string(), @@ -1007,7 +1257,7 @@ async fn add_node_should_return_error_if_custom_port_is_used_and_more_than_one_s Ok(_) => panic!("This test should result in an error"), Err(e) => { assert_eq!( - format!("Custom node port can only be used when adding a single service"), + format!("The number of services to add (2) does not match the number of ports (1)"), e.to_string() ) } diff --git a/sn_node_manager/src/bin/cli/main.rs b/sn_node_manager/src/bin/cli/main.rs index 534a595385..d8560f2ed6 100644 --- a/sn_node_manager/src/bin/cli/main.rs +++ b/sn_node_manager/src/bin/cli/main.rs @@ -8,7 +8,10 @@ use clap::{Parser, Subcommand}; use color_eyre::{eyre::eyre, Result}; -use sn_node_manager::{cmd, VerbosityLevel}; +use sn_node_manager::{ + add_services::config::{parse_port_range, PortRange}, + cmd, VerbosityLevel, +}; use sn_peers_acquisition::PeersArgs; use std::{net::Ipv4Addr, path::PathBuf}; @@ -65,11 +68,15 @@ pub enum SubCmd { log_dir_path: Option, #[command(flatten)] peers: PeersArgs, - /// Specify a port for the node to run on. If not used, a port will be selected at random. + /// Specify a port for the safenode service(s). /// - /// This option only applies when a single service is being added. - #[clap(long)] - port: Option, + /// If not used, ports will be selected at random. + /// + /// If multiple services are being added and this argument is used, you must specify a + /// range. For example, '12000-12004'. The length of the range must match the number of + /// services, which in this case would be 5. The range must also go from lower to higher. + #[clap(long, value_parser = parse_port_range)] + port: Option, #[clap(long)] /// Specify an Ipv4Addr for the node's RPC server to run on. /// diff --git a/sn_node_manager/src/cmd/node.rs b/sn_node_manager/src/cmd/node.rs index 67d1dac6a0..7dcbc16d37 100644 --- a/sn_node_manager/src/cmd/node.rs +++ b/sn_node_manager/src/cmd/node.rs @@ -10,7 +10,10 @@ use super::{download_and_get_upgrade_bin_path, is_running_as_root, print_upgrade_summary}; use crate::{ - add_services::{add_node, config::AddNodeServiceOptions}, + add_services::{ + add_node, + config::{AddNodeServiceOptions, PortRange}, + }, config, helpers::download_and_extract_release, status_report, ServiceManager, VerbosityLevel, @@ -36,7 +39,7 @@ pub async fn add( local: bool, log_dir_path: Option, peers: PeersArgs, - port: Option, + port: Option, rpc_address: Option, url: Option, user: Option,