refactor(NODE-1484): refactor os_tools and networking code

rs/ic_os/network/src/lib.rs

@@ -1,9 +1,8 @@
 use std::path::Path;
 
-use anyhow::{Context, Result};
+use anyhow::Result;
 
-use crate::mac_address::generate_mac_address;
-use crate::node_type::NodeType;
+use crate::mac_address::UnformattedMacAddress;
 use crate::systemd::generate_systemd_config_files;
 use info::NetworkInfo;
 use ipv6::generate_ipv6_address;
@@ -20,22 +19,14 @@ pub mod systemd;
 /// Requires superuser permissions to run `ipmitool` and write to the systemd directory
 pub fn generate_network_config(
     network_info: &NetworkInfo,
-    mgmt_mac: Option<&str>,
-    deployment_name: Option<&str>,
-    node_type: NodeType,
+    generated_mac: UnformattedMacAddress,
     output_directory: &Path,
 ) -> Result<()> {
-    let deployment_name = deployment_name
-        .context("Error: Deployment name not found when attempting to generate mac address")?;
-
-    let mac = generate_mac_address(deployment_name, &node_type, mgmt_mac)?;
-    eprintln!("Using generated mac (unformatted) {}", mac.get());
-
     eprintln!("Generating ipv6 address");
-    let ipv6_address = generate_ipv6_address(&network_info.ipv6_prefix, &mac)?;
+    let ipv6_address = generate_ipv6_address(&network_info.ipv6_prefix, &generated_mac)?;
     eprintln!("Using ipv6 address: {}", ipv6_address);
 
-    let formatted_mac = FormattedMacAddress::from(&mac);
+    let formatted_mac = FormattedMacAddress::from(&generated_mac);
     generate_systemd_config_files(
         output_directory,
         network_info,

rs/ic_os/network/src/mac_address.rs

@@ -95,61 +95,31 @@ pub fn get_mac_address_from_ipmitool_output(output: &str) -> Result<FormattedMac
 
 /// Generate a deterministic unformatted MAC address
 /// E.g. "6a01eb49a2b0"
-fn generate_mac_address_internal(
+pub fn generate_mac_address(
     mgmt_mac: &FormattedMacAddress,
     deployment_name: &str,
     node_type: &NodeType,
-    version: char,
 ) -> Result<UnformattedMacAddress> {
-    if version != '4' && version != '6' {
-        bail!("Invalid version used to generate MAC address: {}", version);
-    }
-
     // Newline added to match behavior
     let seed = format!("{}{}\n", mgmt_mac.get(), deployment_name);
     let vendor_part: String = hex::encode(Sha256::digest(seed)).chars().take(8).collect();
-    // When IPv4 and IPv6 were split, a different MAC for each bond was desired.
-    // Leave for compatibility until later
-    let version_octet = match version {
-        '4' => "4a",
-        _ => "6a",
-    };
     let node_index = node_type.to_char();
-    let mac = format!("{}0{}{}", version_octet, node_index, vendor_part);
+    let mac = format!("6a0{}{}", node_index, vendor_part);
     UnformattedMacAddress::try_from(mac.as_str())
 }
 
-/// Query the BMC MAC address and return deterministically generated MAC
-pub fn generate_mac_address(
-    deployment_name: &str,
-    node_type: &NodeType,
-    mgmt_mac: Option<&str>,
-) -> Result<UnformattedMacAddress> {
-    let mgmt_mac = if let Some(mgmt_mac) = mgmt_mac {
-        let mgmt_mac = FormattedMacAddress::try_from(mgmt_mac)?;
+/// Retrieves the MAC address from the IPMI LAN interface
+pub fn get_ipmi_mac() -> Result<FormattedMacAddress> {
+    let output = Command::new("ipmitool").arg("lan").arg("print").output()?;
+    if !output.status.success() {
         eprintln!(
-            "Using mgmt_mac address found in deployment.json: {}",
-            mgmt_mac.get()
+            "Error running ipmitool: {}",
+            std::str::from_utf8(&output.stderr)?
         );
-        Ok(mgmt_mac)
-    } else {
-        // A bug in our version of ipmitool causes it to exit with an error
-        // status, but we have enough output to work with anyway.
-        // https://github.com/ipmitool/ipmitool/issues/388
-
-        // let ipmitool_output = get_command_stdout("ipmitool", ["lan", "print"])?;
-        let output = Command::new("ipmitool").arg("lan").arg("print").output()?;
-        if !output.status.success() {
-            eprintln!(
-                "Error running ipmitool: {}",
-                std::str::from_utf8(&output.stderr)?
-            );
-        }
-        let ipmitool_output = String::from_utf8(output.stdout)?;
+    }
+    let ipmitool_output = String::from_utf8(output.stdout)?;
 
-        get_mac_address_from_ipmitool_output(&ipmitool_output)
-    }?;
-    generate_mac_address_internal(&mgmt_mac, deployment_name, node_type, '6')
+    get_mac_address_from_ipmitool_output(&ipmitool_output)
 }
 
 #[cfg(test)]
@@ -186,44 +156,20 @@ pub mod tests {
     #[test]
     fn test_generate_mac_address() {
         assert_eq!(
-            generate_mac_address_internal(
-                &FormattedMacAddress::try_from("de:ad:de:ad:de:ad").unwrap(),
-                "mainnet",
-                &NodeType::SetupOS,
-                '4'
-            )
-            .unwrap()
-            .get(),
-            "4a0ff7e0c684"
-        );
-        assert_eq!(
-            generate_mac_address_internal(
-                &FormattedMacAddress::try_from("de:ad:de:ad:de:ad").unwrap(),
-                "mainnet",
-                &NodeType::GuestOS,
-                '4'
-            )
-            .unwrap()
-            .get(),
-            "4a01f7e0c684"
-        );
-        assert_eq!(
-            generate_mac_address_internal(
+            generate_mac_address(
                 &FormattedMacAddress::try_from("de:ad:de:ad:de:ad").unwrap(),
                 "mainnet",
                 &NodeType::GuestOS,
-                '6'
             )
             .unwrap()
             .get(),
             "6a01f7e0c684"
         );
         assert_eq!(
-            generate_mac_address_internal(
+            generate_mac_address(
                 &FormattedMacAddress::try_from("00:aa:bb:cc:dd:ee").unwrap(),
                 "mainnet",
                 &NodeType::GuestOS,
-                '6'
             )
             .unwrap()
             .get(),

rs/ic_os/os_tools/hostos_tool/src/main.rs

@@ -4,12 +4,12 @@ use anyhow::{anyhow, Context, Result};
 use clap::{Parser, Subcommand};
 
 use config::config_ini::config_map_from_path;
-use config::deployment_json::get_deployment_settings;
+use config::deployment_json::{get_deployment_settings, DeploymentSettings};
 use config::{DEFAULT_HOSTOS_CONFIG_INI_FILE_PATH, DEFAULT_HOSTOS_DEPLOYMENT_JSON_PATH};
 use network::generate_network_config;
 use network::info::NetworkInfo;
 use network::ipv6::generate_ipv6_address;
-use network::mac_address::{generate_mac_address, FormattedMacAddress};
+use network::mac_address::{generate_mac_address, get_ipmi_mac, FormattedMacAddress};
 use network::node_type::NodeType;
 use network::systemd::DEFAULT_SYSTEMD_NETWORK_DIR;
 use utils::to_cidr;
@@ -55,83 +55,82 @@ pub fn main() -> Result<()> {
     let opts = HostOSArgs::parse();
 
     match opts.command {
-        Some(Commands::GenerateNetworkConfig { output_directory }) => {
-            let config_map = config_map_from_path(Path::new(&opts.config))
-                .context("Please specify a valid config file with '--config'")?;
-            eprintln!("Using config: {:?}", config_map);
-
-            let network_info = NetworkInfo::from_config_map(&config_map)?;
-            eprintln!("Network info config: {:?}", &network_info);
-
-            let deployment_settings = get_deployment_settings(Path::new(&opts.deployment_file));
-
-            let deployment_name: Option<&str> = match &deployment_settings {
-                Ok(deployment) => Some(deployment.deployment.name.as_str()),
-                Err(e) => {
-                    eprintln!("Error retrieving deployment file: {e}. Continuing without it");
-                    None
-                }
-            };
-
-            let mgmt_mac: Option<&str> = match &deployment_settings {
-                Ok(deployment) => deployment.deployment.mgmt_mac.as_deref(),
-                Err(_) => None,
-            };
-
-            generate_network_config(
-                &network_info,
-                mgmt_mac,
-                deployment_name,
-                NodeType::HostOS,
-                Path::new(&output_directory),
-            )
-        }
-        Some(Commands::GenerateIpv6Address { node_type }) => {
-            let deployment_settings = get_deployment_settings(Path::new(&opts.deployment_file))
-                .context("Please specify a valid deployment file with '--deployment-file'")?;
-            eprintln!("Deployment config: {:?}", deployment_settings);
-
-            let config_map = config_map_from_path(Path::new(&opts.config))
-                .context("Please specify a valid config file with '--config'")?;
-            eprintln!("Using config: {:?}", config_map);
+        Some(Commands::GenerateNetworkConfig {
+            ref output_directory,
+        }) => {
+            let (network_info, mgmt_mac, deployment_settings) = get_config_settings(&opts)?;
+
+            let generated_mac = generate_mac_address(
+                &mgmt_mac,
+                deployment_settings.deployment.name.as_str(),
+                &NodeType::HostOS,
+            )?;
 
-            let network_info = NetworkInfo::from_config_map(&config_map)?;
-            eprintln!("Network info config: {:?}", &network_info);
+            generate_network_config(&network_info, generated_mac, Path::new(&output_directory))
+        }
+        Some(Commands::GenerateIpv6Address { ref node_type }) => {
+            let (network_info, mgmt_mac, deployment_settings) = get_config_settings(&opts)?;
 
             let node_type = node_type.parse::<NodeType>()?;
-            let mac = generate_mac_address(
-                &deployment_settings.deployment.name,
+            let generated_mac = generate_mac_address(
+                &mgmt_mac,
+                deployment_settings.deployment.name.as_str(),
                 &node_type,
-                deployment_settings.deployment.mgmt_mac.as_deref(),
             )?;
-            let ipv6_address = generate_ipv6_address(&network_info.ipv6_prefix, &mac)?;
+
+            let ipv6_address = generate_ipv6_address(&network_info.ipv6_prefix, &generated_mac)?;
             println!("{}", to_cidr(ipv6_address, network_info.ipv6_subnet));
             Ok(())
         }
-        Some(Commands::GenerateMacAddress { node_type }) => {
-            let config_map = config_map_from_path(Path::new(&opts.config))
-                .context("Please specify a valid config file with '--config'")?;
-            eprintln!("Using config: {:?}", config_map);
-
-            let network_info = NetworkInfo::from_config_map(&config_map)?;
-            eprintln!("Network info config: {:?}", &network_info);
-
-            let deployment_settings = get_deployment_settings(Path::new(&opts.deployment_file))
-                .context("Please specify a valid deployment file with '--deployment-file'")?;
-            eprintln!("Deployment config: {:?}", deployment_settings);
+        Some(Commands::GenerateMacAddress { ref node_type }) => {
+            let (_, mgmt_mac, deployment_settings) = get_config_settings(&opts)?;
 
             let node_type = node_type.parse::<NodeType>()?;
-            let mac = generate_mac_address(
-                &deployment_settings.deployment.name,
+            let generated_mac = generate_mac_address(
+                &mgmt_mac,
+                deployment_settings.deployment.name.as_str(),
                 &node_type,
-                deployment_settings.deployment.mgmt_mac.as_deref(),
             )?;
-            let mac = FormattedMacAddress::from(&mac);
-            println!("{}", mac.get());
+            let generated_mac = FormattedMacAddress::from(&generated_mac);
+            println!("{}", generated_mac.get());
             Ok(())
         }
         None => Err(anyhow!(
             "No subcommand specified. Run with '--help' for subcommands"
         )),
     }
 }
+
+fn get_config_settings(
+    opts: &HostOSArgs,
+) -> Result<(NetworkInfo, FormattedMacAddress, DeploymentSettings)> {
+    let config_map = config_map_from_path(Path::new(&opts.config)).context(format!(
+        "Failed to get config.ini settings for path: {}",
+        &opts.config
+    ))?;
+    eprintln!("Using config: {:?}", config_map);
+
+    let network_info = NetworkInfo::from_config_map(&config_map)?;
+    eprintln!("Network info config: {:?}", &network_info);
+
+    let deployment_settings =
+        get_deployment_settings(Path::new(&opts.deployment_file)).context(format!(
+            "Failed to get deployment settings for file: {}",
+            &opts.deployment_file
+        ))?;
+    eprintln!("Deployment config: {:?}", deployment_settings);
+
+    let mgmt_mac = match deployment_settings.deployment.mgmt_mac {
+        Some(ref config_mac) => {
+            let mgmt_mac = FormattedMacAddress::try_from(config_mac.as_str())?;
+            eprintln!(
+                "Using mgmt_mac address found in deployment.json: {}",
+                mgmt_mac.get()
+            );
+            mgmt_mac
+        }
+        None => get_ipmi_mac()?,
+    };
+
+    Ok((network_info, mgmt_mac, deployment_settings))
+}