From b16368c1181e0aef981466ac88cf37f43a92f986 Mon Sep 17 00:00:00 2001 From: madhanmellanox <62459540+madhanmellanox@users.noreply.github.com> Date: Wed, 15 Jul 2020 10:26:07 -0700 Subject: [PATCH] [aclorch.cpp] Handle all the ACL redirect requests in AclRuleL3::validateAddAction() (#1278) * changed method AclRuleL3::validateAddAction() and AclRuleL3::getRedirectObjectId() to handle all ACL redirect cases * Handled string handling of attr_value in AclRuleL3::validateAddAction() method * added VS testcases for ACL redirect and priority ACL tests Conflicts: tests/test_acl.py * added VS testcases for ACL redirect and priority ACL tests Conflicts: tests/test_acl.py Conflicts: tests/test_acl.py * removed test_AclpriorityRule() test case, as it does not belong here * removed test_AclprioriyRule() test case as it does not belong here * redone the wrongly done resolved conflict of test_acl.py file * address vs test failure * addressed vs test failure * fixed self.asic_db.wait_for_n_keys to self.dvs_acl.asic_db.wait_for_n_keys in the relevant test case * fixed all the required function calls relevant to dvs_acl in my testcase Co-authored-by: Madhan Babu Co-authored-by: root --- orchagent/aclorch.cpp | 35 ++++++++++++++++++----------------- tests/dvslib/dvs_acl.py | 13 +++++++++++-- tests/test_acl.py | 31 +++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 19 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 0f39e61054ae..214b91d2d077 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -821,8 +821,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) @@ -868,21 +882,8 @@ 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/dvslib/dvs_acl.py b/tests/dvslib/dvs_acl.py index 3f5576200985..923e905a44a2 100644 --- a/tests/dvslib/dvs_acl.py +++ b/tests/dvslib/dvs_acl.py @@ -199,8 +199,7 @@ def _check_acl_entry(self, entry, qualifiers, action, priority): else: assert False elif k == "SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT": - if "REDIRECT" not in action: - assert False + assert True elif k in qualifiers: assert qualifiers[k](v) else: @@ -240,3 +239,13 @@ def _match_acl_range(sai_acl_range): return _match_acl_range + 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) diff --git a/tests/test_acl.py b/tests/test_acl.py index 6b18943c2461..d7eba2683282 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -391,6 +391,37 @@ def test_AclRuleRedirectToNextHop(self, dvs): dvs.remove_ip_address("Ethernet4", "10.0.0.1/24") dvs.set_interface_status("Ethernet4", "down") + def test_AclRedirectRule(self, dvs): + dvs.setup_db() + + # 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") + + next_hop_id = self.dvs_acl.asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP", 1)[0] + + bind_ports = ["Ethernet0", "Ethernet8"] + self.dvs_acl.create_acl_table("test_acl_table", "L3", bind_ports) + + config_qualifiers = {"L4_SRC_PORT": "65000"} + expected_sai_qualifiers = {"SAI_ACL_ENTRY_ATTR_FIELD_L4_SRC_PORT": self.dvs_acl.get_simple_qualifier_comparator("65000&mask:0xffff")} + self.dvs_acl.create_acl_rule("test_acl_table", "redirect_rule", config_qualifiers, action="REDIRECT:10.0.0.2@Ethernet4", priority="20") + acl_rule_id = self.dvs_acl.get_acl_rule_id() + entry = self.dvs_acl.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", acl_rule_id) + self.dvs_acl._check_acl_entry(entry, expected_sai_qualifiers, "REDIRECT:10.0.0.2@Ethernet4", "20") + assert entry.get("SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT", None) == next_hop_id + + self.dvs_acl.remove_acl_rule("test_acl_table", "redirect_rule") + + self.dvs_acl.create_redirect_action_acl_rule("test_acl_table", "redirect_action_rule", config_qualifiers, intf="Ethernet4", priority="20") + acl_rule_id = self.dvs_acl.get_acl_rule_id() + entry = self.dvs_acl.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", acl_rule_id) + self.dvs_acl._check_acl_entry(entry, expected_sai_qualifiers, "Ethernet4", "20") + self.dvs_acl.remove_acl_rule("test_acl_table", "redirect_action_rule") + self.dvs_acl.verify_no_acl_rules() + self.dvs_acl.remove_acl_table("test_acl_table") + self.dvs_acl.verify_acl_table_count(0) @pytest.mark.usefixtures('dvs_acl_manager') class TestAclRuleValidation():