[ACL] Match TCP protocol while matching TCP_FLAG (sonic-net#1854)

* Match TCP protocol while matching TCP_FLAG

Signed-off-by: bingwang <[email protected]>

Author: bingwang-ms

orchagent/aclorch.cpp

@@ -35,6 +35,8 @@ extern CrmOrch *gCrmOrch;
 #define MIN_VLAN_ID 1    // 0 is a reserved VLAN ID
 #define MAX_VLAN_ID 4095 // 4096 is a reserved VLAN ID
 
+const int TCP_PROTOCOL_NUM = 6; // TCP protocol number
+
 acl_rule_attr_lookup_t aclMatchLookup =
 {
     { MATCH_IN_PORTS,          SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS },
@@ -645,7 +647,7 @@ void AclRule::updateInPorts()
     attr.id = SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS;
     attr.value = m_matches[SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS];
     attr.value.aclfield.enable = true;
-    
+
     status = sai_acl_api->set_acl_entry_attribute(m_ruleOid, &attr);
     if (status != SAI_STATUS_SUCCESS)
     {
@@ -1378,14 +1380,14 @@ bool AclTable::create()
         attr.id = SAI_ACL_TABLE_ATTR_ACL_STAGE;
         attr.value.s32 = (stage == ACL_STAGE_INGRESS) ? SAI_ACL_STAGE_INGRESS : SAI_ACL_STAGE_EGRESS;
         table_attrs.push_back(attr);
-        
+
         if (stage == ACL_STAGE_INGRESS)
         {
             attr.id = SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS;
             attr.value.booldata = true;
             table_attrs.push_back(attr);
         }
-        
+
         sai_status_t status = sai_acl_api->create_acl_table(&m_oid, gSwitchId, (uint32_t)table_attrs.size(), table_attrs.data());
 
         if (status == SAI_STATUS_SUCCESS)
@@ -2985,11 +2987,11 @@ AclRule* AclOrch::getAclRule(string table_id, string rule_id)
 bool AclOrch::updateAclRule(string table_id, string rule_id, string attr_name, void *data, bool oper)
 {
     SWSS_LOG_ENTER();
-    
+
     sai_object_id_t table_oid = getTableById(table_id);
     string attr_value;
 
-    if (table_oid == SAI_NULL_OBJECT_ID) 
+    if (table_oid == SAI_NULL_OBJECT_ID)
     {
         SWSS_LOG_ERROR("Failed to update ACL rule in ACL table %s. Table doesn't exist", table_id.c_str());
         return false;
@@ -3002,29 +3004,29 @@ bool AclOrch::updateAclRule(string table_id, string rule_id, string attr_name, v
         return false;
     }
 
-    switch (aclMatchLookup[attr_name]) 
+    switch (aclMatchLookup[attr_name])
     {
         case SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS:
         {
             sai_object_id_t port_oid = *(sai_object_id_t *)data;
             vector<sai_object_id_t> in_ports = rule_it->second->getInPorts();
 
-            if (oper == RULE_OPER_ADD) 
+            if (oper == RULE_OPER_ADD)
             {
                 in_ports.push_back(port_oid);
-            } 
-            else 
+            }
+            else
             {
                 for (auto port_iter = in_ports.begin(); port_iter != in_ports.end(); port_iter++)
                 {
-                    if (*port_iter == port_oid) 
+                    if (*port_iter == port_oid)
                     {
                         in_ports.erase(port_iter);
                         break;
                     }
                 }
             }
-            
+
             for (const auto& port_iter: in_ports)
             {
                 Port p;
@@ -3277,14 +3279,22 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
                 it = consumer.m_toSync.erase(it);
                 return;
             }
-
+            bool bHasTCPFlag = false;
+            bool bHasIPProtocol = false;
             for (const auto& itr : kfvFieldsValues(t))
             {
                 string attr_name = to_upper(fvField(itr));
                 string attr_value = fvValue(itr);
 
                 SWSS_LOG_INFO("ATTRIBUTE: %s %s", attr_name.c_str(), attr_value.c_str());
-
+                if (attr_name == MATCH_TCP_FLAGS)
+                {
+                    bHasTCPFlag = true;
+                }
+                if (attr_name == MATCH_IP_PROTOCOL || attr_name == MATCH_NEXT_HEADER)
+                {
+                    bHasIPProtocol = true;
+                }
                 if (newRule->validateAddPriority(attr_name, attr_value))
                 {
                     SWSS_LOG_INFO("Added priority attribute");
@@ -3304,6 +3314,30 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
                     break;
                 }
             }
+            // If acl rule is to match TCP_FLAGS, and IP_PROTOCOL(NEXT_HEADER) is not set
+            // we set IP_PROTOCOL(NEXT_HEADER) to 6 to match TCP explicitly
+            if (bHasTCPFlag && !bHasIPProtocol)
+            {
+                string attr_name;
+                if (type == ACL_TABLE_MIRRORV6 || type == ACL_TABLE_L3V6)
+                {
+                    attr_name = MATCH_NEXT_HEADER;
+                }
+                else
+                {
+                    attr_name = MATCH_IP_PROTOCOL;
+
+                }
+                string attr_value = std::to_string(TCP_PROTOCOL_NUM);
+                if (newRule->validateAddMatch(attr_name, attr_value))
+                {
+                    SWSS_LOG_INFO("Automatically added match attribute '%s : %s'", attr_name.c_str(), attr_value.c_str());
+                }
+                else
+                {
+                    SWSS_LOG_ERROR("Failed to add attribute '%s : %s'", attr_name.c_str(), attr_value.c_str());
+                }
+            }
 
             // validate and create ACL rule
             if (bAllAttributesOk && newRule->validate())

tests/test_acl.py

@@ -88,6 +88,23 @@ def test_AclRuleIpProtocol(self, dvs_acl, l3_acl_table):
         dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME)
         dvs_acl.verify_no_acl_rules()
 
+    def test_AclRuleTCPProtocolAppendedForTCPFlags(self, dvs_acl, l3_acl_table):
+        """
+        Verify TCP Protocol number (6) will be appended for ACL rules matching TCP_FLAGS
+        """
+        config_qualifiers = {"TCP_FLAGS": "0x07/0x3f"}
+        expected_sai_qualifiers = {
+            "SAI_ACL_ENTRY_ATTR_FIELD_TCP_FLAGS":
+                dvs_acl.get_simple_qualifier_comparator("7&mask:0x3f"),
+            "SAI_ACL_ENTRY_ATTR_FIELD_IP_PROTOCOL":
+                dvs_acl.get_simple_qualifier_comparator("6&mask:0xff")
+        }
+        dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers)
+        dvs_acl.verify_acl_rule(expected_sai_qualifiers)
+
+        dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME)
+        dvs_acl.verify_no_acl_rules()
+
     def test_AclRuleNextHeader(self, dvs_acl, l3_acl_table):
         config_qualifiers = {"NEXT_HEADER": "6"}
 
@@ -98,6 +115,24 @@ def test_AclRuleNextHeader(self, dvs_acl, l3_acl_table):
         dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME)
         dvs_acl.verify_no_acl_rules()
 
+    def test_V6AclRuleNextHeaderAppendedForTCPFlags(self, dvs_acl, l3v6_acl_table):
+        """
+        Verify next heder (6) will be appended for IPv6 ACL rules matching TCP_FLAGS
+        """
+        config_qualifiers = {"TCP_FLAGS": "0x07/0x3f"}
+        expected_sai_qualifiers = {
+            "SAI_ACL_ENTRY_ATTR_FIELD_TCP_FLAGS":
+                dvs_acl.get_simple_qualifier_comparator("7&mask:0x3f"),
+            "SAI_ACL_ENTRY_ATTR_FIELD_IPV6_NEXT_HEADER":
+                dvs_acl.get_simple_qualifier_comparator("6&mask:0xff")
+        }
+
+        dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers)
+        dvs_acl.verify_acl_rule(expected_sai_qualifiers)
+
+        dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME)
+        dvs_acl.verify_no_acl_rules()
+
     def test_AclRuleInOutPorts(self, dvs_acl, l3_acl_table):
         config_qualifiers = {
             "IN_PORTS": "Ethernet0,Ethernet4",
@@ -263,19 +298,6 @@ def test_V6AclRuleL4DstPort(self, dvs_acl, l3v6_acl_table):
         dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME)
         dvs_acl.verify_no_acl_rules()
 
-    def test_V6AclRuleTCPFlags(self, dvs_acl, l3v6_acl_table):
-        config_qualifiers = {"TCP_FLAGS": "0x07/0x3f"}
-        expected_sai_qualifiers = {
-            "SAI_ACL_ENTRY_ATTR_FIELD_TCP_FLAGS":
-                dvs_acl.get_simple_qualifier_comparator("7&mask:0x3f")
-        }
-
-        dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers)
-        dvs_acl.verify_acl_rule(expected_sai_qualifiers)
-
-        dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME)
-        dvs_acl.verify_no_acl_rules()
-
     def test_V6AclRuleL4SrcPortRange(self, dvs_acl, l3v6_acl_table):
         config_qualifiers = {"L4_SRC_PORT_RANGE": "1-100"}
         expected_sai_qualifiers = {