From c4068517dd8dd9cbc28133d3005f2c1d6cd753b2 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Fri, 16 Oct 2026 11:08:51 +0100 Subject: [PATCH 1/3] Count ICMP-needed drops from L3Router Packets which are dropped in L3Router and its nested controls all need to be handled by the host, so we cannot actually drop these packets in the Tofino. However the counters were only ever modified when the packet is truly dropped, so these counters were non-functional. This PR separates out the counter increment to occur for both classes of drop. I did look into centralising everything onto `ig_dprsr_md.drop_ctl` and ridding ourselves of `meta.dropped`, but this ended up both uglier and took up more stages under multicast. Simple seems best, here! Fixes #404. --- .../tests/integration_tests/counters.rs | 44 ++++++++++++++----- .../tests/integration_tests/route_ipv4.rs | 29 ++++++++++-- .../tests/integration_tests/route_ipv6.rs | 14 +++++- dpd/p4/sidecar.p4 | 12 +++-- 4 files changed, 78 insertions(+), 21 deletions(-) diff --git a/dpd-client/tests/integration_tests/counters.rs b/dpd-client/tests/integration_tests/counters.rs index f793f2a6..239fbf87 100644 --- a/dpd-client/tests/integration_tests/counters.rs +++ b/dpd-client/tests/integration_tests/counters.rs @@ -22,21 +22,20 @@ use dpd_client::ClientInfo; use dpd_client::types::Ipv4Entry; use dpd_client::types::Ipv6Entry; -// Run a single drop test. This sends a packet that we expect to be dropped, -// and verifies that the expected drop counter is bumped by one. If the test -// runs to completion we return the counter evaluation as a boolean rather than -// an Error. This allows us to trigger the error in the test routine, which -// gives us a more specific test failure message. -async fn one_drop_test( +/// Process a single packet, asserting that a given counter is incremented by +/// one and a packet is appropriately transformed (or dropped). +/// +/// If the test runs to completion we return the counter evaluation as a +/// boolean rather than an Error. This allows us to trigger the error in +/// the test routine, which gives us a more specific test failure message +pub async fn one_counted_test( switch: &Switch, - port: PhysPort, - packet: packet::Packet, + packet: TestPacket, + packet_after: Option, counter: &str, ) -> anyhow::Result { - let send = TestPacket { packet: Arc::new(packet), port }; - let old = switch.get_counter(counter, None).await?; - switch.packet_test(vec![send], Vec::new())?; + switch.packet_test(vec![packet], packet_after.into_iter().collect())?; let mut new = 0; for _i in 0..20 { @@ -45,7 +44,7 @@ async fn one_drop_test( // simulator and when that updated value is available to the SDE. To // avoid a long pointless delay here, we try multiple times with a short // sleep rather than once with a long sleep. - std::thread::sleep(std::time::Duration::from_millis(100)); + tokio::time::sleep(std::time::Duration::from_millis(100)).await; new = switch.get_counter(counter, None).await?; if old + 1 == new { break; @@ -55,6 +54,27 @@ async fn one_drop_test( Ok(old + 1 == new) } +/// Run a single drop test. This sends a packet that we expect to be dropped, +/// and verifies that the expected drop counter is bumped by one. +/// +/// If the test runs to completion we return the counter evaluation as a +/// boolean rather than an Error. This allows us to trigger the error in +/// the test routine, which gives us a more specific test failure message. +async fn one_drop_test( + switch: &Switch, + port: PhysPort, + packet: packet::Packet, + counter: &str, +) -> anyhow::Result { + one_counted_test( + switch, + TestPacket { packet: Arc::new(packet), port }, + None, + counter, + ) + .await +} + // Add an IPv4 address to a port on the switch async fn add_switch_ipv4(switch: &Switch, port: PhysPort, addr: &str) { let (port_id, link_id) = switch.link_id(port).unwrap(); diff --git a/dpd-client/tests/integration_tests/route_ipv4.rs b/dpd-client/tests/integration_tests/route_ipv4.rs index 75f7eceb..7e2d2aac 100644 --- a/dpd-client/tests/integration_tests/route_ipv4.rs +++ b/dpd-client/tests/integration_tests/route_ipv4.rs @@ -14,6 +14,7 @@ use reqwest::StatusCode; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; +use crate::integration_tests::counters::one_counted_test; use packet::Endpoint; use packet::eth::EthQHdr; @@ -159,7 +160,12 @@ async fn test_deleted_route_ipv4_impl(switch: &Switch) -> TestResult { common::set_icmp_unreachable(switch, &mut to_recv, ingress); let expected = TestPacket { packet: Arc::new(to_recv), port: egress }; - switch.packet_test(vec![send], vec![expected]) + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_unrouteable") + .await? + ); + + Ok(()) } #[tokio::test] @@ -262,7 +268,12 @@ async fn test_route_ipv4_unrouteable() -> TestResult { common::set_icmp_unreachable(switch, &mut to_recv, ingress); let expected = TestPacket { packet: Arc::new(to_recv), port: egress }; - switch.packet_test(vec![send], vec![expected]) + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_unrouteable") + .await? + ); + + Ok(()) } #[tokio::test] @@ -934,6 +945,7 @@ async fn test_v4_over_v6() -> TestResult { .expect_err("route should be gone after deleting its only target"); // Verify that packets to the deleted route produce ICMP unreachable. + // This must *also* increment the relevant drop counter. let to_send = common::gen_udp_packet( Endpoint::parse("e0:d5:5e:67:89:ab", "10.10.10.10", 3333).unwrap(), Endpoint::parse("e0:d5:5e:67:89:ac", "10.10.10.11", 4444).unwrap(), @@ -943,7 +955,11 @@ async fn test_v4_over_v6() -> TestResult { let send = TestPacket { packet: Arc::new(to_send), port: ingress }; let expected = TestPacket { packet: Arc::new(to_recv), port: SERVICE_PORT }; - switch.packet_test(vec![send], vec![expected])?; + + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_unrouteable") + .await? + ); // Re-add the route and verify forwarding works again. common::set_route_ipv4_over_ipv6( @@ -979,7 +995,12 @@ async fn test_v4_over_v6() -> TestResult { let send = TestPacket { packet: Arc::new(to_send), port: ingress }; let expected = TestPacket { packet: Arc::new(to_recv), port: SERVICE_PORT }; - switch.packet_test(vec![send], vec![expected]) + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_unrouteable") + .await? + ); + + Ok(()) } /// Test that individual v4 and v6 targets can be deleted from a mixed diff --git a/dpd-client/tests/integration_tests/route_ipv6.rs b/dpd-client/tests/integration_tests/route_ipv6.rs index 81932cac..1ae6398f 100644 --- a/dpd-client/tests/integration_tests/route_ipv6.rs +++ b/dpd-client/tests/integration_tests/route_ipv6.rs @@ -16,6 +16,7 @@ use packet::{Endpoint, ipv6, sidecar}; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; +use crate::integration_tests::counters::one_counted_test; use packet::eth::EthQHdr; use dpd_client::types; @@ -201,7 +202,12 @@ async fn test_deleted_unicast_impl(switch: &Switch) -> TestResult { common::set_icmp6_unreachable(switch, &mut to_recv, ingress); let expected = TestPacket { packet: Arc::new(to_recv), port: egress }; - switch.packet_test(vec![send], vec![expected]) + assert!( + one_counted_test(switch, send, Some(expected), "ipv6_unrouteable") + .await? + ); + + Ok(()) } #[tokio::test] @@ -279,8 +285,12 @@ async fn test_unrouteable() -> TestResult { common::set_icmp6_unreachable(switch, &mut to_recv, ingress); let expected = TestPacket { packet: Arc::new(to_recv), port: egress }; + assert!( + one_counted_test(switch, send, Some(expected), "ipv6_unrouteable") + .await? + ); - switch.packet_test(vec![send], vec![expected]) + Ok(()) } // Define two routes, one with a 56-bit prefix and one with a 64-bit prefix. diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index b06a842a..108dba15 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -2187,11 +2187,17 @@ control Ingress( } } - if (meta.dropped) { - // Handle dropped packets - ig_dprsr_md.drop_ctl = 1; + // Drops recorded in the L3 router need to be counted, but + // we do not convert them to _actual_ packet drops in the + // deparser. We forward such packets to the host to generate an + // ICMP response in our stead. + if (meta.drop_reason != 0) { drop_port_ctr.count(ig_intr_md.ingress_port); drop_reason_ctr.count(meta.drop_reason); + } + + if (meta.dropped) { + ig_dprsr_md.drop_ctl = 1; } else if (!meta.is_mcast) { egress_ctr.count(ig_tm_md.ucast_egress_port); if (ig_tm_md.ucast_egress_port != USER_SPACE_SERVICE_PORT) { From 241e705bcaba8410923a654bf6e0eb48a58b98ec Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Mon, 26 Oct 2026 17:43:05 +0000 Subject: [PATCH 2/3] Review feedback: breakout `check_counter_increment`. --- .../tests/integration_tests/counters.rs | 68 ++++++++++++------- dpd-client/tests/integration_tests/mcast.rs | 32 +-------- 2 files changed, 44 insertions(+), 56 deletions(-) diff --git a/dpd-client/tests/integration_tests/counters.rs b/dpd-client/tests/integration_tests/counters.rs index 239fbf87..1f50d7ff 100644 --- a/dpd-client/tests/integration_tests/counters.rs +++ b/dpd-client/tests/integration_tests/counters.rs @@ -22,12 +22,47 @@ use dpd_client::ClientInfo; use dpd_client::types::Ipv4Entry; use dpd_client::types::Ipv6Entry; -/// Process a single packet, asserting that a given counter is incremented by -/// one and a packet is appropriately transformed (or dropped). +/// Awaits an increase in a given counter's value from `baseline` to +/// `baseline + increment`. /// -/// If the test runs to completion we return the counter evaluation as a -/// boolean rather than an Error. This allows us to trigger the error in -/// the test routine, which gives us a more specific test failure message +/// Returns `Ok(new_value)` if the expected increment is seen. +pub async fn check_counter_incremented( + switch: &Switch, + counter_name: &str, + baseline: u64, + expected_increment: u64, + client_name: Option<&str>, +) -> anyhow::Result { + let mut new_value = 0; + + // Briefly delay before reading the counter each time, as there is + // occasionally some lag between when we see the counter set in the + // simulator and when that updated value is available to the SDE. To + // avoid a long pointless delay here, we try multiple times with a short + // sleep rather than once with a long sleep. + for _ in 0..20 { + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + new_value = + switch.get_counter(counter_name, client_name).await.unwrap(); + + if new_value == baseline + expected_increment { + return Ok(new_value); + } + } + + // Counter didn't increment as expected. + Err(anyhow::anyhow!( + "Counter '{}' expected to increase by {} (from {} to {}), but only reached {}", + counter_name, + expected_increment, + baseline, + baseline + expected_increment, + new_value + )) +} + +/// Process a single packet, asserting that a given counter is incremented by +/// one and a packet is appropriately transformed (or dropped).. pub async fn one_counted_test( switch: &Switch, packet: TestPacket, @@ -36,30 +71,13 @@ pub async fn one_counted_test( ) -> anyhow::Result { let old = switch.get_counter(counter, None).await?; switch.packet_test(vec![packet], packet_after.into_iter().collect())?; - - let mut new = 0; - for _i in 0..20 { - // Briefly delay before reading the counter for a second time, as there is - // occasionally some lag between when we see the counter set in the - // simulator and when that updated value is available to the SDE. To - // avoid a long pointless delay here, we try multiple times with a short - // sleep rather than once with a long sleep. - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - new = switch.get_counter(counter, None).await?; - if old + 1 == new { - break; - } - } - - Ok(old + 1 == new) + check_counter_incremented(switch, counter, old, 1, None) + .await + .map(|new| old + 1 == new) } /// Run a single drop test. This sends a packet that we expect to be dropped, /// and verifies that the expected drop counter is bumped by one. -/// -/// If the test runs to completion we return the counter evaluation as a -/// boolean rather than an Error. This allows us to trigger the error in -/// the test routine, which gives us a more specific test failure message. async fn one_drop_test( switch: &Switch, port: PhysPort, diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 6c5ac347..4bf0992f 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -12,6 +12,7 @@ use std::{ use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; +use crate::integration_tests::counters::check_counter_incremented; use ::common::network::MacAddr; use anyhow::anyhow; use dpd_client::{Error, types}; @@ -71,37 +72,6 @@ fn has_vlan_action_for_ip( }) } -async fn check_counter_incremented( - switch: &Switch, - counter_name: &str, - baseline: u64, - expected_increment: u64, - client_name: Option<&str>, -) -> anyhow::Result { - let mut new_value = 0; - - // Poll for the counter value (with timeout) - for _i in 0..20 { - std::thread::sleep(std::time::Duration::from_millis(100)); - new_value = - switch.get_counter(counter_name, client_name).await.unwrap(); - - if new_value == baseline + expected_increment { - return Ok(new_value); - } - } - - // Counter didn't increment as expected - Err(anyhow!( - "Counter '{}' expected to increase by {} (from {} to {}), but only reached {}", - counter_name, - expected_increment, - baseline, - baseline + expected_increment, - new_value - )) -} - async fn create_test_multicast_group( switch: &Switch, group_ip: IpAddr, From 8e0b3420204f270809c20f73e68316833f90a794 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Mon, 26 Oct 2026 18:32:24 +0000 Subject: [PATCH 3/3] Review: add counter checks to existing service tests --- dpd-client/tests/integration_tests/service.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/dpd-client/tests/integration_tests/service.rs b/dpd-client/tests/integration_tests/service.rs index 2aeff4a5..7441dad2 100644 --- a/dpd-client/tests/integration_tests/service.rs +++ b/dpd-client/tests/integration_tests/service.rs @@ -13,6 +13,7 @@ use packet::{Endpoint, eth, icmp, ipv4, sidecar}; use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; +use crate::integration_tests::counters::one_counted_test; // Packets sent to an IP address assigned to a switch port should be forwarded // to the service port with an added sidecar header. @@ -221,7 +222,9 @@ async fn test_arp_needed() -> TestResult { let expected = TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; - switch.packet_test(vec![send], vec![expected]) + assert!(one_counted_test(switch, send, Some(expected), "arp_miss").await?); + + Ok(()) } #[tokio::test] @@ -266,7 +269,13 @@ async fn test_ttl_exceeded() -> TestResult { ); let expected = TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; - switch.packet_test(vec![send], vec![expected]) + + assert!( + one_counted_test(switch, send, Some(expected), "ipv4_ttl_exceeded") + .await? + ); + + Ok(()) } // Arp packets sent to an IP address assigned to a switch port should be