[201911]Handled both REDIRECT and REDIRECT_ACTION ACL rules in ACL or… (

sonic-net#1397)

* [201911]Handled both REDIRECT and REDIRECT_ACTION ACL rules in ACL orchagent

* Fixed the LGTM failure in test_acl.py script

Co-authored-by: Madhan Babu <madhan@arc-build-server.mtr.labs.mlnx>

orchagent/aclorch.cpp

@@ -815,8 +815,22 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value)
         // handle PACKET_ACTION_REDIRECT in ACTION_PACKET_ACTION for backward compatibility
         else if (attr_value.find(PACKET_ACTION_REDIRECT) != string::npos)
         {
-            // resize attr_value to remove argument, _attr_value still has the argument
-            attr_value.resize(string(PACKET_ACTION_REDIRECT).length());
+            // check that we have a colon after redirect rule
+            size_t colon_pos = string(PACKET_ACTION_REDIRECT).length();
+
+            if (attr_value.c_str()[colon_pos] != ':')
+            {
+                SWSS_LOG_ERROR("Redirect action rule must have ':' after REDIRECT");
+                return false;
+            }
+
+            if (colon_pos + 1 == attr_value.length())
+            {
+                SWSS_LOG_ERROR("Redirect action rule must have a target after 'REDIRECT:' action");
+                return false;
+            }
+
+            _attr_value = _attr_value.substr(colon_pos+1);
 
             sai_object_id_t param_id = getRedirectObjectId(_attr_value);
             if (param_id == SAI_NULL_OBJECT_ID)
@@ -862,21 +876,7 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value)
 // This method should return sai attribute id of the redirect destination
 sai_object_id_t AclRuleL3::getRedirectObjectId(const string& redirect_value)
 {
-    // check that we have a colon after redirect rule
-    size_t colon_pos = string(PACKET_ACTION_REDIRECT).length();
-    if (redirect_value[colon_pos] != ':')
-    {
-        SWSS_LOG_ERROR("Redirect action rule must have ':' after REDIRECT");
-        return SAI_NULL_OBJECT_ID;
-    }
-
-    if (colon_pos + 1 == redirect_value.length())
-    {
-        SWSS_LOG_ERROR("Redirect action rule must have a target after 'REDIRECT:' action");
-        return SAI_NULL_OBJECT_ID;
-    }
-
-    string target = redirect_value.substr(colon_pos + 1);
+    string target = redirect_value;
 
     // Try to parse physical port and LAG first
     Port port;

tests/test_acl.py

@@ -184,6 +184,17 @@ def verify_acl_rule(self, dvs, field, value):
             else:
                 assert False
 
+    def create_redirect_action_acl_rule(self, table_name, rule_name, qualifiers, intf, priority="2020"):
+        fvs = {
+            "priority": priority,
+            "REDIRECT_ACTION": intf
+        }
+
+        for k, v in qualifiers.items():
+            fvs[k] = v
+
+        self.config_db.create_entry("ACL_RULE", "{}|{}".format(table_name, rule_name), fvs)
+
 
 class TestAcl(BaseTestAcl):
     def test_AclTableCreation(self, dvs, testlog):
@@ -1370,6 +1381,116 @@ def test_AclRuleRedirectToNexthop(self, dvs, testlog):
         # bring down interface
         dvs.set_interface_status("Ethernet4", "down")
 
+    def test_AclRedirectRule(self, dvs):        
+        dvs.setup_db()
+        self.setup_db(dvs)
+
+        # Bring up an IP interface with a neighbor
+        dvs.set_interface_status("Ethernet4", "up")
+        dvs.add_ip_address("Ethernet4", "10.0.0.1/24")
+        dvs.add_neighbor("Ethernet4", "10.0.0.2", "00:01:02:03:04:05")
+
+        atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP")
+        keys = atbl.getKeys()
+        assert len(keys) == 1
+        next_hop_id = keys[0]
+
+        bind_ports = ["Ethernet0", "Ethernet8"]
+        self.create_acl_table("test_acl_table", "L3", bind_ports)
+        acl_table_id = self.get_acl_table_id(dvs)
+
+        # create acl rule
+        tbl = swsscommon.Table(self.cdb, "ACL_RULE")
+        fvs = swsscommon.FieldValuePairs([
+                                        ("priority", "20"),
+                                        ("L4_SRC_PORT", "65000"),
+                                        ("PACKET_ACTION", "REDIRECT:10.0.0.2@Ethernet4")])
+        tbl.set("test_acl_table|redirect_rule", fvs)
+
+        # check acl table in asic db
+        atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY")
+        keys = atbl.getKeys()
+
+        acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries]
+        assert len(acl_entry) == 1
+
+        (status, fvs) = atbl.get(acl_entry[0])
+        assert status == True
+        assert len(fvs) == 6
+        for fv in fvs:
+            if fv[0] == "SAI_ACL_ENTRY_ATTR_TABLE_ID":
+                assert fv[1] == acl_table_id
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_ADMIN_STATE":
+                assert fv[1] == "true"
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_PRIORITY":
+                assert fv[1] == "20"
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_COUNTER":
+                assert True
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_FIELD_L4_SRC_PORT":
+                assert fv[1] == "65000&mask:0xffff"
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT":
+                assert fv[1] == next_hop_id
+            else:
+                assert False
+
+        # remove acl rule
+        tbl = swsscommon.Table(self.cdb, "ACL_TABLE")
+        tbl._del("test_acl_table|redirect_rule")
+
+        time.sleep(1)
+
+        (status, fvs) = atbl.get(acl_entry[0])
+        assert status == False
+
+        config_qualifiers = {"L4_SRC_PORT": "65000"}
+        self.dvs_acl.create_redirect_action_acl_rule("test_acl_table", "redirect_action_rule", config_qualifiers, intf="Ethernet4", priority="20")
+        keys = atbl.getKeys()
+
+        acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries]
+        assert len(acl_entry) == 1
+
+        (status, fvs) = atbl.get(acl_entry[0])
+        assert status == True
+        assert len(fvs) == 6
+        for fv in fvs:
+            if fv[0] == "SAI_ACL_ENTRY_ATTR_TABLE_ID":
+                assert fv[1] == acl_table_id
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_ADMIN_STATE":
+                assert fv[1] == "true"
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_PRIORITY":
+                assert fv[1] == "20"
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_COUNTER":
+                assert True
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_FIELD_L4_SRC_PORT":
+                assert fv[1] == "65000&mask:0xffff"
+            elif fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT":
+                assert fv[1] == next_hop_id
+            else:
+                assert False
+
+        tbl._del("test_acl_table|redirect_action_rule")
+
+        time.sleep(1)
+
+        (status, fvs) = atbl.get(acl_entry[0])
+        assert status == False
+
+        tbl._del("test_acl_table")
+
+        time.sleep(1)
+
+        keys = atbl.getKeys()
+        assert len(keys) >= 1
+
+        # remove neighbor
+        dvs.remove_neighbor("Ethernet4", "10.0.0.2")
+
+        # remove interface ip
+        dvs.remove_ip_address("Ethernet4", "10.0.0.1/24")
+
+        # bring down interface
+        dvs.set_interface_status("Ethernet4", "down")
+
 class TestAclRuleValidation(BaseTestAcl):
     """ Test class for cases that check if orchagent corectly validates
     ACL rules input