From 174fdd5f3f5dac5e72d4882c1398c23cfd89dba1 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Fri, 21 Jun 2019 11:10:18 +0300 Subject: [PATCH 01/19] [aclorch] support egress mirror action Signed-off-by: Stepan Blyschak [test_mirror] add integration test for egress mirror rule creation Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 65 ++++++++++++++++++++++++++---------- orchagent/aclorch.h | 6 ++-- orchagent/acltable.h | 6 ++-- tests/test_mirror.py | 76 +++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 130 insertions(+), 23 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 780b38f84c..c1acaf5906 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -99,8 +99,14 @@ static acl_table_type_lookup_t aclTableTypeLookUp = static acl_stage_type_lookup_t aclStageLookUp = { - {TABLE_INGRESS, ACL_STAGE_INGRESS }, - {TABLE_EGRESS, ACL_STAGE_EGRESS } + {STAGE_INGRESS, ACL_STAGE_INGRESS }, + {STAGE_EGRESS, ACL_STAGE_EGRESS } +}; + +static acl_rule_attr_lookup_t aclMirrorStageLookup = +{ + {STAGE_INGRESS, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS}, + {STAGE_EGRESS, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_EGRESS}, }; static acl_ip_type_lookup_t aclIpTypeLookup = @@ -957,18 +963,57 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) { SWSS_LOG_ENTER(); + sai_attribute_value_t value; + string stage = STAGE_INGRESS; + vector attr_values; + if (attr_name != ACTION_MIRROR_ACTION) { return false; } - if (!m_pMirrorOrch->sessionExists(attr_value)) + split(attr_value, attr_values, ':'); + if (attr_values.size() > 2) { + SWSS_LOG_ERROR("Invalid mirror rule action %s:%s", + attr_name.c_str(), attr_value.c_str()); return false; } + else if (attr_values.size() == 2) + { + stage = to_upper(attr_values[0]); + // strip stage value + attr_value = attr_values[1]; + } m_sessionName = attr_value; + if (!m_pMirrorOrch->sessionExists(m_sessionName)) + { + SWSS_LOG_ERROR("Mirror rule reference mirror session that does not exists %s", m_sessionName.c_str()); + return false; + } + + if (!m_pMirrorOrch->getSessionOid(m_sessionName, m_mirrorSessionOid)) + { + throw runtime_error("Failed to get mirror session OID"); + } + + value.aclaction.enable = true; + value.aclaction.parameter.objlist.list = &m_mirrorSessionOid; + value.aclaction.parameter.objlist.count = 1; + + if (aclMirrorStageLookup.find(stage) == aclMirrorStageLookup.end()) + { + SWSS_LOG_ERROR("Invalid stage parameter in mirror rule: %s", stage.c_str()); + return false; + } + + m_mirrorStage = stage; + + m_actions.clear(); + m_actions[aclMirrorStageLookup[m_mirrorStage]] = value; + return true; } @@ -1049,9 +1094,7 @@ bool AclRuleMirror::create() { SWSS_LOG_ENTER(); - sai_attribute_value_t value; bool state = false; - sai_object_id_t oid = SAI_NULL_OBJECT_ID; if (!m_pMirrorOrch->getSessionStatus(m_sessionName, state)) { @@ -1070,18 +1113,6 @@ bool AclRuleMirror::create() return true; } - if (!m_pMirrorOrch->getSessionOid(m_sessionName, oid)) - { - throw runtime_error("Failed to get mirror session OID"); - } - - value.aclaction.enable = true; - value.aclaction.parameter.objlist.list = &oid; - value.aclaction.parameter.objlist.count = 1; - - m_actions.clear(); - m_actions[SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS] = value; - if (!AclRule::create()) { return false; diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 558a032645..c008d4a228 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -272,10 +272,12 @@ class AclRuleMirror: public AclRule AclRuleCounters getCounters(); protected: - bool m_state; + bool m_state {false}; + string m_mirrorStage; string m_sessionName; + sai_object_id_t m_mirrorSessionOid {SAI_NULL_OBJECT_ID}; AclRuleCounters counters; - MirrorOrch *m_pMirrorOrch; + MirrorOrch *m_pMirrorOrch {nullptr}; }; class AclRuleDTelFlowWatchListEntry: public AclRule diff --git a/orchagent/acltable.h b/orchagent/acltable.h index 097ead33ae..4244678843 100644 --- a/orchagent/acltable.h +++ b/orchagent/acltable.h @@ -14,8 +14,10 @@ using namespace std; /* TODO: move all acltable and aclrule implementation out of aclorch */ -#define TABLE_INGRESS "INGRESS" -#define TABLE_EGRESS "EGRESS" +#define STAGE_INGRESS "INGRESS" +#define STAGE_EGRESS "EGRESS" +#define TABLE_INGRESS STAGE_INGRESS +#define TABLE_EGRESS STAGE_EGRESS #define TABLE_STAGE "STAGE" typedef enum diff --git a/tests/test_mirror.py b/tests/test_mirror.py index bcb5b62236..94bc34ce47 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -701,10 +701,14 @@ def remove_acl_table(self, table): tbl._del(table) time.sleep(1) - def create_mirror_acl_dscp_rule(self, table, rule, dscp, session): + def create_mirror_acl_dscp_rule(self, table, rule, dscp, session, stage=None): + action_value = session + if stage is not None: # else it should be treated as ingress by default in orchagent + assert stage in ('ingress', 'egress'), "invalid stage input {}".format(stage) + action_value = "{stage}:{session}".format(stage=stage, session=session) tbl = swsscommon.Table(self.cdb, "ACL_RULE") fvs = swsscommon.FieldValuePairs([("priority", "1000"), - ("mirror_action", session), + ("mirror_action", action_value), ("DSCP", dscp)]) tbl.set(table + "|" + rule, fvs) time.sleep(1) @@ -714,6 +718,74 @@ def remove_mirror_acl_dscp_rule(self, table, rule): tbl._del(table + "|" + rule) time.sleep(1) + def test_AclBindMirrorPerStage(self, dvs, testlog): + """ + This test configures mirror rules with specifying explicitely + the mirror action stage (ingress, egress) and verifies ASIC db + entry set with correct mirror action + """ + self.setup_db(dvs) + + session = "MIRROR_SESSION" + acl_table = "MIRROR_TABLE" + acl_rule = "MIRROR_RULE" + + # bring up port; assign ip; create neighbor; create route + self.set_interface_status(dvs, "Ethernet32", "up") + self.add_ip_address("Ethernet32", "20.0.0.0/31") + self.add_neighbor("Ethernet32", "20.0.0.1", "02:04:06:08:10:12") + self.add_route(dvs, "4.4.4.4", "20.0.0.1") + + # create mirror session + self.create_mirror_session(session, "3.3.3.3", "4.4.4.4", "0x6558", "8", "100", "0") + assert self.get_mirror_session_state(session)["status"] == "active" + + # assert mirror session in asic database + tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_MIRROR_SESSION") + assert len(tbl.getKeys()) == 1 + mirror_session_oid = tbl.getKeys()[0] + + # create acl table + self.create_acl_table(acl_table, ["Ethernet0", "Ethernet4"]) + + for stage, asic_attr in (("ingress", "SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS"), + ("egress", "SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_EGRESS")): + # create acl rule with dscp value 48 + self.create_mirror_acl_dscp_rule(acl_table, acl_rule, "48", session, stage=stage) + + # assert acl rule is created + tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY") + rule_entries = [k for k in tbl.getKeys() if k not in dvs.asicdb.default_acl_entries] + assert len(rule_entries) == 1 + + (status, fvs) = tbl.get(rule_entries[0]) + assert status == True + + asic_attr_found = False + for fv in fvs: + if fv[0] == asic_attr: + asic_attr_found = True + + assert asic_attr_found == True + + # remove acl rule + self.remove_mirror_acl_dscp_rule(acl_table, acl_rule) + + # remove acl table + self.remove_acl_table(acl_table) + + # remove mirror session + self.remove_mirror_session(session) + + # assert no mirror session in asic database + tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_MIRROR_SESSION") + assert len(tbl.getKeys()) == 0 + + # remove route; remove neighbor; remove ip; bring down port + self.remove_route(dvs, "4.4.4.4") + self.remove_neighbor("Ethernet32", "20.0.0.1") + self.remove_ip_address("Ethernet32", "20.0.0.0/31") + self.set_interface_status(dvs, "Ethernet32", "down") def test_AclBindMirror(self, dvs, testlog): """ From 2f241b5477a9c19520856f84e77fa45f13b3add8 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Fri, 21 Jun 2019 16:06:40 +0300 Subject: [PATCH 02/19] [aclorch] query acl action capabilities per stage Signed-off-by: Stepan Blyschak [aclorch] add getAclActionTypeFromAttr in AclOrch Signed-off-by: Stepan Blyschak [aclorch] add isAclActionSupported in AclOrch Signed-off-by: Stepan Blyschak [aclorch] add generic AclRule::validateAddAction implementation Each derivative of AclRule now has to call AclRule::validateAddAction to validate if action is supported. Signed-off-by: Stepan Blyschak [aclorch] move action support check in different method Signed-off-by: Stepan Blyschak [aclorch] fix acl action support check Signed-off-by: Stepan Blyschak --- orchagent/Makefile.am | 2 +- orchagent/aclorch.cpp | 115 ++++++++++++++++++++++++++++++++++++++++-- orchagent/aclorch.h | 14 ++++- 3 files changed, 124 insertions(+), 7 deletions(-) diff --git a/orchagent/Makefile.am b/orchagent/Makefile.am index 22600af304..3327079a28 100644 --- a/orchagent/Makefile.am +++ b/orchagent/Makefile.am @@ -56,7 +56,7 @@ orchagent_SOURCES = \ orchagent_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) orchagent_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) -orchagent_LDADD = -lnl-3 -lnl-route-3 -lpthread -lsairedis -lswsscommon -lsaimetadata +orchagent_LDADD = -lnl-3 -lnl-route-3 -lpthread -lsairedis -lswsscommon -lsaimeta -lsaimetadata routeresync_SOURCES = routeresync.cpp routeresync_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index c1acaf5906..fc038f77bb 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -10,6 +10,7 @@ #include "tokenize.h" #include "timer.h" #include "crmorch.h" +#include "sai_serialize.h" using namespace std; using namespace swss; @@ -385,6 +386,20 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return true; } +bool AclRule::validateAddAction(string attr_name, string attr_value) +{ + for (const auto& it: m_actions) + { + if (!AclRule::isActionSupported(it.first)) + { + SWSS_LOG_ERROR("Action %s:%s is not supported by ASIC", + attr_name.c_str(), attr_value.c_str()); + return false; + } + } + return true; +} + bool AclRule::processIpType(string type, sai_uint32_t &ip_type) { SWSS_LOG_ENTER(); @@ -533,6 +548,17 @@ void AclRule::decreaseNextHopRefCount() return; } +bool AclRule::isActionSupported(sai_acl_entry_attr_t action) const +{ + auto action_type = AclOrch::getAclActionFromAclEntry(action); + const auto* pTable = m_pAclOrch->getTableByOid(m_tableOid); + if (pTable == nullptr) + { + SWSS_LOG_THROW("ACL table does not exist for oid %lu", m_tableOid); + } + return m_pAclOrch->isAclActionSupported(pTable->stage, action_type); +} + bool AclRule::remove() { SWSS_LOG_ENTER(); @@ -775,7 +801,7 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value) m_actions[aclL3ActionLookup[attr_value]] = value; - return true; + return AclRule::validateAddAction(attr_name, attr_value); } // This method should return sai attribute id of the redirect destination @@ -1011,10 +1037,9 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) m_mirrorStage = stage; - m_actions.clear(); m_actions[aclMirrorStageLookup[m_mirrorStage]] = value; - return true; + return AclRule::validateAddAction(attr_name, attr_value); } bool AclRuleMirror::validateAddMatch(string attr_name, string attr_value) @@ -1689,7 +1714,7 @@ bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string a m_actions[aclDTelActionLookup[attr_name]] = value; - return true; + return AclRule::validateAddAction(attr_name, attr_value); } bool AclRuleDTelFlowWatchListEntry::validate() @@ -1848,7 +1873,7 @@ bool AclRuleDTelDropWatchListEntry::validateAddAction(string attr_name, string a m_actions[aclDTelActionLookup[attr_name]] = value; - return true; + return AclRule::validateAddAction(attr_name, attr_value); } bool AclRuleDTelDropWatchListEntry::validate() @@ -2079,6 +2104,8 @@ void AclOrch::init(vector& connectors, PortsOrch *portOrch, Mirr throw "AclOrch initialization failure"; } + queryAclActionCapability(); + // Attach observers m_mirrorOrch->attach(this); gPortsOrch->attach(this); @@ -2092,6 +2119,64 @@ void AclOrch::init(vector& connectors, PortsOrch *portOrch, Mirr timer->start(); } +void AclOrch::queryAclActionCapability() +{ + SWSS_LOG_ENTER(); + + sai_status_t status {SAI_STATUS_FAILURE}; + sai_attribute_t attr; + vector action_list; + + attr.id = SAI_SWITCH_ATTR_MAX_ACL_ACTION_COUNT; + status = sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_THROW("AclOrch initialization failed: " + "failed to query maximum ACL action count"); + } + + const auto max_action_count = attr.value.u32; + + for (auto stage_attr: {SAI_SWITCH_ATTR_ACL_STAGE_INGRESS, SAI_SWITCH_ATTR_ACL_STAGE_EGRESS}) + { + action_list.resize(static_cast(max_action_count)); + + attr.id = stage_attr; + attr.value.aclcapability.action_list.list = action_list.data(); + attr.value.aclcapability.action_list.count = max_action_count; + + status = sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_THROW("AclOrch initialization failed: " + "failed to query supported %s ACL actions", + (attr.id == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ? "ingress" : "egress")); + } + + SWSS_LOG_INFO("Supported %s action count %d:", + (attr.id == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ? "ingress" : "egress"), + attr.value.aclcapability.action_list.count); + + for (size_t i = 0; i < static_cast(attr.value.aclcapability.action_list.count); i++) + { + auto action = static_cast(action_list[i]); + auto stage = (attr.id == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ? ACL_STAGE_INGRESS : ACL_STAGE_EGRESS); + m_aclCapabilities[stage].insert(action); + SWSS_LOG_INFO(" %s", sai_serialize_enum(action, &sai_metadata_enum_sai_acl_action_type_t).c_str()); + } + } +} + +sai_acl_action_type_t AclOrch::getAclActionFromAclEntry(sai_acl_entry_attr_t attr) +{ + if (attr < SAI_ACL_ENTRY_ATTR_ACTION_START || attr > SAI_ACL_ENTRY_ATTR_ACTION_END) + { + SWSS_LOG_THROW("Invalid ACL entry attribute passed in: %d", attr); + } + + return static_cast(attr - SAI_ACL_ENTRY_ATTR_ACTION_START); +}; + AclOrch::AclOrch(vector& connectors, TableConnector switchTable, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch, DTelOrch *dtelOrch) : Orch(connectors), @@ -2344,6 +2429,16 @@ bool AclOrch::isCombinedMirrorV6Table() return m_isCombinedMirrorV6Table; } +bool AclOrch::isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t action) const +{ + const auto& it = m_aclCapabilities.find(stage); + if (it == m_aclCapabilities.cend()) + { + return false; + } + return it->second.find(action) != it->second.cend(); +} + void AclOrch::doAclTableTask(Consumer &consumer) { SWSS_LOG_ENTER(); @@ -2682,6 +2777,16 @@ sai_object_id_t AclOrch::getTableById(string table_id) return SAI_NULL_OBJECT_ID; } +const AclTable *AclOrch::getTableByOid(sai_object_id_t oid) const +{ + const auto& it = m_AclTables.find(oid); + if (it == m_AclTables.cend()) + { + return nullptr; + } + return &it->second; +} + bool AclOrch::createBindAclTable(AclTable &aclTable, sai_object_id_t &table_oid) { SWSS_LOG_ENTER(); diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index c008d4a228..d6acf729f1 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -114,6 +114,7 @@ typedef map acl_rule_attr_lookup_t; typedef map acl_ip_type_lookup_t; typedef map acl_dtel_flow_op_type_lookup_t; typedef tuple acl_range_properties_t; +typedef map> acl_capabilities_t; class AclOrch; @@ -164,13 +165,15 @@ struct AclRuleCounters } }; +class AclTable; + class AclRule { public: AclRule(AclOrch *m_pAclOrch, string rule, string table, acl_table_type_t type, bool createCounter = true); virtual bool validateAddPriority(string attr_name, string attr_value); virtual bool validateAddMatch(string attr_name, string attr_value); - virtual bool validateAddAction(string attr_name, string attr_value) = 0; + virtual bool validateAddAction(string attr_name, string attr_value); virtual bool validate() = 0; bool processIpType(string type, sai_uint32_t &ip_type); inline static void setRulePriorities(sai_uint32_t min, sai_uint32_t max) @@ -209,6 +212,8 @@ class AclRule void decreaseNextHopRefCount(); + bool isActionSupported(sai_acl_entry_attr_t) const; + static sai_uint32_t m_minPriority; static sai_uint32_t m_maxPriority; AclOrch *m_pAclOrch; @@ -388,6 +393,7 @@ class AclOrch : public Orch, public Observer void update(SubjectType, void *); sai_object_id_t getTableById(string table_id); + const AclTable* getTableByOid(sai_object_id_t oid) const; static swss::Table& getCountersTable() { @@ -408,10 +414,13 @@ class AclOrch : public Orch, public Observer bool removeAclRule(string table_id, string rule_id); bool isCombinedMirrorV6Table(); + bool isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t action) const; bool m_isCombinedMirrorV6Table = true; map m_mirrorTableCapabilities; + static sai_acl_action_type_t getAclActionFromAclEntry(sai_acl_entry_attr_t attr); + private: void doTask(Consumer &consumer); void doAclTableTask(Consumer &consumer); @@ -420,6 +429,7 @@ class AclOrch : public Orch, public Observer void init(vector& connectors, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch); void queryMirrorTableCapability(); + void queryAclActionCapability(); static void collectCountersThread(AclOrch *pAclOrch); @@ -446,6 +456,8 @@ class AclOrch : public Orch, public Observer string m_mirrorTableId; string m_mirrorV6TableId; + + acl_capabilities_t m_aclCapabilities; }; #endif /* SWSS_ACLORCH_H */ From 0da7d7c4c105affa2a084d49b276ba6482129df3 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 26 Jun 2019 18:12:11 +0300 Subject: [PATCH 03/19] [aclorch] put acl actions capabilities in state db Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 56 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index fc038f77bb..669ca47cda 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -2126,6 +2126,7 @@ void AclOrch::queryAclActionCapability() sai_status_t status {SAI_STATUS_FAILURE}; sai_attribute_t attr; vector action_list; + vector fvVector; attr.id = SAI_SWITCH_ATTR_MAX_ACL_ACTION_COUNT; status = sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr); @@ -2139,6 +2140,8 @@ void AclOrch::queryAclActionCapability() for (auto stage_attr: {SAI_SWITCH_ATTR_ACL_STAGE_INGRESS, SAI_SWITCH_ATTR_ACL_STAGE_EGRESS}) { + auto stage = (stage_attr == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ? ACL_STAGE_INGRESS : ACL_STAGE_EGRESS); + auto stage_str = (stage_attr == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ? STAGE_INGRESS : STAGE_EGRESS); action_list.resize(static_cast(max_action_count)); attr.id = stage_attr; @@ -2149,21 +2152,64 @@ void AclOrch::queryAclActionCapability() if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_THROW("AclOrch initialization failed: " - "failed to query supported %s ACL actions", - (attr.id == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ? "ingress" : "egress")); + "failed to query supported %s ACL actions", stage_str); } - SWSS_LOG_INFO("Supported %s action count %d:", - (attr.id == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ? "ingress" : "egress"), + SWSS_LOG_INFO("Supported %s action count %d:", stage_str, attr.value.aclcapability.action_list.count); for (size_t i = 0; i < static_cast(attr.value.aclcapability.action_list.count); i++) { auto action = static_cast(action_list[i]); - auto stage = (attr.id == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ? ACL_STAGE_INGRESS : ACL_STAGE_EGRESS); m_aclCapabilities[stage].insert(action); SWSS_LOG_INFO(" %s", sai_serialize_enum(action, &sai_metadata_enum_sai_acl_action_type_t).c_str()); } + + // put capabilities in state DB + + string delimiter; + ostringstream acl_action_value_stream; + auto& acl_action_set = m_aclCapabilities[stage]; + auto get_table_field = [stage_str](const std::string& action_name) { + return std::string("ACL_ACTION") + "|" + stage_str + "|" + action_name; + }; + + for (const auto& it: aclL3ActionLookup) + { + auto saiAction = getAclActionFromAclEntry(it.second); + if (acl_action_set.find(saiAction) != acl_action_set.cend()) + { + acl_action_value_stream << delimiter << it.first; + delimiter = comma; + } + } + fvVector.emplace_back(get_table_field(ACTION_PACKET_ACTION), acl_action_value_stream.str()); + acl_action_value_stream.str(std::string()); + delimiter.clear(); + + for (const auto& it: aclMirrorStageLookup) + { + auto saiAction = getAclActionFromAclEntry(it.second); + if (acl_action_set.find(saiAction) != acl_action_set.cend()) + { + acl_action_value_stream << delimiter << it.first; + delimiter = comma; + } + } + fvVector.emplace_back(get_table_field(ACTION_MIRROR_ACTION), acl_action_value_stream.str()); + acl_action_value_stream.str(std::string()); + delimiter.clear(); + + for (const auto& it: aclDTelActionLookup) + { + auto saiAction = getAclActionFromAclEntry(it.second); + if (acl_action_set.find(saiAction) != acl_action_set.cend()) + { + fvVector.emplace_back(get_table_field(it.first), string()); + } + } + + m_switchTable.set("switch", fvVector); } } From de0275939653f20d507703f6b732029645c944eb Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Sun, 30 Jun 2019 18:59:35 +0300 Subject: [PATCH 04/19] [test_acl] add acl rule validation test cases Signed-off-by: Stepan Blyschak --- tests/test_acl.py | 217 +++++++++++++++++++++++++++++++++------------- 1 file changed, 158 insertions(+), 59 deletions(-) diff --git a/tests/test_acl.py b/tests/test_acl.py index 197026d548..3f7199bd70 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -3,18 +3,25 @@ import re import json -class TestAcl(object): + +class BaseTestAcl(object): + """ base class with helpers for Test classes """ def setup_db(self, dvs): self.pdb = swsscommon.DBConnector(0, dvs.redis_sock, 0) self.adb = swsscommon.DBConnector(1, dvs.redis_sock, 0) self.cdb = swsscommon.DBConnector(4, dvs.redis_sock, 0) self.sdb = swsscommon.DBConnector(6, dvs.redis_sock, 0) - def create_acl_table(self, table, type, ports): + def create_acl_table(self, table, type, ports, stage=None): tbl = swsscommon.Table(self.cdb, "ACL_TABLE") - fvs = swsscommon.FieldValuePairs([("policy_desc", "test"), - ("type", type), - ("ports", ",".join(ports))]) + table_props = [("policy_desc", "test"), + ("type", type), + ("ports", ",".join(ports))] + + if stage is not None: + table_props += [("stage", stage)] + + fvs = swsscommon.FieldValuePairs(table_props) tbl.set(table, fvs) time.sleep(1) @@ -124,6 +131,61 @@ def verify_acl_port_binding(self, dvs, adb, bind_ports): assert len(port_groups) == len(bind_ports) assert set(port_groups) == set(acl_table_groups) + def check_rule_existence(self, entry, rules, verifs): + """ helper function to verify if rule exists """ + + for rule in rules: + ruleD = dict(rule) + # find the rule to match with based on priority + if ruleD["PRIORITY"] == entry['SAI_ACL_ENTRY_ATTR_PRIORITY']: + ruleIndex = rules.index(rule) + # use verification dictionary to match entry to rule + for key in verifs[ruleIndex]: + assert verifs[ruleIndex][key] == entry[key] + return True + return False + + def create_acl_rule(self, table, rule, field, value): + tbl = swsscommon.Table(self.cdb, "ACL_RULE") + fvs = swsscommon.FieldValuePairs([("priority", "666"), + ("PACKET_ACTION", "FORWARD"), + (field, value)]) + tbl.set(table + "|" + rule, fvs) + time.sleep(1) + + def remove_acl_rule(self, table, rule): + tbl = swsscommon.Table(self.cdb, "ACL_RULE") + tbl._del(table + "|" + rule) + time.sleep(1) + + def verify_acl_rule(self, dvs, field, value): + acl_table_id = self.get_acl_table_id(dvs) + + tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY") + acl_entries = [k for k in tbl.getKeys() if k not in dvs.asicdb.default_acl_entries] + assert len(acl_entries) == 1 + + (status, fvs) = tbl.get(acl_entries[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] == "666" + elif fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_COUNTER": + assert True + elif fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION": + assert fv[1] == "SAI_PACKET_ACTION_FORWARD" + elif fv[0] == field: + assert fv[1] == value + else: + assert False + + +class TestAcl(BaseTestAcl): def test_AclTableCreation(self, dvs, testlog): self.setup_db(dvs) db = swsscommon.DBConnector(4, dvs.redis_sock, 0) @@ -923,21 +985,6 @@ def test_V6AclTableDeletion(self, dvs, testlog): # only the default table was left assert len(keys) >= 1 - #helper function to verify if rule exists - def check_rule_existence(self, entry, rules, verifs): - for rule in rules: - ruleD = dict(rule) - #find the rule to match with based on priority - if ruleD["PRIORITY"] == entry['SAI_ACL_ENTRY_ATTR_PRIORITY']: - ruleIndex = rules.index(rule) - #use verification dictionary to match entry to rule - for key in verifs[ruleIndex]: - assert verifs[ruleIndex][key] == entry[key] - #found the rule - return True - #did not find the rule - return False - def test_InsertAclRuleBetweenPriorities(self, dvs, testlog): self.setup_db(dvs) db = swsscommon.DBConnector(4, dvs.redis_sock, 0) @@ -1123,45 +1170,6 @@ def test_RulesWithDiffMaskLengths(self, dvs, testlog): keys = atbl.getKeys() assert len(keys) >= 1 - def create_acl_rule(self, table, rule, field, value): - tbl = swsscommon.Table(self.cdb, "ACL_RULE") - fvs = swsscommon.FieldValuePairs([("priority", "666"), - ("PACKET_ACTION", "FORWARD"), - (field, value)]) - tbl.set(table + "|" + rule, fvs) - time.sleep(1) - - def remove_acl_rule(self, table, rule): - tbl = swsscommon.Table(self.cdb, "ACL_RULE") - tbl._del(table + "|" + rule) - time.sleep(1) - - def verify_acl_rule(self, dvs, field, value): - acl_table_id = self.get_acl_table_id(dvs) - - tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY") - acl_entries = [k for k in tbl.getKeys() if k not in dvs.asicdb.default_acl_entries] - assert len(acl_entries) == 1 - - (status, fvs) = tbl.get(acl_entries[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] == "666" - elif fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_COUNTER": - assert True - elif fv[0] == "SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION": - assert fv[1] == "SAI_PACKET_ACTION_FORWARD" - elif fv[0] == field: - assert fv[1] == value - else: - assert False - def test_AclRuleIcmp(self, dvs, testlog): self.setup_db(dvs) @@ -1206,3 +1214,94 @@ def test_AclRuleIcmpV6(self, dvs, testlog): self.remove_acl_rule(acl_table, acl_rule) self.remove_acl_table(acl_table) + + +class TestAclRuleValidation(BaseTestAcl): + """ Test class for cases that check if orchagent corectly validates + ACL rules input + """ + + SWITCH_CAPABILITY_TABLE = "SWITCH_CAPABILITY" + + def get_acl_actions_supported(self, stage, action_name): + capability_table = swsscommon.Table(self.sdb, self.SWITCH_CAPABILITY_TABLE) + keys = capability_table.getKeys() + # one switch available + assert len(keys) == 1, str(keys) + status, fvs = capability_table.get(keys[0]) + assert status == True, str(fvs) + + field = "ACL_ACTION|{}|{}".format(stage.upper(), action_name.upper()) + fvs = dict(fvs) + + values_list = fvs.get(field, None) + + if values_list is not None: + values_list = values_list.split(",") + + return values_list + + def test_AclActionValidation(self, dvs, testlog): + """ The test overrides R/O SAI_SWITCH_ATTR_ACL_STAGE_INGRESS/EGRESS switch attributes + to check the case when orchagent refuses to process rules with action that is not supported + by the ASIC + """ + + self.setup_db(dvs) + + stage_name_map = { + "ingress": "SAI_SWITCH_ATTR_ACL_STAGE_INGRESS", + "egress": "SAI_SWITCH_ATTR_ACL_STAGE_EGRESS", + } + + for stage in stage_name_map: + action_values = self.get_acl_actions_supported(stage, "PACKET_ACTION") + + # virtual switch supports all actions + assert action_values is not None + assert "FORWARD" in action_values + + sai_acl_stage = stage_name_map[stage] + + # mock switch attribute in VS so only REDIRECT action is supported on this stage + dvs.setReadOnlyAttr("SAI_OBJECT_TYPE_SWITCH", + sai_acl_stage, + # FIXME: here should use sai_serialize_value() for acl_capability_t + # but it is not available in VS testing infrastructure + "false:1:SAI_ACL_ACTION_TYPE_REDIRECT") + + # restart SWSS so orchagent will query updated switch attributes + dvs.stop_swss() + dvs.start_swss() + # wait for daemons to start + time.sleep(2) + # reinit ASIC DB validator object + dvs.init_asicdb_validator() + + action_values = self.get_acl_actions_supported(stage, "PACKET_ACTION") + # now, FORWARD is not supported + # and REDIRECT is supported + assert "FORWARD" not in action_values + assert "REDIRECT" in action_values + + # try to create a forward rule + + acl_table = "TEST_TABLE" + acl_rule = "TEST_RULE" + + bind_ports = ["Ethernet0", "Ethernet4"] + + self.create_acl_table(acl_table, "L3", bind_ports, stage=stage) + self.create_acl_rule(acl_table, acl_rule, "ICMP_TYPE", "8") + + atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY") + keys = atbl.getKeys() + + # verify there are not non-default ACL rules created + acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries] + assert len(acl_entry) == 0 + + self.remove_acl_table(acl_table) + + dvs.restart() + time.sleep(20) From dce9fa683a2ffd11fb253a2ffd5f57651ac8c24a Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Mon, 1 Jul 2019 12:19:53 +0300 Subject: [PATCH 05/19] [doc] update schema for mirror rule Signed-off-by: Stepan Blyschak --- doc/swss-schema.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/swss-schema.md b/doc/swss-schema.md index befe0c248c..0a64713ece 100644 --- a/doc/swss-schema.md +++ b/doc/swss-schema.md @@ -459,7 +459,8 @@ Stores rules associated with a specific ACL table on the switch. : next-hop ip address Example: "10.0.0.1" : next-hop group set of addresses Example: "10.0.0.1,10.0.0.3" - mirror_action = 1*255VCHAR ; refer to the mirror session + mirror_action = "ingress:"/"egress:"1*255VCHAR + ; refer to the mirror session ether_type = h16 ; Ethernet type field From 758735eb1142a2cd5749ab86952f87f41e91f437 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Mon, 1 Jul 2019 17:41:22 +0300 Subject: [PATCH 06/19] [aclorch] replace throw with SWSS_LOG_THROW and limit scope for stringstream in queryAclActionCapabilities Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 64 +++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 669ca47cda..cda154e7aa 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -999,6 +999,9 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) } split(attr_value, attr_values, ':'); + // expect format "stage:mirror_session_name" + // or just "mirror_session_name" and stage + // is ingress implicitely if (attr_values.size() > 2) { SWSS_LOG_ERROR("Invalid mirror rule action %s:%s", @@ -1022,7 +1025,7 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) if (!m_pMirrorOrch->getSessionOid(m_sessionName, m_mirrorSessionOid)) { - throw runtime_error("Failed to get mirror session OID"); + SWSS_LOG_THROW("Failed to get mirror session OID for session %s", m_sessionName.c_str()); } value.aclaction.enable = true; @@ -1123,14 +1126,14 @@ bool AclRuleMirror::create() if (!m_pMirrorOrch->getSessionStatus(m_sessionName, state)) { - throw runtime_error("Failed to get mirror session state"); + SWSS_LOG_THROW("Failed to get mirror session state for session %s", m_sessionName.c_str()); } // Increase session reference count regardless of state to deny // attempt to remove mirror session with attached ACL rules. if (!m_pMirrorOrch->increaseRefCount(m_sessionName)) { - throw runtime_error("Failed to increase mirror session reference count"); + SWSS_LOG_THROW("Failed to increase mirror session reference count for session %s", m_sessionName.c_str()); } if (!state) @@ -2167,45 +2170,54 @@ void AclOrch::queryAclActionCapability() // put capabilities in state DB - string delimiter; - ostringstream acl_action_value_stream; - auto& acl_action_set = m_aclCapabilities[stage]; auto get_table_field = [stage_str](const std::string& action_name) { return std::string("ACL_ACTION") + "|" + stage_str + "|" + action_name; }; + auto& acl_action_set = m_aclCapabilities[stage]; - for (const auto& it: aclL3ActionLookup) + // PACKET_ACTION { - auto saiAction = getAclActionFromAclEntry(it.second); - if (acl_action_set.find(saiAction) != acl_action_set.cend()) + string delimiter; + ostringstream acl_action_value_stream; + + for (const auto& it: aclL3ActionLookup) { - acl_action_value_stream << delimiter << it.first; - delimiter = comma; + auto saiAction = getAclActionFromAclEntry(it.second); + if (acl_action_set.find(saiAction) != acl_action_set.cend()) + { + acl_action_value_stream << delimiter << it.first; + delimiter = comma; + } } + fvVector.emplace_back(get_table_field(ACTION_PACKET_ACTION), acl_action_value_stream.str()); } - fvVector.emplace_back(get_table_field(ACTION_PACKET_ACTION), acl_action_value_stream.str()); - acl_action_value_stream.str(std::string()); - delimiter.clear(); - for (const auto& it: aclMirrorStageLookup) + // MIRROR_ACTION { - auto saiAction = getAclActionFromAclEntry(it.second); - if (acl_action_set.find(saiAction) != acl_action_set.cend()) + string delimiter; + ostringstream acl_action_value_stream; + + for (const auto& it: aclMirrorStageLookup) { - acl_action_value_stream << delimiter << it.first; - delimiter = comma; + auto saiAction = getAclActionFromAclEntry(it.second); + if (acl_action_set.find(saiAction) != acl_action_set.cend()) + { + acl_action_value_stream << delimiter << it.first; + delimiter = comma; + } } + fvVector.emplace_back(get_table_field(ACTION_MIRROR_ACTION), acl_action_value_stream.str()); } - fvVector.emplace_back(get_table_field(ACTION_MIRROR_ACTION), acl_action_value_stream.str()); - acl_action_value_stream.str(std::string()); - delimiter.clear(); - for (const auto& it: aclDTelActionLookup) + // DTEL actions { - auto saiAction = getAclActionFromAclEntry(it.second); - if (acl_action_set.find(saiAction) != acl_action_set.cend()) + for (const auto& it: aclDTelActionLookup) { - fvVector.emplace_back(get_table_field(it.first), string()); + auto saiAction = getAclActionFromAclEntry(it.second); + if (acl_action_set.find(saiAction) != acl_action_set.cend()) + { + fvVector.emplace_back(get_table_field(it.first), string()); + } } } From d2039632af4b72bdda64824ed2288c4a4ceee70e Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Tue, 2 Jul 2019 11:54:29 +0300 Subject: [PATCH 07/19] [aclorch] new schema Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 167 +++++++++++++++++------------------------- orchagent/aclorch.h | 4 +- tests/test_acl.py | 22 +++--- tests/test_mirror.py | 10 ++- 4 files changed, 89 insertions(+), 114 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index cda154e7aa..14423f9452 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -61,14 +61,19 @@ acl_rule_attr_lookup_t aclMatchLookup = { MATCH_INNER_L4_DST_PORT, SAI_ACL_ENTRY_ATTR_FIELD_INNER_L4_DST_PORT } }; -acl_rule_attr_lookup_t aclL3ActionLookup = +static acl_rule_attr_lookup_t aclL3ActionLookup = { - { PACKET_ACTION_FORWARD, SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION }, - { PACKET_ACTION_DROP, SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION }, - { PACKET_ACTION_REDIRECT, SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT } + { ACTION_PACKET_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION }, + { ACTION_REDIRECT_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_REDIRECT }, }; -acl_rule_attr_lookup_t aclDTelActionLookup = +static acl_rule_attr_lookup_t aclMirrorStageLookup = +{ + {ACTION_MIRROR_INGRESS_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS}, + {ACTION_MIRROR_EGRESS_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_EGRESS}, +}; + +static acl_rule_attr_lookup_t aclDTelActionLookup = { { ACTION_DTEL_FLOW_OP, SAI_ACL_ENTRY_ATTR_ACTION_ACL_DTEL_FLOW_OP }, { ACTION_DTEL_INT_SESSION, SAI_ACL_ENTRY_ATTR_ACTION_DTEL_INT_SESSION }, @@ -78,7 +83,7 @@ acl_rule_attr_lookup_t aclDTelActionLookup = { ACTION_DTEL_REPORT_ALL_PACKETS, SAI_ACL_ENTRY_ATTR_ACTION_DTEL_REPORT_ALL_PACKETS } }; -acl_dtel_flow_op_type_lookup_t aclDTelFlowOpTypeLookup = +static acl_dtel_flow_op_type_lookup_t aclDTelFlowOpTypeLookup = { { DTEL_FLOW_OP_NOP, SAI_ACL_DTEL_FLOW_OP_NOP }, { DTEL_FLOW_OP_POSTCARD, SAI_ACL_DTEL_FLOW_OP_POSTCARD }, @@ -104,12 +109,6 @@ static acl_stage_type_lookup_t aclStageLookUp = {STAGE_EGRESS, ACL_STAGE_EGRESS } }; -static acl_rule_attr_lookup_t aclMirrorStageLookup = -{ - {STAGE_INGRESS, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS}, - {STAGE_EGRESS, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_EGRESS}, -}; - static acl_ip_type_lookup_t aclIpTypeLookup = { { IP_TYPE_ANY, SAI_ACL_IP_TYPE_ANY }, @@ -616,12 +615,10 @@ shared_ptr AclRule::makeShared(acl_table_type_t type, AclOrch *acl, Mir { string attr_name = to_upper(fvField(itr)); string attr_value = fvValue(itr); - if (attr_name == ACTION_PACKET_ACTION || attr_name == ACTION_MIRROR_ACTION || - attr_name == ACTION_DTEL_FLOW_OP || attr_name == ACTION_DTEL_INT_SESSION || - attr_name == ACTION_DTEL_DROP_REPORT_ENABLE || - attr_name == ACTION_DTEL_TAIL_DROP_REPORT_ENABLE || - attr_name == ACTION_DTEL_FLOW_SAMPLE_PERCENT || - attr_name == ACTION_DTEL_REPORT_ALL_PACKETS) + if (aclL3ActionLookup.find(attr_name) != aclL3ActionLookup.cend() || + aclMirrorStageLookup.find(attr_name) != aclMirrorStageLookup.cend() || + attr_name == ACTION_MIRROR_ACTION || + aclDTelActionLookup.find(attr_name) != aclDTelActionLookup.cend()) { action_found = true; action = attr_name; @@ -646,7 +643,8 @@ shared_ptr AclRule::makeShared(acl_table_type_t type, AclOrch *acl, Mir } /* Mirror rules can exist in both tables */ - if (action == ACTION_MIRROR_ACTION) + if (aclMirrorStageLookup.find(action) != aclMirrorStageLookup.cend() || + action == ACTION_MIRROR_ACTION /* implicitly ingress in old schema */) { return make_shared(acl, mirror, rule, table, type); } @@ -768,24 +766,41 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value) string attr_value = to_upper(_attr_value); sai_attribute_value_t value; - if (attr_name != ACTION_PACKET_ACTION) - { - return false; - } + auto action_str = attr_name; - if (attr_value == PACKET_ACTION_FORWARD) - { - value.aclaction.parameter.s32 = SAI_PACKET_ACTION_FORWARD; - } - else if (attr_value == PACKET_ACTION_DROP) + if (attr_name == ACTION_PACKET_ACTION) { - value.aclaction.parameter.s32 = SAI_PACKET_ACTION_DROP; + if (attr_value == PACKET_ACTION_FORWARD) + { + value.aclaction.parameter.s32 = SAI_PACKET_ACTION_FORWARD; + } + else if (attr_value == PACKET_ACTION_DROP) + { + value.aclaction.parameter.s32 = SAI_PACKET_ACTION_DROP; + } + else if (attr_value.find(PACKET_ACTION_REDIRECT) != string::npos) + { + // handle PACKET_ACTION_REDIRECT in ACTION_PACKET_ACTION for backward compatibility + + // resize attr_value to remove argument, _attr_value still has the argument + attr_value.resize(string(PACKET_ACTION_REDIRECT).length()); + + sai_object_id_t param_id = getRedirectObjectId(_attr_value); + if (param_id == SAI_NULL_OBJECT_ID) + { + return false; + } + value.aclaction.parameter.oid = param_id; + + action_str = ACTION_REDIRECT_ACTION; + } + else + { + return false; + } } - else if (attr_value.find(PACKET_ACTION_REDIRECT) != string::npos) + else if (attr_name == ACTION_REDIRECT_ACTION) { - // resize attr_value to remove argument, _attr_value still has the argument - attr_value.resize(string(PACKET_ACTION_REDIRECT).length()); - sai_object_id_t param_id = getRedirectObjectId(_attr_value); if (param_id == SAI_NULL_OBJECT_ID) { @@ -797,9 +812,10 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value) { return false; } + value.aclaction.enable = true; - m_actions[aclL3ActionLookup[attr_value]] = value; + m_actions[aclL3ActionLookup[action_str]] = value; return AclRule::validateAddAction(attr_name, attr_value); } @@ -990,29 +1006,19 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) SWSS_LOG_ENTER(); sai_attribute_value_t value; - string stage = STAGE_INGRESS; - vector attr_values; + sai_acl_entry_attr_t action; - if (attr_name != ACTION_MIRROR_ACTION) + if (attr_name == ACTION_MIRROR_ACTION) { - return false; + action = SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS; } - - split(attr_value, attr_values, ':'); - // expect format "stage:mirror_session_name" - // or just "mirror_session_name" and stage - // is ingress implicitely - if (attr_values.size() > 2) + else if (aclMirrorStageLookup.find(attr_name) != aclMirrorStageLookup.cend()) { - SWSS_LOG_ERROR("Invalid mirror rule action %s:%s", - attr_name.c_str(), attr_value.c_str()); - return false; + action = aclMirrorStageLookup[attr_name]; } - else if (attr_values.size() == 2) + else { - stage = to_upper(attr_values[0]); - // strip stage value - attr_value = attr_values[1]; + return false; } m_sessionName = attr_value; @@ -1032,15 +1038,7 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) value.aclaction.parameter.objlist.list = &m_mirrorSessionOid; value.aclaction.parameter.objlist.count = 1; - if (aclMirrorStageLookup.find(stage) == aclMirrorStageLookup.end()) - { - SWSS_LOG_ERROR("Invalid stage parameter in mirror rule: %s", stage.c_str()); - return false; - } - - m_mirrorStage = stage; - - m_actions[aclMirrorStageLookup[m_mirrorStage]] = value; + m_actions[action] = value; return AclRule::validateAddAction(attr_name, attr_value); } @@ -2170,55 +2168,26 @@ void AclOrch::queryAclActionCapability() // put capabilities in state DB - auto get_table_field = [stage_str](const std::string& action_name) { - return std::string("ACL_ACTION") + "|" + stage_str + "|" + action_name; - }; + auto field = std::string("ACL_ACTION") + '|' + stage_str; auto& acl_action_set = m_aclCapabilities[stage]; - // PACKET_ACTION - { - string delimiter; - ostringstream acl_action_value_stream; - - for (const auto& it: aclL3ActionLookup) - { - auto saiAction = getAclActionFromAclEntry(it.second); - if (acl_action_set.find(saiAction) != acl_action_set.cend()) - { - acl_action_value_stream << delimiter << it.first; - delimiter = comma; - } - } - fvVector.emplace_back(get_table_field(ACTION_PACKET_ACTION), acl_action_value_stream.str()); - } - - // MIRROR_ACTION { string delimiter; ostringstream acl_action_value_stream; - for (const auto& it: aclMirrorStageLookup) + for (const auto& action_map: {aclL3ActionLookup, aclMirrorStageLookup, aclDTelActionLookup}) { - auto saiAction = getAclActionFromAclEntry(it.second); - if (acl_action_set.find(saiAction) != acl_action_set.cend()) + for (const auto& it: action_map) { - acl_action_value_stream << delimiter << it.first; - delimiter = comma; - } - } - fvVector.emplace_back(get_table_field(ACTION_MIRROR_ACTION), acl_action_value_stream.str()); - } - - // DTEL actions - { - for (const auto& it: aclDTelActionLookup) - { - auto saiAction = getAclActionFromAclEntry(it.second); - if (acl_action_set.find(saiAction) != acl_action_set.cend()) - { - fvVector.emplace_back(get_table_field(it.first), string()); + auto saiAction = getAclActionFromAclEntry(it.second); + if (acl_action_set.find(saiAction) != acl_action_set.cend()) + { + acl_action_value_stream << delimiter << it.first; + delimiter = comma; + } } } + fvVector.emplace_back(field, acl_action_value_stream.str()); } m_switchTable.set("switch", fvVector); diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index d6acf729f1..733e8c7462 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -62,7 +62,10 @@ #define MATCH_INNER_L4_DST_PORT "INNER_L4_DST_PORT" #define ACTION_PACKET_ACTION "PACKET_ACTION" +#define ACTION_REDIRECT_ACTION "REDIRECT_ACTION" #define ACTION_MIRROR_ACTION "MIRROR_ACTION" +#define ACTION_MIRROR_INGRESS_ACTION "MIRROR_INGRESS_ACTION" +#define ACTION_MIRROR_EGRESS_ACTION "MIRROR_EGRESS_ACTION" #define ACTION_DTEL_FLOW_OP "FLOW_OP" #define ACTION_DTEL_INT_SESSION "INT_SESSION" #define ACTION_DTEL_DROP_REPORT_ENABLE "DROP_REPORT_ENABLE" @@ -278,7 +281,6 @@ class AclRuleMirror: public AclRule protected: bool m_state {false}; - string m_mirrorStage; string m_sessionName; sai_object_id_t m_mirrorSessionOid {SAI_NULL_OBJECT_ID}; AclRuleCounters counters; diff --git a/tests/test_acl.py b/tests/test_acl.py index 3f7199bd70..38220e8168 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -1223,15 +1223,15 @@ class TestAclRuleValidation(BaseTestAcl): SWITCH_CAPABILITY_TABLE = "SWITCH_CAPABILITY" - def get_acl_actions_supported(self, stage, action_name): + def get_acl_actions_supported(self, stage): capability_table = swsscommon.Table(self.sdb, self.SWITCH_CAPABILITY_TABLE) keys = capability_table.getKeys() # one switch available - assert len(keys) == 1, str(keys) + assert len(keys) == 1 status, fvs = capability_table.get(keys[0]) - assert status == True, str(fvs) + assert status == True - field = "ACL_ACTION|{}|{}".format(stage.upper(), action_name.upper()) + field = "ACL_ACTION|{}".format(stage.upper()) fvs = dict(fvs) values_list = fvs.get(field, None) @@ -1255,11 +1255,11 @@ def test_AclActionValidation(self, dvs, testlog): } for stage in stage_name_map: - action_values = self.get_acl_actions_supported(stage, "PACKET_ACTION") + action_values = self.get_acl_actions_supported(stage) # virtual switch supports all actions assert action_values is not None - assert "FORWARD" in action_values + assert "PACKET_ACTION" in action_values sai_acl_stage = stage_name_map[stage] @@ -1278,11 +1278,11 @@ def test_AclActionValidation(self, dvs, testlog): # reinit ASIC DB validator object dvs.init_asicdb_validator() - action_values = self.get_acl_actions_supported(stage, "PACKET_ACTION") - # now, FORWARD is not supported - # and REDIRECT is supported - assert "FORWARD" not in action_values - assert "REDIRECT" in action_values + action_values = self.get_acl_actions_supported(stage) + # now, PACKET_ACTION is not supported + # and REDIRECT_ACTION is supported + assert "PACKET_ACTION" not in action_values + assert "REDIRECT_ACTION" in action_values # try to create a forward rule diff --git a/tests/test_mirror.py b/tests/test_mirror.py index 94bc34ce47..c3da54d43d 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -702,13 +702,17 @@ def remove_acl_table(self, table): time.sleep(1) def create_mirror_acl_dscp_rule(self, table, rule, dscp, session, stage=None): - action_value = session + action_name = "mirror_action" + action_name_map = { + "ingress": "MIRROR_INGRESS_ACTION", + "egress": "MIRROR_EGRESS_ACTION" + } if stage is not None: # else it should be treated as ingress by default in orchagent assert stage in ('ingress', 'egress'), "invalid stage input {}".format(stage) - action_value = "{stage}:{session}".format(stage=stage, session=session) + action_name = action_name_map[stage] tbl = swsscommon.Table(self.cdb, "ACL_RULE") fvs = swsscommon.FieldValuePairs([("priority", "1000"), - ("mirror_action", action_value), + (action_name, session), ("DSCP", dscp)]) tbl.set(table + "|" + rule, fvs) time.sleep(1) From 489e22e3a66cf7342b282c04ee802dbfe90ce8c1 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 3 Jul 2019 10:53:16 +0000 Subject: [PATCH 08/19] [aclorch] fix indent Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 14423f9452..6477442d82 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -2171,25 +2171,23 @@ void AclOrch::queryAclActionCapability() auto field = std::string("ACL_ACTION") + '|' + stage_str; auto& acl_action_set = m_aclCapabilities[stage]; - { - string delimiter; - ostringstream acl_action_value_stream; + string delimiter; + ostringstream acl_action_value_stream; - for (const auto& action_map: {aclL3ActionLookup, aclMirrorStageLookup, aclDTelActionLookup}) + for (const auto& action_map: {aclL3ActionLookup, aclMirrorStageLookup, aclDTelActionLookup}) + { + for (const auto& it: action_map) { - for (const auto& it: action_map) + auto saiAction = getAclActionFromAclEntry(it.second); + if (acl_action_set.find(saiAction) != acl_action_set.cend()) { - auto saiAction = getAclActionFromAclEntry(it.second); - if (acl_action_set.find(saiAction) != acl_action_set.cend()) - { - acl_action_value_stream << delimiter << it.first; - delimiter = comma; - } + acl_action_value_stream << delimiter << it.first; + delimiter = comma; } } - fvVector.emplace_back(field, acl_action_value_stream.str()); } + fvVector.emplace_back(field, acl_action_value_stream.str()); m_switchTable.set("switch", fvVector); } } From 625388715c26e6cd293c93a5011364ee52722660 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 3 Jul 2019 13:59:29 +0300 Subject: [PATCH 09/19] [aclorch] fix ACL_ACTION -> ACL_ACTIONS Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 6477442d82..74de825b65 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -2168,7 +2168,7 @@ void AclOrch::queryAclActionCapability() // put capabilities in state DB - auto field = std::string("ACL_ACTION") + '|' + stage_str; + auto field = std::string("ACL_ACTIONS") + '|' + stage_str; auto& acl_action_set = m_aclCapabilities[stage]; string delimiter; From 1d10f150f2028594a4d6d036bbca9d4ecb4163e0 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 3 Jul 2019 14:33:31 +0300 Subject: [PATCH 10/19] [test_acl] fix ACL_ACTION -> ACL_ACTIONS --- tests/test_acl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_acl.py b/tests/test_acl.py index 38220e8168..7bf8cbc777 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -1231,7 +1231,7 @@ def get_acl_actions_supported(self, stage): status, fvs = capability_table.get(keys[0]) assert status == True - field = "ACL_ACTION|{}".format(stage.upper()) + field = "ACL_ACTIONS|{}".format(stage.upper()) fvs = dict(fvs) values_list = fvs.get(field, None) From 808629db39c89ec8188badd44b988bc9bfa8d8ce Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 3 Jul 2019 20:37:26 +0300 Subject: [PATCH 11/19] [aclorch] update according to design Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 32 ++++++++++++++++++-------------- orchagent/aclorch.h | 1 + 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 74de825b65..674e7afd8d 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -69,8 +69,8 @@ static acl_rule_attr_lookup_t aclL3ActionLookup = static acl_rule_attr_lookup_t aclMirrorStageLookup = { - {ACTION_MIRROR_INGRESS_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS}, - {ACTION_MIRROR_EGRESS_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_EGRESS}, + { ACTION_MIRROR_INGRESS_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS}, + { ACTION_MIRROR_EGRESS_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_EGRESS}, }; static acl_rule_attr_lookup_t aclDTelActionLookup = @@ -83,6 +83,12 @@ static acl_rule_attr_lookup_t aclDTelActionLookup = { ACTION_DTEL_REPORT_ALL_PACKETS, SAI_ACL_ENTRY_ATTR_ACTION_DTEL_REPORT_ALL_PACKETS } }; +static acl_packet_action_lookup_t aclPacketActionLookup = +{ + { PACKET_ACTION_FORWARD, SAI_PACKET_ACTION_FORWARD }, + { PACKET_ACTION_DROP, SAI_PACKET_ACTION_DROP }, +}; + static acl_dtel_flow_op_type_lookup_t aclDTelFlowOpTypeLookup = { { DTEL_FLOW_OP_NOP, SAI_ACL_DTEL_FLOW_OP_NOP }, @@ -770,18 +776,14 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value) if (attr_name == ACTION_PACKET_ACTION) { - if (attr_value == PACKET_ACTION_FORWARD) - { - value.aclaction.parameter.s32 = SAI_PACKET_ACTION_FORWARD; - } - else if (attr_value == PACKET_ACTION_DROP) + const auto it = aclPacketActionLookup.find(attr_value); + if (it != aclPacketActionLookup.cend()) { - value.aclaction.parameter.s32 = SAI_PACKET_ACTION_DROP; + value.aclaction.parameter.s32 = it->second; } + // handle PACKET_ACTION_REDIRECT in ACTION_PACKET_ACTION for backward compatibility else if (attr_value.find(PACKET_ACTION_REDIRECT) != string::npos) { - // handle PACKET_ACTION_REDIRECT in ACTION_PACKET_ACTION for backward compatibility - // resize attr_value to remove argument, _attr_value still has the argument attr_value.resize(string(PACKET_ACTION_REDIRECT).length()); @@ -1008,13 +1010,15 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) sai_attribute_value_t value; sai_acl_entry_attr_t action; - if (attr_name == ACTION_MIRROR_ACTION) + const auto it = aclMirrorStageLookup.find(attr_name); + if (it != aclMirrorStageLookup.cend()) { - action = SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS; + action = it->second; } - else if (aclMirrorStageLookup.find(attr_name) != aclMirrorStageLookup.cend()) + // handle ACTION_MIRROR_ACTION as ingress by default for backward compatibility + else if (attr_name == ACTION_MIRROR_ACTION) { - action = aclMirrorStageLookup[attr_name]; + action = SAI_ACL_ENTRY_ATTR_ACTION_MIRROR_INGRESS; } else { diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 733e8c7462..fe1a8f1d40 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -116,6 +116,7 @@ typedef map acl_table_type_lookup_t; typedef map acl_rule_attr_lookup_t; typedef map acl_ip_type_lookup_t; typedef map acl_dtel_flow_op_type_lookup_t; +typedef map acl_packet_action_lookup_t; typedef tuple acl_range_properties_t; typedef map> acl_capabilities_t; From b3a598f08cb90f414a1bdc203e926fd4c01b9f27 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 4 Jul 2019 14:55:14 +0300 Subject: [PATCH 12/19] [aclorch] perform secondary query for supported enum values Right now ACL actions that have enum values are - PACKET_ACTION and DTEL_FLOW_OP. Once SAI object API is supported by libsairedis we will query which values are supported by calling "sai_query_attribute_enum_values_capability"; untill then we will assume all values are supported and populate this in DB Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 123 ++++++++++++++++++++++++++++++++++++++++++ orchagent/aclorch.h | 10 +++- 2 files changed, 131 insertions(+), 2 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 674e7afd8d..f2fe3abf23 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -401,6 +401,26 @@ bool AclRule::validateAddAction(string attr_name, string attr_value) attr_name.c_str(), attr_value.c_str()); return false; } + + // check if ACL action attribute entry parameter is an enum value + const auto* meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, it.first); + if (meta == nullptr) + { + SWSS_LOG_THROW("Metadata null pointer returned by sai_metadata_get_attr_metadata for action %s", + attr_name.c_str()); + } + if (meta->isenum) + { + // if ACL action attribute requiers enum value check if value is supported by the ASIC + if (!m_pAclOrch->isAclActionEnumValueSupported(AclOrch::getAclActionFromAclEntry(it.first), + it.second.aclaction.parameter)) + { + SWSS_LOG_ERROR("Action %s:%s is not supported by ASIC", + attr_name.c_str(), attr_value.c_str()); + return false; + } + } + } return true; } @@ -2194,6 +2214,99 @@ void AclOrch::queryAclActionCapability() fvVector.emplace_back(field, acl_action_value_stream.str()); m_switchTable.set("switch", fvVector); } + + /* For those ACL action entry attributes for which acl parameter is enumeration (metadata->isenum == true) + * we can query enum values which are implemented by vendor SAI. + * For this purpose we may want to use "sai_query_attribute_enum_values_capability" + * from SAI object API call (saiobject.h). + * However, right now libsairedis does not support SAI object API, so we will just + * put all values as supported for now. + */ + + queryAclActionAttrEnumValues(ACTION_PACKET_ACTION, + aclL3ActionLookup, + aclPacketActionLookup); + queryAclActionAttrEnumValues(ACTION_DTEL_FLOW_OP, + aclDTelActionLookup, + aclDTelFlowOpTypeLookup); +} + +template +void AclOrch::queryAclActionAttrEnumValues(const string &action_name, + const acl_rule_attr_lookup_t& ruleAttrLookupMap, + const AclActionAttrLookupT lookupMap) +{ + vector fvVector; + auto acl_attr = ruleAttrLookupMap.at(action_name); + auto acl_action = getAclActionFromAclEntry(acl_attr); + + /* if the action is not supported then no need to do secondary query for + * supported values + */ + if (isAclActionSupported(ACL_STAGE_INGRESS, acl_action) || + isAclActionSupported(ACL_STAGE_EGRESS, acl_action)) + { + string delimiter; + ostringstream acl_action_value_stream; + auto field = std::string("ACL_ACTION") + '|' + action_name; + + const auto* meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, acl_attr); + if (meta == nullptr) + { + SWSS_LOG_THROW("Metadata null pointer returned by sai_metadata_get_attr_metadata for action %s", + action_name.c_str()); + } + + if (!meta->isenum) + { + SWSS_LOG_THROW("%s is not an enum", action_name.c_str()); + } + + // TODO: once sai object api is available make this code compile +#if 0 + vector values_list(meta->enummetadata->valuescount); + sai_s32_list_t values; + values.count = static_cast(values_list.size()); + values.list = values_list.data(); + + auto status = sai_query_attribute_enum_values_capability(gSwitchId, + SAI_OBJECT_TYPE_ACL_ENTRY, + acl_attr, + &values); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_THROW("sai_query_attribute_enum_values_capability failed for %s", + action_name.c_str()); + } + + for (size_t i = 0; i < values.count; i++) + { + m_aclEnumActionCapabilities[acl_action].insert(values.list[i]); + } +#else + /* assume all enum values are supported untill sai object api is available */ + for (size_t i = 0; i < meta->enummetadata->valuescount; i++) + { + m_aclEnumActionCapabilities[acl_action].insert(meta->enummetadata->values[i]); + } +#endif + + // put supported values in DB + for (const auto& it: lookupMap) + { + const auto foundIt = m_aclEnumActionCapabilities[acl_action].find(it.second); + if (foundIt == m_aclEnumActionCapabilities[acl_action].cend()) + { + continue; + } + acl_action_value_stream << delimiter << it.first; + delimiter = comma; + } + + fvVector.emplace_back(field, acl_action_value_stream.str()); + } + + m_switchTable.set("switch", fvVector); } sai_acl_action_type_t AclOrch::getAclActionFromAclEntry(sai_acl_entry_attr_t attr) @@ -2468,6 +2581,16 @@ bool AclOrch::isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t return it->second.find(action) != it->second.cend(); } +bool AclOrch::isAclActionEnumValueSupported(sai_acl_action_type_t action, sai_acl_action_parameter_t param) const +{ + const auto& it = m_aclEnumActionCapabilities.find(action); + if (it == m_aclEnumActionCapabilities.cend()) + { + return false; + } + return it->second.find(param.s32) != it->second.cend(); +} + void AclOrch::doAclTableTask(Consumer &consumer) { SWSS_LOG_ENTER(); diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index fe1a8f1d40..c84c8681ac 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -119,6 +119,7 @@ typedef map acl_dtel_flow_op_type_lookup_t; typedef map acl_packet_action_lookup_t; typedef tuple acl_range_properties_t; typedef map> acl_capabilities_t; +typedef map> acl_action_enum_values_capabilities_t; class AclOrch; @@ -169,8 +170,6 @@ struct AclRuleCounters } }; -class AclTable; - class AclRule { public: @@ -418,6 +417,7 @@ class AclOrch : public Orch, public Observer bool isCombinedMirrorV6Table(); bool isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t action) const; + bool isAclActionEnumValueSupported(sai_acl_action_type_t action, sai_acl_action_parameter_t param) const; bool m_isCombinedMirrorV6Table = true; map m_mirrorTableCapabilities; @@ -434,6 +434,11 @@ class AclOrch : public Orch, public Observer void queryMirrorTableCapability(); void queryAclActionCapability(); + template + void queryAclActionAttrEnumValues(const string& action_name, + const acl_rule_attr_lookup_t& ruleAttrLookupMap, + const AclActionAttrLookupT lookupMap); + static void collectCountersThread(AclOrch *pAclOrch); bool createBindAclTable(AclTable &aclTable, sai_object_id_t &table_oid); @@ -461,6 +466,7 @@ class AclOrch : public Orch, public Observer string m_mirrorV6TableId; acl_capabilities_t m_aclCapabilities; + acl_action_enum_values_capabilities_t m_aclEnumActionCapabilities; }; #endif /* SWSS_ACLORCH_H */ From b24782739becd657dcd3c0332fbfff9ca89bc97b Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 4 Jul 2019 15:28:50 +0300 Subject: [PATCH 13/19] [swss-schema.md] update schema according to design --- doc/swss-schema.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/doc/swss-schema.md b/doc/swss-schema.md index 0a64713ece..ac7e25ce5e 100644 --- a/doc/swss-schema.md +++ b/doc/swss-schema.md @@ -459,8 +459,17 @@ Stores rules associated with a specific ACL table on the switch. : next-hop ip address Example: "10.0.0.1" : next-hop group set of addresses Example: "10.0.0.1,10.0.0.3" - mirror_action = "ingress:"/"egress:"1*255VCHAR - ; refer to the mirror session + redirect_action = 1*255CHAR ; redirect parameter + ; This parameter defines a destination for redirected packets + ; it could be: + : name of physical port. Example: "Ethernet10" + : name of LAG port Example: "PortChannel5" + : next-hop ip address Example: "10.0.0.1" + : next-hop group set of addresses Example: "10.0.0.1,10.0.0.3" + + mirror_action = 1*255VCHAR ; refer to the mirror session (by default it will be ingress mirror action) + mirror_ingress_action = 1*255VCHAR ; refer to the mirror session + mirror_egress_action = 1*255VCHAR ; refer to the mirror session ether_type = h16 ; Ethernet type field From 0c806bc1f0643d475bcf8a01636ba54fd73e9400 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 4 Jul 2019 17:11:14 +0300 Subject: [PATCH 14/19] [aclorch] 'if 0' -> 'ifdef SAIREDIS_SUPPORT_OBJECT_API' --- orchagent/aclorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index f2fe3abf23..1130618574 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -2263,7 +2263,7 @@ void AclOrch::queryAclActionAttrEnumValues(const string &action_name, } // TODO: once sai object api is available make this code compile -#if 0 +#ifdef SAIREDIS_SUPPORT_OBJECT_API vector values_list(meta->enummetadata->valuescount); sai_s32_list_t values; values.count = static_cast(values_list.size()); From 9da8e9c237146308f6567bb332eed03242c2ed1a Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 4 Jul 2019 18:11:57 +0300 Subject: [PATCH 15/19] [test_acl] fix vs test Signed-off-by: Stepan Blyschak --- tests/test_acl.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_acl.py b/tests/test_acl.py index 7bf8cbc777..a50b869c5f 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -1303,5 +1303,5 @@ def test_AclActionValidation(self, dvs, testlog): self.remove_acl_table(acl_table) - dvs.restart() - time.sleep(20) + dvs.runcmd("systemctl restart all") + time.sleep(5) From 747501a592583931b0835d69a9dbbb96e8871f80 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 4 Jul 2019 18:51:07 +0300 Subject: [PATCH 16/19] [aclorch] fix mirror rule creation Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 26 +++++++++++++++----------- orchagent/aclorch.h | 1 - 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 1130618574..38cf849fac 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -1027,7 +1027,6 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) { SWSS_LOG_ENTER(); - sai_attribute_value_t value; sai_acl_entry_attr_t action; const auto it = aclMirrorStageLookup.find(attr_name); @@ -1053,16 +1052,8 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) return false; } - if (!m_pMirrorOrch->getSessionOid(m_sessionName, m_mirrorSessionOid)) - { - SWSS_LOG_THROW("Failed to get mirror session OID for session %s", m_sessionName.c_str()); - } - - value.aclaction.enable = true; - value.aclaction.parameter.objlist.list = &m_mirrorSessionOid; - value.aclaction.parameter.objlist.count = 1; - - m_actions[action] = value; + // insert placeholder value, we'll set the session oid in AclRuleMirror::create() + m_actions[action] = sai_attribute_value_t{}; return AclRule::validateAddAction(attr_name, attr_value); } @@ -1144,6 +1135,7 @@ bool AclRuleMirror::create() { SWSS_LOG_ENTER(); + sai_object_id_t oid = SAI_NULL_OBJECT_ID; bool state = false; if (!m_pMirrorOrch->getSessionStatus(m_sessionName, state)) @@ -1163,6 +1155,18 @@ bool AclRuleMirror::create() return true; } + if (!m_pMirrorOrch->getSessionOid(m_sessionName, oid)) + { + SWSS_LOG_THROW("Failed to get mirror session OID for session %s", m_sessionName.c_str()); + } + + for (auto& it: m_actions) + { + it.second.aclaction.enable = true; + it.second.aclaction.parameter.objlist.list = &oid; + it.second.aclaction.parameter.objlist.count = 1; + } + if (!AclRule::create()) { return false; diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index c84c8681ac..45cfd675e7 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -282,7 +282,6 @@ class AclRuleMirror: public AclRule protected: bool m_state {false}; string m_sessionName; - sai_object_id_t m_mirrorSessionOid {SAI_NULL_OBJECT_ID}; AclRuleCounters counters; MirrorOrch *m_pMirrorOrch {nullptr}; }; From f122cee593a5ed20736c6aa9a984cbe6a87a6479 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 7 Aug 2019 12:07:01 +0300 Subject: [PATCH 17/19] [aclorch] fix typos and remove reduntant spaces, new lines Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 38cf849fac..d224b29f20 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -411,7 +411,7 @@ bool AclRule::validateAddAction(string attr_name, string attr_value) } if (meta->isenum) { - // if ACL action attribute requiers enum value check if value is supported by the ASIC + // if ACL action attribute requires enum value check if value is supported by the ASIC if (!m_pAclOrch->isAclActionEnumValueSupported(AclOrch::getAclActionFromAclEntry(it.first), it.second.aclaction.parameter)) { @@ -420,7 +420,6 @@ bool AclRule::validateAddAction(string attr_name, string attr_value) return false; } } - } return true; } @@ -643,6 +642,7 @@ shared_ptr AclRule::makeShared(acl_table_type_t type, AclOrch *acl, Mir string attr_value = fvValue(itr); if (aclL3ActionLookup.find(attr_name) != aclL3ActionLookup.cend() || aclMirrorStageLookup.find(attr_name) != aclMirrorStageLookup.cend() || + /* handle "MIRROR_ACTION" key without mirror stage specified for backward compatibility */ attr_name == ACTION_MIRROR_ACTION || aclDTelActionLookup.find(attr_name) != aclDTelActionLookup.cend()) { @@ -2152,7 +2152,7 @@ void AclOrch::queryAclActionCapability() { SWSS_LOG_ENTER(); - sai_status_t status {SAI_STATUS_FAILURE}; + sai_status_t status {SAI_STATUS_FAILURE}; sai_attribute_t attr; vector action_list; vector fvVector; From 8d6492e94ab7daf9abb32b5215920f8b8ab2c496 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 7 Aug 2019 18:35:14 +0300 Subject: [PATCH 18/19] [test_acl] remove rule in validation test Signed-off-by: Stepan Blyschak --- tests/test_acl.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_acl.py b/tests/test_acl.py index a50b869c5f..c159c61db0 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -1297,11 +1297,13 @@ def test_AclActionValidation(self, dvs, testlog): atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY") keys = atbl.getKeys() - # verify there are not non-default ACL rules created + # verify there are no non-default ACL rules created acl_entry = [k for k in keys if k not in dvs.asicdb.default_acl_entries] assert len(acl_entry) == 0 self.remove_acl_table(acl_table) + # remove rules from CFG DB + self.remove_acl_rule(acl_table, acl_rule) - dvs.runcmd("systemctl restart all") + dvs.runcmd("supervisorctl restart all") time.sleep(5) From fdc3c24ed07dc43d076298cb64094b9e17faac2a Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 7 Aug 2019 19:09:28 +0300 Subject: [PATCH 19/19] [test_acl] restart syncd explicitely Signed-off-by: Stepan Blyschak --- tests/test_acl.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_acl.py b/tests/test_acl.py index c159c61db0..5cfeb49b4e 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -1305,5 +1305,7 @@ def test_AclActionValidation(self, dvs, testlog): # remove rules from CFG DB self.remove_acl_rule(acl_table, acl_rule) - dvs.runcmd("supervisorctl restart all") + dvs.runcmd("supervisorctl restart syncd") + dvs.stop_swss() + dvs.start_swss() time.sleep(5)