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..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 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) {