Add tunterm acl plugin support. #122

dypet · 2024-10-15T20:51:50Z

Overview:

These changes add support for the recently added tunterm_acl vpp plugin (#114) in sonic-vpp.

Changes:

Added API calls to the new plugin in SaiVppXlate.c, and added logic to SwitchStateBaseAcl.cpp so that the new plugin is called with the appropriate fields instead of the regular ACL plugin when SAI_ACL_ENTRY_ATTR_FIELD_TUNNEL_TERMINATED is received. Ports are recorded in m_acl_tbl_hw_ports_map at table creation so that they may be passed to the tunterm_acl plugin and bound by the new plugin the first time an ACE with SAI_ACL_ENTRY_ATTR_FIELD_TUNNEL_TERMINATED is configured. tunterm_acl ACLs are tracked in m_tunterm_acl_swindex_map similarly to m_acl_swindex_map for a given tbl_oid.

linux-foundation-easycla · 2024-10-15T20:51:54Z

The committers listed above are authorized under a signed CLA.

✅ login: dypet (406757d, 117d0bf, dd40807)

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

platform/saivpp/vpplib/vppxlate/SaiVppXlate.c

AkeelAli

If not already done, can you please re-run the basic ACL redirect tests with the latest plugin (as some changes were made before it was merged). You can wait until the final version of this review to do that.

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

AkeelAli · 2024-10-17T22:42:07Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

 	    ret = vpp_acl_interface_unbind(hwif_name.c_str(), acl_swindex, is_input);
-
+        if(ret == 0) {
+            hwif_list.remove(hwif_name);


In the unbind case, wouldn't hwif_list always be empty?

AkeelAli · 2024-10-17T22:43:01Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

@@ -1290,6 +1639,11 @@ sai_status_t SwitchStateBase::aclBindUnbindPorts(
 			is_bind ? "bound to": "unbound from", hwif_name.c_str());
    }

+    for(auto hwif: hwif_list) {


Does this correctly update the ports_map in the unbind case?

Moved the ports_map unbind update above directly in the unbind case

AkeelAli · 2024-10-17T22:45:44Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

+        for (uint32_t i = 0; i < p_ace->attrs_count; i++) {
+            attr = &p_ace->attrs[i];
+            if(attr->id == SAI_ACL_ENTRY_ATTR_FIELD_TUNNEL_TERMINATED) {
+                if(attr->value.aclfield.data.booldata == true) {


Collapse these into a single if statement as they are part of the same check.

In this case want to still break out of the loop when SAI_ACL_ENTRY_ATTR_FIELD_TUNNEL_TERMINATED is false - updated the same check later on to be in a single 'if' statement.

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

platform/saivpp/vpplib/vppxlate/SaiVppXlate.h

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

AkeelAli · 2024-10-17T22:54:24Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

-    } else {
-	acl_swindex = vpp_idx_it->second;
-	acl_replace = true;
+    if(acl->count > 0) {


This block shouldn't have changed, yet it's marked as a diff here due to indentation changes. Please revert this.

The check for acl->count > 0 was added here to skip this block for the cases where there are only tunterm acl rules and no standard acl rules

yue-fred-gao · 2024-10-22T21:47:02Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

@@ -563,6 +825,30 @@ sai_status_t SwitchStateBase::AclTblConfig(
    ordered_aces.sort(cmp_priority);
    // SWSS_LOG_NOTICE("# aces %u after sort", ordered_aces.size());

+    size_t n_entries = 0;
+    size_t n_tunterm_entries  = 0;
+    for (auto ace: ordered_aces) {


This function is too long. We'd better break it into a few smaller functions. For example, for the part that converting ACE to acl_tbl_entries_t and sort can be one function. From sorted acl_tbl_entries_t to vpp_acl_t and vpp_tunerm_acl_t can be another function.

Conceptually with tunterm feature, each ACL table can have 1 or 2 ACL objects in VPP: one for regular ACL and one for tunterm ACL. In the create/update request, we go through the ACEs and convert them to 2 lists of vpp_acl_t and vpp_tunterm_acl_t (you can create a super data structure including the 2 lists). Then we pass the 2 lists along with table_id to a function. The function reads acl_swindex and tunterm_acl_swindex from table_id and does necessary operation to create/update/delete acl or tunterm acl from vpp.

yue-fred-gao · 2024-10-22T21:51:16Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

+
+    if(update_tunterm_rule) {
+        if(acl->count>0 && (rule<(&acl->rules[0]+n_entries))) {
+            memset(rule, 0, sizeof(*rule)); // clear any fields that were added to regular rule


Can you explain this?

When iterating through the incoming rule's sai attributes we don't know if it's a tunterm acl or regular acl until SAI_ACL_ENTRY_ATTR_FIELD_TUNNEL_TERMINATED is encountered. There are common attr between the two, like SAI_ACL_ENTRY_ATTR_FIELD_DST_IP, which would need to be populated in both cases. Both rules (regular acl and tunterm acl) are updated in parallel while looping through the sai attributes, and then at the end once we know if it's a tunterm rule or not we clear the other rule which potentially had fields populated.

yue-fred-gao · 2024-10-22T21:51:46Z

platform/saivpp/vpplib/SwitchStateBase.h

@@ -1016,6 +1018,12 @@ namespace saivpp
 		_In_ sai_object_id_t tbl_oid,
 		_In_ bool is_add);

+            sai_status_t tbl_hw_ports_map_delete(


Please add some documents to explain these new functions

yue-fred-gao · 2024-10-22T21:53:22Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

+        SWSS_LOG_NOTICE("Tunterm ACL table %s %s status %d", tbl_sid.c_str(),
+		    acl_replace ? "replace" : "add", status);
+    } else {
+        SWSS_LOG_NOTICE("ACL plugin operation failed, skipping tunterm configuration.");;


This should be an error

yue-fred-gao · 2024-10-23T15:50:58Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

+        auto it_hw_ports = m_acl_tbl_hw_ports_map.find(tbl_oid);
+        if (it_hw_ports == m_acl_tbl_hw_ports_map.end()) {
+            auto sid = sai_serialize_object_id(tbl_oid);
+            SWSS_LOG_WARN("Tunterm acl - no ports bound for table id %s", sid.c_str());


Is this a warning? It is normal that an ACL not attached to any ports, right?

yue-fred-gao · 2024-10-23T16:00:23Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

    SWSS_LOG_NOTICE("ACL table %s %s status %d", tbl_sid.c_str(),
 		    acl_replace ? "replace" : "add", status);
+
+    if(status == SAI_STATUS_SUCCESS) {
+        status = tunterm_acl_add_replace(tunterm_acl, tbl_oid);


what will happen if tunterm ACEs removed from ACL? Do you need to call tunterm_acl_delete?

yue-fred-gao · 2024-10-23T17:31:17Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

@@ -778,6 +1081,8 @@ sai_status_t SwitchStateBase::AclTblRemove(
 	m_acl_tbl_rules_map.erase(it);
    }

+    status = tunterm_acl_delete(tbl_oid);
+
    auto vpp_idx_it = m_acl_swindex_map.find(tbl_oid);


if the ACL has tunterm only, will you see below error?

yue-fred-gao · 2024-10-23T18:00:31Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

@@ -563,6 +825,30 @@ sai_status_t SwitchStateBase::AclTblConfig(
    ordered_aces.sort(cmp_priority);
    // SWSS_LOG_NOTICE("# aces %u after sort", ordered_aces.size());

+    size_t n_entries = 0;
+    size_t n_tunterm_entries  = 0;
+    for (auto ace: ordered_aces) {


Conceptually with tunterm feature, each ACL table can have 1 or 2 ACL objects in VPP: one for regular ACL and one for tunterm ACL. In the create/update request, we go through the ACEs and convert them to 2 lists of vpp_acl_t and vpp_tunterm_acl_t (you can create a super data structure including the 2 lists). Then we pass the 2 lists along with table_id to a function. The function reads acl_swindex and tunterm_acl_swindex from table_id and does necessary operation to create/update/delete acl or tunterm acl from vpp.

yue-fred-gao · 2024-10-23T18:09:50Z

platform/saivpp/vpplib/SwitchStateBaseAcl.cpp

+        sin =  &ip_mask->addr.ip4;
+        sin->sin_addr.s_addr = value->aclfield.mask.ip4;
+        vpp_ip_addr_t_to_string(ip_addr, dst_ip_str, INET6_ADDRSTRLEN);
+        SWSS_LOG_NOTICE("Tunterm acl rule has dst IP: %s", dst_ip_str);


Please change all NOTICE to INFO except very few important one like tunterm acl created/modify/deleted successfully. See issue #116 .

yue-fred-gao · 2024-10-23T18:33:22Z

platform/saivpp/vpplib/SwitchStateBase.h

@@ -992,7 +992,9 @@ namespace saivpp

        private:
            std::map<sai_object_id_t, std::list<sai_object_id_t>> m_acl_tbl_rules_map;
+            std::map<sai_object_id_t, std::list<std::string>> m_acl_tbl_hw_ports_map;


There is already m_acl_tbl_grp_ports_map. If we see acl and tunterm_acl as two objects created from the same acl_table, it is possible to share the same mapping. even m_acl_swindex_map and m_tunterm_acl_swindex_map can be combined to one if we use std::pair or std::Array to keep both indexes in one

yue-fred-gao · 2024-10-23T18:36:09Z

platform/saivpp/vpplib/vppxlate/SaiVppXlate.c

+            return -EINVAL;
+        }
+    }
+    mp->context = store_ptr(tunterm_index);


why do we need it in context? Is it already passed in with tunterm_acl_index?

Add tunterm acl plugin support.

dd40807

yue-fred-gao requested review from abdbaig and yue-fred-gao October 17, 2024 15:29

abdbaig reviewed Oct 17, 2024

View reviewed changes

Merge branch 'main' into sonic_vpp_tunterm_acl_support

117d0bf

AkeelAli reviewed Oct 17, 2024

View reviewed changes

yue-fred-gao reviewed Oct 22, 2024

View reviewed changes

Add ip helper functions, fix indentation.

406757d

yue-fred-gao reviewed Oct 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tunterm acl plugin support. #122

Add tunterm acl plugin support. #122

dypet commented Oct 15, 2024

linux-foundation-easycla bot commented Oct 15, 2024 •

edited

Loading

AkeelAli left a comment

AkeelAli Oct 17, 2024

dypet Oct 23, 2024

AkeelAli Oct 17, 2024

dypet Oct 23, 2024 •

edited

Loading

AkeelAli Oct 17, 2024

dypet Oct 23, 2024

AkeelAli Oct 17, 2024

dypet Oct 23, 2024

yue-fred-gao Oct 22, 2024

yue-fred-gao Oct 23, 2024

yue-fred-gao Oct 22, 2024

dypet Oct 23, 2024

yue-fred-gao Oct 22, 2024

yue-fred-gao Oct 22, 2024

yue-fred-gao Oct 23, 2024

yue-fred-gao Oct 23, 2024

yue-fred-gao Oct 23, 2024

yue-fred-gao Oct 23, 2024

yue-fred-gao Oct 23, 2024

yue-fred-gao Oct 23, 2024

yue-fred-gao Oct 23, 2024

Add tunterm acl plugin support. #122

Are you sure you want to change the base?

Add tunterm acl plugin support. #122

Conversation

dypet commented Oct 15, 2024

Overview:

Changes:

linux-foundation-easycla bot commented Oct 15, 2024 • edited Loading

AkeelAli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dypet Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Oct 15, 2024 •

edited

Loading

dypet Oct 23, 2024 •

edited

Loading