[dhcpmon] Validate downstream DHCPv4 reply fan-out

src/dhcp_check_profile_relay.cpp

@@ -62,14 +62,14 @@ dhcp_check_profile_t dhcp_check_profile_first_relay_rx = {
 };
 
 // DHCP messages sent to client
-// Relay sends reply packets to client with broadcast ip, and giaddr remains the first relay identifier.
+// Relay replies may use broadcast or unicast IP based on the client's broadcast flag, not a relay decision.
+// SONiC relay supports broadcast only while ISC supports both; giaddr remains the first relay identifier.
 // In single-ToR, giaddr_ip and vlan_ip are the same downstream VLAN SVI address. In dualtor, giaddr_ip is
 // Loopback0 for the server-facing relay identity, but client-facing replies are still sent from the downstream
 // VLAN SVI. Therefore src ip should be vlan_ip, while giaddr should remain giaddr_ip.
 static dhcp_msg_check_profile_t tx_first_relay_reply = {
     {DHCP_CHECK_INTF_TYPE, (const void *)(new std::vector<dhcp_device_intf_t>{DHCP_DEVICE_INTF_TYPE_DOWNLINK, DHCP_DEVICE_INTF_TYPE_MGMT})},
     {DHCP_CHECK_SRC_IP, (const void *)(new std::vector<const in_addr *>{&vlan_ip})},
-    {DHCP_CHECK_DST_IP, (const void *)(new std::vector<const in_addr *>{&broadcast_ip})},
     {DHCP_CHECK_GIADDR, (const void *)(new std::vector<const in_addr *>{&giaddr_ip})},
 };
 

src/dhcp_device.h

@@ -135,15 +135,11 @@ typedef enum
     DHCP_DEVICE_CHECK_NEGATIVE,            /** Presence of relayed DHCP packets activity is flagged as unhealthy state */
     DHCP_DEVICE_CHECK_POSITIVE,            /** Validate that received DORA packets are relayed */
     DHCP_DEVICE_CHECK_NEGATIVE_V6,         /** Presence of relayed DHCPv6 packets activity is flagged as unhealthy state */
-    DHCP_DEVICE_CHECK_POSITIVE_V6,         /** Validate that received SARR packets are relayed */
-    DHCP_DEVICE_CHECK_AGG_EQUAL_RX,        /** Validate that aggregate device rx counters equal sum of member interfaces rx counters */
-    DHCP_DEVICE_CHECK_AGG_EQUAL_TX,        /** Validate that aggregate device tx counters equal sum of member interfaces tx counters */
-    DHCP_DEVICE_CHECK_AGG_EQUAL_RX_V6,     /** Validate that aggregate device rx counters equal sum of member interfaces rx counters for IPv6 */
-    DHCP_DEVICE_CHECK_AGG_EQUAL_TX_V6,     /** Validate that aggregate device tx counters equal sum of member interfaces tx counters for IPv6 */
-    DHCP_DEVICE_CHECK_AGG_MULTIPLE_RX,     /** Validate that aggregate device rx counters are multiple of member interfaces rx counters */
-    DHCP_DEVICE_CHECK_AGG_MULTIPLE_TX,     /** Validate that aggregate device tx counters are multiple of member interfaces tx counters */
-    DHCP_DEVICE_CHECK_AGG_MULTIPLE_RX_V6,  /** Validate that aggregate device rx counters are multiple of member interfaces rx counters for IPv6 */
-    DHCP_DEVICE_CHECK_AGG_MULTIPLE_TX_V6   /** Validate that aggregate device tx counters are multiple of member interfaces tx counters for IPv6 */
+    DHCP_DEVICE_CHECK_POSITIVE_V6,         /** Validate SARR and DHCPv6 relay-wrapper transformations */
+    DHCP_DEVICE_CHECK_AGG_RX,              /** Compare IPv4 RX on a parent interface with its member aggregate */
+    DHCP_DEVICE_CHECK_AGG_TX,              /** Compare IPv4 TX on a parent interface with its member aggregate */
+    DHCP_DEVICE_CHECK_AGG_RX_V6,           /** Compare IPv6 RX on a parent interface with its member aggregate */
+    DHCP_DEVICE_CHECK_AGG_TX_V6            /** Compare IPv6 TX on a parent interface with its member aggregate */
 } dhcp_device_check_t;
 
 /** Monitored DHCP message type */

src/dhcp_devman.cpp

@@ -180,22 +180,9 @@ int dhcp_devman_setup_dual_tor_mode(const char *name)
 
 bool dhcp_devman_is_tracked_interface(const std::string &ifname)
 {
-    auto itr = intfs.find(ifname);
-    if (itr != intfs.end()) {
-        return true;
-    }
-    auto vlan_itr = vlan_map.find(ifname);
-    if (vlan_itr != vlan_map.end()) {
-        return true;
-    }
-    auto portchan_itr = portchan_map.find(ifname);
-    if (portchan_itr != portchan_map.end()) {
-        return true;
-    }
-    if (ifname == mgmt_ifname) {
-        return true;
-    }
-    return false;
+    return intfs.find(ifname) != intfs.end() ||
+           !dhcp_devman_get_parent_ifname(ifname).empty() ||
+           ifname == mgmt_ifname;
 }
 
 /**
@@ -248,7 +235,10 @@ static void update_portchannel_mapping()
         auto second = key.find_last_of('|');
         auto portchannel = key.substr(first + 1, second - first - 1);
         auto ifname = key.substr(second + 1);
-        if (intfs.find(portchannel) == intfs.end()) {
+        bool portchannel_is_context = intfs.find(portchannel) != intfs.end();
+        bool portchannel_is_vlan_member = vlan_map.find(portchannel) != vlan_map.end();
+        // Dual-ToR downlink counters require MUX attribution that is unavailable on a nested PortChannel.
+        if (!portchannel_is_context && (!portchannel_is_vlan_member || dual_tor_mode)) {
             all_skipped_ifname += "<" + ifname + ", " + portchannel + ">, ";
             continue;
         }
@@ -296,7 +286,7 @@ int dhcp_devman_init()
     agg_dev_all = "Agg-" + downstream_ifname;
     agg_dev_prefix = agg_dev_all + "-";
 
-    // vlan and its members, portchannel and its members are initialized regardless of whether they are in cmdline
+    // PortChannel members depend on VLAN mappings to recognize a PortChannel under a monitored VLAN.
     update_vlan_mapping();
     update_portchannel_mapping();
 
@@ -315,21 +305,59 @@ void dhcp_devman_free()
     intfs.clear();
 }
 
-const dhcp_device_context_t *dhcp_devman_get_device_context(const std::string &ifname)
+static constexpr unsigned int MAX_CONTEXT_DEPTH = 3;
+
+std::string dhcp_devman_get_parent_ifname(const std::string &ifname)
 {
-    const auto iter = intfs.find(ifname);
-    if (iter != intfs.end()) {
-        return iter->second;
+    if (intfs.find(ifname) != intfs.end()) {
+        return "";
     }
     const auto vlan = vlan_map.find(ifname);
     if (vlan != vlan_map.end() && ifname != vlan->second) {
-        return dhcp_devman_get_device_context(vlan->second);
+        return vlan->second;
     }
     const auto port_channel = portchan_map.find(ifname);
     if (port_channel != portchan_map.end() && ifname != port_channel->second) {
-        return dhcp_devman_get_device_context(port_channel->second);
+        return port_channel->second;
     }
-    return NULL;
+    return "";
+}
+
+/**
+ * @code get_device_context(ifname, depth);
+ *
+ * @brief Follow parent mappings until reaching a tracked input interface
+ *
+ * @param ifname    Interface name to resolve
+ * @param depth     Current parent traversal depth
+ *
+ * @return Tracked interface context, or NULL when no context is found
+ */
+static const dhcp_device_context_t *get_device_context(
+    const std::string &ifname, unsigned int depth)
+{
+    if (depth > MAX_CONTEXT_DEPTH) {
+        syslog_debug(LOG_WARNING, "Exceeded interface membership depth at %s", ifname.c_str());
+        return NULL;
+    }
+    const auto iter = intfs.find(ifname);
+    if (iter != intfs.end()) {
+        return iter->second;
+    }
+    const std::string parent_ifname = dhcp_devman_get_parent_ifname(ifname);
+    return parent_ifname.empty() ? NULL : get_device_context(parent_ifname, depth + 1);
+}
+
+const dhcp_device_context_t *dhcp_devman_get_device_context(const std::string &ifname)
+{
+    return get_device_context(ifname, 0);
+}
+
+std::string dhcp_devman_get_agg_counter_ifname(const std::string &ifname)
+{
+    const std::string parent_ifname = dhcp_devman_get_parent_ifname(ifname);
+    return parent_ifname.empty() ? agg_dev_all :
+           get_agg_counter_ifname(parent_ifname);
 }
 
 void dhcp_devman_print_all_status(dhcp_counters_type_t type)

src/dhcp_devman.h

@@ -127,17 +127,39 @@ int dhcp_devman_init();
  */
 void dhcp_devman_free();
 
+/**
+ * @code dhcp_devman_get_parent_ifname(ifname);
+ *
+ * @brief find the immediate parent interface of a tracked interface.
+ *
+ * @param ifname           interface name
+ *
+ * @return Immediate parent interface name, or an empty string when the interface is a tracked root or is unmapped
+ */
+std::string dhcp_devman_get_parent_ifname(const std::string &ifname);
+
 /**
  * @code dhcp_devman_get_device_context(ifname);
  *
- * @brief find device context, if its physical interface, will query vlan_map and portchannel_map first
+ * @brief find the tracked input interface that owns an interface.
  *
  * @param ifname           interface name
  *
- * @return pointer to device (interface) context if found, NULL otherwise
+ * @return The interface's tracked context; a tracked input interface returns its own context
  */
 const dhcp_device_context_t* dhcp_devman_get_device_context(const std::string &ifname);
 
+/**
+ * @code dhcp_devman_get_agg_counter_ifname(ifname);
+ *
+ * @brief find the aggregate counter updated by an interface observation.
+ *
+ * @param ifname           interface name
+ *
+ * @return immediate-parent aggregate, or the root aggregate when no parent exists
+ */
+std::string dhcp_devman_get_agg_counter_ifname(const std::string &ifname);
+
 /**
  * @code dhcp_devman_print_all_status(type);
  *

src/dhcp_mon.cpp

@@ -81,7 +81,7 @@ static void recalculate_agg_counter(all_counters_t &all_counters)
             if (mgmt_ifname == context->intf) {
                 continue;
             }
-            counter_t &agg_counter = all_counters.at(get_agg_counter_ifname(ifname, context->intf));
+            counter_t &agg_counter = all_counters.at(dhcp_devman_get_agg_counter_ifname(ifname));
             for (const auto &[msg_type, count] : counter) {
                 agg_counter[msg_type] += count;
             }
@@ -125,21 +125,21 @@ static bool db_counters_initialized(const std::string &ifname)
 
     table_name = construct_counter_db_table_key(ifname, false);
     auto rx_v4 = mCountersDbPtr->hget(table_name, "RX");
-    if (rx_v4 == nullptr || rx_v4->empty()) {
+    if (rx_v4 == NULL || rx_v4->empty()) {
         return false;
     }
     auto tx_v4 = mCountersDbPtr->hget(table_name, "TX");
-    if (tx_v4 == nullptr || tx_v4->empty()) {
+    if (tx_v4 == NULL || tx_v4->empty()) {
         return false;
     }
 
     table_name = construct_counter_db_table_key(ifname, true);
     auto rx_v6 = mCountersDbPtr->hget(table_name, "RX");
-    if (rx_v6 == nullptr || rx_v6->empty()) {
+    if (rx_v6 == NULL || rx_v6->empty()) {
         return false;
     }
     auto tx_v6 = mCountersDbPtr->hget(table_name, "TX");
-    if (tx_v6 == nullptr || tx_v6->empty()) {
+    if (tx_v6 == NULL || tx_v6->empty()) {
         return false;
     }
 
@@ -466,15 +466,19 @@ static void initialize_all_intf_counters()
             initialize_all_counters(ifname);
         }
         initialize_all_counters(vlan);
-        sock_mgr_init_cache_counters(agg_dev_prefix + vlan, DHCP_MESSAGE_TYPE_COUNT, DHCPV6_MESSAGE_TYPE_COUNT);
+        sock_mgr_init_cache_counters(
+            get_agg_counter_ifname(vlan),
+            DHCP_MESSAGE_TYPE_COUNT, DHCPV6_MESSAGE_TYPE_COUNT);
     }
 
     for (const auto &[portchan, intfs] : rev_portchan_map) {
         for (const auto &ifname : intfs) {
             initialize_all_counters(ifname);
         }
         initialize_all_counters(portchan);
-        sock_mgr_init_cache_counters(agg_dev_prefix + portchan, DHCP_MESSAGE_TYPE_COUNT, DHCPV6_MESSAGE_TYPE_COUNT);
+        sock_mgr_init_cache_counters(
+            get_agg_counter_ifname(portchan),
+            DHCP_MESSAGE_TYPE_COUNT, DHCPV6_MESSAGE_TYPE_COUNT);
     }
 
     // Now all vlan and portchannel related interfaces have entries in counters, now do the rest (uplink)
@@ -520,6 +524,7 @@ int dhcp_mon_init(size_t snaplen, int window_sec, int max_count, int db_update_i
     // deinitialization of counters is not our responsibility
     // cache counter will be cleanup by sock_mgr_free and the initialized db we intend to keep
     initialize_all_intf_counters();
+    initialize_dhcp_relay_health();
     syslog(LOG_INFO, "Initialized all counters for tracked interfaces");
 
     window_interval_sec = window_sec;