From e6e15dec9f5b46749e02680991155b876f5f0d0a Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Thu, 30 Apr 2026 20:58:04 +0000 Subject: [PATCH] Ensure we use a stable DUID for DHCPv6 exchanges - Package `ndpd.conf` in the switch zone with defaults that preclude DHCPv6 on any interface. - After fetching the correct, stable MAC addresses from the switch SP, `dpd` now uses the base MAC to write out a DUID to a file where illumos's `dhpcagent` can pick it up and use it later in exchanges. - For the VLANs representing the techports, `tfportd` now creates both a link-local address and allows DHCPv6 to run on the interface as well. This should trigger `dhcpagent`, which would pick up the stable DUID from the previous item. - Some misc cleanup, logging improvements, `IdOrdMap` over `BTreeMap` --- Cargo.lock | 44 +++++---- Cargo.toml | 1 + common/src/illumos.rs | 27 ++++- common/src/network.rs | 5 + dpd/Cargo.toml | 1 + dpd/misc/ndpd.conf | 8 ++ dpd/src/api_server.rs | 2 +- dpd/src/duid.rs | 163 +++++++++++++++++++++++++++++++ dpd/src/macaddrs.rs | 51 +++++----- dpd/src/main.rs | 44 ++++++--- tfportd/Cargo.toml | 1 + tfportd/src/linklocal.rs | 40 ++++++-- tfportd/src/ports.rs | 61 +++++++----- tfportd/src/vlans.rs | 112 +++++++++++++++------ tools/omicron-asic-manifest.toml | 3 +- uplinkd/src/main.rs | 7 +- xtask/src/codegen.rs | 5 +- 17 files changed, 446 insertions(+), 129 deletions(-) create mode 100644 dpd/misc/ndpd.conf create mode 100644 dpd/src/duid.rs diff --git a/Cargo.lock b/Cargo.lock index 75952de7..495f734b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -927,7 +927,7 @@ version = "3.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fde0e0ec90c9dfb3b4b1a0891a7dcd0e2bffde2f7efed5fe7c9bb00e5bfb915e" dependencies = [ - "windows-sys 0.48.0", + "windows-sys 0.59.0", ] [[package]] @@ -1308,9 +1308,9 @@ dependencies = [ [[package]] name = "daft" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ce0bf145758082da552af574185d4726000cd4b6e017d7966c99e7b2a5a086ce" +checksum = "b6a26f1f0a7934549bf8d8448d9da072c31f14e1e407b6cbacfdc07b3777988e" dependencies = [ "daft-derive", "newtype-uuid", @@ -1321,9 +1321,9 @@ dependencies = [ [[package]] name = "daft-derive" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7ad40aef90652e771af668d28abcc3ef35fd0d39438706a76a61588cf8e8e84a" +checksum = "27c6a4a4003df965e441d13b2a7044efa44334b567c984701f8a2773f815c5e2" dependencies = [ "proc-macro2", "quote", @@ -1587,6 +1587,7 @@ dependencies = [ "aal_macros", "anyhow", "asic", + "bytes", "cfg-if", "chrono", "clap", @@ -2912,7 +2913,7 @@ dependencies = [ "libc", "percent-encoding", "pin-project-lite", - "socket2 0.5.10", + "socket2 0.6.0", "system-configuration", "tokio", "tower-layer", @@ -3033,9 +3034,9 @@ dependencies = [ [[package]] name = "iddqd" -version = "0.3.17" +version = "0.3.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6b215e67ed1d1a4b1702acd787c487d16e4c977c5dcbcc4587bdb5ea26b6ce06" +checksum = "616230c7d641ef971a3a5bfcc654c6b7524ab9c9fb665693c6a397dba9a14aca" dependencies = [ "allocator-api2", "daft", @@ -3637,9 +3638,9 @@ checksum = "d26c52dbd32dccf2d10cac7725f8eae5296885fb5703b261f7d0a0739ec807ab" [[package]] name = "linux-raw-sys" -version = "0.11.0" +version = "0.12.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "df1d3c3b53da64cf5760482273a98e575c651a67eec7f77df96b5b642de8f039" +checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53" [[package]] name = "litemap" @@ -5601,7 +5602,7 @@ dependencies = [ "quinn-udp", "rustc-hash", "rustls 0.23.32", - "socket2 0.5.10", + "socket2 0.6.0", "thiserror 2.0.18", "tokio", "tracing", @@ -5638,7 +5639,7 @@ dependencies = [ "cfg_aliases", "libc", "once_cell", - "socket2 0.5.10", + "socket2 0.6.0", "tracing", "windows-sys 0.60.2", ] @@ -5995,14 +5996,14 @@ dependencies = [ [[package]] name = "rustix" -version = "1.1.2" +version = "1.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cd15f8a2c5551a84d56efdc1cd049089e409ac19a3072d5037a17fd70719ff3e" +checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190" dependencies = [ "bitflags 2.9.4", "errno", "libc", - "linux-raw-sys 0.11.0", + "linux-raw-sys 0.12.1", "windows-sys 0.61.0", ] @@ -7192,14 +7193,14 @@ dependencies = [ [[package]] name = "tempfile" -version = "3.23.0" +version = "3.27.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2d31c77bdf42a745371d260a26ca7163f1e0924b64afa0b688e61b5a9fa02f16" +checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", "getrandom 0.3.3", "once_cell", - "rustix 1.1.2", + "rustix 1.1.4", "windows-sys 0.61.0", ] @@ -7218,7 +7219,7 @@ version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "60b8cb979cb11c32ce1603f8137b22262a9d131aaa5c37b5678025f22b8becd0" dependencies = [ - "rustix 1.1.2", + "rustix 1.1.4", "windows-sys 0.60.2", ] @@ -7275,6 +7276,7 @@ dependencies = [ "dpd-client 0.1.0", "futures", "http", + "iddqd", "internet-checksum", "ispf", "kstat-rs", @@ -8686,7 +8688,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.48.0", + "windows-sys 0.61.0", ] [[package]] @@ -9076,7 +9078,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" dependencies = [ "libc", - "rustix 1.1.2", + "rustix 1.1.4", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 51a04a23..b5d9b79e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -78,6 +78,7 @@ expectorate = "1" futures = "0.3" http = "1.4.0" humantime = "2.3" +iddqd = "0.3.18" kstat-rs = "0.2.4" lazy_static = "1.5" libc = "0.2" diff --git a/common/src/illumos.rs b/common/src/illumos.rs index 5eca9c2b..c29fe25d 100644 --- a/common/src/illumos.rs +++ b/common/src/illumos.rs @@ -14,6 +14,11 @@ pub mod smf; type Result = std::result::Result; +/// The suffix for the addrobj name for IPv6 link-local addresses on each tfport. +/// +/// E.g., all IPv6 addresses are named like `tfportrear0_0/ll`. +pub const IPV6_LINK_LOCAL_NAME: &str = "ll"; + #[derive(Debug, PartialEq, thiserror::Error)] pub enum IllumosError { /// This error indicates that the requested command wasn't able to run @@ -147,11 +152,8 @@ pub async fn address_add( let addr_obj = format!("{iface}/{tag}"); let addr: oxnet::IpNet = addr.into(); - if addr.is_ipv6() { - let tag = "ll"; - if !address_exists(iface, tag).await? { - linklocal_add(iface, tag).await?; - } + if addr.is_ipv6() && !address_exists(iface, IPV6_LINK_LOCAL_NAME).await? { + linklocal_add(iface, IPV6_LINK_LOCAL_NAME).await?; } let addr = addr.to_string(); @@ -171,6 +173,21 @@ pub async fn linklocal_add(iface: &str, tag: &str) -> Result<()> { ipadm_quiet(&["create-addr", "-t", "-T", "addrconf", &addr_obj]).await } +/// Add link-local and DHCPv6 addresses to an existing link. +pub async fn linklocal_add_with_dhcpv6(iface: &str, tag: &str) -> Result<()> { + let addr_obj = format!("{iface}/{tag}"); + ipadm_quiet(&[ + "create-addr", + "-t", + "-T", + "addrconf", + "-p", + "stateful=yes,stateless=no", + &addr_obj, + ]) + .await +} + /// Create a vlan link on top of the specified link pub async fn vlan_create(over: &str, vlan_id: u16, vlan: &str) -> Result<()> { let vlan_id = vlan_id.to_string(); diff --git a/common/src/network.rs b/common/src/network.rs index 6727804b..1a448820 100644 --- a/common/src/network.rs +++ b/common/src/network.rs @@ -129,6 +129,11 @@ impl MacAddr { self.a[5], ] } + + /// Return the bytes of the MAC as a slice. + pub const fn as_slice(&self) -> &[u8] { + self.a.as_slice() + } } #[derive(Error, Debug, Clone)] diff --git a/dpd/Cargo.toml b/dpd/Cargo.toml index 7a15905f..400b9172 100644 --- a/dpd/Cargo.toml +++ b/dpd/Cargo.toml @@ -31,6 +31,7 @@ dpd-types.workspace = true dpd-types-versions.workspace = true anyhow.workspace = true +bytes.workspace = true cfg-if.workspace = true chrono.workspace = true clap.workspace = true diff --git a/dpd/misc/ndpd.conf b/dpd/misc/ndpd.conf new file mode 100644 index 00000000..f56e124e --- /dev/null +++ b/dpd/misc/ndpd.conf @@ -0,0 +1,8 @@ +# Do not run DHCPv6 on any interfaces by default. +# +# DHCPv6 servers rely on unique identifiers, DUIDs, to identify clients. +# Those are usually based on the link-layer address. The Omicron switch zone +# starts with a random locally-administered MAC, which means we _don't_ have a +# stable ID. Dendrite software manually creates the DUID once we've collected +# stable MAC addresses from the Sidecar SP. +ifdefault StatefulAddrConf false diff --git a/dpd/src/api_server.rs b/dpd/src/api_server.rs index 3025d3cc..acc21638 100644 --- a/dpd/src/api_server.rs +++ b/dpd/src/api_server.rs @@ -2933,7 +2933,7 @@ fn path_to_qsfp(path: Path) -> Result { } } -fn build_info() -> BuildInfo { +pub(crate) fn build_info() -> BuildInfo { BuildInfo { version: env!("CARGO_PKG_VERSION").to_string(), git_sha: env!("VERGEN_GIT_SHA").to_string(), diff --git a/dpd/src/duid.rs b/dpd/src/duid.rs new file mode 100644 index 00000000..326def30 --- /dev/null +++ b/dpd/src/duid.rs @@ -0,0 +1,163 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/ +// +// Copyright 2026 Oxide Computer Company + +//! Code for ensuring stable DHCPv6 unique identifier exist in the switch zone. + +use common::network::MacAddr; +use slog::Logger; +use slog::debug; + +cfg_if::cfg_if! { + if #[cfg(target_os = "illumos")] { + use slog::error; + use slog::info; + use std::time::Duration; + } +} + +/// Ensure our DHCPv6 Unique Identifier (DUID) is written persistently to disk. +/// +/// Some deployments run DHCPv6 over our technician ports. In that protocol, +/// clients identify themselves with a DUID, which is supposed to be a stable, +/// unique identifier so that servers can assign consistent configuration data +/// to the client. By default illumos's `dhcpagent` uses the Link-Layer Address +/// Plus Time option for this ID. However both the time and MAC address can +/// change, violating the stability requirement. The latter changes because the +/// switch zone is assigned a VNIC as its "first" datalink from the sled-agent +/// in the global zone. That VNIC has a random locally-administered prefix, +/// 02:08:20:... +/// +/// Once the `dhcpagent` has a DUID, it persists it to a file and reads that +/// whenever starting a new exchange. In the Oxide product, we're ensuring this +/// file contains our expected, stable ID, based on the MAC address stored in +/// the switch's SP FRUID EEPROM. +pub(crate) async fn ensure_duid_file_exists(log: Logger, base_mac: MacAddr) { + ensure_duid_file_exists_impl(log, base_mac).await +} + +#[cfg(target_os = "illumos")] +async fn ensure_duid_file_exists_impl(log: Logger, base_mac: MacAddr) { + // `dhcpagent` reads this path for a DUID, or persists one there on startup + // if the path does not exist. + const DUID_PATH: &str = "/etc/dhcp/duid"; + + // We'll write into a temporary file, and atomically swap it to the final + // path. + const TEMP_FILE_NAME: &str = "temporary-duid"; + let mut temp_file = std::env::temp_dir(); + temp_file.push(TEMP_FILE_NAME); + let bytes = create_duid_bytes(base_mac); + + // Loop until the file exists successfully. There's no real reason this + // should fail, but we'll keep trying because (1) it's really bad if we fail + // and (2) it's cheap to retry it. + const RETRY_INTERVAL: Duration = Duration::from_secs(5); + let mut is_retry = false; + 'retry: loop { + if is_retry { + tokio::time::sleep(RETRY_INTERVAL).await; + } + is_retry = true; + + // Write the DUID into the tempfile. + // + // This creates the file if needed, and replaces the entire contents if + // it already exists. + match tokio::fs::write(&temp_file, &bytes).await { + Ok(_) => debug!(log, "wrote DUID to tempfile"), + Err(e) => { + error!( + log, + "failed to write DUID to tempfile"; + "path" => temp_file.display(), + "error" => ?e, + ); + continue 'retry; + } + } + + // Swap it into place. + match tokio::fs::rename(&temp_file, DUID_PATH).await { + Ok(_) => { + info!( + log, + "wrote stable DHCPv6 DUID"; + "path" => DUID_PATH, + "MAC" => base_mac.as_slice(), + ); + return; + } + Err(e) => { + error!( + log, + "failed to rename DUID temp file to real path"; + "temp_path" => temp_file.display(), + "path" => DUID_PATH, + "error" => ?e, + ); + continue 'retry; + } + } + } +} + +#[cfg(any(target_os = "illumos", test))] +fn create_duid_bytes(base_mac: MacAddr) -> Vec { + use bytes::BufMut as _; + + // To ensure we have a _stable_ DUID, which doesn't change during zone + // reboots, we use only the link-layer address. + // + // See https://www.rfc-editor.org/rfc/rfc8415#section-11.4 for details. + const DUID_TYPE: u16 = 0x03; + + // We're running on Ethernet links. + // + // See + // https://www.iana.org/assignments/arp-parameters/arp-parameters.xhtml#arp-parameters-2. + const HARDWARE_TYPE: u16 = 0x01; + + // illumos creates its DUIDs using the `make_stable_duid()` function + // defined here: + // https://github.com/oxidecomputer/illumos-gate/blob/71b1f26fe641fba9ad5b9bca63cb9d00024578e5/usr/src/lib/libdhcpagent/common/dhcp_stable.c#L130. + // + // Importantly, it does no interpretation of the contents of the file + // when _using_ the DUID in a DHCPv6 exchange, so we're writing out the + // literal contents of the DUID-LL object, defined here: + // https://www.rfc-editor.org/rfc/rfc8415#section-11.4. + let sl = base_mac.as_slice(); + let mut bytes = Vec::with_capacity( + std::mem::size_of::() + std::mem::size_of::() + sl.len(), + ); + bytes.put_u16(DUID_TYPE); + bytes.put_u16(HARDWARE_TYPE); + bytes.put(sl); + + bytes +} + +#[cfg(not(target_os = "illumos"))] +pub(crate) async fn ensure_duid_file_exists_impl( + log: Logger, + _base_mac: MacAddr, +) { + debug!(log, "No DUID file needed on non-illumos systems, nothing to write"); +} + +#[cfg(test)] +mod tests { + use super::create_duid_bytes; + + #[tokio::test] + async fn test_create_duid_bytes() { + let mac = [0xa8, 0x40, 0x25, 0xfe, 0xfe, 0xfe]; + let bytes = create_duid_bytes(mac.into()); + assert_eq!(bytes.len(), 2 + 2 + 6); + assert_eq!(u16::from_be_bytes(bytes[..2].try_into().unwrap()), 3); + assert_eq!(u16::from_be_bytes(bytes[2..4].try_into().unwrap()), 1); + assert_eq!(&bytes[4..], &mac); + } +} diff --git a/dpd/src/macaddrs.rs b/dpd/src/macaddrs.rs index 7acff4e6..f526098b 100644 --- a/dpd/src/macaddrs.rs +++ b/dpd/src/macaddrs.rs @@ -371,27 +371,31 @@ impl Switch { } } - // We may start `dpd` without a base MAC address for assigning addresses to - // links. In that situation, we need to fetch the base MAC from the Sidecar - // SP via the management network. This presents us with a bootstrapping - // problem: we need a MAC to bring up that link, via which we'd fetch the - // MACs. To break the circularity, we will use a random MAC to temporarily - // bring up the CPU link; fetch the real MAC addresses; tear down the CPU - // link; and the continue as normal. We'll cache that as an SMF property to - // avoid the complexity each time we restart. - // - // In general, our process is: - // - // - Create a link on the CPU port, using a random MAC address. - // - Create a transceiver controller. This _will block_ until tfportd makes - // us the `sidecar0` VLAN interface that we're expecting to use. - // - Fetch the MAC address range from the SP using the controller. - // - Update the MAC address on the CPU link with the final address derived - // from the base_mac + /// Set the base MAC address for Dendrite. + /// + /// We may start `dpd` without a base MAC address for assigning addresses to + /// links. In that situation, we need to fetch the base MAC from the Sidecar + /// SP via the management network. This presents us with a bootstrapping + /// problem: we need a MAC to bring up that link, via which we'd fetch the + /// MACs. To break the circularity, we will use a random MAC to temporarily + /// bring up the CPU link; fetch the real MAC addresses; tear down the CPU + /// link; and the continue as normal. We'll cache that as an SMF property to + /// avoid the complexity each time we restart. + /// + /// In general, our process is: + /// + /// - Create a link on the CPU port, using a random MAC address. + /// - Create a transceiver controller. This _will block_ until tfportd makes + /// us the `sidecar0` VLAN interface that we're expecting to use. + /// - Fetch the MAC address range from the SP using the controller. + /// - Update the MAC address on the CPU link with the final address derived + /// from the base_mac + /// + /// This returns the base MAC address in any case. pub async fn set_base_mac_address( &self, autoconfig_links: &Option, - ) -> anyhow::Result { + ) -> anyhow::Result { slog::info!( self.log, "no base MAC address found, fetching from Sidecar FRUID" @@ -472,19 +476,22 @@ impl Switch { .expect("Expected a MAC for the internal CPU link"); self.set_link_mac_address(port_id, link_id, cpu_mac)?; - Ok(true) + Ok(base_mac) } } #[cfg(not(feature = "tofino_asic"))] impl Switch { /// Assign a permanent but random base MAC address on non-Tofino systems. + /// + /// Return the MAC address. pub async fn set_base_mac_address( &self, _autoconfig_links: &Option, - ) -> anyhow::Result { + ) -> anyhow::Result { // For non-ASIC builds, we just use a random MAC as our base address. - let base_mac = BaseMac::Permanent(MacAddr::random_oxide()); + let mac = MacAddr::random_oxide(); + let base_mac = BaseMac::Permanent(mac); debug!( self.log, "assigning random base MAC address"; @@ -492,7 +499,7 @@ impl Switch { ); let mut mgr = self.mac_mgmt.lock().unwrap(); mgr.set_base_mac(base_mac)?; - Ok(false) + Ok(mac) } } diff --git a/dpd/src/main.rs b/dpd/src/main.rs index a2b37ddc..0bf3abe6 100644 --- a/dpd/src/main.rs +++ b/dpd/src/main.rs @@ -59,6 +59,7 @@ mod arp; mod attached_subnet; mod config; mod counters; +mod duid; mod fault; mod freemap; mod link; @@ -411,9 +412,11 @@ impl Switch { entries: t .get_entries::(&self.asic_hdl, from_hardware) .map_err(|e| { - error!(self.log, "failed to get table contents"; - "table" => t.type_.to_string(), - "error" => %e); + error!( + self.log, "failed to get table contents"; + "table" => t.type_.to_string(), + "error" => %e + ); e }) .map(|vec| { @@ -436,9 +439,11 @@ impl Switch { t.get_counters::(&self.asic_hdl, force_sync) .map_err(|e| { - error!(self.log, "failed to get counter data"; - "table" => t.type_.to_string(), - "error" => %e); + error!( + self.log, "failed to get counter data"; + "table" => t.type_.to_string(), + "error" => %e + ); e }) .map(|vec| { @@ -701,9 +706,9 @@ async fn sidecar_main(mut switch: Switch) -> anyhow::Result<()> { // If there has been no base mac address configured via SMF or the command // line, then we need to fetch it from the SP (for real sidecars) or just // make one up (everywhere else). - let skip_cpu_link = match config_base_mac { - Some(base_mac) => { - let base_mac = BaseMac::Permanent(base_mac); + let (skip_cpu_link, base_mac) = match config_base_mac { + Some(config_mac) => { + let base_mac = BaseMac::Permanent(config_mac); debug!( switch.log, "permanent base MAC address already set, it will be kept"; @@ -711,11 +716,20 @@ async fn sidecar_main(mut switch: Switch) -> anyhow::Result<()> { ); let mut mgr = switch.mac_mgmt.lock().unwrap(); assert_eq!(mgr.set_base_mac(base_mac)?, None); - false + (false, config_mac) + } + None => { + let base_mac = + switch.set_base_mac_address(&autoconfig_links).await?; + (true, base_mac) } - None => switch.set_base_mac_address(&autoconfig_links).await?, }; + // Write our DHCPv6 unique ID to disk, based on the base MAC address. + let duid_log = switch.log.new(slog::o!("unit" => "duid-writer")); + let duid_task = + tokio::task::spawn(duid::ensure_duid_file_exists(duid_log, base_mac)); + if let Some(auto_conf) = &autoconfig_links { // If we've created the link on the CPU port above, to fetch the MAC // addresses from the Sidecar, then we skip that particular link in this @@ -780,6 +794,7 @@ async fn sidecar_main(mut switch: Switch) -> anyhow::Result<()> { api_server_manager .await .expect("while shutting down the api_server_manager"); + duid_task.await?; info!(switch.log, "shutting down switch driver"); switch.asic_hdl.fini(); @@ -788,7 +803,6 @@ async fn sidecar_main(mut switch: Switch) -> anyhow::Result<()> { Ok(()) } - fn main() -> anyhow::Result<()> { let cli = Cli::parse(); @@ -802,7 +816,11 @@ async fn run_dpd(opt: Opt) -> anyhow::Result<()> { let log = common::logging::init("dpd", &config.log_file, config.log_format)?; - info!(log, "dpd config: {config:#?}"); + info!( + log, "starting dpd"; + "config" => ?config, + "build_info" => ?api_server::build_info(), + ); let p4_name = std::env::var("P4_NAME").unwrap_or_else(|_| String::from("sidecar")); diff --git a/tfportd/Cargo.toml b/tfportd/Cargo.toml index 06476957..5ef214ed 100644 --- a/tfportd/Cargo.toml +++ b/tfportd/Cargo.toml @@ -15,6 +15,7 @@ chrono.workspace = true csv.workspace = true futures.workspace = true http.workspace = true +iddqd.workspace = true internet-checksum.workspace = true ispf.workspace = true libc.workspace = true diff --git a/tfportd/src/linklocal.rs b/tfportd/src/linklocal.rs index 4b61b242..c459436a 100644 --- a/tfportd/src/linklocal.rs +++ b/tfportd/src/linklocal.rs @@ -7,17 +7,15 @@ use std::collections::BTreeMap; use std::net::Ipv6Addr; +use anyhow::Context as _; use anyhow::Result; use anyhow::anyhow; use slog::debug; +use slog::error; use crate::Global; use common::illumos; - -/// The suffix for the addrobj name for IPv6 link-local addresses on each tfport. -/// -/// E.g., all IPv6 addresses are named like `tfportrear0_0/ll`. -const IPV6_LINK_LOCAL_NAME: &str = "ll"; +use common::illumos::IPV6_LINK_LOCAL_NAME; // Parse a single line of ipadm output to extract the addrobj name and link-local // address. This function returns an error if the ipadm command fails or the @@ -77,14 +75,44 @@ pub async fn get_all() -> Result> { } // Create a link-local address for an interface +// +// TODO-cleanup: This function is actually infallible. We should either +// propagate the error or actually reflect its infallibility in the return type. pub async fn create(g: &Global, iface: &str) -> anyhow::Result<()> { debug!(g.log, "creating link-local address for {iface}"); if let Err(e) = illumos::linklocal_add(iface, IPV6_LINK_LOCAL_NAME).await { - slog::error!(g.log, "failed to create link-local address: {e:?}"); + error!(g.log, "failed to create link-local address: {e:?}"); } Ok(()) } +/// Create link-local and DHCPv6 addresses on an interface. +pub async fn create_with_dhcpv6(g: &Global, iface: &str) -> anyhow::Result<()> { + debug!( + g.log, + "creating link-local and DHCPv6 addresses"; + "interface" => iface + ); + let res = + illumos::linklocal_add_with_dhcpv6(iface, IPV6_LINK_LOCAL_NAME).await; + match &res { + Ok(_) => debug!( + g.log, + "created link-local and DHCPv6 addresses"; + "interface" => iface, + ), + Err(e) => error!( + g.log, + "failed to create link-local and DHCPv6 addresses"; + "interface" => iface, + "error" => ?e, + ), + } + res.with_context(|| { + format!("creating link-local and DHCPv6 addresses on {iface}") + }) +} + #[test] fn test_parse_ipadm() -> Result<()> { let good_addr: Ipv6Addr = "fe80::aa40:25ff:fe04:392".parse().unwrap(); diff --git a/tfportd/src/ports.rs b/tfportd/src/ports.rs index c300fa1a..8005d598 100644 --- a/tfportd/src/ports.rs +++ b/tfportd/src/ports.rs @@ -134,17 +134,21 @@ async fn dpd_port_update(g: &Global, links: &mut LinkMap) -> Result<()> { // Any change here should cause a mismatch when looking at the existing // tfports. if link.mac != entry_mac { - warn!(g.log, "tfport changed mac addresses"; - "tfport" => &expected_tfport, - "mac" => entry_mac.to_string(), - "stale_mac" => link.mac.to_string()); + warn!( + g.log, "tfport changed mac addresses"; + "tfport" => &expected_tfport, + "mac" => entry_mac.to_string(), + "stale_mac" => link.mac.to_string() + ); link.mac = entry_mac; } if link.asic_id != entry.asic_id { - warn!(g.log, "tfport changed asic IDs"; - "tfport" => &expected_tfport, - "asic_id" => entry.asic_id, - "stale_asic_id" => link.asic_id); + warn!( + g.log, "tfport changed asic IDs"; + "tfport" => &expected_tfport, + "asic_id" => entry.asic_id, + "stale_asic_id" => link.asic_id + ); link.asic_id = entry.asic_id; } } @@ -184,18 +188,19 @@ async fn illumos_port_update( link.tfport_link_local = data.link_local; continue; } else { - info!(g.log, "tfport found with stale data"; - "tfport" => tfport, - "mac" => link.mac.to_string(), - "stale_mac" => data.mac.to_string(), - "asic_id" => link.asic_id, - "stale_asic_id" => data.port); + info!( + g.log, "tfport found with stale data"; + "tfport" => tfport, + "mac" => link.mac.to_string(), + "stale_mac" => data.mac.to_string(), + "asic_id" => link.asic_id, + "stale_asic_id" => data.port + ); } } None => { if link.tfport.is_some() { - info!(g.log, "tfport disappeared"; - "tfport" => tfport); + info!(g.log, "tfport disappeared"; "tfport" => tfport); } } } @@ -282,9 +287,11 @@ pub async fn port_loop(g: Arc) { // Clean up any orphaned tfports for tfport in orphans { if let Err(e) = tfport::tfport_delete(&g, &tfport).await { - error!(g.log, - "failed to clean up stale tfport: {e:?}"; - "tfport" => tfport) + error!( + g.log, + "failed to clean up stale tfport: {e:?}"; + "tfport" => tfport + ); } } @@ -300,15 +307,19 @@ pub async fn port_loop(g: Arc) { } if let Err(e) = tfport::tfport_ensure(&g, tfport, link).await { - error!(g.log, - "tfport_ensure() failed: {e:?}"; - "tfport" => tfport) + error!( + g.log, + "tfport_ensure() failed: {e:?}"; + "tfport" => tfport + ); } if let Err(e) = ensure_address_match(&g, link).await { - error!(g.log, - "ensure_address_match() failed: {e:?}"; - "tfport" => tfport) + error!( + g.log, + "ensure_address_match() failed: {e:?}"; + "tfport" => tfport + ); } } *g.tfport_to_asic.lock().unwrap() = tfport_to_asic; diff --git a/tfportd/src/vlans.rs b/tfportd/src/vlans.rs index 6aac5f8d..5448672f 100644 --- a/tfportd/src/vlans.rs +++ b/tfportd/src/vlans.rs @@ -11,19 +11,27 @@ use std::net::Ipv6Addr; use anyhow::Context; use anyhow::anyhow; use anyhow::bail; +use iddqd::IdOrdItem; +use iddqd::IdOrdMap; +use iddqd::id_upcast; use serde::Deserialize; use slog::error; use slog::info; +use slog::warn; use crate::Global; use crate::linklocal; use crate::oxstats::link; use common::illumos; +/// An entry in the `port_map.csv` file provided at program startup. #[derive(Debug, Deserialize)] struct PortMapEntry { + /// The VSC7448 port number. port: u16, + /// A human-friendly string naming the logical partner on the link. _link_partner: String, + /// The name of the VLAN object to be created mapping to the link partner. vlan_name: String, } @@ -34,9 +42,11 @@ pub struct Vlan { pub name: String, } -/// The information illumos maintains about a single vlan +/// The information illumos maintains about a single VLAN #[derive(Debug)] pub struct VlanInfo { + /// The name of the VLAN device. + pub name: String, /// VLAN ID pub vid: u16, /// index of the interface created by `ipadm` @@ -45,18 +55,35 @@ pub struct VlanInfo { pub link_local: Option, } +impl VlanInfo { + /// Return true if this VLAN should allow DHCPv6 autoconfiguration. + fn supports_dhcp(&self) -> bool { + self.name.starts_with("techport") + } +} + +impl IdOrdItem for VlanInfo { + type Key<'a> = &'a str; + + fn key(&self) -> Self::Key<'_> { + &self.name + } + + id_upcast!(); +} + /// Get the list of vlans created on top of a tfport -async fn vlans_get(tfport: &str) -> anyhow::Result> { +async fn vlans_get(tfport: &str) -> anyhow::Result> { let link_locals = linklocal::get_all().await?; let lines = illumos::dladm(&["show-vlan", "-p", "-o", "link,vid,over"]).await?; // Iterate over the dladm output, extracting the vlan name and vid from each // line. For each vlan created on top of this tfport, add an entry to the - // BTreeMap with the network configuration for each one. - let mut rval = BTreeMap::new(); + // map with the network configuration for each one. + let mut rval = IdOrdMap::new(); for vlan in lines { - let fields: Vec = vlan.split(':').map(str::to_string).collect(); + let fields: Vec<_> = vlan.split(':').collect(); if fields.len() != 3 { bail!("show-vlan returned invalid result: {vlan}"); } @@ -68,7 +95,11 @@ async fn vlans_get(tfport: &str) -> anyhow::Result> { let vid = fields[1].parse::().context("invalid vlan_id")?; let ifindex = crate::netsupport::get_ifindex(&link); let link_local = link_locals.get(&link).copied(); - rval.insert(link, VlanInfo { vid, ifindex, link_local }); + let vlan = VlanInfo { name: link, vid, ifindex, link_local }; + + // NOTE: We previously used a BTreeMap here, and ignored any duplicates. + // Keep the same behavior, ignoring the error. + let _ = rval.insert_overwrite(vlan); } Ok(rval) } @@ -92,7 +123,8 @@ pub async fn vlans_cleanup(g: &Global, tfport: &str) -> anyhow::Result<()> { .await .map_err(|e| anyhow!("failed to get vlan list for {tfport}: {e:?}"))?; - for (name, vlan) in &vlans { + for vlan in &vlans { + let name = &vlan.name; let vid = vlan.vid; match vlan_delete(g, name).await { Ok(_) => info!(g.log, "deleted vlan {vid}:{name} on {tfport}"), @@ -117,14 +149,17 @@ pub async fn ensure_vlans(g: &Global, link: &str) -> anyhow::Result<()> { // links that should be created and/or deleted. We start by pessimistically // assuming that each vlan needs to be deleted, removing them from the // to_delete list if we find them on the "expected" list below. - let mut to_delete = - existing_vlans.keys().cloned().collect::>(); + let mut to_delete = existing_vlans + .iter() + .map(|vlan| vlan.name.to_string()) + .collect::>(); for expected_vlan in &g.vlans { - if let Some(current_vlan) = existing_vlans.get(&expected_vlan.name) + if let Some(current_vlan) = + existing_vlans.get(expected_vlan.name.as_str()) && current_vlan.vid == expected_vlan.vid { // This vlan has the right name and ID, so we leave it alone - let _ = to_delete.remove(&expected_vlan.name); + let _ = to_delete.remove(expected_vlan.name.as_str()); continue; } to_create.insert(expected_vlan.name.to_string(), expected_vlan.vid); @@ -135,7 +170,7 @@ pub async fn ensure_vlans(g: &Global, link: &str) -> anyhow::Result<()> { if let Err(e) = vlan_delete(g, name).await { error!(g.log, "failed to delete vlan {name}: {e:?}"); } - let _ = existing_vlans.remove(name); + let _ = existing_vlans.remove(name.as_str()); } // Create any missing vlans @@ -143,16 +178,28 @@ pub async fn ensure_vlans(g: &Global, link: &str) -> anyhow::Result<()> { match illumos::vlan_create(link, vid, &name).await { Ok(()) => { info!(g.log, "created vlan {vid}:{name} on {link}"); - existing_vlans.insert( - name.to_string(), - VlanInfo { vid, ifindex: None, link_local: None }, - ); + let vlan = VlanInfo { + name: name.clone(), + vid, + ifindex: None, + link_local: None, + }; + // NOTE: We previously used a BTreeMap here, and ignored any duplicates. + // Keep the same behavior, logging an error. + if let Some(old) = existing_vlans.insert_overwrite(vlan) { + warn!( + &g.log, + "overwriting duplicate VLAN for tfport"; + "tfport" => link, + "vlan" => old.name, + "vid" => old.vid, + ); + } // Once the vlan is created, we can track it as a potential // network link. - if let Err(e) = g - .link_tracker - .track_link(name.to_string(), link::ModelType::Vlan) + if let Err(e) = + g.link_tracker.track_link(&name, link::ModelType::Vlan) { error!(g.log, "failed to track vlan {name}: {e:?}"); } @@ -165,38 +212,39 @@ pub async fn ensure_vlans(g: &Global, link: &str) -> anyhow::Result<()> { // Iterate over all of the vlans (old and new) and ensure that they have a // link-local address. - for (name, info) in existing_vlans.iter_mut() { + for mut info in existing_vlans.iter_mut() { // If the interface exists but the address doesn't, we need to remove the // interface before creating the link-local address due to stlouis#531. if info.link_local.is_none() && info.ifindex.is_some() - && illumos::iface_remove(name).await.is_ok() + && illumos::iface_remove(&info.name).await.is_ok() { info.ifindex = None; } if info.ifindex.is_none() { - match illumos::iface_ensure(name).await { + match illumos::iface_ensure(&info.name).await { Ok(()) => { - slog::debug!(g.log, "created interface for vlan: {name}") + slog::debug!( + g.log, + "created interface for vlan: {}", + &info.name + ) } Err(e) => { slog::error!( g.log, - "failed to create interface for vlan: {name}: {e}" + "failed to create interface for vlan: {}: {e}", + &info.name, ); continue; } } } if info.link_local.is_none() { - match linklocal::create(g, name).await { - Ok(()) => { - slog::debug!(g.log, "created link-local for vlan: {name}") - } - Err(e) => slog::error!( - g.log, - "failed to create link-local for vlan: {name}: {e}" - ), + if info.supports_dhcp() { + let _ = linklocal::create_with_dhcpv6(g, &info.name).await; + } else { + let _ = linklocal::create(g, &info.name).await; } } } diff --git a/tools/omicron-asic-manifest.toml b/tools/omicron-asic-manifest.toml index 01b7231d..8bc68a2b 100644 --- a/tools/omicron-asic-manifest.toml +++ b/tools/omicron-asic-manifest.toml @@ -23,6 +23,7 @@ source.paths = [ {from = "target/proto/opt/oxide/tofino_sde/lib/libtarget_utils.so" , to = "/opt/oxide/tofino_sde/lib/libtarget_utils.so"}, {from = "target/proto/opt/oxide/tofino_sde/lib/libclish.so" , to = "/opt/oxide/tofino_sde/lib/libclish.so"}, {from = "target/proto/opt/oxide/dendrite/sidecar/share/platforms/board-maps/oxide/" , to = "/opt/oxide/dendrite/sidecar/share/platforms/board-maps/oxide/"}, - {from = "target/proto/opt/oxide/dendrite/sidecar/share/cli/xml" , to = "/opt/oxide/dendrite/sidecar/share/cli/xml"} + {from = "target/proto/opt/oxide/dendrite/sidecar/share/cli/xml" , to = "/opt/oxide/dendrite/sidecar/share/cli/xml"}, + {from = "dpd/misc/ndpd.conf" , to = "/etc/inet/ndpd.conf"} ] output.type = "zone" diff --git a/uplinkd/src/main.rs b/uplinkd/src/main.rs index 7ac1d757..08d3c83a 100644 --- a/uplinkd/src/main.rs +++ b/uplinkd/src/main.rs @@ -40,6 +40,7 @@ use anyhow::Result; use anyhow::anyhow; use clap::Parser; use common::illumos::AddressFamily; +use common::illumos::IPV6_LINK_LOCAL_NAME; use libc::c_int; use oxnet::IpNet; use oxnet::Ipv4Net; @@ -413,9 +414,11 @@ async fn create_link(iface: &str, tag: &str, addr: &IpNet) -> Result { // Add a link-local address using ipadm async fn create_linklocal(iface: &str) -> Result { - illumos::linklocal_add(iface, "ll") + illumos::linklocal_add(iface, IPV6_LINK_LOCAL_NAME) .await - .map(|_| format!("created link-local as {iface}/ll")) + .map(|_| { + format!("created link-local as {iface}/{IPV6_LINK_LOCAL_NAME}") + }) .map_err(|e| e.into()) } diff --git a/xtask/src/codegen.rs b/xtask/src/codegen.rs index a89078a7..87fedc8a 100644 --- a/xtask/src/codegen.rs +++ b/xtask/src/codegen.rs @@ -175,7 +175,10 @@ pub fn build( args.push(app_path); println!("op: {args:?}"); - let out = Command::new(&p4c_path).args(&args).output()?; + let out = Command::new(&p4c_path) + .args(&args) + .output() + .with_context(|| format!("opening '{p4c_path}'"))?; let stdout = String::from_utf8_lossy(&out.stdout); let stderr = String::from_utf8_lossy(&out.stderr); if !out.status.success() {