diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 7b25b3185f7..2de07727f85 100644 --- a/orchagent/aclorch.cpp +++ b/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; diff --git a/tests/test_acl.py b/tests/test_acl.py index 43316bd34cd..ce68d0047dd 100644 --- a/tests/test_acl.py +++ b/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