Count ICMP-needed drops from L3Router

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

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<u64> {
-    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,

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.