From e6d97900a9aec3b18cc757b0a0eb138e1d19c051 Mon Sep 17 00:00:00 2001 From: Prince Sunny Date: Wed, 17 Mar 2021 11:13:48 -0700 Subject: [PATCH] [MUX/PFCWD] Use in_ports for acls instead of seperate ACL table (#1670) *Use ACL_TABLE_DROP for both PFC_WD and MUX *Use MATCH_IN_PORTS instead of binding port to ACL table and program single ACL rule --- orchagent/aclorch.cpp | 40 ++++++++++++++++--- orchagent/aclorch.h | 3 +- orchagent/muxorch.cpp | 72 ++++++++++++++++++++++++++++------ orchagent/muxorch.h | 5 ++- orchagent/pfcactionhandler.cpp | 32 +++++++++------ tests/test_mux.py | 49 +++++++++++++++++++++++ 6 files changed, 168 insertions(+), 33 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index c24eeedb6df7..02b5d47177dd 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -2961,7 +2961,26 @@ bool AclOrch::removeAclRule(string table_id, string rule_id) return m_AclTables[table_oid].remove(rule_id); } -bool AclOrch::updateAclRule(shared_ptr rule, string table_id, string attr_name, void *data, bool oper) +AclRule* AclOrch::getAclRule(string table_id, string rule_id) +{ + sai_object_id_t table_oid = getTableById(table_id); + if (table_oid == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_INFO("Table %s does not exist", table_id.c_str()); + return nullptr; + } + + const auto& rule_it = m_AclTables[table_oid].rules.find(rule_id); + if (rule_it == m_AclTables[table_oid].rules.end()) + { + SWSS_LOG_INFO("Rule %s doesn't exist", rule_id.c_str()); + return nullptr; + } + + return rule_it->second.get(); +} + +bool AclOrch::updateAclRule(string table_id, string rule_id, string attr_name, void *data, bool oper) { SWSS_LOG_ENTER(); @@ -2974,12 +2993,19 @@ bool AclOrch::updateAclRule(shared_ptr rule, string table_id, string at return false; } + auto rule_it = m_AclTables[table_oid].rules.find(rule_id); + if (rule_it == m_AclTables[table_oid].rules.end()) + { + SWSS_LOG_ERROR("Failed to update ACL rule in ACL table %s. Rule doesn't exist", rule_id.c_str()); + return false; + } + switch (aclMatchLookup[attr_name]) { case SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS: { sai_object_id_t port_oid = *(sai_object_id_t *)data; - vector in_ports = rule->getInPorts(); + vector in_ports = rule_it->second->getInPorts(); if (oper == RULE_OPER_ADD) { @@ -3004,10 +3030,14 @@ bool AclOrch::updateAclRule(shared_ptr rule, string table_id, string at attr_value += p.m_alias; attr_value += ','; } - attr_value.pop_back(); - rule->validateAddMatch(MATCH_IN_PORTS, attr_value); - m_AclTables[table_oid].rules[rule->getId()]->updateInPorts(); + if (!attr_value.empty()) + { + attr_value.pop_back(); + } + + rule_it->second->validateAddMatch(MATCH_IN_PORTS, attr_value); + rule_it->second->updateInPorts(); } break; diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index d8eac7a95ad9..403fb3b44aee 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -440,7 +440,8 @@ class AclOrch : public Orch, public Observer bool updateAclTable(AclTable ¤tTable, AclTable &newTable); bool addAclRule(shared_ptr aclRule, string table_id); bool removeAclRule(string table_id, string rule_id); - bool updateAclRule(shared_ptr aclRule, string table_id, string attr_name, void *data, bool oper); + bool updateAclRule(string table_id, string rule_id, string attr_name, void *data, bool oper); + AclRule* getAclRule(string table_id, string rule_id); bool isCombinedMirrorV6Table(); bool isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t action) const; diff --git a/orchagent/muxorch.cpp b/orchagent/muxorch.cpp index 8e581a11ca4d..bb1fdceffaa0 100644 --- a/orchagent/muxorch.cpp +++ b/orchagent/muxorch.cpp @@ -40,8 +40,8 @@ extern sai_router_interface_api_t* sai_router_intfs_api; /* Constants */ #define MUX_TUNNEL "MuxTunnel0" -#define MUX_ACL_TABLE_NAME "mux_acl_table"; -#define MUX_ACL_RULE_NAME "mux_acl_rule"; +#define MUX_ACL_TABLE_NAME INGRESS_TABLE_DROP +#define MUX_ACL_RULE_NAME "mux_acl_rule" #define MUX_HW_STATE_UNKNOWN "unknown" #define MUX_HW_STATE_ERROR "error" @@ -202,6 +202,10 @@ static sai_object_id_t create_tunnel(const IpAddress* p_dst_ip, const IpAddress* attr.value.s32 = SAI_TUNNEL_TTL_MODE_PIPE_MODEL; tunnel_attrs.push_back(attr); + attr.id = SAI_TUNNEL_ATTR_LOOPBACK_PACKET_ACTION; + attr.value.s32 = SAI_PACKET_ACTION_DROP; + tunnel_attrs.push_back(attr); + if (p_src_ip != nullptr) { attr.id = SAI_TUNNEL_ATTR_ENCAP_SRC_IP; @@ -343,7 +347,7 @@ bool MuxCable::stateActive() return false; } - if (!aclHandler(port.m_port_id, false)) + if (!aclHandler(port.m_port_id, mux_name_, false)) { SWSS_LOG_INFO("Remove ACL drop rule failed for %s", mux_name_.c_str()); return false; @@ -373,7 +377,7 @@ bool MuxCable::stateStandby() return false; } - if (!aclHandler(port.m_port_id)) + if (!aclHandler(port.m_port_id, mux_name_)) { SWSS_LOG_INFO("Add ACL drop rule failed for %s", mux_name_.c_str()); return false; @@ -416,6 +420,7 @@ void MuxCable::setState(string new_state) } st_chg_in_progress_ = false; + st_chg_failed_ = false; SWSS_LOG_INFO("Changed state to %s", new_state.c_str()); return; @@ -429,11 +434,11 @@ string MuxCable::getState() return (muxStateValToString.at(state_)); } -bool MuxCable::aclHandler(sai_object_id_t port, bool add) +bool MuxCable::aclHandler(sai_object_id_t port, string alias, bool add) { if (add) { - acl_handler_ = make_shared(port); + acl_handler_ = make_shared(port, alias); } else { @@ -685,16 +690,18 @@ sai_object_id_t MuxNbrHandler::getNextHopId(const NextHopKey nhKey) std::map MuxAclHandler::acl_table_; -MuxAclHandler::MuxAclHandler(sai_object_id_t port) +MuxAclHandler::MuxAclHandler(sai_object_id_t port, string alias) { SWSS_LOG_ENTER(); // There is one handler instance per MUX port - acl_table_type_t table_type = ACL_TABLE_MUX; + acl_table_type_t table_type = ACL_TABLE_DROP; string table_name = MUX_ACL_TABLE_NAME; string rule_name = MUX_ACL_RULE_NAME; port_ = port; + alias_ = alias; + auto found = acl_table_.find(table_name); if (found == acl_table_.end()) { @@ -709,8 +716,18 @@ MuxAclHandler::MuxAclHandler(sai_object_id_t port) else { SWSS_LOG_NOTICE("Binding port %" PRIx64 "", port); - // Otherwise just bind ACL table with the port - found->second.bind(port); + + AclRule* rule = gAclOrch->getAclRule(table_name, rule_name); + if (rule == nullptr) + { + shared_ptr newRule = + make_shared(gAclOrch, rule_name, table_name, table_type); + createMuxAclRule(newRule, table_name); + } + else + { + gAclOrch->updateAclRule(table_name, rule_name, MATCH_IN_PORTS, &port, RULE_OPER_ADD); + } } } @@ -718,11 +735,25 @@ MuxAclHandler::~MuxAclHandler(void) { SWSS_LOG_ENTER(); string table_name = MUX_ACL_TABLE_NAME; + string rule_name = MUX_ACL_RULE_NAME; SWSS_LOG_NOTICE("Un-Binding port %" PRIx64 "", port_); - auto found = acl_table_.find(table_name); - found->second.unbind(port_); + AclRule* rule = gAclOrch->getAclRule(table_name, rule_name); + if (rule == nullptr) + { + SWSS_LOG_THROW("ACL Rule does not exist for port %s, rule %s", alias_.c_str(), rule_name.c_str()); + } + + vector port_set = rule->getInPorts(); + if ((port_set.size() == 1) && (port_set[0] == port_)) + { + gAclOrch->removeAclRule(table_name, rule_name); + } + else + { + gAclOrch->updateAclRule(table_name, rule_name, MATCH_IN_PORTS, &port_, RULE_OPER_DELETE); + } } void MuxAclHandler::createMuxAclTable(sai_object_id_t port, string strTable) @@ -736,7 +767,17 @@ void MuxAclHandler::createMuxAclTable(sai_object_id_t port, string strTable) assert(inserted.second); AclTable& acl_table = inserted.first->second; - acl_table.type = ACL_TABLE_MUX; + + sai_object_id_t table_oid = gAclOrch->getTableById(strTable); + if (table_oid != SAI_NULL_OBJECT_ID) + { + // DROP ACL table is already created + SWSS_LOG_NOTICE("ACL table %s exists, reuse the same", strTable.c_str()); + acl_table = *(gAclOrch->getTableByOid(table_oid)); + return; + } + + acl_table.type = ACL_TABLE_DROP; acl_table.id = strTable; acl_table.link(port); acl_table.stage = ACL_STAGE_INGRESS; @@ -753,6 +794,11 @@ void MuxAclHandler::createMuxAclRule(shared_ptr rule, string strTabl attr_value = "999"; rule->validateAddPriority(attr_name, attr_value); + // Add MATCH_IN_PORTS as match criteria for ingress table + attr_name = MATCH_IN_PORTS; + attr_value = alias_; + rule->validateAddMatch(attr_name, attr_value); + attr_name = ACTION_PACKET_ACTION; attr_value = PACKET_ACTION_DROP; rule->validateAddAction(attr_name, attr_value); diff --git a/orchagent/muxorch.h b/orchagent/muxorch.h index 3264e4967add..0c34936f75fc 100644 --- a/orchagent/muxorch.h +++ b/orchagent/muxorch.h @@ -38,7 +38,7 @@ class MuxStateOrch; class MuxAclHandler { public: - MuxAclHandler(sai_object_id_t port); + MuxAclHandler(sai_object_id_t port, string alias); ~MuxAclHandler(void); private: @@ -48,6 +48,7 @@ class MuxAclHandler // class shared dict: ACL table name -> ACL table static std::map acl_table_; sai_object_id_t port_ = SAI_NULL_OBJECT_ID; + string alias_; }; // IP to nexthop index mapping @@ -101,7 +102,7 @@ class MuxCable bool stateInitActive(); bool stateStandby(); - bool aclHandler(sai_object_id_t, bool add = true); + bool aclHandler(sai_object_id_t port, string alias, bool add = true); bool nbrHandler(bool enable, bool update_routes = true); string mux_name_; diff --git a/orchagent/pfcactionhandler.cpp b/orchagent/pfcactionhandler.cpp index 6bc65a611451..34c513e5d60e 100644 --- a/orchagent/pfcactionhandler.cpp +++ b/orchagent/pfcactionhandler.cpp @@ -226,7 +226,6 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, SWSS_LOG_ENTER(); acl_table_type_t table_type; - sai_object_id_t table_oid; string queuestr = to_string(queueId); m_strRule = "Rule_PfcWdAclHandler_" + queuestr; @@ -244,18 +243,15 @@ PfcWdAclHandler::PfcWdAclHandler(sai_object_id_t port, sai_object_id_t queue, } else { - table_oid = gAclOrch->getTableById(m_strIngressTable); - map table_map = gAclOrch->getAclTables(); - auto rule_iter = table_map[table_oid].rules.find(m_strRule); - - if (rule_iter == table_map[table_oid].rules.end()) + AclRule* rule = gAclOrch->getAclRule(m_strIngressTable, m_strRule); + if (rule == nullptr) { shared_ptr newRule = make_shared(gAclOrch, m_strRule, m_strIngressTable, table_type); createPfcAclRule(newRule, queueId, m_strIngressTable, port); } else { - gAclOrch->updateAclRule(rule_iter->second, m_strIngressTable, MATCH_IN_PORTS, &port, RULE_OPER_ADD); + gAclOrch->updateAclRule(m_strIngressTable, m_strRule, MATCH_IN_PORTS, &port, RULE_OPER_ADD); } } @@ -281,11 +277,13 @@ PfcWdAclHandler::~PfcWdAclHandler(void) { SWSS_LOG_ENTER(); - sai_object_id_t table_oid = gAclOrch->getTableById(m_strIngressTable); - map table_map = gAclOrch->getAclTables(); - auto rule_iter = table_map[table_oid].rules.find(m_strRule); + AclRule* rule = gAclOrch->getAclRule(m_strIngressTable, m_strRule); + if (rule == nullptr) + { + SWSS_LOG_THROW("ACL Rule does not exist for rule %s", m_strRule.c_str()); + } - vector port_set = rule_iter->second->getInPorts(); + vector port_set = rule->getInPorts(); sai_object_id_t port = getPort(); if ((port_set.size() == 1) && (port_set[0] == port)) @@ -294,7 +292,7 @@ PfcWdAclHandler::~PfcWdAclHandler(void) } else { - gAclOrch->updateAclRule(rule_iter->second, m_strIngressTable, MATCH_IN_PORTS, &port, RULE_OPER_DELETE); + gAclOrch->updateAclRule(m_strIngressTable, m_strRule, MATCH_IN_PORTS, &port, RULE_OPER_DELETE); } auto found = m_aclTables.find(m_strEgressTable); @@ -323,6 +321,16 @@ void PfcWdAclHandler::createPfcAclTable(sai_object_id_t port, string strTable, b assert(inserted.second); AclTable& aclTable = inserted.first->second; + + sai_object_id_t table_oid = gAclOrch->getTableById(strTable); + if (ingress && table_oid != SAI_NULL_OBJECT_ID) + { + // DROP ACL table is already created + SWSS_LOG_NOTICE("ACL table %s exists, reuse the same", strTable.c_str()); + aclTable = *(gAclOrch->getTableByOid(table_oid)); + return; + } + aclTable.link(port); aclTable.id = strTable; diff --git a/tests/test_mux.py b/tests/test_mux.py index c1eaae75ed8d..c398a076568e 100644 --- a/tests/test_mux.py +++ b/tests/test_mux.py @@ -29,6 +29,7 @@ class TestMuxTunnelBase(object): IPV4_MASK = "/32" IPV6_MASK = "/128" TUNNEL_NH_ID = 0 + ACL_PRIORITY = "999" ecn_modes_map = { "standard" : "SAI_TUNNEL_DECAP_ECN_MODE_STANDARD", @@ -310,6 +311,43 @@ def create_and_test_route(self, appdb, asicdb, dvs, dvs_route): self.check_nexthop_group_in_asic_db(asicdb, rtkeys[0]) + def get_expected_sai_qualifiers(self, portlist, dvs_acl): + expected_sai_qualifiers = { + "SAI_ACL_ENTRY_ATTR_PRIORITY": self.ACL_PRIORITY, + "SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS": dvs_acl.get_port_list_comparator(portlist) + } + + return expected_sai_qualifiers + + + def create_and_test_acl(self, appdb, asicdb, dvs, dvs_acl): + + # Start with active, verify NO ACL rules exists + self.set_mux_state(appdb, "Ethernet0", "active") + self.set_mux_state(appdb, "Ethernet4", "active") + + dvs_acl.verify_no_acl_rules() + + # Set one mux port to standby, verify ACL rule with inport bitmap (1 port) + self.set_mux_state(appdb, "Ethernet0", "standby") + sai_qualifier = self.get_expected_sai_qualifiers(["Ethernet0"], dvs_acl) + dvs_acl.verify_acl_rule(sai_qualifier, action="DROP", priority=self.ACL_PRIORITY) + + # Set two mux ports to standby, verify ACL rule with inport bitmap (2 ports) + self.set_mux_state(appdb, "Ethernet4", "standby") + sai_qualifier = self.get_expected_sai_qualifiers(["Ethernet0","Ethernet4"], dvs_acl) + dvs_acl.verify_acl_rule(sai_qualifier, action="DROP", priority=self.ACL_PRIORITY) + + # Set one mux port to active, verify ACL rule with inport bitmap (1 port) + self.set_mux_state(appdb, "Ethernet0", "active") + sai_qualifier = self.get_expected_sai_qualifiers(["Ethernet4"], dvs_acl) + dvs_acl.verify_acl_rule(sai_qualifier, action="DROP", priority=self.ACL_PRIORITY) + + # Set last mux port to active, verify ACL rule is deleted + self.set_mux_state(appdb, "Ethernet4", "active") + dvs_acl.verify_no_acl_rules() + + def check_interface_exists_in_asicdb(self, asicdb, sai_oid): asicdb.wait_for_entry(self.ASIC_RIF_TABLE, sai_oid) return True @@ -364,6 +402,8 @@ def create_and_test_peer(self, db, asicdb, peer_name, peer_ip, src_ip): assert self.check_interface_exists_in_asicdb(asicdb, value) elif field == "SAI_TUNNEL_ATTR_ENCAP_TTL_MODE": assert value == "SAI_TUNNEL_TTL_MODE_PIPE_MODEL" + elif field == "SAI_TUNNEL_ATTR_LOOPBACK_PACKET_ACTION": + assert value == "SAI_PACKET_ACTION_DROP" else: assert False, "Field %s is not tested" % field @@ -539,6 +579,15 @@ def test_Route(self, dvs, dvs_route, testlog): self.create_and_test_route(appdb, asicdb, dvs, dvs_route) + def test_acl(self, dvs, dvs_acl, testlog): + """ test Route entries and mux state change """ + + appdb = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0) + asicdb = dvs.get_asic_db() + + self.create_and_test_acl(appdb, asicdb, dvs, dvs_acl) + + # Add Dummy always-pass test at end as workaroud # for issue when Flaky fail on final test it invokes module tear-down before retrying def test_nonflaky_dummy():