review feedback

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<Option<String>, ExecutionError> {
+    ) -> Result<Option<IpNet>, 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<bool, ExecutionError> {
         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(())
     }
 }

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,
 }

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<Ipv6Addr> {
         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}",
             ),