diff --git a/dpd-client/tests/integration_tests/counters.rs b/dpd-client/tests/integration_tests/counters.rs index f793f2a6..1f50d7ff 100644 --- a/dpd-client/tests/integration_tests/counters.rs +++ b/dpd-client/tests/integration_tests/counters.rs @@ -22,37 +22,75 @@ 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. +/// Awaits an increase in a given counter's value from `baseline` to +/// `baseline + increment`. +/// +/// 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, + packet_after: Option, + counter: &str, +) -> anyhow::Result { + let old = switch.get_counter(counter, None).await?; + switch.packet_test(vec![packet], packet_after.into_iter().collect())?; + 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. async fn one_drop_test( switch: &Switch, port: PhysPort, packet: packet::Packet, 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())?; - - 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. - std::thread::sleep(std::time::Duration::from_millis(100)); - new = switch.get_counter(counter, None).await?; - if old + 1 == new { - break; - } - } - - Ok(old + 1 == new) + one_counted_test( + switch, + TestPacket { packet: Arc::new(packet), port }, + None, + counter, + ) + .await } // Add an IPv4 address to a port on the switch 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, 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-client/tests/integration_tests/service.rs b/dpd-client/tests/integration_tests/service.rs index 2aeff4a5..78e70bee 100644 --- a/dpd-client/tests/integration_tests/service.rs +++ b/dpd-client/tests/integration_tests/service.rs @@ -4,15 +4,17 @@ // // Copyright 2026 Oxide Computer Company +use std::net::Ipv6Addr; use std::sync::Arc; use ::common::network::MacAddr; use dpd_client::ClientInfo; use dpd_client::types::Ipv4Entry; -use packet::{Endpoint, eth, icmp, ipv4, sidecar}; +use packet::{Endpoint, eth, icmp, ipv4, ipv6, 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 +223,58 @@ 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] +#[ignore] +async fn test_ndp_needed() -> TestResult { + let switch = &*get_switch().await; + + let ingress_port = PhysPort(10); + let egress_port = PhysPort(15); + let router_ip = "fd00:1122:7788:0101::1"; + let src_mac = "e0:d5:5e:67:89:ab"; + let router_mac = switch.get_port_mac(ingress_port).unwrap(); + + // As in test_arp_needed, set a route without installing a neighbor. + common::set_route_ipv6(switch, "::/0", egress_port, router_ip).await?; + + let (send_pkt, mut recv_pkt) = common::gen_udp_routed_pair( + switch, + egress_port, + router_mac, + Endpoint::parse(src_mac, "fd00:1122:7788:0202::4", 3333).unwrap(), + Endpoint::parse("e0:d5:5e:67:89:ac", "fd00:1122:7788:0101::5", 4444) + .unwrap(), + ); + + let send = TestPacket { packet: Arc::new(send_pkt), port: ingress_port }; + + let payload = router_ip.parse::().unwrap().octets(); + common::add_sidecar_hdr( + switch, + &mut recv_pkt, + sidecar::SC_NEIGHBOR_NEEDED, + ingress_port, + egress_port, + Some(&payload[..]), + ); + + // Because the packet wasn't successfully routed, the src/dst mac addresses + // should be unchanged. + let src_eth = send.packet.hdrs.eth_hdr.as_ref().unwrap(); + eth::EthHdr::rewrite_smac(&mut recv_pkt, src_eth.eth_smac); + eth::EthHdr::rewrite_dmac(&mut recv_pkt, src_eth.eth_dmac); + + let expected = + TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; + + assert!(one_counted_test(switch, send, Some(expected), "ndp_miss").await?); + + Ok(()) } #[tokio::test] @@ -262,11 +315,70 @@ async fn test_ttl_exceeded() -> TestResult { ingress, SERVICE_PORT, icmp::ICMP_TIME_EXCEEDED, - 0, + icmp::ICMP_EXC_TTL, ); 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(()) +} + +#[tokio::test] +#[ignore] +async fn test_hop_limit_exceeded() -> TestResult { + let switch = &*get_switch().await; + + let ingress = PhysPort(10); + let egress = PhysPort(14); + let port_mac = "a8:40:25:00:00:0c".parse::()?; + + let orig_src = + Endpoint::parse("e0:d5:5e:67:89:ab", "fd00:1122:7788:0202::4", 3333) + .unwrap(); + let orig_dst = + Endpoint::parse("e0:d5:5e:67:89:ac", "fd00:1122:3344:0101::5", 4444) + .unwrap(); + + let (port_id, link_id) = switch.link_id(ingress).unwrap(); + switch + .client + .link_mac_set(&port_id, &link_id, &port_mac.into()) + .await + .unwrap(); + + common::set_route_ipv6(switch, "::/0", egress, "fd00:1122:7788:0101::1") + .await?; + + // build the initial packet and set its ttl to expire in 1 hop + let mut send_pkt = common::gen_udp_packet(orig_src, orig_dst); + ipv6::Ipv6Hdr::adjust_hlim(&mut send_pkt, -254); + + let mut recv_pkt = send_pkt.clone(); + + let send = TestPacket { packet: Arc::new(send_pkt), port: ingress }; + + common::set_icmp_needed( + switch, + &mut recv_pkt, + ingress, + SERVICE_PORT, + icmp::ICMP6_TIME_EXCEEDED, + icmp::ICMP6_TIME_EXCEED_TRANSIT, + ); + let expected = + TestPacket { packet: Arc::new(recv_pkt), port: SERVICE_PORT }; + + assert!( + one_counted_test(switch, send, Some(expected), "ipv6_ttl_exceeded") + .await? + ); + + Ok(()) } // Arp packets sent to an IP address assigned to a switch port should be 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) { diff --git a/packet/src/icmp.rs b/packet/src/icmp.rs index bd81c1fc..f2e46048 100644 --- a/packet/src/icmp.rs +++ b/packet/src/icmp.rs @@ -69,8 +69,8 @@ pub const ICMP6_DST_UNREACH_BEYONDSCOPE: u8 = 2; pub const ICMP6_DST_UNREACH_ADDR: u8 = 3; pub const ICMP6_DST_UNREACH_NOPORT: u8 = 4; -pub const ICMP6_TIME_EXCEED_TRANSIT: u8 = 1; -pub const ICMP6_TIME_EXCEED_REASSEMBLY: u8 = 2; +pub const ICMP6_TIME_EXCEED_TRANSIT: u8 = 0; +pub const ICMP6_TIME_EXCEED_REASSEMBLY: u8 = 1; pub const ICMP6_PARAMPROB_HEADER: u8 = 0; pub const ICMP6_PARAMPROB_NEXTHEADER: u8 = 1;