From 9df3b85cd52957cf62fca451fbddf938736dbf0b Mon Sep 17 00:00:00 2001 From: Cory Zimmerman Date: Sun, 23 Aug 2026 16:58:22 +0000 Subject: [PATCH 1/3] Add repro tests for address isolation issue - Tagged port_settings_apply can delete addresses from another tag. - Tagged port_settings_get returns addresses belonging to other tags. - Tagged port_settings_clear can delete addresses from another tag. --- dpd-client/tests/chaos_tests/port_settings.rs | 403 +++++++++++++++--- dpd-client/tests/chaos_tests/util.rs | 75 +++- 2 files changed, 417 insertions(+), 61 deletions(-) diff --git a/dpd-client/tests/chaos_tests/port_settings.rs b/dpd-client/tests/chaos_tests/port_settings.rs index bd781779..710ea4ae 100644 --- a/dpd-client/tests/chaos_tests/port_settings.rs +++ b/dpd-client/tests/chaos_tests/port_settings.rs @@ -9,18 +9,23 @@ use super::harness::{ new_dpd_client, run_dpd, }; use super::util::{link_list_ipv4, link_list_ipv6}; +use crate::chaos_tests::harness; +use crate::chaos_tests::util::IpRng; + +use anyhow::bail; use asic::chaos::{AsicConfig, Chaos, TableChaos}; use asic::table_chaos; use common::table::TableType; use dpd_client::types::{ - LinkCreate, LinkId, LinkSettings, PortFec, PortId, PortSettings, PortSpeed, + Ipv4Entry, Ipv6Entry, LinkCreate, LinkId, LinkSettings, PortFec, PortId, + PortSettings, PortSpeed, }; use dpd_client::{Client, ROLLBACK_FAILURE_ERROR_CODE}; use http::status::StatusCode; use pretty_assertions::{Comparison, assert_eq}; use rand::Rng; use std::collections::HashMap; -use std::net::{Ipv4Addr, Ipv6Addr}; +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::sync::Arc; use std::sync::atomic::{AtomicU8, Ordering}; use tokio::time::Duration; @@ -32,6 +37,17 @@ const TESTING_RADIX: usize = 33; const RETRY_INTERVAL: Duration = Duration::from_millis(200); const RETRY_MAX: Duration = Duration::from_secs(5); +/// A `LinkCreate` config with common defaults. +const LINK_CREATE: LinkCreate = LinkCreate { + lane: None, + autoneg: false, + kr: false, + speed: PortSpeed::Speed100G, + fec: Some(PortFec::None), + tx_eq: None, + allow_ddm_traffic: false, +}; + #[cfg(test)] mod retry { use std::future::Future; @@ -83,18 +99,7 @@ async fn test_basic_autoneg_chaos() -> anyhow::Result<()> { let (_guard, client) = init_harness("autoneg", &config); let err = client - .link_create( - &"qsfp0".parse().unwrap(), - &LinkCreate { - lane: None, - autoneg: false, - kr: false, - speed: PortSpeed::Speed100G, - fec: Some(PortFec::None), - tx_eq: None, - allow_ddm_traffic: false, - }, - ) + .link_create(&"qsfp0".parse().unwrap(), &LINK_CREATE) .await .expect_err("Expected error on create"); @@ -122,15 +127,7 @@ async fn test_port_settings_addr_fail_1() -> anyhow::Result<()> { settings.links.insert( "0".into(), LinkSettings { - params: LinkCreate { - lane: None, - autoneg: false, - kr: false, - fec: Some(PortFec::None), - speed: PortSpeed::Speed100G, - tx_eq: None, - allow_ddm_traffic: false, - }, + params: LINK_CREATE, addrs: vec!["203.0.113.47".parse().unwrap()], }, ); @@ -164,15 +161,7 @@ async fn test_port_settings_addr_success_1() -> anyhow::Result<()> { settings.links.insert( "0".into(), LinkSettings { - params: LinkCreate { - lane: None, - autoneg: false, - kr: true, - fec: Some(PortFec::None), - speed: PortSpeed::Speed100G, - tx_eq: None, - allow_ddm_traffic: false, - }, + params: LinkCreate { kr: true, ..LINK_CREATE }, addrs: vec!["203.0.113.47".parse().unwrap()], }, ); @@ -204,15 +193,7 @@ async fn test_port_settings_addr_success_multi() -> anyhow::Result<()> { settings.links.insert( "0".into(), LinkSettings { - params: LinkCreate { - lane: None, - autoneg: false, - kr: true, - fec: Some(PortFec::None), - speed: PortSpeed::Speed100G, - tx_eq: None, - allow_ddm_traffic: false, - }, + params: LinkCreate { kr: true, ..LINK_CREATE }, addrs: vec!["203.0.113.47".parse().unwrap()], }, ); @@ -234,15 +215,7 @@ async fn test_port_settings_addr_success_multi() -> anyhow::Result<()> { settings.links.insert( "0".into(), LinkSettings { - params: LinkCreate { - lane: None, - autoneg: false, - kr: true, - fec: Some(PortFec::None), - speed: PortSpeed::Speed100G, - tx_eq: None, - allow_ddm_traffic: false, - }, + params: LinkCreate { kr: true, ..LINK_CREATE }, addrs: vec![ "203.0.113.46".parse().unwrap(), "203.0.113.48".parse().unwrap(), @@ -275,15 +248,7 @@ async fn test_port_settings_addr_success_multi() -> anyhow::Result<()> { settings.links.insert( "0".into(), LinkSettings { - params: LinkCreate { - lane: None, - autoneg: false, - kr: true, - fec: Some(PortFec::None), - speed: PortSpeed::Speed100G, - tx_eq: None, - allow_ddm_traffic: false, - }, + params: LinkCreate { kr: true, ..LINK_CREATE }, addrs: vec![ "203.0.113.47".parse().unwrap(), "fd00:1701::d".parse().unwrap(), @@ -552,3 +517,323 @@ fn random_port_settings() -> PortSettings { )]), } } + +const TAG1: &str = "chaos1"; +const TAG2: &str = "chaos2"; + +/// Verifies tagged port_settings_apply actions don't affect +/// resources from other tags. +#[tokio::test] +async fn addr_ns_persistent_create() -> anyhow::Result<()> { + let no_failures = AsicConfig::uniform_set(TESTING_RADIX, 0.); + let (_guard, client) = + harness::init_harness("addr_ns_persistent_create", &no_failures); + + let mut rng = IpRng::new(12345); + let port_id: PortId = "qsfp0".parse()?; + let link_id = + client.link_create(&port_id, &LINK_CREATE).await?.into_inner(); + + let tag1 = TestAddrs::new( + &mut rng, + TAG1.to_string(), + &client, + port_id.clone(), + link_id, + ); + let tag2 = TestAddrs::new( + &mut rng, + TAG2.to_string(), + &client, + port_id.clone(), + link_id, + ); + + tag1.create_addrs().await?; + tag2.apply_addrs().await?; + + tag1.verify_addrs_exist(Verify::NonExhaustive).await?; + tag2.verify_addrs_exist(Verify::NonExhaustive).await?; + + client + .port_settings_apply( + &port_id, + Some(TAG2), + &PortSettings { + links: HashMap::from([( + link_id.to_string(), + LinkSettings { params: LINK_CREATE, addrs: Vec::new() }, + )]), + }, + ) + .await?; + + tag1.verify_addrs_exist(Verify::Exhaustive).await?; + + Ok(()) +} + +/// Verifies tagged address_*_create and delete don't affect +/// resources under different tags. +#[tokio::test] +async fn addr_ns_spot_delete() -> anyhow::Result<()> { + let no_failures = AsicConfig::uniform_set(TESTING_RADIX, 0.); + let (_guard, client) = + harness::init_harness("addr_ns_spot_delete", &no_failures); + + let mut rng = IpRng::new(54321); + let port_id: PortId = "qsfp0".parse()?; + let link_id = LinkId(0); + + let tag1 = TestAddrs::new( + &mut rng, + TAG1.to_string(), + &client, + port_id.clone(), + link_id, + ); + let tag2 = TestAddrs::new( + &mut rng, + TAG2.to_string(), + &client, + port_id.clone(), + link_id, + ); + + tag2.apply_addrs().await?; + + client + .link_ipv4_create( + &port_id, + &link_id, + &Ipv4Entry { addr: tag2.v4_entry.addr, tag: TAG1.to_string() }, + ) + .await + .expect_err( + "Registering the same address under different tags should fail", + ); + + tag1.create_addrs().await?; + + tag1.verify_addrs_exist(Verify::NonExhaustive).await?; + tag2.verify_addrs_exist(Verify::NonExhaustive).await?; + + client.link_ipv4_delete(&port_id, &link_id, &tag1.v4_entry.addr).await?; + client.link_ipv6_delete(&port_id, &link_id, &tag1.v6_entry.addr).await?; + + tag2.verify_addrs_exist(Verify::Exhaustive).await?; + tag1.verify_addrs_exist(Verify::NonExhaustive) + .await + .expect_err("tag1 addresses should have been deleted"); + + Ok(()) +} + +/// Verifies port_settings_clear only affects resources of the given tag. +#[tokio::test] +async fn addr_ns_settings_clear() -> anyhow::Result<()> { + let no_failures = AsicConfig::uniform_set(TESTING_RADIX, 0.); + let (_guard, client) = + harness::init_harness("addr_ns_settings_clear", &no_failures); + + let mut rng = IpRng::new(1010101); + let port_id: PortId = "qsfp0".parse()?; + let link_id = + client.link_create(&port_id, &LINK_CREATE).await?.into_inner(); + + let tag1 = TestAddrs::new( + &mut rng, + TAG1.to_string(), + &client, + port_id.clone(), + link_id, + ); + let tag2 = TestAddrs::new( + &mut rng, + TAG2.to_string(), + &client, + port_id.clone(), + link_id, + ); + + tag1.create_addrs().await?; + tag2.apply_addrs().await?; + + tag1.verify_addrs_exist(Verify::NonExhaustive).await?; + tag2.verify_addrs_exist(Verify::NonExhaustive).await?; + + client.port_settings_clear(&port_id, Some(TAG2)).await?; + + tag1.verify_addrs_exist(Verify::Exhaustive).await?; + tag2.verify_addrs_exist(Verify::NonExhaustive).await.expect_err( + "Addresses do not exist because we cleared the tag2 port settings.", + ); + + Ok(()) +} + +/// This struct simplifies repetitive CRUD operations +/// on tagged links with random address registrations. +struct TestAddrs<'a> { + v4_entry: Ipv4Entry, + v6_entry: Ipv6Entry, + client: &'a Client, + port_id: PortId, + link_id: LinkId, +} + +impl<'a> TestAddrs<'a> { + /// Creates a new instance with a random IPv4 and IPv6 address + /// for this port and link. + fn new( + rng: &mut IpRng, + tag: String, + client: &'a Client, + port_id: PortId, + link_id: LinkId, + ) -> Self { + Self { + v4_entry: Ipv4Entry { addr: rng.unique_ipv4(), tag: tag.clone() }, + v6_entry: Ipv6Entry { addr: rng.unique_ipv6(), tag }, + client, + port_id, + link_id, + } + } + + /// Adds both tagged addresses to this link using dpd's `link_*_create` endpoints. + async fn create_addrs(&self) -> anyhow::Result<()> { + self.client + .link_ipv4_create(&self.port_id, &self.link_id, &self.v4_entry) + .await?; + self.client + .link_ipv6_create(&self.port_id, &self.link_id, &self.v6_entry) + .await?; + Ok(()) + } + + /// Adds both tagged addresses to this link using dpd's `port_settings_apply` endpoint. + async fn apply_addrs(&self) -> anyhow::Result<()> { + self.client + .port_settings_apply( + &self.port_id, + Some(&self.v4_entry.tag), + &PortSettings { + links: HashMap::from([( + self.link_id.to_string(), + LinkSettings { + params: LINK_CREATE, + addrs: vec![ + self.v4_entry.addr.into(), + self.v6_entry.addr.into(), + ], + }, + )]), + }, + ) + .await?; + + Ok(()) + } + + /// Fetches this tag's addresses using `link_*_list` and `port_settings_get`. + /// Returns Err if both addresses are not found. + /// If `scope == Verify::Exhaustive`, returns Err if other addresses + /// are found on the link besides those in `self`. + async fn verify_addrs_exist(&self, scope: Verify) -> anyhow::Result<()> { + let v4 = self + .client + .link_ipv4_list(&self.port_id, &self.link_id, None, None) + .await? + .into_inner(); + let v6 = self + .client + .link_ipv6_list(&self.port_id, &self.link_id, None, None) + .await? + .into_inner(); + + if !v4.items.contains(&self.v4_entry) { + bail!( + "Entry {:?} not found in listed addresses: {:?}", + self.v4_entry, + v4.items + ); + } + + if !v6.items.contains(&self.v6_entry) { + bail!( + "Entry {:?} not found in listed addresses: {:?}", + self.v6_entry, + v6.items + ); + } + + if scope == Verify::Exhaustive && v4.items.len() != 1 { + bail!( + "Link IPv4 items don't exactly match. Expected({:?}) v. Found({:?})", + [&self.v4_entry], + &v4.items + ); + } + + if scope == Verify::Exhaustive && v6.items.len() != 1 { + bail!( + "Link IPv6 items don't exactly match. Expected({:?}) v. Found({:?})", + [&self.v6_entry], + &v6.items + ); + } + + // Verify the port_settings endpoint returns the same. + let mut settings = self + .client + .port_settings_get(&self.port_id, Some(&self.v4_entry.tag)) + .await? + .into_inner(); + + let Some(mut settings) = + settings.links.remove(&self.link_id.to_string()).map(|s| s.addrs) + else { + bail!( + "port_settings_get should return the target link id({:?}): found {settings:?}", + self.link_id + ); + }; + + let mut listed = v4 + .items + .into_iter() + .filter_map(|entry| { + (entry.tag == self.v4_entry.tag) + .then(|| IpAddr::from(entry.addr)) + }) + .chain(v6.items.into_iter().filter_map(|entry| { + (entry.tag == self.v4_entry.tag) + .then(|| IpAddr::from(entry.addr)) + })) + .collect::>(); + + listed.sort(); + settings.sort(); + + if listed != settings { + bail!( + "Tagged address sources disagree: link_*_list({listed:?}) v. port_settings_get({settings:?})", + ); + } + + Ok(()) + } +} + +/// Informs the behavior of address registration verification. +#[derive(Debug, PartialEq, Eq)] +enum Verify { + /// Expect that the target resources are the only of their + /// kind on this link regardless of tag. + Exhaustive, + + /// Expect that the target resources exist on the link, but + /// resources from other tags may also exist. + NonExhaustive, +} diff --git a/dpd-client/tests/chaos_tests/util.rs b/dpd-client/tests/chaos_tests/util.rs index 22bc5c91..d0dc57eb 100644 --- a/dpd-client/tests/chaos_tests/util.rs +++ b/dpd-client/tests/chaos_tests/util.rs @@ -4,9 +4,19 @@ // // Copyright 2025 Oxide Computer Company -use dpd_client::Client; -use dpd_client::types::{Ipv4Entry, Ipv6Entry}; +use std::collections::HashSet; +use std::net::IpAddr; +use std::net::Ipv4Addr; +use std::net::Ipv6Addr; + use futures::TryStreamExt; +use rand::Rng; +use rand::SeedableRng; +use rand::rngs::StdRng; + +use dpd_client::Client; +use dpd_client::types::Ipv4Entry; +use dpd_client::types::Ipv6Entry; pub(crate) async fn link_list_ipv4( client: &Client, @@ -37,3 +47,64 @@ pub(crate) async fn link_list_ipv6( .try_collect::>() .await } + +/// A random IP address generator. +pub struct IpRng { + rng: StdRng, + claimed: HashSet, +} + +impl IpRng { + /// Creates a new ip address generator from the given seed. + pub fn new(seed: u64) -> Self { + Self { rng: StdRng::seed_from_u64(seed), claimed: HashSet::default() } + } + + /// Returns a random IPv4 address that this instance + /// has never created before. + pub fn unique_ipv4(&mut self) -> Ipv4Addr { + Self::roll_unique(&mut self.claimed, || { + Ipv4Addr::from_bits(self.rng.random()) + }) + } + + /// Returns a random IPv6 address that this instance + /// has never created before. + pub fn unique_ipv6(&mut self) -> Ipv6Addr { + Self::roll_unique(&mut self.claimed, || { + Ipv6Addr::from_bits(self.rng.random()) + }) + } + + fn roll_unique( + tracker: &mut HashSet, + mut random_addr: impl FnMut() -> T, + ) -> T + where + T: Into + Copy, + { + loop { + // Executes infinitely if we've already generated the entire + // IPv4 or IPv6 address space, in which case the offending test + // has earned a more bespoke solution :) + let addr = random_addr(); + if tracker.insert(addr.into()) { + return addr; + } + } + } +} + +#[cfg(test)] +mod util_tests { + use crate::chaos_tests::util::IpRng; + + /// Basic check on unique ipv6 address generation. + #[test] + fn unique_v6() { + let mut rng = IpRng::new(7); + let one = rng.unique_ipv6(); + let two = rng.unique_ipv6(); + assert_ne!(one, two); + } +} From e2b80b4134fedcc8976903860ec62ee7a93323c7 Mon Sep 17 00:00:00 2001 From: Cory Zimmerman Date: Wed, 2 Sep 2026 13:43:33 +0000 Subject: [PATCH 2/3] Respect address tags in port_settings fns Scope link address CRUD to user-provided tags. Much of this is already implied by the dropshot API. --- dpd/src/api_server.rs | 53 ++++---- dpd/src/link.rs | 260 +++++++++++++++++++++------------------ dpd/src/port_settings.rs | 214 +++++++++++++++++--------------- 3 files changed, 284 insertions(+), 243 deletions(-) diff --git a/dpd/src/api_server.rs b/dpd/src/api_server.rs index 5e108a04..d939a970 100644 --- a/dpd/src/api_server.rs +++ b/dpd/src/api_server.rs @@ -1149,7 +1149,7 @@ impl DpdApi for DpdApiImpl { let link_id = path.link_id; let entry = entry.into_inner(); switch - .create_ipv4_address(port_id, link_id, entry) + .create_ipv4_address(port_id, link_id, entry.addr, entry.tag) .map(|_| HttpResponseUpdatedNoContent()) .map_err(|e| e.into()) } @@ -1178,7 +1178,7 @@ impl DpdApi for DpdApiImpl { let link_id = path.link_id; let address = path.address; switch - .delete_ipv4_address(port_id, link_id, address) + .delete_ipv4_address(port_id, link_id, address, None) .map(|_| HttpResponseDeleted()) .map_err(|e| e.into()) } @@ -1224,7 +1224,7 @@ impl DpdApi for DpdApiImpl { let link_id = path.link_id; let entry = entry.into_inner(); switch - .create_ipv6_address(port_id, link_id, entry) + .create_ipv6_address(port_id, link_id, entry.addr, entry.tag) .map(|_| HttpResponseUpdatedNoContent()) .map_err(|e| e.into()) } @@ -1253,7 +1253,7 @@ impl DpdApi for DpdApiImpl { let link_id = path.link_id; let address = path.address; switch - .delete_ipv6_address(port_id, link_id, address) + .delete_ipv6_address(port_id, link_id, address, None) .map(|_| HttpResponseDeleted()) .map_err(|e| e.into()) } @@ -1842,9 +1842,10 @@ impl DpdApi for DpdApiImpl { let query = query.into_inner(); let port_id = path.port_id; let settings = body.into_inner(); + let tag = query.tag.as_deref().unwrap_or(""); switch - .apply_port_settings(port_id, settings, query.tag) + .apply_port_settings(port_id, settings, tag) .await .map(HttpResponseOk) .map_err(HttpError::from) @@ -1859,9 +1860,10 @@ impl DpdApi for DpdApiImpl { let path = path.into_inner(); let query = query.into_inner(); let port_id = path.port_id; + let tag = query.tag.as_deref().unwrap_or(""); switch - .clear_port_settings(port_id, query.tag) + .clear_port_settings(port_id, tag) .await .map(HttpResponseOk) .map_err(HttpError::from) @@ -1876,9 +1878,10 @@ impl DpdApi for DpdApiImpl { let path = path.into_inner(); let query = query.into_inner(); let port_id = path.port_id; + let tag = query.tag.as_deref().unwrap_or(""); switch - .get_port_settings(port_id, query.tag) + .get_port_settings(port_id, tag) .await .map(HttpResponseOk) .map_err(HttpError::from) @@ -2946,24 +2949,28 @@ pub(crate) fn build_info() -> BuildInfo { } } -impl From<&crate::link::Link> for LinkSettings { - fn from(l: &crate::link::Link) -> Self { - let mut addrs: HashSet = HashSet::new(); - for a in &l.ipv4 { - addrs.insert(a.addr.into()); - } - for a in &l.ipv6 { - addrs.insert(a.addr.into()); - } +impl crate::link::Link { + /// Creates a serializable [`LinkSettings`] representation with the + /// current link state and this tag's resources. + pub fn settings(&self, tag: &str) -> LinkSettings { + let addrs: HashSet = self + .ipv4 + .iter() + .map(|(&addr, t)| (IpAddr::from(addr), t)) + .chain(self.ipv6.iter().map(|(&addr, t)| (addr.into(), t))) + .filter(|(_, t)| *t == tag) + .map(|(addr, _)| addr) + .collect(); + LinkSettings { params: LinkCreate { - lane: Some(l.link_id), - speed: l.config.speed, - fec: l.config.fec, - autoneg: l.config.autoneg, - kr: l.config.kr, - tx_eq: l.tx_eq, - allow_ddm_traffic: l.config.allow_ddm_traffic, + lane: Some(self.link_id), + speed: self.config.speed, + fec: self.config.fec, + autoneg: self.config.autoneg, + kr: self.config.kr, + tx_eq: self.tx_eq, + allow_ddm_traffic: self.config.allow_ddm_traffic, }, addrs, } diff --git a/dpd/src/link.rs b/dpd/src/link.rs index ae60baf4..a1451a63 100644 --- a/dpd/src/link.rs +++ b/dpd/src/link.rs @@ -50,7 +50,9 @@ use slog::o; use slog::warn; use std::collections::BTreeMap; use std::collections::BTreeSet; +use std::collections::btree_map; use std::collections::btree_map::Entry; +use std::fmt::Debug; use std::net::Ipv4Addr; use std::net::Ipv6Addr; use std::sync::Arc; @@ -59,6 +61,19 @@ use std::time::Duration; use std::time::Instant; use tokio::sync::mpsc; +// Some decisions in this design are not immediately obvious. +// Here are a few observations: +// +// - IP addresses live in a BTree map/set (not Hash map/set) because the +// dropshot API for listing addresses is paginated. BTree gives a stable +// way to iterate segments of addresses. +// - IP address CRUD functions are duplicated and not generic. A generic +// v4/v6 design either motivates even more wrapper functions, or it leaks +// complexity into the `port_settings` winding and `port_ip` table +// handlers. This quickly degrades the LOC reductions here from using +// generics. I still think a generic approach is attractive, but the +// existing design expresses reasonable skepticism of that marginal benefit. + #[derive(Default)] /// Structure that stores all of the per-link state. The map is indexed using a /// (PortId, LinkId) tuple and each of the links has its own Mutex. @@ -236,10 +251,11 @@ pub struct Link { pub link_state: LinkState, /// The kind of media in the link. pub media: PortMedia, - /// A list of IPv4 addresses assigned to this link. - pub ipv4: BTreeSet, - /// A list of IPv6 addresses assigned to this link. - pub ipv6: BTreeSet, + /// IPv4 addresses assigned to this link. + /// Each is associated with its creator's ID tag. + pub ipv4: BTreeMap, + /// IPv6 addresses assigned to this link. + pub ipv6: BTreeMap, /// Tracks the history of linkup/linkdown transitions, allowing us to /// detect flapping links. pub linkup_tracker: LinkUpTracker, @@ -475,8 +491,8 @@ impl Link { fsm_state: asic::PortFsmState::default(), link_state: LinkState::Unknown, media: PortMedia::None, - ipv4: BTreeSet::new(), - ipv6: BTreeSet::new(), + ipv4: BTreeMap::new(), + ipv6: BTreeMap::new(), linkup_tracker: LinkUpTracker::default(), autoneg_tracker: AutonegTracker::default(), @@ -487,10 +503,7 @@ impl Link { /// Return the link-local address for this link, if one has been added. pub fn link_local(&self) -> Option { - self.ipv6 - .iter() - .find(|entry| (entry.addr.segments()[0] & 0xffc0) == 0xfe80) - .map(|entry| entry.addr) + self.ipv6.keys().find(|addr| addr.is_unicast_link_local()).copied() } /// Return the FEC scheme in use for this link. If the link has not yet @@ -711,15 +724,11 @@ impl Switch { // Delete all addresses in the switch tables for this link. if !link.ipv4.is_empty() { - let to_delete = std::mem::take(&mut link.ipv4) - .into_iter() - .map(|entry| entry.addr); + let to_delete = std::mem::take(&mut link.ipv4).into_keys(); port_ip::ipv4_delete_many(self, link.asic_port_id, to_delete)?; } if !link.ipv6.is_empty() { - let to_delete = std::mem::take(&mut link.ipv6) - .into_iter() - .map(|entry| entry.addr); + let to_delete = std::mem::take(&mut link.ipv6).into_keys(); port_ip::ipv6_delete_many(self, link.asic_port_id, to_delete)?; } @@ -741,15 +750,11 @@ impl Switch { // Swap out an empty map with the existing one, so that we can // retain an iterable for calling `ipv{4,6}_delete_many`. if !link.ipv4.is_empty() { - let to_delete = std::mem::take(&mut link.ipv4) - .into_iter() - .map(|entry| entry.addr); + let to_delete = std::mem::take(&mut link.ipv4).into_keys(); port_ip::ipv4_delete_many(self, link.asic_port_id, to_delete)?; } if !link.ipv6.is_empty() { - let to_delete = std::mem::take(&mut link.ipv6) - .into_iter() - .map(|entry| entry.addr); + let to_delete = std::mem::take(&mut link.ipv6).into_keys(); port_ip::ipv6_delete_many(self, link.asic_port_id, to_delete)?; } } @@ -772,42 +777,26 @@ impl Switch { } fn clear_link_addresses_locked(&self, link: &mut Link, tag: &str) { - // Remove all entries from the set with the provided tag. - // - // TODO-cleanup: It'd be nice to use `drain_filter` here, - // but that is unstable. - let mut to_remove = Vec::new(); - link.ipv4.retain(|entry| { - if entry.tag == tag { - to_remove.push(entry.addr); - false - } else { - true - } - }); - // Delete the entries from the ASIC tables. let _ = port_ip::ipv4_delete_many( self, link.asic_port_id, - to_remove.into_iter(), + link.ipv4 + .iter() + .filter(|entry| entry.1 == tag) + .map(|entry| *entry.0), ); + link.ipv4.retain(|_, t| t != tag); - // TODO-cleanup: See note above about `drain_filter`. - let mut to_remove = Vec::new(); - link.ipv6.retain(|entry| { - if entry.tag == tag { - to_remove.push(entry.addr); - false - } else { - true - } - }); let _ = port_ip::ipv6_delete_many( self, link.asic_port_id, - to_remove.into_iter(), + link.ipv6 + .iter() + .filter(|entry| entry.1 == tag) + .map(|entry| *entry.0), ); + link.ipv6.retain(|_, t| t != tag); } // Update the state of a link with a closure. @@ -1047,17 +1036,18 @@ impl Switch { pub fn create_ipv4_address_locked( &self, link: &mut Link, - entry: Ipv4Entry, + addr: Ipv4Addr, + tag: String, ) -> DpdResult<()> { - if link.ipv4.contains(&entry) { - Err(DpdError::Exists(format!( - "IP address {} already exists", - entry.addr - ))) - } else { - port_ip::ipv4_add(self, link.asic_port_id, entry.addr)?; - link.ipv4.insert(entry); - Ok(()) + match link.ipv4.entry(addr) { + btree_map::Entry::Occupied(_) => Err(DpdError::Exists(format!( + "IP address {addr} already exists", + ))), + btree_map::Entry::Vacant(slot) => { + port_ip::ipv4_add(self, link.asic_port_id, addr)?; + slot.insert(tag); + Ok(()) + } } } @@ -1066,10 +1056,11 @@ impl Switch { &self, port_id: PortId, link_id: LinkId, - entry: Ipv4Entry, + addr: Ipv4Addr, + tag: String, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - self.create_ipv4_address_locked(link, entry) + self.create_ipv4_address_locked(link, addr, tag) }) } @@ -1082,40 +1073,50 @@ impl Switch { limit: usize, ) -> DpdResult> { self.link_fetch(port_id, link_id, |link| { - if let Some(addr) = last_address { - // Equality only considers the address, so create an entry - // with an empty tag. - use std::ops::Bound; - let entry = Ipv4Entry { tag: String::new(), addr }; - link.ipv4 - .range((Bound::Excluded(entry), Bound::Unbounded)) - .take(limit) - .cloned() - .collect() + let bounds = if let Some(addr) = last_address { + (std::ops::Bound::Excluded(addr), std::ops::Bound::Unbounded) } else { - link.ipv4.iter().take(limit).cloned().collect() - } + (std::ops::Bound::Unbounded, std::ops::Bound::Unbounded) + }; + + link.ipv4 + .range(bounds) + .take(limit) + .map(|(&addr, tag)| Ipv4Entry { addr, tag: tag.clone() }) + .collect() }) } - /// Delete one IPv4 address on the provided link. + /// Deletes this IPv4 address from the link. + /// + /// Returns Err if the address is not found. + /// + /// If tag is None, the address is deleted regardless of tag. + /// If tag is Some, the address is only deleted if its registration + /// tag matches the given tag. pub fn delete_ipv4_address_locked( &self, link: &mut Link, - address: Ipv4Addr, + addr: Ipv4Addr, + tag: Option<&str>, ) -> DpdResult<()> { - let entry = Ipv4Entry { tag: String::new(), addr: address }; - - if link.ipv4.contains(&entry) { - port_ip::ipv4_delete(self, link.asic_port_id, address)?; - link.ipv4.remove(&entry); - Ok(()) - } else { - Err(DpdError::NoSuchAddress { + match link.ipv4.entry(addr) { + btree_map::Entry::Vacant(_) => Err(DpdError::NoSuchAddress { port_id: link.port_id, link_id: link.link_id, - address: address.into(), - }) + address: addr.into(), + }), + btree_map::Entry::Occupied(slot) + if tag.is_some_and(|t| t != slot.get()) => + { + // Don't delete an addr that belongs to another tag. + Ok(()) + } + btree_map::Entry::Occupied(slot) => { + port_ip::ipv4_delete(self, link.asic_port_id, addr)?; + slot.remove(); + Ok(()) + } } } @@ -1124,10 +1125,11 @@ impl Switch { &self, port_id: PortId, link_id: LinkId, - address: Ipv4Addr, + addr: Ipv4Addr, + tag: Option<&str>, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - self.delete_ipv4_address_locked(link, address) + self.delete_ipv4_address_locked(link, addr, tag) }) } @@ -1138,7 +1140,7 @@ impl Switch { link_id: LinkId, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - while let Some(Ipv4Entry { addr, .. }) = link.ipv4.pop_first() { + while let Some((addr, _)) = link.ipv4.pop_first() { port_ip::ipv4_delete(self, link.asic_port_id, addr)?; } Ok(()) @@ -1149,17 +1151,18 @@ impl Switch { pub fn create_ipv6_address_locked( &self, link: &mut Link, - entry: Ipv6Entry, + addr: Ipv6Addr, + tag: String, ) -> DpdResult<()> { - if link.ipv6.contains(&entry) { - Err(DpdError::Exists(format!( - "IP address {} already exists", - entry.addr - ))) - } else { - port_ip::ipv6_add(self, link.asic_port_id, entry.addr)?; - link.ipv6.insert(entry); - Ok(()) + match link.ipv6.entry(addr) { + btree_map::Entry::Occupied(_) => Err(DpdError::Exists(format!( + "IP address {addr} already exists" + ))), + btree_map::Entry::Vacant(slot) => { + port_ip::ipv6_add(self, link.asic_port_id, addr)?; + slot.insert(tag); + Ok(()) + } } } @@ -1168,10 +1171,11 @@ impl Switch { &self, port_id: PortId, link_id: LinkId, - entry: Ipv6Entry, + addr: Ipv6Addr, + tag: String, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - self.create_ipv6_address_locked(link, entry) + self.create_ipv6_address_locked(link, addr, tag) }) } @@ -1184,40 +1188,49 @@ impl Switch { limit: usize, ) -> DpdResult> { self.link_fetch(port_id, link_id, |link| { - if let Some(addr) = last_address { - // Equality only considers the address, so create an entry - // with an empty tag. - use std::ops::Bound; - let entry = Ipv6Entry { tag: String::new(), addr }; - link.ipv6 - .range((Bound::Excluded(entry), Bound::Unbounded)) - .take(limit) - .cloned() - .collect() + let bounds = if let Some(addr) = last_address { + (std::ops::Bound::Excluded(addr), std::ops::Bound::Unbounded) } else { - link.ipv6.iter().take(limit).cloned().collect() - } + (std::ops::Bound::Unbounded, std::ops::Bound::Unbounded) + }; + + link.ipv6 + .range(bounds) + .take(limit) + .map(|(&addr, tag)| Ipv6Entry { addr, tag: tag.clone() }) + .collect() }) } - /// Delete one IPv6 address on the provided link. + /// Deletes this IPv6 address from the link. + /// + /// Returns Err if the address is not found. + /// + /// If tag is None, the address is deleted regardless of tag. + /// If tag is Some, the address is only deleted if its registration + /// tag matches the given tag. pub fn delete_ipv6_address_locked( &self, link: &mut Link, address: Ipv6Addr, + tag: Option<&str>, ) -> DpdResult<()> { - let entry = Ipv6Entry { tag: String::new(), addr: address }; - - if link.ipv6.contains(&entry) { - port_ip::ipv6_delete(self, link.asic_port_id, address)?; - link.ipv6.remove(&entry); - Ok(()) - } else { - Err(DpdError::NoSuchAddress { + match link.ipv6.entry(address) { + btree_map::Entry::Vacant(_) => Err(DpdError::NoSuchAddress { port_id: link.port_id, link_id: link.link_id, address: address.into(), - }) + }), + btree_map::Entry::Occupied(slot) + if tag.is_some_and(|t| t != slot.get()) => + { + Ok(()) + } + btree_map::Entry::Occupied(slot) => { + port_ip::ipv6_delete(self, link.asic_port_id, address)?; + slot.remove(); + Ok(()) + } } } @@ -1227,9 +1240,10 @@ impl Switch { port_id: PortId, link_id: LinkId, address: Ipv6Addr, + tag: Option<&str>, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - self.delete_ipv6_address_locked(link, address) + self.delete_ipv6_address_locked(link, address, tag) }) } @@ -1240,7 +1254,7 @@ impl Switch { link_id: LinkId, ) -> DpdResult<()> { self.link_update(port_id, link_id, |link| { - while let Some(Ipv6Entry { addr, .. }) = link.ipv6.pop_first() { + while let Some((addr, _)) = link.ipv6.pop_first() { port_ip::ipv6_delete(self, link.asic_port_id, addr)?; } Ok(()) diff --git a/dpd/src/port_settings.rs b/dpd/src/port_settings.rs index cfcea772..1b47422f 100644 --- a/dpd/src/port_settings.rs +++ b/dpd/src/port_settings.rs @@ -10,8 +10,6 @@ use crate::Switch; use crate::link::Link; use crate::link::LinkParams; use aal::AsicOps; -use common::ports::Ipv4Entry; -use common::ports::Ipv6Entry; use common::ports::PortFec; use common::ports::PortId; use common::ports::PortSpeed; @@ -23,6 +21,7 @@ use slog::Logger; use slog::debug; use slog::error; use slog::trace; +use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; use std::net::IpAddr; @@ -99,8 +98,9 @@ struct LinkSpec { pub autoneg: bool, pub kr: bool, pub delete_me: bool, - pub ipv4: BTreeSet, - pub ipv6: BTreeSet, + // (address, owner tag) + pub ipv4: BTreeMap, + pub ipv6: BTreeMap, pub tx_eq: Option, pub allow_ddm_traffic: bool, } @@ -114,15 +114,55 @@ impl From<&Link> for LinkSpec { kr: p.config.kr, tx_eq: p.tx_eq, delete_me: p.config.delete_me, - ipv4: p.ipv4.iter().map(|x| x.addr).collect(), - ipv6: p.ipv6.iter().map(|x| x.addr).collect(), + ipv4: p.ipv4.clone(), + ipv6: p.ipv6.clone(), allow_ddm_traffic: p.config.allow_ddm_traffic, } } } -impl From<&LinkSettings> for LinkSpec { - fn from(l: &LinkSettings) -> Self { +impl LinkSpec { + /// Constructs a [`LinkSpec`] based on the given [`LinkSettings`]. + /// + /// If `master` is Some, the resulting LinkSpec includes all + /// tagged resources belonging to *other* tags. + pub fn from_settings( + l: &LinkSettings, + tag: &str, + master: Option<&LinkSpec>, + ) -> Self { + let mut ipv4 = BTreeMap::new(); + let mut ipv6 = BTreeMap::new(); + + for addr in &l.addrs { + match addr { + IpAddr::V4(v4) => { + ipv4.insert(*v4, tag.to_string()); + } + IpAddr::V6(v6) => { + ipv6.insert(*v6, tag.to_string()); + } + } + } + + if let Some(master) = master { + ipv4.extend( + master + .ipv4 + .iter() + .filter(|(_, entry_tag)| entry_tag.as_str() != tag) + .map(|(addr, entry_tag)| (*addr, entry_tag.to_string())), + ); + + ipv6.extend( + master + .ipv6 + .iter() + .filter(|(_, entry_tag)| entry_tag.as_str() != tag) + .map(|(addr, entry_tag)| (*addr, entry_tag.to_string())), + ); + } + Self { speed: l.params.speed, fec: l.params.fec, @@ -130,22 +170,8 @@ impl From<&LinkSettings> for LinkSpec { kr: l.params.kr, tx_eq: l.params.tx_eq, delete_me: false, - ipv4: l - .addrs - .iter() - .filter_map( - |x| if let IpAddr::V4(a) = x { Some(a) } else { None }, - ) - .copied() - .collect(), - ipv6: l - .addrs - .iter() - .filter_map( - |x| if let IpAddr::V6(a) = x { Some(a) } else { None }, - ) - .copied() - .collect(), + ipv4, + ipv6, allow_ddm_traffic: l.params.allow_ddm_traffic, } } @@ -194,7 +220,9 @@ impl PortSettingsDiff { self.links.add = links_to_add .map(|id| { - (*id, ChangeNode::Unchanged((&settings.links[&id.0]).into())) + let conf = &settings.links[&id.0]; + let spec = LinkSpec::from_settings(conf, ctx.tag, None); + (*id, ChangeNode::Unchanged(spec)) }) .collect(); @@ -211,21 +239,18 @@ impl PortSettingsDiff { self.links.modify = links_to_mod .map(|id| { - let settings_link = (&settings.links[&id.0]).into(); - let switch_link = ctx.link_spec(*id).expect( + let before = ctx.link_spec(*id).expect( "link existence is guaranteed by the locked link map", ); - (id, settings_link, switch_link) + let conf = &settings.links[&id.0]; + let after = + LinkSpec::from_settings(conf, ctx.tag, Some(&before)); + + (id, after, before) }) .filter(|(_, settings, switch)| settings != switch) - .map(|(id, settings, switch)| { - ( - *id, - ChangeNode::Unchanged(Modify { - before: switch, - after: settings, - }), - ) + .map(|(id, after, before)| { + (*id, ChangeNode::Unchanged(Modify { after, before })) }) .collect(); @@ -351,13 +376,13 @@ impl PortSettingsDiff { }); // Create the IPv4 addresses - for addr in spec.ipv4.iter().copied() { - Self::addr_add_v4(ctx, &mut link, rb, addr)?; + for (addr, tag) in &spec.ipv4 { + Self::addr_add_v4(ctx, &mut link, rb, *addr, tag.clone())?; } // Create the IPv6 addresses - for addr in spec.ipv6.iter().copied() { - Self::addr_add_v6(ctx, &mut link, rb, addr)?; + for (addr, tag) in &spec.ipv6 { + Self::addr_add_v6(ctx, &mut link, rb, *addr, tag.clone())?; } Ok(()) @@ -382,13 +407,13 @@ impl PortSettingsDiff { }); // Delete the IPv4 addresses - for addr in spec.ipv4.iter().copied() { - Self::addr_del_v4(ctx, &mut link, rb, addr)?; + for (addr, tag) in &spec.ipv4 { + Self::addr_del_v4(ctx, &mut link, rb, *addr, tag.clone())?; } // Delete the IPv6 addresses - for addr in spec.ipv6.iter().copied() { - Self::addr_del_v6(ctx, &mut link, rb, addr)?; + for (addr, tag) in &spec.ipv6 { + Self::addr_del_v6(ctx, &mut link, rb, *addr, tag.clone())?; } Ok(()) @@ -472,32 +497,34 @@ impl PortSettingsDiff { Ok(()) }); - // ipv4 addrs - let v4_add: BTreeSet = - ipv4_after.difference(ipv4_before).copied().collect(); - - let v4_del: BTreeSet = - ipv4_before.difference(ipv4_after).copied().collect(); + let v4_add = ipv4_after + .iter() + .filter(|(addr, _)| !ipv4_before.contains_key(addr)); + let v4_del = ipv4_before + .iter() + .filter(|(addr, _)| !ipv4_after.contains_key(addr)); - for addr in v4_add { - Self::addr_add_v4(ctx, &mut link, rb, addr)?; - } - for addr in v4_del { - Self::addr_del_v4(ctx, &mut link, rb, addr)?; + for (addr, tag) in v4_add { + Self::addr_add_v4(ctx, &mut link, rb, *addr, tag.clone())?; } - // ipv6 addrs - let v6_add: BTreeSet = - ipv6_after.difference(ipv6_before).copied().collect(); + for (addr, tag) in v4_del { + Self::addr_del_v4(ctx, &mut link, rb, *addr, tag.clone())?; + } - let v6_del: BTreeSet = - ipv6_before.difference(ipv6_after).copied().collect(); + let v6_add = ipv6_after + .iter() + .filter(|(addr, _)| !ipv6_before.contains_key(addr)); + let v6_del = ipv6_before + .iter() + .filter(|(addr, _)| !ipv6_after.contains_key(addr)); - for addr in v6_add { - Self::addr_add_v6(ctx, &mut link, rb, addr)?; + for (addr, tag) in v6_add { + Self::addr_add_v6(ctx, &mut link, rb, *addr, tag.clone())?; } - for addr in v6_del { - Self::addr_del_v6(ctx, &mut link, rb, addr)?; + + for (addr, tag) in v6_del { + Self::addr_del_v6(ctx, &mut link, rb, *addr, tag.clone())?; } Ok(()) @@ -508,20 +535,19 @@ impl PortSettingsDiff { link: &mut Link, rb: &mut Rollback, addr: Ipv4Addr, + tag: String, ) -> DpdResult<()> { - trace!(ctx.log, "ipv4 add {addr}"); + trace!(ctx.log, "ipv4 add ({addr}: {tag})"); // Create address on ASIC first. - let entry = - Ipv4Entry { tag: ctx.tag.clone().unwrap_or("".into()), addr }; let switch = ctx.switch; - switch.create_ipv4_address_locked(link, entry)?; + switch.create_ipv4_address_locked(link, addr, tag.clone())?; let link_id = link.link_id; rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> { let switch = ctx.switch; let link_lock = ctx.link(link_id)?; let mut link = link_lock.lock().unwrap(); - switch.delete_ipv4_address_locked(&mut link, addr) + switch.delete_ipv4_address_locked(&mut link, addr, Some(&tag)) }); Ok(()) } @@ -531,19 +557,18 @@ impl PortSettingsDiff { link: &mut Link, rb: &mut Rollback, addr: Ipv4Addr, + tag: String, ) -> DpdResult<()> { - trace!(ctx.log, "ipv4 del {addr}"); - let entry = - Ipv4Entry { tag: ctx.tag.clone().unwrap_or("".into()), addr }; + trace!(ctx.log, "ipv4 del ({addr}: {tag})"); let switch = ctx.switch; let link_id = link.link_id; - switch.delete_ipv4_address_locked(link, addr)?; + switch.delete_ipv4_address_locked(link, addr, Some(&tag))?; rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> { let switch = ctx.switch; let link_lock = ctx.link(link_id)?; let mut link = link_lock.lock().unwrap(); - switch.create_ipv4_address_locked(&mut link, entry) + switch.create_ipv4_address_locked(&mut link, addr, tag) }); Ok(()) } @@ -553,20 +578,19 @@ impl PortSettingsDiff { link: &mut Link, rb: &mut Rollback, addr: Ipv6Addr, + tag: String, ) -> DpdResult<()> { - trace!(ctx.log, "ipv6 add {addr}"); + trace!(ctx.log, "ipv6 add ({addr}: {tag})"); // Create address on ASIC first. - let entry = - Ipv6Entry { tag: ctx.tag.clone().unwrap_or("".into()), addr }; let switch = ctx.switch; let link_id = link.link_id; - switch.create_ipv6_address_locked(link, entry)?; + switch.create_ipv6_address_locked(link, addr, tag.clone())?; rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> { let switch = ctx.switch; let link_lock = ctx.link(link_id)?; let mut link = link_lock.lock().unwrap(); - switch.delete_ipv6_address_locked(&mut link, addr) + switch.delete_ipv6_address_locked(&mut link, addr, Some(&tag)) }); Ok(()) } @@ -576,19 +600,18 @@ impl PortSettingsDiff { link: &mut Link, rb: &mut Rollback, addr: Ipv6Addr, + tag: String, ) -> DpdResult<()> { - trace!(ctx.log, "ipv6 del {addr}"); + trace!(ctx.log, "ipv6 del ({addr}: {tag})"); let switch = ctx.switch; let link_id = link.link_id; - switch.delete_ipv6_address_locked(link, addr)?; + switch.delete_ipv6_address_locked(link, addr, Some(&tag))?; rb.wind(move |ctx: &mut Context<'_>| -> DpdResult<()> { - let entry = - Ipv6Entry { tag: ctx.tag.clone().unwrap_or("".into()), addr }; let switch = ctx.switch; let link_lock = ctx.link(link_id)?; let mut link = link_lock.lock().unwrap(); - switch.create_ipv6_address_locked(&mut link, entry) + switch.create_ipv6_address_locked(&mut link, addr, tag) }); Ok(()) } @@ -628,18 +651,18 @@ struct Context<'a> { port_id: PortId, switch: &'a Switch, link_map: MutexGuard<'a, crate::link::LinkMap>, - tag: Option, + tag: &'a str, log: Logger, rollback: bool, } macro_rules! context { - ($port_id:expr, $switch:expr) => { + ($port_id:expr, $switch:expr, $tag:expr) => { Context { port_id: $port_id, switch: $switch, link_map: $switch.links.lock().unwrap(), - tag: None, + tag: $tag, log: $switch.log.clone(), rollback: false, } @@ -664,10 +687,9 @@ impl Switch { &self, port_id: PortId, settings: PortSettings, - tag: Option, + tag: &str, ) -> DpdResult { - let mut ctx = context!(port_id, self); - ctx.tag = tag; + let mut ctx = context!(port_id, self, tag); let mut diff = PortSettingsDiff::calculate(&mut ctx, &settings)?; trace!(self.log, "port settings diff: {:#?}", diff); @@ -680,10 +702,9 @@ impl Switch { pub async fn clear_port_settings( &self, port_id: PortId, - tag: Option, + tag: &str, ) -> DpdResult { - let mut ctx = context!(port_id, self); - ctx.tag = tag; + let mut ctx = context!(port_id, self, tag); let settings = PortSettings::default(); let mut diff = PortSettingsDiff::calculate(&mut ctx, &settings)?; @@ -697,10 +718,9 @@ impl Switch { pub async fn get_port_settings( &self, port_id: PortId, - tag: Option, + tag: &str, ) -> DpdResult { - let mut ctx = context!(port_id, self); - ctx.tag = tag; + let mut ctx = context!(port_id, self, tag); Self::get_port_settings_locked(&mut ctx, false) } @@ -737,7 +757,7 @@ impl Switch { if ignore_deleting && link.config.delete_me { None } else { - Some(((*link_id).into(), LinkSettings::from(&*link))) + Some(((*link_id).into(), link.settings(ctx.tag))) } } else { None From fba64cced23aeb0ba4749509b1909626e1bb9e67 Mon Sep 17 00:00:00 2001 From: Cory Zimmerman Date: Thu, 3 Sep 2026 22:03:46 +0000 Subject: [PATCH 3/3] Remove tag arg from port_settings_clear It was not used in the previous version, and implying support for tags is incorrect. port_settings_clear just deletes links from a port. --- dpd-api/src/lib.rs | 28 +++++++++++++++---- dpd-client/tests/chaos_tests/port_settings.rs | 18 ++++++------ dpd/src/api_server.rs | 5 +--- dpd/src/port_settings.rs | 7 +++-- openapi/dpd/dpd-13.0.0-5db8bd.json.gitstub | 1 + ...0.0-5db8bd.json => dpd-14.0.0-5d7e89.json} | 13 ++------- openapi/dpd/dpd-latest.json | 2 +- 7 files changed, 41 insertions(+), 33 deletions(-) create mode 100644 openapi/dpd/dpd-13.0.0-5db8bd.json.gitstub rename openapi/dpd/{dpd-13.0.0-5db8bd.json => dpd-14.0.0-5d7e89.json} (99%) diff --git a/dpd-api/src/lib.rs b/dpd-api/src/lib.rs index 7441178e..7f777d4b 100644 --- a/dpd-api/src/lib.rs +++ b/dpd-api/src/lib.rs @@ -29,6 +29,7 @@ api_versions!([ // | example for the next person. // v // (next_int, IDENT), + (14, UNTAG_PORT_SETTINGS_CLEAR), (13, ALLOW_DDM_TRAFFIC), (12, PRBS_ERROR_TRACKING), (11, WALLCLOCK_HISTORY), @@ -1693,20 +1694,35 @@ pub trait DpdApi { .map(|resp| resp.map(Into::into)) } - /** - * Clear port settings atomically. - */ + /// Deletes all the links belonging to a port. #[endpoint { method = DELETE, - versions = VERSION_ALLOW_DDM_TRAFFIC.., + versions = VERSION_UNTAG_PORT_SETTINGS_CLEAR.., path = "/port/{port_id}/settings" }] async fn port_settings_clear( rqctx: RequestContext, path: Path, - query: Query, ) -> Result, HttpError>; + /// This version was deprecated because `port_settings_clear` + /// did not respect [`latest::port::PortSettingsTag`] and had no clear way + /// of doing so. Callers (sled-agent) can use the new version identically. + #[endpoint { + method = DELETE, + versions = VERSION_ALLOW_DDM_TRAFFIC..VERSION_UNTAG_PORT_SETTINGS_CLEAR, + path = "/port/{port_id}/settings", + operation_id = "port_settings_clear", + }] + async fn port_settings_clear_v2( + rqctx: RequestContext, + path: Path, + query: Query, + ) -> Result, HttpError> { + let _ = query; + Self::port_settings_clear(rqctx, path).await + } + /** * Clear port settings atomically. */ @@ -1721,7 +1737,7 @@ pub trait DpdApi { path: Path, query: Query, ) -> Result, HttpError> { - Self::port_settings_clear(rqctx, path, query) + Self::port_settings_clear_v2(rqctx, path, query) .await .map(|resp| resp.map(Into::into)) } diff --git a/dpd-client/tests/chaos_tests/port_settings.rs b/dpd-client/tests/chaos_tests/port_settings.rs index 710ea4ae..a1b3390a 100644 --- a/dpd-client/tests/chaos_tests/port_settings.rs +++ b/dpd-client/tests/chaos_tests/port_settings.rs @@ -272,9 +272,7 @@ async fn test_port_settings_addr_success_multi() -> anyhow::Result<()> { // Clear all settings - client - .port_settings_clear(&"qsfp0".parse().unwrap(), Some("chaos")) - .await?; + client.port_settings_clear(&"qsfp0".parse().unwrap()).await?; // The addresses are all cleared synchronously, but the link deletion is // async. We pause briefly to give it a chance to complete. The subsequent @@ -629,7 +627,7 @@ async fn addr_ns_spot_delete() -> anyhow::Result<()> { Ok(()) } -/// Verifies port_settings_clear only affects resources of the given tag. +/// Verifies port_settings_clear does as the API docs proclaim. #[tokio::test] async fn addr_ns_settings_clear() -> anyhow::Result<()> { let no_failures = AsicConfig::uniform_set(TESTING_RADIX, 0.); @@ -662,12 +660,14 @@ async fn addr_ns_settings_clear() -> anyhow::Result<()> { tag1.verify_addrs_exist(Verify::NonExhaustive).await?; tag2.verify_addrs_exist(Verify::NonExhaustive).await?; - client.port_settings_clear(&port_id, Some(TAG2)).await?; + client.port_settings_clear(&port_id).await?; - tag1.verify_addrs_exist(Verify::Exhaustive).await?; - tag2.verify_addrs_exist(Verify::NonExhaustive).await.expect_err( - "Addresses do not exist because we cleared the tag2 port settings.", - ); + tag1.verify_addrs_exist(Verify::Exhaustive) + .await + .expect_err("tag1: should have cleared all config"); + tag2.verify_addrs_exist(Verify::NonExhaustive) + .await + .expect_err("tag2: should have cleared all config"); Ok(()) } diff --git a/dpd/src/api_server.rs b/dpd/src/api_server.rs index d939a970..6a81da74 100644 --- a/dpd/src/api_server.rs +++ b/dpd/src/api_server.rs @@ -1854,16 +1854,13 @@ impl DpdApi for DpdApiImpl { async fn port_settings_clear( rqctx: RequestContext>, path: Path, - query: Query, ) -> Result, HttpError> { let switch = rqctx.context(); let path = path.into_inner(); - let query = query.into_inner(); let port_id = path.port_id; - let tag = query.tag.as_deref().unwrap_or(""); switch - .clear_port_settings(port_id, tag) + .clear_port_settings(port_id) .await .map(HttpResponseOk) .map_err(HttpError::from) diff --git a/dpd/src/port_settings.rs b/dpd/src/port_settings.rs index 1b47422f..08d6e824 100644 --- a/dpd/src/port_settings.rs +++ b/dpd/src/port_settings.rs @@ -702,9 +702,12 @@ impl Switch { pub async fn clear_port_settings( &self, port_id: PortId, - tag: &str, ) -> DpdResult { - let mut ctx = context!(port_id, self, tag); + // This could be a tagged function, but making it so + // reaches far beyond the existing design and is + // unnecessary for current usage. + let irrelevant_tag = ""; + let mut ctx = context!(port_id, self, irrelevant_tag); let settings = PortSettings::default(); let mut diff = PortSettingsDiff::calculate(&mut ctx, &settings)?; diff --git a/openapi/dpd/dpd-13.0.0-5db8bd.json.gitstub b/openapi/dpd/dpd-13.0.0-5db8bd.json.gitstub new file mode 100644 index 00000000..c886f19e --- /dev/null +++ b/openapi/dpd/dpd-13.0.0-5db8bd.json.gitstub @@ -0,0 +1 @@ +2df9101e95757037f2cc4e31e8bf00fd9e79efc7:openapi/dpd/dpd-13.0.0-5db8bd.json diff --git a/openapi/dpd/dpd-13.0.0-5db8bd.json b/openapi/dpd/dpd-14.0.0-5d7e89.json similarity index 99% rename from openapi/dpd/dpd-13.0.0-5db8bd.json rename to openapi/dpd/dpd-14.0.0-5d7e89.json index 22694237..d04b61fe 100644 --- a/openapi/dpd/dpd-13.0.0-5db8bd.json +++ b/openapi/dpd/dpd-14.0.0-5d7e89.json @@ -7,7 +7,7 @@ "url": "https://oxide.computer", "email": "api@oxide.computer" }, - "version": "13.0.0" + "version": "14.0.0" }, "paths": { "/all-settings": { @@ -2466,7 +2466,7 @@ } }, "delete": { - "summary": "Clear port settings atomically.", + "summary": "Deletes all the links belonging to a port.", "operationId": "port_settings_clear", "parameters": [ { @@ -2477,15 +2477,6 @@ "schema": { "$ref": "#/components/schemas/PortId" } - }, - { - "in": "query", - "name": "tag", - "description": "Restrict operations on this port to the provided tag.", - "schema": { - "nullable": true, - "type": "string" - } } ], "responses": { diff --git a/openapi/dpd/dpd-latest.json b/openapi/dpd/dpd-latest.json index d1a5660e..b138952f 120000 --- a/openapi/dpd/dpd-latest.json +++ b/openapi/dpd/dpd-latest.json @@ -1 +1 @@ -dpd-13.0.0-5db8bd.json \ No newline at end of file +dpd-14.0.0-5d7e89.json \ No newline at end of file