From 698d10a72110227b8d0790fa2d09ff97425fc398 Mon Sep 17 00:00:00 2001 From: Tyler Li Date: Fri, 20 Sep 2019 02:12:53 -0700 Subject: [PATCH] Revise for review comments 1. create separate file for NextHopKey and NextHopGroupKey 2. remove test_AclRuleRedirectToNexthop --- orchagent/neighorch.h | 62 +---------------- orchagent/nexthopgroupkey.h | 126 +++++++++++++++++++++++++++++++++ orchagent/nexthopkey.h | 67 ++++++++++++++++++ orchagent/routeorch.h | 122 +------------------------------- tests/test_acl.py | 135 ------------------------------------ 5 files changed, 195 insertions(+), 317 deletions(-) create mode 100644 orchagent/nexthopgroupkey.h create mode 100644 orchagent/nexthopkey.h diff --git a/orchagent/neighorch.h b/orchagent/neighorch.h index 7f3136851a..60c40f9053 100644 --- a/orchagent/neighorch.h +++ b/orchagent/neighorch.h @@ -7,70 +7,10 @@ #include "intfsorch.h" #include "ipaddress.h" -#include "tokenize.h" +#include "nexthopkey.h" #define NHFLAGS_IFDOWN 0x1 // nexthop's outbound i/f is down -#define VRF_PREFIX "Vrf" -extern IntfsOrch *gIntfsOrch; - -struct NextHopKey -{ - IpAddress ip_address; // neighbor IP address - string alias; // incoming interface alias - - NextHopKey() = default; - NextHopKey(const std::string &ipstr, const std::string &alias) : ip_address(ipstr), alias(alias) {} - NextHopKey(const IpAddress &ip, const std::string &alias) : ip_address(ip), alias(alias) {} - NextHopKey(const std::string &str) - { - if (str.find(',') != string::npos) - { - std::string err = "Error converting " + str + " to NextHop"; - throw std::invalid_argument(err); - } - auto keys = tokenize(str, '|'); - if (keys.size() == 1) - { - ip_address = keys[0]; - alias = gIntfsOrch->getRouterIntfsAlias(ip_address); - } - else if (keys.size() == 2) - { - ip_address = keys[0]; - alias = keys[1]; - if (!alias.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) - { - alias = gIntfsOrch->getRouterIntfsAlias(ip_address, alias); - } - } - else - { - std::string err = "Error converting " + str + " to NextHop"; - throw std::invalid_argument(err); - } - } - const std::string to_string() const - { - return ip_address.to_string() + "|" + alias; - } - - bool operator<(const NextHopKey &o) const - { - return tie(ip_address, alias) < tie(o.ip_address, o.alias); - } - - bool operator==(const NextHopKey &o) const - { - return (ip_address == o.ip_address) && (alias == o.alias); - } - - bool operator!=(const NextHopKey &o) const - { - return !(*this == o); - } -}; - typedef NextHopKey NeighborEntry; struct NextHopEntry diff --git a/orchagent/nexthopgroupkey.h b/orchagent/nexthopgroupkey.h new file mode 100644 index 0000000000..bc3dcef35b --- /dev/null +++ b/orchagent/nexthopgroupkey.h @@ -0,0 +1,126 @@ +#ifndef SWSS_NEXTHOPGROUPKEY_H +#define SWSS_NEXTHOPGROUPKEY_H + +#include "nexthopkey.h" + +class NextHopGroupKey +{ +public: + NextHopGroupKey() = default; + + /* ip_string|if_alias separated by ',' */ + NextHopGroupKey(const std::string &nexthops) + { + auto nhv = tokenize(nexthops, ','); + for (const auto &nh : nhv) + { + m_nexthops.insert(nh); + } + } + + inline const std::set &getNextHops() const + { + return m_nexthops; + } + + inline size_t getSize() const + { + return m_nexthops.size(); + } + + inline bool operator<(const NextHopGroupKey &o) const + { + return m_nexthops < o.m_nexthops; + } + + inline bool operator==(const NextHopGroupKey &o) const + { + return m_nexthops == o.m_nexthops; + } + + inline bool operator!=(const NextHopGroupKey &o) const + { + return !(*this == o); + } + + void add(const std::string &ip, const std::string &alias) + { + m_nexthops.emplace(ip, alias); + } + + void add(const std::string &nh) + { + m_nexthops.insert(nh); + } + + void add(const NextHopKey &nh) + { + m_nexthops.insert(nh); + } + + bool contains(const std::string &ip, const std::string &alias) const + { + NextHopKey nh(ip, alias); + return m_nexthops.find(nh) != m_nexthops.end(); + } + + bool contains(const std::string &nh) const + { + return m_nexthops.find(nh) != m_nexthops.end(); + } + + bool contains(const NextHopKey &nh) const + { + return m_nexthops.find(nh) != m_nexthops.end(); + } + + bool contains(const NextHopGroupKey &nhs) const + { + for (const auto &nh : nhs.getNextHops()) + { + if (!contains(nh)) + { + return false; + } + } + return true; + } + + void remove(const std::string &ip, const std::string &alias) + { + NextHopKey nh(ip, alias); + m_nexthops.erase(nh); + } + + void remove(const std::string &nh) + { + m_nexthops.erase(nh); + } + + void remove(const NextHopKey &nh) + { + m_nexthops.erase(nh); + } + + const std::string to_string() const + { + string nhs_str; + + for (auto it = m_nexthops.begin(); it != m_nexthops.end(); ++it) + { + if (it != m_nexthops.begin()) + { + nhs_str += ","; + } + + nhs_str += it->to_string(); + } + + return nhs_str; + } + +private: + std::set m_nexthops; +}; + +#endif /* SWSS_NEXTHOPGROUPKEY_H */ diff --git a/orchagent/nexthopkey.h b/orchagent/nexthopkey.h new file mode 100644 index 0000000000..3efd544e32 --- /dev/null +++ b/orchagent/nexthopkey.h @@ -0,0 +1,67 @@ +#ifndef SWSS_NEXTHOPKEY_H +#define SWSS_NEXTHOPKEY_H + +#include "ipaddress.h" +#include "tokenize.h" + +#define VRF_PREFIX "Vrf" +extern IntfsOrch *gIntfsOrch; + +struct NextHopKey +{ + IpAddress ip_address; // neighbor IP address + string alias; // incoming interface alias + + NextHopKey() = default; + NextHopKey(const std::string &ipstr, const std::string &alias) : ip_address(ipstr), alias(alias) {} + NextHopKey(const IpAddress &ip, const std::string &alias) : ip_address(ip), alias(alias) {} + NextHopKey(const std::string &str) + { + if (str.find(',') != string::npos) + { + std::string err = "Error converting " + str + " to NextHop"; + throw std::invalid_argument(err); + } + auto keys = tokenize(str, '|'); + if (keys.size() == 1) + { + ip_address = keys[0]; + alias = gIntfsOrch->getRouterIntfsAlias(ip_address); + } + else if (keys.size() == 2) + { + ip_address = keys[0]; + alias = keys[1]; + if (!alias.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) + { + alias = gIntfsOrch->getRouterIntfsAlias(ip_address, alias); + } + } + else + { + std::string err = "Error converting " + str + " to NextHop"; + throw std::invalid_argument(err); + } + } + const std::string to_string() const + { + return ip_address.to_string() + "|" + alias; + } + + bool operator<(const NextHopKey &o) const + { + return tie(ip_address, alias) < tie(o.ip_address, o.alias); + } + + bool operator==(const NextHopKey &o) const + { + return (ip_address == o.ip_address) && (alias == o.alias); + } + + bool operator!=(const NextHopKey &o) const + { + return !(*this == o); + } +}; + +#endif /* SWSS_NEXTHOPKEY_H */ diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index df4c4946af..e00223245c 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -9,7 +9,7 @@ #include "ipaddress.h" #include "ipaddresses.h" #include "ipprefix.h" -#include "tokenize.h" +#include "nexthopgroupkey.h" #include @@ -18,126 +18,6 @@ /* Length of the Interface Id value in EUI64 format */ #define EUI64_INTF_ID_LEN 8 -class NextHopGroupKey -{ -public: - NextHopGroupKey() = default; - - /* ip_string|if_alias separated by ',' */ - NextHopGroupKey(const std::string &nexthops) - { - auto nhv = tokenize(nexthops, ','); - for (const auto &nh : nhv) - { - m_nexthops.insert(nh); - } - } - - inline const std::set &getNextHops() const - { - return m_nexthops; - } - - inline size_t getSize() const - { - return m_nexthops.size(); - } - - inline bool operator<(const NextHopGroupKey &o) const - { - return m_nexthops < o.m_nexthops; - } - - inline bool operator==(const NextHopGroupKey &o) const - { - return m_nexthops == o.m_nexthops; - } - - inline bool operator!=(const NextHopGroupKey &o) const - { - return !(*this == o); - } - - void add(const std::string &ip, const std::string &alias) - { - m_nexthops.emplace(ip, alias); - } - - void add(const std::string &nh) - { - m_nexthops.insert(nh); - } - - void add(const NextHopKey &nh) - { - m_nexthops.insert(nh); - } - - bool contains(const std::string &ip, const std::string &alias) const - { - NextHopKey nh(ip, alias); - return m_nexthops.find(nh) != m_nexthops.end(); - } - - bool contains(const std::string &nh) const - { - return m_nexthops.find(nh) != m_nexthops.end(); - } - - bool contains(const NextHopKey &nh) const - { - return m_nexthops.find(nh) != m_nexthops.end(); - } - - bool contains(const NextHopGroupKey &nhs) const - { - for (const auto &nh : nhs.getNextHops()) - { - if (!contains(nh)) - { - return false; - } - } - return true; - } - - void remove(const std::string &ip, const std::string &alias) - { - NextHopKey nh(ip, alias); - m_nexthops.erase(nh); - } - - void remove(const std::string &nh) - { - m_nexthops.erase(nh); - } - - void remove(const NextHopKey &nh) - { - m_nexthops.erase(nh); - } - - const std::string to_string() const - { - string nhs_str; - - for (auto it = m_nexthops.begin(); it != m_nexthops.end(); ++it) - { - if (it != m_nexthops.begin()) - { - nhs_str += ","; - } - - nhs_str += it->to_string(); - } - - return nhs_str; - } - -private: - std::set m_nexthops; -}; - typedef std::map NextHopGroupMembers; struct NextHopGroupEntry diff --git a/tests/test_acl.py b/tests/test_acl.py index 41fce57062..5cfeb49b4e 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -1215,141 +1215,6 @@ def test_AclRuleIcmpV6(self, dvs, testlog): self.remove_acl_table(acl_table) - def set_admin_status(self, interface, status): - tbl = swsscommon.Table(self.cdb, "PORT") - fvs = swsscommon.FieldValuePairs([("admin_status", status)]) - tbl.set(interface, fvs) - time.sleep(1) - - def create_l3_intf(self, interface, vrf_name): - tbl = swsscommon.Table(self.cdb, "INTERFACE") - if len(vrf_name) == 0: - fvs = swsscommon.FieldValuePairs([("NULL", "NULL")]) - else: - fvs = swsscommon.FieldValuePairs([("vrf_name", vrf_name)]) - tbl.set(interface, fvs) - time.sleep(1) - - def remove_l3_intf(self, interface): - tbl = swsscommon.Table(self.cdb, "INTERFACE") - tbl._del(interface) - time.sleep(1) - - def add_ip_address(self, interface, ip): - tbl = swsscommon.Table(self.cdb, "INTERFACE") - fvs = swsscommon.FieldValuePairs([("NULL", "NULL")]) - tbl.set(interface + "|" + ip, fvs) - time.sleep(1) - - def remove_ip_address(self, interface, ip): - tbl = swsscommon.Table(self.cdb, "INTERFACE") - tbl._del(interface + "|" + ip) - time.sleep(1) - - def add_neighbor(self, interface, ip, mac): - tbl = swsscommon.Table(self.cdb, "NEIGH") - fvs = swsscommon.FieldValuePairs([("neigh", mac)]) - tbl.set(interface + "|" + ip, fvs) - time.sleep(1) - - def remove_neighbor(self, interface, ip): - tbl = swsscommon.Table(self.cdb, "NEIGH") - tbl._del(interface + "|" + ip) - time.sleep(1) - - def test_AclRuleRedirectToNexthop(self, dvs, testlog): - self.setup_db(dvs) - - # bring up interface - self.set_admin_status("Ethernet4", "up") - - # create interface - self.create_l3_intf("Ethernet4", "") - - # assign IP to interface - self.add_ip_address("Ethernet4", "10.0.0.1/24") - - # add neighbor - self.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 - nhi_oid = keys[0] - - # create ACL_TABLE in config db - tbl = swsscommon.Table(self.cdb, "ACL_TABLE") - fvs = swsscommon.FieldValuePairs([("policy_desc", "test"), ("type", "L3"), ("ports", "Ethernet0")]) - tbl.set("test_redirect", fvs) - - time.sleep(2) - - # create acl rule - tbl = swsscommon.Table(self.cdb, "ACL_RULE") - fvs = swsscommon.FieldValuePairs([ - ("priority", "100"), - ("L4_SRC_PORT", "65000"), - ("PACKET_ACTION", "REDIRECT:10.0.0.2|Ethernet4")]) - tbl.set("test_redirect|test_rule1", fvs) - - time.sleep(1) - - test_acl_table_id = self.get_acl_table_id(dvs) - - # 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] == test_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] == "100" - 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] == nhi_oid - else: - assert False - - # remove acl rule - tbl._del("test_redirect|test_rule1") - - time.sleep(1) - - (status, fvs) = atbl.get(acl_entry[0]) - assert status == False - - tbl = swsscommon.Table(self.cdb, "ACL_TABLE") - tbl._del("test_redirect") - - time.sleep(1) - - atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE") - keys = atbl.getKeys() - assert len(keys) >= 1 - - # remove neighbor - self.remove_neighbor("Ethernet4", "10.0.0.2") - - # remove interface ip - self.remove_ip_address("Ethernet4", "10.0.0.1/24") - - # remove interface - self.remove_l3_intf("Ethernet4") - - # bring down interface - self.set_admin_status("Ethernet4", "down") class TestAclRuleValidation(BaseTestAcl): """ Test class for cases that check if orchagent corectly validates