From: Marcin Siodelski <marcin@isc.org>
Date: Mon, 15 Apr 2024 16:22:27 +0000 (+0200)
Subject: [#3125] Pkt6 drop statistics not increased
X-Git-Tag: Kea-2.5.8~57
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=408deee875a43c33a9489e4e982c60f42f0b0863;p=thirdparty%2Fkea.git

[#3125] Pkt6 drop statistics not increased

The drop statistics should be maintained by the hook libraries rather than
the server. We had a discrepancy between the DHCPv4 and DHCPv6 server where
the latter increased the drop statistics when the hook library returned
the DROP status for the pkt6-receive or buffer6-receive callout. It
resulted in the increased drop stats for the load balanced packets. It was
not the case for the DHCPv4 server.
---

diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc
index 63ed6a152f..41f8661b82 100644
--- a/src/bin/dhcp4/tests/hooks_unittest.cc
+++ b/src/bin/dhcp4/tests/hooks_unittest.cc
@@ -1324,6 +1324,9 @@ TEST_F(HooksDhcpv4SrvTest, buffer4ReceiveDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(discover);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt4-receive-drop"));
 }
 
 // Checks if callouts installed on pkt4_receive are indeed called and the
@@ -1496,6 +1499,9 @@ TEST_F(HooksDhcpv4SrvTest, pkt4ReceiveDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(discover);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt4-receive-drop"));
 }
 
 // Checks if callouts installed on pkt4_send are indeed called and the
@@ -2030,6 +2036,9 @@ TEST_F(HooksDhcpv4SrvTest, subnet4SelectDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(discover);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt4-receive-drop"));
 }
 
 // This test verifies that the leases4_committed hook point is not triggered
diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 85338837e9..a0facd7923 100644
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -786,9 +786,11 @@ Dhcpv6Srv::processPacket(Pkt6Ptr query) {
                 .arg(query->getLocalAddr().toText())
                 .arg(query->getIface());
 
-            // Increase the statistic of dropped packets.
-            StatsMgr::instance().addValue("pkt6-receive-drop",
-                                          static_cast<int64_t>(1));
+            // Not increasing the statistics of the dropped packets because it
+            // is the callouts' responsibility to increase it. There are some
+            // cases when the callouts may elect to not increase the statistics.
+            // For example, packets dropped by the load-balancing algorithm must
+            // not increase the statistics.
             return (Pkt6Ptr());
         }
 
@@ -900,9 +902,11 @@ Dhcpv6Srv::processPacket(Pkt6Ptr query) {
             (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) {
             LOG_DEBUG(hooks_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_PACKET_RCVD_SKIP)
                 .arg(query->getLabel());
-            // Increase the statistic of dropped packets.
-            StatsMgr::instance().addValue("pkt6-receive-drop",
-                                          static_cast<int64_t>(1));
+            // Not increasing the statistics of the dropped packets because it
+            // is the callouts' responsibility to increase it. There are some
+            // cases when the callouts may elect to not increase the statistics.
+            // For example, packets dropped by the load-balancing algorithm must
+            // not increase the statistics.
             return (Pkt6Ptr());
         }
 
diff --git a/src/bin/dhcp6/tests/hooks_unittest.cc b/src/bin/dhcp6/tests/hooks_unittest.cc
index 7ee5e1f6b6..c093de6c6a 100644
--- a/src/bin/dhcp6/tests/hooks_unittest.cc
+++ b/src/bin/dhcp6/tests/hooks_unittest.cc
@@ -1244,6 +1244,9 @@ TEST_F(HooksDhcpv6SrvTest, buffer6ReceiveDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(sol);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt6-receive-drop"));
 }
 
 // Checks if callouts installed on pkt6_receive are indeed called and the
@@ -1406,6 +1409,9 @@ TEST_F(HooksDhcpv6SrvTest, pkt6ReceiveDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(sol);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt6-receive-drop"));
 }
 
 // Checks if callouts installed on pkt6_send are indeed called and the
@@ -1907,6 +1913,9 @@ TEST_F(HooksDhcpv6SrvTest, subnet6SelectDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(sol);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt6-receive-drop"));
 }
 
 // This test verifies that the leases6_committed hook point is not triggered