[Dash] Update ENI Based Forwarding Orchagent (sonic-net#3905)

What I did

Support FNIC pipeline changes for DashEniFwd Orch (No match on Tunnel VNI and only match on INNER_DST_MAC)
Update DashEniFwdOrch to handle the updated schema for DPU, VDPU, REMOTE_DPU & DASH_ENI_FORWARD_TABLE
Delete the ACL Table if all the ENI acl rules are deleted
While creating Tunnel NH, also pass VNI.
Update Aclorch to accept Tunnel NH in the following format: endpoint_ip@tunnel_name[,vni][,mac]
Simplify the logic by removing the reference counting logic for Remote NH Tunnel tracking
Added REQ_T_STRING_LIST option to Request parser
Allow Relaxed Attribute parsing to Request parser
This is needed because the Request parser expects a strict schema of field-value pairs.

Author: vivekrnv

orchagent/aclorch.cpp

@@ -2,6 +2,7 @@
 #include <limits.h>
 #include <unordered_map>
 #include <algorithm>
+#include <sstream>
 #include "aclorch.h"
 #include "logger.h"
 #include "schema.h"
@@ -641,15 +642,37 @@ void AclRule::TunnelNH::load(const std::string& target)
 
 void AclRule::TunnelNH::parse(const std::string& target)
 {
-    /* Supported Format: endpoint_ip@tunnel_name */
+    /* Expected Format: endpoint_ip@tunnel_name[,vni][,mac] */
     auto at_pos = target.find('@');
     if (at_pos == std::string::npos)
     {
         throw std::logic_error("Invalid format for Tunnel Next Hop");
     }
 
     endpoint_ip = swss::IpAddress(target.substr(0, at_pos));
-    tunnel_name = target.substr(at_pos + 1);
+    std::stringstream ss(target.substr(at_pos + 1));
+
+    vector<string> components;
+    while (ss.good())
+    {
+        std::string substr;
+        getline(ss, substr, ',');
+        components.push_back(substr);
+    }
+    if (components.empty())
+    {
+        throw std::logic_error("Invalid format for Tunnel Next Hop");
+    }
+
+    tunnel_name = components[0];
+    if (components.size() >= 2)
+    {
+        vni = static_cast<uint32_t>(std::stoul(components[1]));
+    }
+    if (components.size() == 3)
+    {
+        mac = swss::MacAddress(components[2]);
+    }
 }
 
 void AclRule::TunnelNH::clear()

orchagent/dash/dashenifwdinfo.cpp

@@ -4,12 +4,6 @@ using namespace swss;
 using namespace std;
 
 const int EniAclRule::BASE_PRIORITY = 9996;
-const vector<string> EniAclRule::RULE_NAMES = {
-    "IN",
-    "OUT",
-    "IN_TERM",
-    "OUT_TERM"
-};
 
 unique_ptr<EniNH> EniNH::createNextHop(dpu_type_t type, const IpAddress& ip)
 {
@@ -55,30 +49,33 @@ void RemoteEniNH::resolve(EniInfo& eni)
         return ;
     }
 
-    if (ctx->handleTunnelNH(tunnel_name_, endpoint_, true))
-    {
-        setStatus(endpoint_status_t::RESOLVED);
-    }
-    else
+    uint64_t vnet_vni;
+    if (!ctx->findVnetVni(vnet, vnet_vni))
     {
+        SWSS_LOG_ERROR("Couldn't find VNI for Vnet %s", vnet.c_str());
         setStatus(endpoint_status_t::UNRESOLVED);
+        return ;
     }
-}
 
-void RemoteEniNH::destroy(EniInfo& eni)
-{
-    auto& ctx = eni.getCtx();
-    ctx->handleTunnelNH(tunnel_name_, endpoint_, false);
+    vni_ = std::to_string(vnet_vni);
+
+    /* Note: AclOrch already has logic to create / delete Tunnel NH, no need to create here */
+    setStatus(endpoint_status_t::RESOLVED);
 }
 
 string RemoteEniNH::getRedirectVal() 
 { 
-    return endpoint_.to_string() + "@" + tunnel_name_; 
+    /* Format Expected by AclOrch: endpoint_ip@tunnel_name[,vni][,mac] */
+    return endpoint_.to_string() + "@" + tunnel_name_ + ',' + vni_;
 }
 
 void EniAclRule::setKey(EniInfo& eni)
 {
-    name_ = string(ENI_REDIRECT_TABLE) + ":" + eni.toKey() + "_" + EniAclRule::RULE_NAMES[type_];
+    name_ = string(DashEniFwd::TABLE) + ":" + eni.toKey();
+    if (type_ == rule_type_t::TUNNEL_TERM)
+    {
+        name_ += "_TERM";
+    }
 }
 
 update_type_t EniAclRule::processUpdate(EniInfo& eni)
@@ -88,9 +85,9 @@ update_type_t EniAclRule::processUpdate(EniInfo& eni)
     IpAddress primary_endp;
     dpu_type_t primary_type = LOCAL;
     update_type_t update_type = PRIMARY_UPDATE;
-    uint64_t primary_id;
+    std::string primary_id;
 
-    if (type_ == rule_type_t::INBOUND_TERM || type_ == rule_type_t::OUTBOUND_TERM)
+    if (type_ == rule_type_t::TUNNEL_TERM)
     {
         /* Tunnel term entries always use local endpoint regardless of primary id */
         if (!eni.findLocalEp(primary_id))
@@ -106,7 +103,7 @@ update_type_t EniAclRule::processUpdate(EniInfo& eni)
 
     if (!ctx->dpu_info.getType(primary_id, primary_type))
     {
-        SWSS_LOG_ERROR("No primaryId in DPU Table %" PRIu64 "", primary_id);
+        SWSS_LOG_ERROR("No primary id %s in DPU Table", primary_id.c_str());
         return update_type_t::INVALID;
     }
 
@@ -182,9 +179,8 @@ void EniAclRule::fire(EniInfo& eni)
             Delete the complete rule before updating it, 
             ACLOrch Doesn't support incremental updates 
         */
-        ctx->rule_table->del(key);
+        ctx->deleteAclRule(key);
         setState(rule_state_t::UNINSTALLED);
-        SWSS_LOG_NOTICE("EniFwd ACL Rule %s deleted", key.c_str());
     }
 
     if (nh_->getStatus() != endpoint_status_t::RESOLVED)
@@ -201,27 +197,17 @@ void EniAclRule::fire(EniInfo& eni)
         { ACTION_REDIRECT_ACTION, nh_->getRedirectVal() }
     };
 
-    if (type_ == rule_type_t::INBOUND_TERM || type_ == rule_type_t::OUTBOUND_TERM)
+    if (type_ == rule_type_t::TUNNEL_TERM)
     {
         fv_.push_back({MATCH_TUNNEL_TERM, "true"});
     }
-
-    if (type_ == rule_type_t::OUTBOUND || type_ == rule_type_t::OUTBOUND_TERM)
-    {
-        fv_.push_back({MATCH_TUNNEL_VNI, to_string(eni.getOutVni())});
-    }
 
-    ctx->rule_table->set(key, fv_);
+    ctx->createAclRule(key, fv_);
     setState(INSTALLED);
-    SWSS_LOG_NOTICE("EniFwd ACL Rule %s installed", key.c_str());
 }
 
 string EniAclRule::getMacMatchDirection(EniInfo& eni)
 {
-    if (type_ == OUTBOUND || type_ == OUTBOUND_TERM)
-    {
-        return eni.getOutMacLookup();
-    }
     return MATCH_INNER_DST_MAC;
 }
 
@@ -231,7 +217,7 @@ void EniAclRule::destroy(EniInfo& eni)
     {
         auto key = getKey();
         auto& ctx = eni.getCtx();
-        ctx->rule_table->del(key);
+        ctx->deleteAclRule(key);
         if (nh_ != nullptr)
         {
             nh_->destroy(eni);
@@ -292,10 +278,8 @@ bool EniInfo::create(const Request& db_request)
     SWSS_LOG_ENTER();
 
     auto updates = db_request.getAttrFieldNames();
-    auto itr_ep_list = updates.find(ENI_FWD_VDPU_IDS);
-    auto itr_primary_id = updates.find(ENI_FWD_PRIMARY);
-    auto itr_out_vni = updates.find(ENI_FWD_OUT_VNI);
-    auto itr_out_mac_dir = updates.find(ENI_FWD_OUT_MAC_LOOKUP);
+    auto itr_ep_list = updates.find(DashEniFwd::VDPU_IDS);
+    auto itr_primary_id = updates.find(DashEniFwd::PRIMARY);
 
     /* Validation Checks */
     if (itr_ep_list == updates.end() || itr_primary_id == updates.end())
@@ -304,85 +288,26 @@ bool EniInfo::create(const Request& db_request)
         return false;
     }
 
-    ep_list_ = db_request.getAttrUintList(ENI_FWD_VDPU_IDS);
-    primary_id_ = db_request.getAttrUint(ENI_FWD_PRIMARY);
+    ep_list_ = db_request.getAttrStringList(DashEniFwd::VDPU_IDS);
+    primary_id_ = db_request.getAttrString(DashEniFwd::PRIMARY);
 
-    uint64_t local_id;
+    std::string local_id;
     bool tunn_term_allow = findLocalEp(local_id);
-    bool outbound_allow = false;
 
     /* Create Rules */
     rule_container_.emplace(piecewise_construct,
-                forward_as_tuple(rule_type_t::INBOUND),
-                forward_as_tuple(rule_type_t::INBOUND, *this));
-    rule_container_.emplace(piecewise_construct,
-                forward_as_tuple(rule_type_t::OUTBOUND),
-                forward_as_tuple(rule_type_t::OUTBOUND, *this));
+                forward_as_tuple(rule_type_t::NO_TUNNEL_TERM),
+                forward_as_tuple(rule_type_t::NO_TUNNEL_TERM, *this));
 
     if (tunn_term_allow)
     {
-        /* Create rules for tunnel termination if required */
-        rule_container_.emplace(piecewise_construct,
-                    forward_as_tuple(rule_type_t::INBOUND_TERM),
-                    forward_as_tuple(rule_type_t::INBOUND_TERM, *this));
+        /* Create rule for tunnel termination if required */
         rule_container_.emplace(piecewise_construct,
-                    forward_as_tuple(rule_type_t::OUTBOUND_TERM),
-                    forward_as_tuple(rule_type_t::OUTBOUND_TERM, *this));
-    }
-
-    /* Infer Direction to check MAC for outbound rules */
-    if (itr_out_mac_dir == updates.end())
-    {
-        outbound_mac_lookup_ = MATCH_INNER_SRC_MAC;
-    }
-    else
-    {
-        auto str = db_request.getAttrString(ENI_FWD_OUT_MAC_LOOKUP);
-        if (str == OUT_MAC_DIR)
-        {
-            outbound_mac_lookup_ = MATCH_INNER_DST_MAC;
-        }
-        else
-        {
-            outbound_mac_lookup_ = MATCH_INNER_SRC_MAC;
-        }
-    }
-
-    /* Infer tunnel_vni for the outbound rules */
-    if (itr_out_vni == updates.end())
-    {
-        if (ctx->findVnetVni(vnet_name_, outbound_vni_))
-        {
-            outbound_allow = true;
-        }
-        else
-        {
-            SWSS_LOG_ERROR("Invalid VNET: No VNI. Cannot install outbound rules: %s", toKey().c_str());
-        }
-    }
-    else
-    {
-        outbound_vni_ = db_request.getAttrUint(ENI_FWD_OUT_VNI);
-        outbound_allow = true;
-    }
-
-    fireRule(rule_type_t::INBOUND);
-
-    if (tunn_term_allow)
-    {
-        fireRule(rule_type_t::INBOUND_TERM);
-    }
-
-    if (outbound_allow)
-    {
-        fireRule(rule_type_t::OUTBOUND);
-    }
-
-    if (tunn_term_allow && outbound_allow)
-    {
-        fireRule(rule_type_t::OUTBOUND_TERM);
+                    forward_as_tuple(rule_type_t::TUNNEL_TERM),
+                    forward_as_tuple(rule_type_t::TUNNEL_TERM, *this));
     }
 
+    fireAllRules();
     return true;
 }
 
@@ -408,34 +333,34 @@ bool EniInfo::update(const Request& db_request)
 
     /* Only primary_id is expected to change after ENI is created */
     auto updates = db_request.getAttrFieldNames();
-    auto itr_primary_id = updates.find(ENI_FWD_PRIMARY);
+    auto itr_primary_id = updates.find(DashEniFwd::PRIMARY);
 
     /* Validation Checks */
     if (itr_primary_id == updates.end())
     {
         throw logic_error("Invalid DASH_ENI_FORWARD_TABLE update: No primary idx");
     }
 
-    if (getPrimaryId() == db_request.getAttrUint(ENI_FWD_PRIMARY))
+    if (getPrimaryId() == db_request.getAttrString(DashEniFwd::PRIMARY))
     {
         /* No update in the primary id, return true */
         return true;
     }
 
     /* Update local primary id and fire the rules */
-    primary_id_ = db_request.getAttrUint(ENI_FWD_PRIMARY);
+    primary_id_ = db_request.getAttrString(DashEniFwd::PRIMARY);
     fireAllRules();
 
     return true;
 }
 
-bool EniInfo::findLocalEp(uint64_t& local_endpoint) const
+bool EniInfo::findLocalEp(std::string& local_endpoint) const
 {
     /* Check if atleast one of the endpoints is local */
     bool found = false;
     for (auto idx : ep_list_)
     {   
-        dpu_type_t val = dpu_type_t::EXTERNAL;
+        dpu_type_t val = dpu_type_t::CLUSTER;
         if (ctx->dpu_info.getType(idx, val) && val == dpu_type_t::LOCAL)
         {
             if (!found)
@@ -445,8 +370,8 @@ bool EniInfo::findLocalEp(uint64_t& local_endpoint) const
             }
             else
             {
-                SWSS_LOG_WARN("Multiple Local Endpoints for the ENI %s found, proceeding with %" PRIu64 ""  ,
-                                mac_.to_string().c_str(), local_endpoint);
+                SWSS_LOG_WARN("Multiple Local Endpoints for the ENI %s found, proceeding with %s",
+                                mac_.to_string().c_str(), local_endpoint.c_str());
             }
         }
     }