From 6375ce425b99d42ab273b24d7690faba75ee993d Mon Sep 17 00:00:00 2001 From: Andy Fiddaman Date: Wed, 31 Jul 2024 17:18:44 +0100 Subject: [PATCH] Centralise IP address object address retrieval and existence checks. Following illumos 16677, the error message that indicates that an address object does not exists has been changed to be consistent, whereas it was previously one of two different messages or sometimes even completely blank. A couple of places in omicron relied on the specific error message so this change centralises the check, and updates it to cater for illumos pre- and post-16677. Longer term this could be replaced by bindings to libipadm. --- illumos-utils/src/ipadm.rs | 128 ++++++++++++++++-------------- sled-agent/src/bin/zone-bundle.rs | 62 ++++----------- 2 files changed, 84 insertions(+), 106 deletions(-) diff --git a/illumos-utils/src/ipadm.rs b/illumos-utils/src/ipadm.rs index 70662b6ccd..a110fb0ca4 100644 --- a/illumos-utils/src/ipadm.rs +++ b/illumos-utils/src/ipadm.rs @@ -13,7 +13,11 @@ use std::net::Ipv6Addr; pub struct Ipadm {} /// Expected error message contents when showing an addrobj that doesn't exist. -const ADDROBJ_NOT_FOUND_ERR: &str = "Address object not found"; +// The message changed to be consistent regardless of the state of the +// system in illumos 16677. It is now always `ERR1` below. Prior to that, it +// would most often be `ERR2` but could sometimes be blank or `ERR1`. +const ADDROBJ_NOT_FOUND_ERR1: &str = "address: Object not found"; +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"; @@ -37,6 +41,34 @@ impl Ipadm { } } + pub fn addrobj_addr( + addrobj: &str, + ) -> 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); + let cmd = cmd.args(&["show-addr", "-po", "addr", addrobj]); + match execute(cmd) { + Err(ExecutionError::CommandFailure(info)) + if [ADDROBJ_NOT_FOUND_ERR1, ADDROBJ_NOT_FOUND_ERR2] + .iter() + .any(|&ss| info.stderr.contains(ss)) => + { + // The address object does not exist. + Ok(None) + } + Err(e) => Err(e), + Ok(output) => { + let stdout = String::from_utf8_lossy(&output.stdout); + Ok(stdout.trim().lines().next().map(|s| s.to_owned())) + } + } + } + + pub fn addrobj_exists(addrobj: &str) -> Result { + Ok(Self::addrobj_addr(addrobj)?.is_some()) + } + // Set MTU to 9000 on both IPv4 and IPv6 pub fn set_interface_mtu(datalink: &str) -> Result<(), ExecutionError> { let mut cmd = std::process::Command::new(PFEXEC); @@ -71,53 +103,40 @@ impl Ipadm { datalink: &str, listen_addr: &Ipv6Addr, ) -> Result<(), ExecutionError> { - // Create auto-configured address on the IP interface if it doesn't already exist + // Create auto-configured address on the IP interface if it doesn't + // already exist let addrobj = format!("{}/{}", datalink, IPV6_LINK_LOCAL_ADDROBJ_NAME); - let mut cmd = std::process::Command::new(PFEXEC); - let cmd = cmd.args(&[IPADM, "show-addr", &addrobj]); - match execute(cmd) { - Err(ExecutionError::CommandFailure(info)) - if info.stderr.contains(ADDROBJ_NOT_FOUND_ERR) => - { - let mut cmd = std::process::Command::new(PFEXEC); - let cmd = cmd.args(&[ - IPADM, - "create-addr", - "-t", - "-T", - "addrconf", - &addrobj, - ]); - execute(cmd)?; - } - Err(other) => return Err(other), - Ok(_) => (), - }; + 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)?; + } // Create static address on the IP interface if it doesn't already exist let addrobj = format!("{}/{}", datalink, IPV6_STATIC_ADDROBJ_NAME); - let mut cmd = std::process::Command::new(PFEXEC); - let cmd = cmd.args(&[IPADM, "show-addr", &addrobj]); - match execute(cmd) { - Err(ExecutionError::CommandFailure(info)) - if info.stderr.contains(ADDROBJ_NOT_FOUND_ERR) => - { - 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).map(|_| ()) - } - Err(other) => Err(other), - Ok(_) => Ok(()), + 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)?; } + + Ok(()) } // Create gateway on the IP interface if it doesn't already exist @@ -125,23 +144,12 @@ impl Ipadm { opte_iface: &String, ) -> Result<(), ExecutionError> { let addrobj = format!("{}/public", opte_iface); - let mut cmd = std::process::Command::new(PFEXEC); - let cmd = cmd.args(&[IPADM, "show-addr", &addrobj]); - match execute(cmd) { - Err(_) => { - let mut cmd = std::process::Command::new(PFEXEC); - let cmd = cmd.args(&[ - IPADM, - "create-addr", - "-t", - "-T", - "dhcp", - &addrobj, - ]); - execute(cmd)?; - } - Ok(_) => (), - }; + 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)?; + } Ok(()) } } diff --git a/sled-agent/src/bin/zone-bundle.rs b/sled-agent/src/bin/zone-bundle.rs index 82433edaf5..b86e692c34 100644 --- a/sled-agent/src/bin/zone-bundle.rs +++ b/sled-agent/src/bin/zone-bundle.rs @@ -16,6 +16,8 @@ use clap::Args; use clap::Parser; use clap::Subcommand; use futures::stream::StreamExt; +#[cfg(target_os = "illumos")] +use illumos_utils::ipadm::Ipadm; use omicron_common::address::SLED_AGENT_PORT; use sled_agent_client::types::CleanupContextUpdate; use sled_agent_client::types::Duration; @@ -247,53 +249,21 @@ async fn fetch_underlay_address() -> anyhow::Result { #[cfg(target_os = "illumos")] { const EXPECTED_ADDR_OBJ: &str = "underlay0/sled6"; - let output = Command::new("ipadm") - .arg("show-addr") - .arg("-p") - .arg("-o") - .arg("addr") - .arg(EXPECTED_ADDR_OBJ) - .output() - .await?; - // If we failed because there was no such interface, then fall back to - // localhost. - if !output.status.success() { - match std::str::from_utf8(&output.stderr) { - Err(_) => bail!( - "ipadm command failed unexpectedly, stderr:\n{}", - String::from_utf8_lossy(&output.stderr) - ), - Ok(out) => { - if out.contains("Address object not found") { - eprintln!( - "Expected addrobj '{}' not found, using localhost", - EXPECTED_ADDR_OBJ, - ); - return Ok(Ipv6Addr::LOCALHOST); - } else { - bail!( - "ipadm subcommand failed unexpectedly, stderr:\n{}", - String::from_utf8_lossy(&output.stderr), - ); - } - } - } + 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"), + Err(e) => bail!( + "failed to get address for addrobj {EXPECTED_ADDR_OBJ}: {e}", + ), } - let out = std::str::from_utf8(&output.stdout) - .context("non-UTF8 output in ipadm")?; - let lines: Vec<_> = out.trim().lines().collect(); - anyhow::ensure!( - lines.len() == 1, - "No addresses or more than one address on expected interface '{}'", - EXPECTED_ADDR_OBJ - ); - lines[0] - .trim() - .split_once('/') - .context("expected a /64 subnet")? - .0 - .parse() - .context("invalid IPv6 address") } }