From 9ee98f100bb91485004c82c186798edb643f8245 Mon Sep 17 00:00:00 2001 From: Andy Fiddaman Date: Mon, 5 Aug 2024 19:59:28 +0000 Subject: [PATCH] review feedback --- illumos-utils/src/ipadm.rs | 118 ++++++++++++++++++++---------- illumos-utils/src/lib.rs | 3 + sled-agent/src/bin/zone-bundle.rs | 19 +++-- 3 files changed, 95 insertions(+), 45 deletions(-) diff --git a/illumos-utils/src/ipadm.rs b/illumos-utils/src/ipadm.rs index a110fb0ca4..8bdeef07ad 100644 --- a/illumos-utils/src/ipadm.rs +++ b/illumos-utils/src/ipadm.rs @@ -7,7 +7,8 @@ use crate::addrobj::{IPV6_LINK_LOCAL_ADDROBJ_NAME, IPV6_STATIC_ADDROBJ_NAME}; use crate::zone::IPADM; use crate::{execute, ExecutionError, PFEXEC}; -use std::net::Ipv6Addr; +use oxnet::IpNet; +use std::net::{IpAddr, Ipv6Addr}; /// Wraps commands for interacting with interfaces. pub struct Ipadm {} @@ -22,6 +23,15 @@ const ADDROBJ_NOT_FOUND_ERR2: &str = "Address object not found"; /// Expected error message when an interface already exists. const INTERFACE_ALREADY_EXISTS: &str = "Interface already exists"; +/// Expected error message when an addrobj already eixsts. +const ADDROBJ_ALREADY_EXISTS: &str = "Address object already exists"; + +pub enum AddrObjType { + DHCP, + AddrConf, + Static(IpAddr), +} + #[cfg_attr(any(test, feature = "testing"), mockall::automock)] impl Ipadm { /// Ensure that an IP interface exists on the provided datalink. @@ -41,9 +51,55 @@ impl Ipadm { } } + /// Ensure that a particular address object exists. Note that this checks + /// whether an address object with the provided name already exists but + /// does not validate its properties. + pub fn ensure_ip_addrobj_exists( + addrobj: &str, + addrtype: AddrObjType, + ) -> Result<(), ExecutionError> { + let mut cmd = std::process::Command::new(PFEXEC); + let cmd = cmd.args(&[IPADM, "create-addr", "-t", "-T"]); + let cmd = match addrtype { + AddrObjType::DHCP => cmd.args(&["dhcp"]), + AddrObjType::AddrConf => cmd.args(&["addrconf"]), + AddrObjType::Static(addr) => { + cmd.args(&["static", "-a", &addr.to_string()]) + } + }; + let cmd = cmd.arg(&addrobj); + match execute(cmd) { + Ok(_) => Ok(()), + Err(ExecutionError::CommandFailure(info)) + if info.stderr.contains(ADDROBJ_ALREADY_EXISTS) => + { + Ok(()) + } + Err(e) => Err(e), + } + } + + /// Remove any scope from an IPv6 address. + /// e.g. fe80::8:20ff:fed0:8687%oxControlService1/10 -> + /// fe80::8:20ff:fed0:8687/10 + fn remove_addr_scope(input: &str) -> String { + if let Some(pos) = input.find('%') { + let (base, rest) = input.split_at(pos); + if let Some(slash_pos) = rest.find('/') { + format!("{}{}", base, &rest[slash_pos..]) + } else { + base.to_string() + } + } else { + input.to_string() + } + } + + /// Return the IP network associated with an address object, or None if + /// there is no address object with this name. pub fn addrobj_addr( addrobj: &str, - ) -> Result, ExecutionError> { + ) -> Result, ExecutionError> { // Note that additional privileges are not required to list address // objects, and so there is no `pfexec` here. let mut cmd = std::process::Command::new(IPADM); @@ -59,12 +115,27 @@ impl Ipadm { } Err(e) => Err(e), Ok(output) => { - let stdout = String::from_utf8_lossy(&output.stdout); - Ok(stdout.trim().lines().next().map(|s| s.to_owned())) + let out = std::str::from_utf8(&output.stdout).map_err(|e| { + let s = String::from_utf8_lossy(&output.stdout); + ExecutionError::ParseFailure(format!("{}: {}", e, s)) + })?; + let lines: Vec<_> = out.trim().lines().collect(); + if lines.is_empty() { + return Ok(None); + } + match Self::remove_addr_scope(lines[0].trim()).parse() { + Ok(ipnet) => Ok(Some(ipnet)), + Err(e) => Err(ExecutionError::ParseFailure(format!( + "{}: {}", + lines[0].trim(), + e + ))), + } } } } + /// Determine if a named address object exists pub fn addrobj_exists(addrobj: &str) -> Result { Ok(Self::addrobj_addr(addrobj)?.is_some()) } @@ -106,36 +177,14 @@ impl Ipadm { // Create auto-configured address on the IP interface if it doesn't // already exist let addrobj = format!("{}/{}", datalink, IPV6_LINK_LOCAL_ADDROBJ_NAME); - if !Self::addrobj_exists(&addrobj)? { - let mut cmd = std::process::Command::new(PFEXEC); - let cmd = cmd.args(&[ - IPADM, - "create-addr", - "-t", - "-T", - "addrconf", - &addrobj, - ]); - execute(cmd)?; - } + Self::ensure_ip_addrobj_exists(&addrobj, AddrObjType::AddrConf)?; // Create static address on the IP interface if it doesn't already exist let addrobj = format!("{}/{}", datalink, IPV6_STATIC_ADDROBJ_NAME); - if !Self::addrobj_exists(&addrobj)? { - let mut cmd = std::process::Command::new(PFEXEC); - let cmd = cmd.args(&[ - IPADM, - "create-addr", - "-t", - "-T", - "static", - "-a", - &listen_addr.to_string(), - &addrobj, - ]); - execute(cmd)?; - } - + Self::ensure_ip_addrobj_exists( + &addrobj, + AddrObjType::Static((*listen_addr).into()), + )?; Ok(()) } @@ -144,12 +193,7 @@ impl Ipadm { opte_iface: &String, ) -> Result<(), ExecutionError> { let addrobj = format!("{}/public", opte_iface); - if !Self::addrobj_exists(&addrobj)? { - let mut cmd = std::process::Command::new(PFEXEC); - let cmd = - cmd.args(&[IPADM, "create-addr", "-t", "-T", "dhcp", &addrobj]); - execute(cmd)?; - } + Self::ensure_ip_addrobj_exists(&addrobj, AddrObjType::DHCP)?; Ok(()) } } diff --git a/illumos-utils/src/lib.rs b/illumos-utils/src/lib.rs index 7140c62981..48a5767f41 100644 --- a/illumos-utils/src/lib.rs +++ b/illumos-utils/src/lib.rs @@ -64,6 +64,9 @@ pub enum ExecutionError { #[error("Failed to manipulate process contract: {err}")] ContractFailure { err: std::io::Error }, + #[error("Failed to parse command output")] + ParseFailure(String), + #[error("Zone is not running")] NotRunning, } diff --git a/sled-agent/src/bin/zone-bundle.rs b/sled-agent/src/bin/zone-bundle.rs index b86e692c34..eb3c2edcdb 100644 --- a/sled-agent/src/bin/zone-bundle.rs +++ b/sled-agent/src/bin/zone-bundle.rs @@ -31,6 +31,8 @@ use slog::Logger; use slog_term::FullFormat; use slog_term::TermDecorator; use std::collections::BTreeSet; +#[cfg(target_os = "illumos")] +use std::net::IpAddr; use std::net::Ipv6Addr; use std::time::SystemTime; use tar::Builder; @@ -252,14 +254,15 @@ async fn fetch_underlay_address() -> anyhow::Result { match Ipadm::addrobj_addr(EXPECTED_ADDR_OBJ) { // If we failed because there was no such interface, then fall back // to localhost. - Ok(None) => return Ok(Ipv6Addr::LOCALHOST), - Ok(Some(addr)) => addr - .trim() - .split_once('/') - .context("expected a /64 subnet")? - .0 - .parse() - .context("invalid IPv6 address"), + Ok(None) => Ok(Ipv6Addr::LOCALHOST), + Ok(Some(addr)) => match addr.addr() { + IpAddr::V6(ipv6) => Ok(ipv6), + IpAddr::V4(ipv4) => bail!( + "Unexpectedly got IPv4 address for {}: {}", + EXPECTED_ADDR_OBJ, + ipv4 + ), + }, Err(e) => bail!( "failed to get address for addrobj {EXPECTED_ADDR_OBJ}: {e}", ),