Count ICMP-needed drops from L3Router

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<TestPacket>,
     counter: &str,
 ) -> anyhow::Result<bool> {
-    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<bool> {
+    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();

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

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.

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