From f72a86b4e59f7eb8fe9b771ecf38a9e7f6faa6ff Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Fri, 8 Oct 2021 14:06:57 +0300 Subject: [PATCH 01/16] [aclorch] add basic AclOrch::updateAclRule method Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 281 +++++++++++++++++++++++++++++++- orchagent/aclorch.h | 34 +++- orchagent/pbh/pbhrule.cpp | 2 +- orchagent/pbh/pbhrule.h | 2 +- orchagent/saihelper.cpp | 209 ++++++++++++++++++++++++ orchagent/saihelper.h | 3 + tests/mock_tests/aclorch_ut.cpp | 102 +++++++++++- 7 files changed, 615 insertions(+), 18 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index c7cfe200a1..cb303d6f09 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -195,6 +195,7 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) SWSS_LOG_ENTER(); sai_attribute_value_t value; + value.aclfield.enable = true; try { @@ -533,7 +534,6 @@ bool AclRule::create() { attr.id = it.first; attr.value = it.second; - attr.value.aclfield.enable = true; rule_attrs.push_back(attr); } } @@ -650,6 +650,222 @@ void AclRule::updateInPorts() } } + +bool AclRule::update(const AclRule& updatedRule) +{ + SWSS_LOG_ENTER(); + + if (!updateCounter(updatedRule)) + { + return false; + } + + if (!updatePriority(updatedRule)) + { + return false; + } + + if (!updateMatches(updatedRule)) + { + return false; + } + + if (!updateActions(updatedRule)) + { + return false; + } + + return true; +} + +bool AclRule::updateCounter(const AclRule& updatedRule) +{ + if (updatedRule.m_createCounter) + { + if (!enableCounter()) + { + return false; + } + } + else + { + if (!disableCounter()) + { + return false; + } + } + + m_createCounter = updatedRule.m_createCounter; + + return true; +} + +bool AclRule::updatePriority(const AclRule& updatedRule) +{ + if (m_priority == updatedRule.m_priority) + { + return true; + } + + sai_attribute_t attr {}; + attr.id = SAI_ACL_ENTRY_ATTR_PRIORITY; + attr.value.s32 = updatedRule.m_priority; + if (!setAttribute(attr)) + { + return false; + } + + m_priority = updatedRule.m_priority; + + return true; +} + +bool AclRule::updateMatches(const AclRule& updatedRule) +{ + vector> matchesUpdated; + vector> matchesDisabled; + + // Diff by value to get new matches and updated matches + // in a single set_difference pass. + set_difference( + updatedRule.m_matches.begin(), + updatedRule.m_matches.end(), + m_matches.begin(), m_matches.end(), + back_inserter(matchesUpdated), + [](auto& oldMatch, auto& newMatch) + { + if (oldMatch.first != newMatch.first) + { + return oldMatch.first < newMatch.first; + } + return compareAclField(oldMatch.first, + oldMatch.second.aclfield, + newMatch.second.aclfield); + } + ); + + // Diff by key only to get delete matches. Assuming that + // deleted matches mean setting a match attribute to disabled state. + set_difference( + m_matches.begin(), m_matches.end(), + updatedRule.m_matches.begin(), + updatedRule.m_matches.end(), + back_inserter(matchesDisabled), + [](auto& oldMatch, auto& newMatch) + { + return oldMatch.first < newMatch.first; + } + ); + + for (const auto& attrPair: matchesDisabled) + { + sai_attribute_t attr {}; + tie(attr.id, attr.value) = attrPair; + attr.value.aclfield.enable = false; + if (!setAttribute(attr)) + { + return false; + } + m_matches.erase(static_cast(attr.id)); + } + + for (const auto& attrPair: matchesUpdated) + { + sai_attribute_t attr {}; + tie(attr.id, attr.value) = attrPair; + if (!setAttribute(attr)) + { + return false; + } + m_matches.emplace(attrPair); + } + + return true; +} + +bool AclRule::updateActions(const AclRule& updatedRule) +{ + vector> actionsUpdated; + vector> actionsDisabled; + + // Diff by value to get new action and updated actions + // in a single set_difference pass. + set_difference( + updatedRule.m_actions.begin(), + updatedRule.m_actions.end(), + m_actions.begin(), m_actions.end(), + back_inserter(actionsUpdated), + [](auto& oldAction, auto& newAction) + { + if (oldAction.first != newAction.first) + { + return oldAction.first < newAction.first; + } + return compareAclAction(oldAction.first, + oldAction.second.aclaction, + newAction.second.aclaction); + } + ); + + // Diff by key only to get delete actions. Assuming that + // action matches mean setting a action attribute to disabled state. + set_difference( + m_actions.begin(), m_actions.end(), + updatedRule.m_actions.begin(), + updatedRule.m_actions.end(), + back_inserter(actionsDisabled), + [](auto& oldAction, auto& newAction) + { + return oldAction.first < newAction.first; + } + ); + + for (const auto& attrPair: actionsDisabled) + { + sai_attribute_t attr {}; + tie(attr.id, attr.value) = attrPair; + attr.value.aclaction.enable = false; + if (!setAttribute(attr)) + { + return false; + } + m_actions.erase(static_cast(attr.id)); + } + + for (const auto& attrPair: actionsUpdated) + { + sai_attribute_t attr {}; + tie(attr.id, attr.value) = attrPair; + if (!setAttribute(attr)) + { + return false; + } + m_actions.emplace(attrPair); + } + + return true; +} + +bool AclRule::setAttribute(sai_attribute_t attr) +{ + auto meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, attr.id); + if (!meta) + { + SWSS_LOG_THROW("Failed to get metadata for attribute id %d", attr.id); + } + auto status = sai_acl_api->set_acl_entry_attribute(m_ruleOid, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to update attribute %s on ACL rule %s in ACL table %s: %s", + meta->attridname, getId().c_str(), getTableId().c_str(), + sai_serialize_status(status).c_str()); + return false; + } + SWSS_LOG_INFO("Successfully updated action attribute %s on ACL rule %s in ACL table %s", + meta->attridname, getId().c_str(), getTableId().c_str()); + return true; +} + AclRuleCounters AclRule::getCounters() { SWSS_LOG_ENTER(); @@ -1110,7 +1326,7 @@ bool AclRuleL3::validate() return true; } -void AclRuleL3::update(SubjectType, void *) +void AclRuleL3::onUpdate(SubjectType, void *) { // Do nothing } @@ -1358,7 +1574,7 @@ bool AclRuleMirror::remove() return true; } -void AclRuleMirror::update(SubjectType type, void *cntx) +void AclRuleMirror::onUpdate(SubjectType type, void *cntx) { if (type != SUBJECT_TYPE_MIRROR_SESSION_CHANGE) { @@ -1807,7 +2023,7 @@ bool AclTable::create() return status == SAI_STATUS_SUCCESS; } -void AclTable::update(SubjectType type, void *cntx) +void AclTable::onUpdate(SubjectType type, void *cntx) { SWSS_LOG_ENTER(); @@ -1995,6 +2211,36 @@ bool AclTable::remove(string rule_id) } } +bool AclTable::updateRule(shared_ptr updatedRule) +{ + SWSS_LOG_ENTER(); + + if (!updatedRule) + { + return false; + } + + auto ruleId = updatedRule->getId(); + auto ruleIter = rules.find(ruleId); + if (ruleIter == rules.end()) + { + SWSS_LOG_ERROR("Failed to update ACL rule %s as it does not exist in %s", + ruleId.c_str(), id.c_str()); + return false; + } + + if (!ruleIter->second->update(*updatedRule)) + { + SWSS_LOG_ERROR("Failed to update ACL rule %s in table %s", + ruleId.c_str(), id.c_str()); + return false; + } + + SWSS_LOG_NOTICE("Successfully updated ACL rule %s in table %s", + ruleId.c_str(), id.c_str()); + return true; +} + bool AclTable::clear() { SWSS_LOG_ENTER(); @@ -2184,7 +2430,7 @@ bool AclRuleDTelFlowWatchListEntry::remove() return true; } -void AclRuleDTelFlowWatchListEntry::update(SubjectType type, void *cntx) +void AclRuleDTelFlowWatchListEntry::onUpdate(SubjectType type, void *cntx) { sai_attribute_value_t value; sai_object_id_t session_oid = SAI_NULL_OBJECT_ID; @@ -2292,7 +2538,7 @@ bool AclRuleDTelDropWatchListEntry::validate() return true; } -void AclRuleDTelDropWatchListEntry::update(SubjectType, void *) +void AclRuleDTelDropWatchListEntry::onUpdate(SubjectType, void *) { // Do nothing } @@ -2806,13 +3052,13 @@ void AclOrch::update(SubjectType type, void *cntx) { if (type == SUBJECT_TYPE_PORT_CHANGE) { - table.second.update(type, cntx); + table.second.onUpdate(type, cntx); } else { for (auto& rule : table.second.rules) { - rule.second->update(type, cntx); + rule.second->onUpdate(type, cntx); } } } @@ -3324,6 +3570,25 @@ bool AclOrch::updateAclRule(string table_id, string rule_id, bool enableCounter) return true; } +bool AclOrch::updateAclRule(shared_ptr updatedRule) +{ + + auto tableId = updatedRule->getTableId(); + sai_object_id_t tableOid = getTableById(tableId); + if (tableOid == SAI_NULL_OBJECT_ID) + { + SWSS_LOG_ERROR("Failed to add ACL rule in ACL table %s. Table doesn't exist", tableId.c_str()); + return false; + } + + if (!m_AclTables[tableOid].updateRule(updatedRule)) + { + return false; + } + + return true; +} + bool AclOrch::isCombinedMirrorV6Table() { return m_isCombinedMirrorV6Table; diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index f2a5e0826b..a0ea903520 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -167,8 +167,9 @@ class AclRule } virtual bool create(); + virtual bool update(const AclRule& updatedRule); virtual bool remove(); - virtual void update(SubjectType, void *) = 0; + virtual void onUpdate(SubjectType, void *) = 0; virtual void updateInPorts(); virtual bool enableCounter(); @@ -203,6 +204,13 @@ class AclRule virtual bool removeCounter(); virtual bool removeRanges(); + virtual bool updatePriority(const AclRule& updatedRule); + virtual bool updateMatches(const AclRule& updatedRule); + virtual bool updateActions(const AclRule& updatedRule); + virtual bool updateCounter(const AclRule& updatedRule); + + virtual bool setAttribute(sai_attribute_t attr); + void decreaseNextHopRefCount(); bool isActionSupported(sai_acl_entry_attr_t) const; @@ -237,7 +245,8 @@ class AclRuleL3: public AclRule bool validateAddAction(string attr_name, string attr_value); bool validateAddMatch(string attr_name, string attr_value); bool validate(); - void update(SubjectType, void *); + void onUpdate(SubjectType, void *) override; + protected: sai_object_id_t getRedirectObjectId(const string& redirect_param); }; @@ -272,7 +281,7 @@ class AclRuleMirror: public AclRule bool validate(); bool create(); bool remove(); - void update(SubjectType, void *); + void onUpdate(SubjectType, void *) override; AclRuleCounters getCounters(); protected: @@ -290,7 +299,7 @@ class AclRuleDTelFlowWatchListEntry: public AclRule bool validate(); bool create(); bool remove(); - void update(SubjectType, void *); + void onUpdate(SubjectType, void *) override; protected: DTelOrch *m_pDTelOrch; @@ -305,7 +314,7 @@ class AclRuleDTelDropWatchListEntry: public AclRule AclRuleDTelDropWatchListEntry(AclOrch *m_pAclOrch, DTelOrch *m_pDTelOrch, string rule, string table, acl_table_type_t type); bool validateAddAction(string attr_name, string attr_value); bool validate(); - void update(SubjectType, void *); + void onUpdate(SubjectType, void *) override; protected: DTelOrch *m_pDTelOrch; @@ -354,12 +363,14 @@ class AclTable void unlink(sai_object_id_t portOid); // Add or overwrite a rule into the ACL table bool add(shared_ptr newRule); + // Update existing ACL rule + bool updateRule(shared_ptr updatedRule); // Remove a rule from the ACL table bool remove(string rule_id); // Remove all rules from the ACL table bool clear(); // Update table subject to changes - void update(SubjectType, void *); + void onUpdate(SubjectType, void *); public: string id; @@ -415,6 +426,15 @@ class AclOrch : public Orch, public Observer bool updateAclTable(string table_id, AclTable &table); bool addAclRule(shared_ptr aclRule, string table_id); bool removeAclRule(string table_id, string rule_id); + + // Update ACL rule based on new configuration in updatedAclRule. + // This method calculates the diff between counter, priority, + // matches and actions and applies the difference to SAI. + // This API is limited to support actions and matches that + // hold a plain value, e.g: actions or matches of type + // SAI_ATTR_VALUE_TYPE_ACL_ACTION/FIELD_DATA_OBJECT_LIST + // are not supported. + bool updateAclRule(shared_ptr updatedAclRule); bool updateAclRule(string table_id, string rule_id, string attr_name, void *data, bool oper); bool updateAclRule(string table_id, string rule_id, bool enableCounter); AclRule* getAclRule(string table_id, string rule_id); @@ -428,7 +448,7 @@ class AclOrch : public Orch, public Observer map m_mirrorTableCapabilities; static sai_acl_action_type_t getAclActionFromAclEntry(sai_acl_entry_attr_t attr); - + // Get the OID for the ACL bind point for a given port static bool getAclBindPortId(Port& port, sai_object_id_t& port_id); diff --git a/orchagent/pbh/pbhrule.cpp b/orchagent/pbh/pbhrule.cpp index 52812e35b6..ab5057a81d 100644 --- a/orchagent/pbh/pbhrule.cpp +++ b/orchagent/pbh/pbhrule.cpp @@ -107,7 +107,7 @@ bool AclRulePbh::validate() return true; } -void AclRulePbh::update(SubjectType, void *) +void AclRulePbh::onUpdate(SubjectType, void *) { // Do nothing } diff --git a/orchagent/pbh/pbhrule.h b/orchagent/pbh/pbhrule.h index 3e753a0f03..2ea38a2c0a 100644 --- a/orchagent/pbh/pbhrule.h +++ b/orchagent/pbh/pbhrule.h @@ -11,5 +11,5 @@ class AclRulePbh: public AclRule bool validateAddMatch(const sai_attribute_t &attr); bool validateAddAction(const sai_attribute_t &attr); bool validate() override; - void update(SubjectType, void *) override; + void onUpdate(SubjectType, void *) override; }; diff --git a/orchagent/saihelper.cpp b/orchagent/saihelper.cpp index 490efb90b3..9f94e98fb2 100644 --- a/orchagent/saihelper.cpp +++ b/orchagent/saihelper.cpp @@ -451,3 +451,212 @@ sai_status_t initSaiPhyApi(swss::gearbox_phy_t *phy) return status; } + +template +int compareList(T first, T second) +{ + if (first.count != second.count) + { + if (first.count < second.count) + { + return -1; + } + else + { + return 1; + } + } + return memcmp(first.list, second.list, first.count); +} + +int compareMacAddress(sai_mac_t first, sai_mac_t second) +{ + return memcmp(first, second, sizeof(sai_mac_t)); +} + +int compareIpV4Address(sai_ip4_t first, sai_ip4_t second) +{ + if (first == second) + { + return 0; + } + else if (first > second) + { + return 1; + } + return -1; +} + +int compareIpV6Address(sai_ip6_t first, sai_ip6_t second) +{ + return memcmp(first, second, sizeof(sai_ip6_t)); +} + +int compareIpAddress(sai_ip_address_t first, sai_ip_address_t second) +{ + if (first.addr_family != second.addr_family) + { + if (first.addr_family < second.addr_family) + { + return -1; + } + else + { + return 1; + } + } + switch (first.addr_family) + { + case SAI_IP_ADDR_FAMILY_IPV4: + return compareIpV4Address(first.addr.ip4, second.addr.ip4); + case SAI_IP_ADDR_FAMILY_IPV6: + return compareIpV6Address(first.addr.ip6, second.addr.ip6); + default: + SWSS_LOG_THROW("Unknown IP address famility %d", first.addr_family); + } + SWSS_LOG_THROW("BUG: Unexpectedly reached dead code block"); +} + +// helper function used in ACL rule attributes diff calculation. +bool compareAclAction(sai_acl_entry_attr_t id, sai_acl_action_data_t first, sai_acl_action_data_t second) +{ + auto meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, id); + if (!meta) + { + SWSS_LOG_THROW("SAI Bug: Failed to get metadata for SAI_OBJECT_TYPE_ACL_ENTRY"); + } + + if (first.enable < second.enable) + { + return true; + } + + switch(meta->attrvaluetype) + { + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_BOOL: + return first.parameter.booldata < second.parameter.booldata; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_UINT8: + return first.parameter.u8 < second.parameter.u8; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_INT8: + return first.parameter.s8 < second.parameter.s8; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_UINT16: + return first.parameter.u16 < second.parameter.u16; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_INT16: + return first.parameter.s16 < second.parameter.s16; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_UINT32: + return first.parameter.u32 < second.parameter.u32; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_INT32: + return first.parameter.s32 < second.parameter.s32; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_MAC: + return compareMacAddress(first.parameter.mac, second.parameter.mac) < 0; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_IPV4: + return compareIpV4Address(first.parameter.ip4, second.parameter.ip4) < 0; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_IPV6: + return compareIpV6Address(first.parameter.ip6, second.parameter.ip6) < 0; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_OBJECT_ID: + return first.parameter.oid < second.parameter.oid; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_OBJECT_LIST: + return compareList(first.parameter.objlist, second.parameter.objlist) < 0; + case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_IP_ADDRESS: + return compareIpAddress(first.parameter.ipaddr, second.parameter.ipaddr) < 0; + default: + SWSS_LOG_THROW("Unhandled ACL field type %d", meta->attrvaluetype); + } + + SWSS_LOG_THROW("BUG: Reached unexpected code block"); +} + +// helper function used in ACL rule attributes diff calculation. +bool compareAclField(sai_acl_entry_attr_t id, sai_acl_field_data_t first, sai_acl_field_data_t second) +{ + auto meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, id); + if (!meta) + { + SWSS_LOG_THROW("SAI Bug: Failed to get metadata for SAI_OBJECT_TYPE_ACL_ENTRY"); + } + + if (first.enable < second.enable) + { + return true; + } + + switch(meta->attrvaluetype) + { + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_BOOL: + return first.data.booldata < second.data.booldata; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT8: + if (first.mask.u8 != second.mask.u8) + { + return first.mask.u8 < second.mask.u8; + } + return first.data.u8 < second.data.u8; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_INT8: + if (first.mask.s8 != second.mask.s8) + { + return first.mask.s8 < second.mask.s8; + } + return first.data.s8 < second.data.s8; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT16: + if (first.mask.u16 != second.mask.u16) + { + return first.mask.u16 < second.mask.u16; + } + return first.data.u16 < second.data.u16; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_INT16: + if (first.mask.s16 != second.mask.s16) + { + return first.mask.s16 < second.mask.s16; + } + return first.data.s16 < second.data.s16; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT32: + if (first.mask.u32 != second.mask.u32) + { + return first.mask.u32 < second.mask.u32; + } + return first.data.u32 < second.data.u32; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_INT32: + if (first.mask.s32 != second.mask.s32) + { + return first.mask.s32 < second.mask.s32; + } + return first.data.s32 < second.data.s32; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT64: + if (first.mask.u64 != second.mask.u64) + { + return first.mask.u64 < second.mask.u64; + } + return first.data.u64 < second.data.u64; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_MAC: + if (compareMacAddress(first.mask.mac, second.mask.mac) != 0) + { + return compareMacAddress(first.mask.mac, second.mask.mac) < 0; + } + return compareMacAddress(first.data.mac, second.data.mac) < 0; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_IPV4: + if (compareIpV4Address(first.mask.ip4, second.mask.ip4) != 0) + { + return compareIpV4Address(first.mask.ip4, second.mask.ip4) < 0; + } + return compareIpV4Address(first.data.ip4, second.data.ip4) < 0; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_IPV6: + if (compareIpV6Address(first.mask.ip6, second.mask.ip6) != 0) + { + return compareIpV6Address(first.mask.ip6, second.mask.ip6) < 0; + } + return compareIpV6Address(first.data.ip6, second.data.ip6) < 0; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_OBJECT_ID: + return first.data.oid < second.data.oid; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_OBJECT_LIST: + return compareList(first.data.objlist, second.data.objlist) < 0; + case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT8_LIST: + if (compareList(first.mask.u8list, second.mask.u8list) != 0) + { + return compareList(first.mask.u8list, second.mask.u8list) < 0; + } + return compareList(first.data.u8list, second.data.u8list) < 0; + default: + SWSS_LOG_THROW("Unhandled ACL field type %d", meta->attrvaluetype); + } + + SWSS_LOG_THROW("BUG: Reached unexpected code block"); +} diff --git a/orchagent/saihelper.h b/orchagent/saihelper.h index 450acf8b69..b652a0a5df 100644 --- a/orchagent/saihelper.h +++ b/orchagent/saihelper.h @@ -7,3 +7,6 @@ void initSaiApi(); void initSaiRedis(const std::string &record_location, const std::string &record_filename); sai_status_t initSaiPhyApi(swss::gearbox_phy_t *phy); + +bool compareAclAction(sai_acl_entry_attr_t id, sai_acl_action_data_t first, sai_acl_action_data_t second); +bool compareAclField(sai_acl_entry_attr_t id, sai_acl_field_data_t first, sai_acl_field_data_t second); diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp index 4cd7688883..188220e880 100644 --- a/tests/mock_tests/aclorch_ut.cpp +++ b/tests/mock_tests/aclorch_ut.cpp @@ -337,7 +337,7 @@ namespace aclorch_test { APP_VXLAN_FDB_TABLE_NAME, FdbOrch::fdborch_pri}, { APP_MCLAG_FDB_TABLE_NAME, fdborch_pri} }; - + TableConnector stateDbFdb(m_state_db.get(), STATE_FDB_TABLE_NAME); TableConnector stateMclagDbFdb(m_state_db.get(), STATE_MCLAG_REMOTE_FDB_TABLE_NAME); ASSERT_EQ(gFdbOrch, nullptr); @@ -966,6 +966,25 @@ namespace aclorch_test return !aclEnable && aclOid == SAI_NULL_OBJECT_ID; } + + string getAclRuleSaiAttribute(const AclRule& rule, sai_acl_entry_attr_t attrId) + { + sai_attribute_t attr{}; + attr.id = attrId; + auto meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, attrId); + if (!meta) + { + SWSS_LOG_THROW("SAI BUG: Failed to get attribute metadata for SAI_OBJECT_TYPE_ACL_ENTRY attribute id %d", attrId); + } + + auto status = sai_acl_api->get_acl_entry_attribute(rule.m_ruleOid, 1, &attr); + EXPECT_TRUE(status == SAI_STATUS_SUCCESS); + + auto actualSaiValue = sai_serialize_attr_value(*meta, attr); + + return actualSaiValue; + } + }; map AclOrchTest::gProfileMap; @@ -1307,4 +1326,85 @@ namespace aclorch_test ASSERT_EQ(tableIt, orch->getAclTables().end()); } + TEST_F(AclOrchTest, AclRuleUpdate) + { + string acl_table_id = "acl_table_1"; + string acl_rule_id = "acl_rule_1"; + + auto orch = createAclOrch(); + + auto kvfAclTable = deque( + { { acl_table_id, + SET_COMMAND, + { { ACL_TABLE_DESCRIPTION, "TEST" }, + { ACL_TABLE_TYPE, TABLE_TYPE_L3 }, + { ACL_TABLE_STAGE, STAGE_INGRESS }, + { ACL_TABLE_PORTS, "1,2" } } } }); + + orch->doAclTableTask(kvfAclTable); + + // validate acl table ... + + auto acl_table_oid = orch->getTableById(acl_table_id); + ASSERT_NE(acl_table_oid, SAI_NULL_OBJECT_ID); + + const auto &acl_tables = orch->getAclTables(); + auto it_table = acl_tables.find(acl_table_oid); + ASSERT_NE(it_table, acl_tables.end()); + + class AclRuleTest : public AclRuleL3 + { + public: + AclRuleTest(AclOrch* orch, string rule, string table): + AclRuleL3(orch, rule, table, ACL_TABLE_L3, true) + {} + + void setCounterEnabled(bool enabled) + { + m_createCounter = enabled; + } + + void disableMatch(sai_acl_entry_attr_t attr) + { + m_matches.erase(attr); + } + }; + + auto rule = make_shared(orch->m_aclOrch, acl_rule_id, acl_table_id); + ASSERT_TRUE(rule->validateAddPriority(RULE_PRIORITY, "800")); + ASSERT_TRUE(rule->validateAddMatch(MATCH_SRC_IP, "1.1.1.1/32")); + ASSERT_TRUE(rule->validateAddAction(ACTION_PACKET_ACTION, PACKET_ACTION_FORWARD)); + + ASSERT_TRUE(orch->m_aclOrch->addAclRule(rule, acl_table_id)); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_PRIORITY), "800"); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_FIELD_SRC_IP), "1.1.1.1&mask:255.255.255.255"); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION), "SAI_PACKET_ACTION_FORWARD"); + + auto updatedRule = make_shared(*rule); + ASSERT_TRUE(updatedRule->validateAddPriority(RULE_PRIORITY, "900")); + ASSERT_TRUE(updatedRule->validateAddMatch(MATCH_SRC_IP, "2.2.2.2/24")); + ASSERT_TRUE(updatedRule->validateAddMatch(MATCH_DST_IP, "3.3.3.3/24")); + ASSERT_TRUE(updatedRule->validateAddAction(ACTION_PACKET_ACTION, PACKET_ACTION_DROP)); + + ASSERT_TRUE(orch->m_aclOrch->updateAclRule(updatedRule)); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_PRIORITY), "900"); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_FIELD_SRC_IP), "2.2.2.2&mask:255.255.255.0"); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_FIELD_DST_IP), "3.3.3.3&mask:255.255.255.0"); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION), "SAI_PACKET_ACTION_DROP"); + + auto updatedRule2 = make_shared(*updatedRule); + updatedRule2->setCounterEnabled(false); + updatedRule2->disableMatch(SAI_ACL_ENTRY_ATTR_FIELD_DST_IP); + ASSERT_TRUE(orch->m_aclOrch->updateAclRule(updatedRule2)); + ASSERT_TRUE(validateAclRuleCounter(*orch->m_aclOrch->getAclRule(acl_table_id, acl_rule_id), false)); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_FIELD_DST_IP), "disabled"); + + auto updatedRule3 = make_shared(*updatedRule2); + updatedRule3->setCounterEnabled(true); + ASSERT_TRUE(orch->m_aclOrch->updateAclRule(updatedRule3)); + ASSERT_TRUE(validateAclRuleCounter(*orch->m_aclOrch->getAclRule(acl_table_id, acl_rule_id), true)); + + ASSERT_TRUE(orch->m_aclOrch->removeAclRule(rule->getTableId(), rule->getId())); + } + } // namespace nsAclOrchTest From 9fb206df38bd13100f967915f62a0122f47977f7 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Mon, 25 Oct 2021 15:57:07 +0300 Subject: [PATCH 02/16] intermediate commit Signed-off-by: Stepan Blyshchak --- orchagent/Makefile.am | 1 + orchagent/aclorch.cpp | 338 ++++++++++++++++---------------- orchagent/aclorch.h | 19 ++ orchagent/saiattr.cpp | 88 +++++++++ orchagent/saiattr.h | 41 ++++ tests/mock_tests/Makefile.am | 1 + tests/mock_tests/aclorch_ut.cpp | 16 +- tests/mock_tests/portal.h | 8 +- 8 files changed, 336 insertions(+), 176 deletions(-) create mode 100644 orchagent/saiattr.cpp create mode 100644 orchagent/saiattr.h diff --git a/orchagent/Makefile.am b/orchagent/Makefile.am index d4f3627203..dc3e1ce8b7 100644 --- a/orchagent/Makefile.am +++ b/orchagent/Makefile.am @@ -56,6 +56,7 @@ orchagent_SOURCES = \ pbh/pbhrule.cpp \ pbhorch.cpp \ saihelper.cpp \ + saiattr.cpp \ switchorch.cpp \ pfcwdorch.cpp \ pfcactionhandler.cpp \ diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index cb303d6f09..a6d7da2c71 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -59,8 +59,8 @@ acl_rule_attr_lookup_t aclMatchLookup = { MATCH_ICMP_CODE, SAI_ACL_ENTRY_ATTR_FIELD_ICMP_CODE }, { MATCH_ICMPV6_TYPE, SAI_ACL_ENTRY_ATTR_FIELD_ICMPV6_TYPE }, { MATCH_ICMPV6_CODE, SAI_ACL_ENTRY_ATTR_FIELD_ICMPV6_CODE }, - { MATCH_L4_SRC_PORT_RANGE, (sai_acl_entry_attr_t)SAI_ACL_RANGE_TYPE_L4_SRC_PORT_RANGE }, - { MATCH_L4_DST_PORT_RANGE, (sai_acl_entry_attr_t)SAI_ACL_RANGE_TYPE_L4_DST_PORT_RANGE }, + { MATCH_L4_SRC_PORT_RANGE, SAI_ACL_ENTRY_ATTR_FIELD_ACL_RANGE_TYPE }, + { MATCH_L4_DST_PORT_RANGE, SAI_ACL_ENTRY_ATTR_FIELD_ACL_RANGE_TYPE }, { MATCH_TUNNEL_VNI, SAI_ACL_ENTRY_ATTR_FIELD_TUNNEL_VNI }, { MATCH_INNER_ETHER_TYPE, SAI_ACL_ENTRY_ATTR_FIELD_INNER_ETHER_TYPE }, { MATCH_INNER_IP_PROTOCOL, SAI_ACL_ENTRY_ATTR_FIELD_INNER_IP_PROTOCOL }, @@ -68,6 +68,12 @@ acl_rule_attr_lookup_t aclMatchLookup = { MATCH_INNER_L4_DST_PORT, SAI_ACL_ENTRY_ATTR_FIELD_INNER_L4_DST_PORT } }; +static acl_range_type_lookup_t aclRangeTypeLookup = +{ + { MATCH_L4_SRC_PORT_RANGE, SAI_ACL_RANGE_TYPE_L4_SRC_PORT_RANGE }, + { MATCH_L4_DST_PORT_RANGE, SAI_ACL_RANGE_TYPE_L4_DST_PORT_RANGE }, +}; + static acl_rule_attr_lookup_t aclL3ActionLookup = { { ACTION_PACKET_ACTION, SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION }, @@ -157,6 +163,16 @@ static acl_ip_type_lookup_t aclIpTypeLookup = { IP_TYPE_ARP_REPLY, SAI_ACL_IP_TYPE_ARP_REPLY } }; +static string getAttributeIdName(sai_object_type_t objectType, sai_attr_id_t attr) +{ + const auto* meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + if (!meta) + { + SWSS_LOG_THROW("Metadata null pointer returned by sai_metadata_get_attr_metadata"); + } + return meta->attridname; +} + AclRule::AclRule(AclOrch *pAclOrch, string rule, string table, acl_table_type_t type, bool createCounter) : m_pAclOrch(pAclOrch), m_id(rule), @@ -179,12 +195,11 @@ bool AclRule::validateAddPriority(string attr_name, string attr_value) { char *endp = NULL; errno = 0; - m_priority = (uint32_t)strtol(attr_value.c_str(), &endp, 0); + auto priority = (uint32_t)strtol(attr_value.c_str(), &endp, 0); // check conversion was successful and the value is within the allowed range status = (errno == 0) && (endp == attr_value.c_str() + attr_value.size()) && - (m_priority >= m_minPriority) && - (m_priority <= m_maxPriority); + setPriority(priority); } return status; @@ -353,20 +368,27 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) } else if ((attr_name == MATCH_L4_SRC_PORT_RANGE) || (attr_name == MATCH_L4_DST_PORT_RANGE)) { - if (sscanf(attr_value.c_str(), "%d-%d", &value.u32range.min, &value.u32range.max) != 2) + AclRangeConfig rangeConfig{}; + if (sscanf(attr_value.c_str(), "%d-%d", &rangeConfig.min, &rangeConfig.max) != 2) { SWSS_LOG_ERROR("Range parse error. Attribute: %s, value: %s", attr_name.c_str(), attr_value.c_str()); return false; } + rangeConfig.rangeType = aclRangeTypeLookup[attr_value]; + // check boundaries - if ((value.u32range.min > USHRT_MAX) || - (value.u32range.max > USHRT_MAX) || - (value.u32range.min > value.u32range.max)) + if ((rangeConfig.min > USHRT_MAX) || + (rangeConfig.max > USHRT_MAX) || + (rangeConfig.min > rangeConfig.max)) { SWSS_LOG_ERROR("Range parse error. Invalid range value. Attribute: %s, value: %s", attr_name.c_str(), attr_value.c_str()); return false; } + + m_rangeConfig.push_back(rangeConfig); + + return true; } else if (attr_name == MATCH_TC) { @@ -416,42 +438,12 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) attr_name = MATCH_NEXT_HEADER; } - m_matches[aclMatchLookup[attr_name]] = value; - - return true; + return setMatch(aclMatchLookup[attr_name], value); } 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; - } - - // 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 requires 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; + return false; } bool AclRule::processIpType(string type, sai_uint32_t &ip_type) @@ -509,49 +501,40 @@ bool AclRule::create() rule_attrs.push_back(attr); } - // store matches - for (auto it : m_matches) + if (!m_rangeConfig.empty()) { - // collect ranges and add them later as a list - if (((sai_acl_range_type_t)it.first == SAI_ACL_RANGE_TYPE_L4_SRC_PORT_RANGE) || - ((sai_acl_range_type_t)it.first == SAI_ACL_RANGE_TYPE_L4_DST_PORT_RANGE)) + for (const auto& rangeConfig: m_rangeConfig) { - SWSS_LOG_INFO("Creating range object %u..%u", it.second.u32range.min, it.second.u32range.max); + SWSS_LOG_INFO("Creating range object %u..%u", rangeConfig.min, rangeConfig.max); - AclRange *range = AclRange::create((sai_acl_range_type_t)it.first, it.second.u32range.min, it.second.u32range.max); + AclRange *range = AclRange::create(rangeConfig.rangeType, rangeConfig.min, rangeConfig.max); if (!range) { // release already created range if any AclRange::remove(range_objects, range_object_list.count); return false; } - else - { - range_objects[range_object_list.count++] = range->getOid(); - } - } - else - { - attr.id = it.first; - attr.value = it.second; - rule_attrs.push_back(attr); + + m_ranges.push_back(range); + range_objects[range_object_list.count++] = range->getOid(); } - } - // store ranges if any - if (range_object_list.count > 0) - { attr.id = SAI_ACL_ENTRY_ATTR_FIELD_ACL_RANGE_TYPE; attr.value.aclfield.enable = true; attr.value.aclfield.data.objlist = range_object_list; rule_attrs.push_back(attr); } + // store matches + for (auto it : matches) + { + rule_attrs.push_back(it.second.getSaiAttr()); + } + // store actions - for (auto it : m_actions) + for (auto it : actions) { - attr.id = it.first; - attr.value = it.second; + attr = it.second.getSaiAttr(); rule_attrs.push_back(attr); } @@ -636,11 +619,9 @@ bool AclRule::remove() void AclRule::updateInPorts() { SWSS_LOG_ENTER(); - sai_attribute_t attr; sai_status_t status; - attr.id = SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS; - attr.value = m_matches[SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS]; + auto attr = matches[SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS].getSaiAttr(); attr.value.aclfield.enable = true; status = sai_acl_api->set_acl_entry_attribute(m_ruleOid, &attr); @@ -722,34 +703,24 @@ bool AclRule::updatePriority(const AclRule& updatedRule) bool AclRule::updateMatches(const AclRule& updatedRule) { - vector> matchesUpdated; - vector> matchesDisabled; + vector> matchesUpdated; + vector> matchesDisabled; // Diff by value to get new matches and updated matches // in a single set_difference pass. set_difference( - updatedRule.m_matches.begin(), - updatedRule.m_matches.end(), - m_matches.begin(), m_matches.end(), - back_inserter(matchesUpdated), - [](auto& oldMatch, auto& newMatch) - { - if (oldMatch.first != newMatch.first) - { - return oldMatch.first < newMatch.first; - } - return compareAclField(oldMatch.first, - oldMatch.second.aclfield, - newMatch.second.aclfield); - } + updatedRule.matches.begin(), + updatedRule.matches.end(), + matches.begin(), matches.end(), + back_inserter(matchesUpdated) ); // Diff by key only to get delete matches. Assuming that // deleted matches mean setting a match attribute to disabled state. set_difference( - m_matches.begin(), m_matches.end(), - updatedRule.m_matches.begin(), - updatedRule.m_matches.end(), + matches.begin(), matches.end(), + updatedRule.matches.begin(), + updatedRule.matches.end(), back_inserter(matchesDisabled), [](auto& oldMatch, auto& newMatch) { @@ -759,25 +730,23 @@ bool AclRule::updateMatches(const AclRule& updatedRule) for (const auto& attrPair: matchesDisabled) { - sai_attribute_t attr {}; - tie(attr.id, attr.value) = attrPair; + auto attr = attrPair.second.getSaiAttr(); attr.value.aclfield.enable = false; if (!setAttribute(attr)) { return false; } - m_matches.erase(static_cast(attr.id)); + matches.erase(static_cast(attr.id)); } for (const auto& attrPair: matchesUpdated) { - sai_attribute_t attr {}; - tie(attr.id, attr.value) = attrPair; + auto attr = attrPair.second.getSaiAttr(); if (!setAttribute(attr)) { return false; } - m_matches.emplace(attrPair); + matches[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); } return true; @@ -785,34 +754,24 @@ bool AclRule::updateMatches(const AclRule& updatedRule) bool AclRule::updateActions(const AclRule& updatedRule) { - vector> actionsUpdated; - vector> actionsDisabled; + vector> actionsUpdated; + vector> actionsDisabled; // Diff by value to get new action and updated actions // in a single set_difference pass. set_difference( - updatedRule.m_actions.begin(), - updatedRule.m_actions.end(), - m_actions.begin(), m_actions.end(), - back_inserter(actionsUpdated), - [](auto& oldAction, auto& newAction) - { - if (oldAction.first != newAction.first) - { - return oldAction.first < newAction.first; - } - return compareAclAction(oldAction.first, - oldAction.second.aclaction, - newAction.second.aclaction); - } + updatedRule.actions.begin(), + updatedRule.actions.end(), + actions.begin(), actions.end(), + back_inserter(actionsUpdated) ); // Diff by key only to get delete actions. Assuming that // action matches mean setting a action attribute to disabled state. set_difference( - m_actions.begin(), m_actions.end(), - updatedRule.m_actions.begin(), - updatedRule.m_actions.end(), + actions.begin(), actions.end(), + updatedRule.actions.begin(), + updatedRule.actions.end(), back_inserter(actionsDisabled), [](auto& oldAction, auto& newAction) { @@ -822,27 +781,82 @@ bool AclRule::updateActions(const AclRule& updatedRule) for (const auto& attrPair: actionsDisabled) { - sai_attribute_t attr {}; - tie(attr.id, attr.value) = attrPair; + auto attr = attrPair.second.getSaiAttr(); attr.value.aclaction.enable = false; if (!setAttribute(attr)) { return false; } - m_actions.erase(static_cast(attr.id)); + actions.erase(static_cast(attr.id)); } for (const auto& attrPair: actionsUpdated) { - sai_attribute_t attr {}; - tie(attr.id, attr.value) = attrPair; + auto attr = attrPair.second.getSaiAttr(); if (!setAttribute(attr)) { return false; } - m_actions.emplace(attrPair); + actions[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + } + + return true; +} + +bool AclRule::setPriority(const sai_uint32_t &value) +{ + if (!(value >= m_minPriority && value <= m_maxPriority)) + { + SWSS_LOG_ERROR("Priority value %d is out of supported priority range %d-%d", + value, m_minPriority, m_maxPriority); + return false; + } + m_priority = value; + return true; +} + +bool AclRule::setAction(sai_acl_entry_attr_t actionId, sai_acl_action_data_t actionData) +{ + if (!isActionSupported(actionId)) + { + SWSS_LOG_ERROR("Action attribute %s is not supported", + getAttributeIdName(SAI_OBJECT_TYPE_ACL_ENTRY, actionId).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, actionId); + if (meta == nullptr) + { + SWSS_LOG_THROW("Metadata null pointer returned by sai_metadata_get_attr_metadata for action %d", actionId); + } + if (meta->isenum) + { + // if ACL action attribute requires enum value check if value is supported by the ASIC + if (!m_pAclOrch->isAclActionEnumValueSupported(AclOrch::getAclActionFromAclEntry(actionId), actionData.parameter)) + { + SWSS_LOG_ERROR("Action %s is not supported by ASIC", + getAttributeIdName(SAI_OBJECT_TYPE_ACL_ENTRY, actionId).c_str()); + return false; + } } + sai_attribute_t attr; + attr.id = actionId; + attr.value.aclaction = actionData; + + actions[actionId] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + + return true; +} + +bool AclRule::setMatch(sai_acl_entry_attr_t matchId, sai_attribute_value_t value) +{ + SaiAttr attribute(SAI_OBJECT_TYPE_ACL_ENTRY, sai_attribute_t{ + .id = static_cast(matchId), + .value = value, + }); + matches[matchId] = attribute; return true; } @@ -1091,12 +1105,11 @@ bool AclRule::createCounter() bool AclRule::removeRanges() { SWSS_LOG_ENTER(); - for (auto it : m_matches) + for (const auto& rangeConfig: m_rangeConfig) { - if (((sai_acl_range_type_t)it.first == SAI_ACL_RANGE_TYPE_L4_SRC_PORT_RANGE) || - ((sai_acl_range_type_t)it.first == SAI_ACL_RANGE_TYPE_L4_DST_PORT_RANGE)) + if (!AclRange::remove(rangeConfig.rangeType, rangeConfig.min, rangeConfig.max)) { - return AclRange::remove((sai_acl_range_type_t)it.first, it.second.u32range.min, it.second.u32range.max); + return false; } } return true; @@ -1139,7 +1152,7 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value) SWSS_LOG_ENTER(); string attr_value = to_upper(_attr_value); - sai_attribute_value_t value; + sai_acl_action_data_t actionData; auto action_str = attr_name; @@ -1148,7 +1161,7 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value) const auto it = aclPacketActionLookup.find(attr_value); if (it != aclPacketActionLookup.cend()) { - value.aclaction.parameter.s32 = it->second; + actionData.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) @@ -1175,14 +1188,14 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value) { return false; } - value.aclaction.parameter.oid = param_id; + actionData.parameter.oid = param_id; action_str = ACTION_REDIRECT_ACTION; } // handle PACKET_ACTION_DO_NOT_NAT in ACTION_PACKET_ACTION else if (attr_value == PACKET_ACTION_DO_NOT_NAT) { - value.aclaction.parameter.booldata = true; + actionData.parameter.booldata = true; action_str = ACTION_DO_NOT_NAT_ACTION; } else @@ -1197,18 +1210,16 @@ bool AclRuleL3::validateAddAction(string attr_name, string _attr_value) { return false; } - value.aclaction.parameter.oid = param_id; + actionData.parameter.oid = param_id; } else { return false; } - value.aclaction.enable = true; + actionData.enable = true; - m_actions[aclL3ActionLookup[action_str]] = value; - - return AclRule::validateAddAction(attr_name, attr_value); + return setAction(aclL3ActionLookup[action_str], actionData); } // This method should return sai attribute id of the redirect destination @@ -1318,7 +1329,7 @@ bool AclRuleL3::validate() { SWSS_LOG_ENTER(); - if (m_matches.size() == 0 || m_actions.size() != 1) + if (matches.size() == 0 || actions.size() != 1) { return false; } @@ -1421,9 +1432,7 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value) m_sessionName = attr_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); + return setAction(action, sai_acl_action_data_t{}); } bool AclRuleMirror::validateAddMatch(string attr_name, string attr_value) @@ -1494,7 +1503,7 @@ bool AclRuleMirror::validate() { SWSS_LOG_ENTER(); - if (m_matches.size() == 0 || m_sessionName.empty()) + if (matches.size() == 0 || m_sessionName.empty()) { return false; } @@ -1530,11 +1539,13 @@ bool AclRuleMirror::create() SWSS_LOG_THROW("Failed to get mirror session OID for session %s", m_sessionName.c_str()); } - for (auto& it: m_actions) + for (auto& it: actions) { - it.second.aclaction.enable = true; - it.second.aclaction.parameter.objlist.list = &oid; - it.second.aclaction.parameter.objlist.count = 1; + auto attr = it.second.getSaiAttr(); + attr.value.aclaction.enable = true; + attr.value.aclaction.parameter.objlist.list = &oid; + attr.value.aclaction.parameter.objlist.count = 1; + actions[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); } if (!AclRule::create()) @@ -1622,7 +1633,7 @@ bool AclRuleMclag::validate() { SWSS_LOG_ENTER(); - if (m_matches.size() == 0) + if (matches.size() == 0) { return false; } @@ -2282,7 +2293,7 @@ bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string a { SWSS_LOG_ENTER(); - sai_attribute_value_t value; + sai_acl_action_data_t actionData; string attr_value = to_upper(attr_val); sai_object_id_t session_oid; @@ -2306,7 +2317,7 @@ bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string a return false; } - value.aclaction.parameter.s32 = it->second; + actionData.parameter.s32 = it->second; if (attr_value == DTEL_FLOW_OP_INT) { @@ -2325,7 +2336,7 @@ bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string a bool ret = m_pDTelOrch->getINTSessionOid(attr_value, session_oid); if (ret) { - value.aclaction.parameter.oid = session_oid; + actionData.parameter.oid = session_oid; // Increase session reference count regardless of state to deny // attempt to remove INT session with attached ACL rules. @@ -2344,22 +2355,20 @@ bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string a if (attr_name == ACTION_DTEL_FLOW_SAMPLE_PERCENT) { - value.aclaction.parameter.u8 = to_uint(attr_value); + actionData.parameter.u8 = to_uint(attr_value); } - value.aclaction.enable = true; + actionData.enable = true; if (attr_name == ACTION_DTEL_REPORT_ALL_PACKETS || attr_name == ACTION_DTEL_DROP_REPORT_ENABLE || attr_name == ACTION_DTEL_TAIL_DROP_REPORT_ENABLE) { - value.aclaction.parameter.booldata = (attr_value == DTEL_ENABLED) ? true : false; - value.aclaction.enable = (attr_value == DTEL_ENABLED) ? true : false; + actionData.parameter.booldata = (attr_value == DTEL_ENABLED) ? true : false; + actionData.enable = (attr_value == DTEL_ENABLED) ? true : false; } - m_actions[aclDTelActionLookup[attr_name]] = value; - - return AclRule::validateAddAction(attr_name, attr_value); + return setAction(aclDTelActionLookup[attr_name], actionData); } bool AclRuleDTelFlowWatchListEntry::validate() @@ -2371,7 +2380,7 @@ bool AclRuleDTelFlowWatchListEntry::validate() return false; } - if (m_matches.size() == 0 || m_actions.size() == 0) + if (matches.size() == 0 || actions.size() == 0) { return false; } @@ -2432,7 +2441,7 @@ bool AclRuleDTelFlowWatchListEntry::remove() void AclRuleDTelFlowWatchListEntry::onUpdate(SubjectType type, void *cntx) { - sai_attribute_value_t value; + sai_acl_action_data_t actionData; sai_object_id_t session_oid = SAI_NULL_OBJECT_ID; if (!m_pDTelOrch) @@ -2463,8 +2472,8 @@ void AclRuleDTelFlowWatchListEntry::onUpdate(SubjectType type, void *cntx) return; } - value.aclaction.enable = true; - value.aclaction.parameter.oid = session_oid; + actionData.enable = true; + actionData.parameter.oid = session_oid; // Increase session reference count regardless of state to deny // attempt to remove INT session with attached ACL rules. @@ -2473,7 +2482,11 @@ void AclRuleDTelFlowWatchListEntry::onUpdate(SubjectType type, void *cntx) throw runtime_error("Failed to increase INT session reference count"); } - m_actions[SAI_ACL_ENTRY_ATTR_ACTION_DTEL_INT_SESSION] = value; + if (!setAction(SAI_ACL_ENTRY_ATTR_ACTION_DTEL_INT_SESSION, actionData)) + { + SWSS_LOG_ERROR("Failed to set action SAI_ACL_ENTRY_ATTR_ACTION_DTEL_INT_SESSION"); + return; + } INT_session_valid = true; @@ -2502,7 +2515,7 @@ bool AclRuleDTelDropWatchListEntry::validateAddAction(string attr_name, string a return false; } - sai_attribute_value_t value; + sai_acl_action_data_t actionData; string attr_value = to_upper(attr_val); if (attr_name != ACTION_DTEL_DROP_REPORT_ENABLE && @@ -2512,13 +2525,10 @@ bool AclRuleDTelDropWatchListEntry::validateAddAction(string attr_name, string a return false; } + actionData.parameter.booldata = (attr_value == DTEL_ENABLED) ? true : false; + actionData.enable = (attr_value == DTEL_ENABLED) ? true : false; - value.aclaction.parameter.booldata = (attr_value == DTEL_ENABLED) ? true : false; - value.aclaction.enable = (attr_value == DTEL_ENABLED) ? true : false; - - m_actions[aclDTelActionLookup[attr_name]] = value; - - return AclRule::validateAddAction(attr_name, attr_value); + return setAction(aclDTelActionLookup[attr_name], actionData); } bool AclRuleDTelDropWatchListEntry::validate() @@ -2530,7 +2540,7 @@ bool AclRuleDTelDropWatchListEntry::validate() return false; } - if (m_matches.size() == 0 || m_actions.size() == 0) + if (matches.size() == 0 || actions.size() == 0) { return false; } diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index a0ea903520..1701cff473 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -18,6 +18,8 @@ #include "acltable.h" +#include "saiattr.h" + // ACL counters update interval in the DB // Value is in seconds. Should not be less than 5 seconds // (in worst case update of 1265 counters takes almost 5 sec) @@ -95,6 +97,7 @@ #define RULE_OPER_DELETE 1 typedef map acl_rule_attr_lookup_t; +typedef map acl_range_type_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; @@ -104,6 +107,13 @@ typedef map> acl_action_enum_values_capabili class AclOrch; +struct AclRangeConfig +{ + sai_acl_range_type_t rangeType; + uint32_t min; + uint32_t max; +}; + class AclRange { public: @@ -209,6 +219,10 @@ class AclRule virtual bool updateActions(const AclRule& updatedRule); virtual bool updateCounter(const AclRule& updatedRule); + virtual bool setPriority(const sai_uint32_t &value); + virtual bool setAction(sai_acl_entry_attr_t actionId, sai_acl_action_data_t actionData); + virtual bool setMatch(sai_acl_entry_attr_t matchId, sai_attribute_value_t value); + virtual bool setAttribute(sai_attribute_t attr); void decreaseNextHopRefCount(); @@ -227,12 +241,17 @@ class AclRule uint32_t m_priority; map m_matches; map m_actions; + map actions; + map matches; string m_redirect_target_next_hop; string m_redirect_target_next_hop_group; vector m_inPorts; vector m_outPorts; + vector m_rangeConfig; + vector m_ranges; + private: bool m_createCounter; }; diff --git a/orchagent/saiattr.cpp b/orchagent/saiattr.cpp new file mode 100644 index 0000000000..a2771bdffd --- /dev/null +++ b/orchagent/saiattr.cpp @@ -0,0 +1,88 @@ +#include "saiattr.h" + +#include +#include + +#include + +SaiAttr::SaiAttr(sai_object_type_t objectType, const sai_attribute_t& attr) +{ + auto meta = sai_metadata_get_attr_metadata(objectType, attr.id); + if (!meta) + { + SWSS_LOG_THROW("Failed to get attribute %d metadata", attr.id); + } + + initializeFrom(objectType, *meta, attr); +} + +SaiAttr::~SaiAttr() +{ + sai_deserialize_free_attribute_value(m_meta->attrvaluetype, m_attr); +} + +SaiAttr::SaiAttr(const SaiAttr& other) +{ + initializeFrom(other.m_objectType, *other.m_meta, other.m_attr); +} + +SaiAttr& SaiAttr::operator=(const SaiAttr& other) +{ + initializeFrom(other.m_objectType, *other.m_meta, other.m_attr); + return *this; +} + +SaiAttr::SaiAttr(const SaiAttr&& other) +{ + swap(std::move(other)); +} + +SaiAttr& SaiAttr::operator=(const SaiAttr&& other) +{ + swap(std::move(other)); + return *this; +} + +bool SaiAttr::operator<(const SaiAttr& other) const +{ + return m_serializedAttr < other.m_serializedAttr; +} + +const sai_attribute_t& SaiAttr::getSaiAttr() const +{ + return m_attr; +} + +std::string SaiAttr::toString() const +{ + return m_serializedAttr; +} + +sai_attr_id_t SaiAttr::getAttrId() const +{ + return m_attr.id; +} + +void SaiAttr::swap(const SaiAttr&& other) +{ + m_objectType = other.m_objectType; + m_meta = other.m_meta; + m_attr = other.m_attr; + m_serializedAttr = other.m_serializedAttr; +} + +void SaiAttr::initializeFrom( + sai_object_type_t objectType, + const sai_attr_metadata_t& meta, + const sai_attribute_t& attr) +{ + m_objectType = objectType; + m_attr.id = attr.id; + m_meta = &meta; + + m_serializedAttr = sai_serialize_attr_value(*m_meta, attr); + + // deserialize to actually preform a deep copy of attr + // and attribute value's dynamically allocated lists. + sai_deserialize_attr_value(m_serializedAttr, *m_meta, m_attr); +} \ No newline at end of file diff --git a/orchagent/saiattr.h b/orchagent/saiattr.h new file mode 100644 index 0000000000..3d6efcc88a --- /dev/null +++ b/orchagent/saiattr.h @@ -0,0 +1,41 @@ +#pragma once + +extern "C" +{ +#include +#include +} + +#include + +class SaiAttr +{ +public: + SaiAttr() = default; + + SaiAttr(sai_object_type_t objectType, const sai_attribute_t& attr); + SaiAttr(const SaiAttr& other); + SaiAttr(const SaiAttr&& other); + SaiAttr& operator=(const SaiAttr& other); + SaiAttr& operator=(const SaiAttr&& other); + virtual ~SaiAttr(); + + bool operator<(const SaiAttr& other) const; + + const sai_attribute_t& getSaiAttr() const; + std::string toString() const; + sai_attr_id_t getAttrId() const; + +private: + + void initializeFrom( + sai_object_type_t objectType, + const sai_attr_metadata_t& meta, + const sai_attribute_t& attr); + void swap(const SaiAttr&& other); + + sai_object_type_t m_objectType; + const sai_attr_metadata_t* m_meta; + sai_attribute_t m_attr; + std::string m_serializedAttr; +}; diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 5fed8001a4..6302d99cee 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -55,6 +55,7 @@ tests_SOURCES = aclorch_ut.cpp \ $(top_srcdir)/orchagent/pbh/pbhrule.cpp \ $(top_srcdir)/orchagent/pbhorch.cpp \ $(top_srcdir)/orchagent/saihelper.cpp \ + $(top_srcdir)/orchagent/saiattr.cpp \ $(top_srcdir)/orchagent/switchorch.cpp \ $(top_srcdir)/orchagent/pfcwdorch.cpp \ $(top_srcdir)/orchagent/pfcactionhandler.cpp \ diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp index 188220e880..2a7bab8571 100644 --- a/tests/mock_tests/aclorch_ut.cpp +++ b/tests/mock_tests/aclorch_ut.cpp @@ -817,21 +817,21 @@ namespace aclorch_test return false; } - if (it->second.aclaction.enable != true) + if (it->second.getSaiAttr().value.aclaction.enable != true) { return false; } if (attr_value == PACKET_ACTION_FORWARD) { - if (it->second.aclaction.parameter.s32 != SAI_PACKET_ACTION_FORWARD) + if (it->second.getSaiAttr().value.aclaction.parameter.s32 != SAI_PACKET_ACTION_FORWARD) { return false; } } else if (attr_value == PACKET_ACTION_DROP) { - if (it->second.aclaction.parameter.s32 != SAI_PACKET_ACTION_DROP) + if (it->second.getSaiAttr().value.aclaction.parameter.s32 != SAI_PACKET_ACTION_DROP) { return false; } @@ -864,14 +864,14 @@ namespace aclorch_test } char addr[20]; - sai_serialize_ip4(addr, it_field->second.aclfield.data.ip4); + sai_serialize_ip4(addr, it_field->second.getSaiAttr().value.aclfield.data.ip4); if (attr_value != addr) { return false; } char mask[20]; - sai_serialize_ip4(mask, it_field->second.aclfield.mask.ip4); + sai_serialize_ip4(mask, it_field->second.getSaiAttr().value.aclfield.mask.ip4); if (string(mask) != "255.255.255.255") { return false; @@ -886,14 +886,14 @@ namespace aclorch_test } char addr[46]; - sai_serialize_ip6(addr, it_field->second.aclfield.data.ip6); + sai_serialize_ip6(addr, it_field->second.getSaiAttr().value.aclfield.data.ip6); if (attr_value != addr) { return false; } char mask[46]; - sai_serialize_ip6(mask, it_field->second.aclfield.mask.ip6); + sai_serialize_ip6(mask, it_field->second.getSaiAttr().value.aclfield.mask.ip6); if (string(mask) != "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff") { return false; @@ -1366,7 +1366,7 @@ namespace aclorch_test void disableMatch(sai_acl_entry_attr_t attr) { - m_matches.erase(attr); + matches.erase(attr); } }; diff --git a/tests/mock_tests/portal.h b/tests/mock_tests/portal.h index 7dab40036b..238947ef79 100644 --- a/tests/mock_tests/portal.h +++ b/tests/mock_tests/portal.h @@ -18,14 +18,14 @@ struct Portal return aclRule->m_ruleOid; } - static const map &getMatches(const AclRule *aclRule) + static const map &getMatches(const AclRule *aclRule) { - return aclRule->m_matches; + return aclRule->matches; } - static const map &getActions(const AclRule *aclRule) + static const map &getActions(const AclRule *aclRule) { - return aclRule->m_actions; + return aclRule->actions; } }; From 855607839bdb1ad13ab90940bc55a8b1830ae777 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Mon, 25 Oct 2021 17:09:17 +0300 Subject: [PATCH 03/16] intermediate commit Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 134 ++++++++++---------- orchagent/aclorch.h | 8 +- orchagent/pbh/pbhrule.cpp | 24 +--- orchagent/saihelper.cpp | 209 -------------------------------- orchagent/saihelper.h | 3 - tests/mock_tests/aclorch_ut.cpp | 2 +- tests/mock_tests/portal.h | 4 +- 7 files changed, 77 insertions(+), 307 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index a6d7da2c71..a6cc563d22 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -209,8 +209,8 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) { SWSS_LOG_ENTER(); - sai_attribute_value_t value; - value.aclfield.enable = true; + sai_acl_field_data_t matchData; + matchData.enable = true; try { @@ -246,8 +246,8 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) m_inPorts.push_back(port.m_port_id); } - value.aclfield.data.objlist.count = static_cast(m_inPorts.size()); - value.aclfield.data.objlist.list = m_inPorts.data(); + matchData.data.objlist.count = static_cast(m_inPorts.size()); + matchData.data.objlist.list = m_inPorts.data(); } else if (attr_name == MATCH_OUT_PORTS) { @@ -277,46 +277,46 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) m_outPorts.push_back(port.m_port_id); } - value.aclfield.data.objlist.count = static_cast(m_outPorts.size()); - value.aclfield.data.objlist.list = m_outPorts.data(); + matchData.data.objlist.count = static_cast(m_outPorts.size()); + matchData.data.objlist.list = m_outPorts.data(); } else if (attr_name == MATCH_IP_TYPE) { - if (!processIpType(attr_value, value.aclfield.data.u32)) + if (!processIpType(attr_value, matchData.data.u32)) { SWSS_LOG_ERROR("Invalid IP type %s", attr_value.c_str()); return false; } - value.aclfield.mask.u32 = 0xFFFFFFFF; + matchData.mask.u32 = 0xFFFFFFFF; } else if (attr_name == MATCH_TCP_FLAGS) { // Support both exact value match and value/mask match auto flag_data = tokenize(attr_value, '/'); - value.aclfield.data.u8 = to_uint(flag_data[0], 0, 0x3F); + matchData.data.u8 = to_uint(flag_data[0], 0, 0x3F); if (flag_data.size() == 2) { - value.aclfield.mask.u8 = to_uint(flag_data[1], 0, 0x3F); + matchData.mask.u8 = to_uint(flag_data[1], 0, 0x3F); } else { - value.aclfield.mask.u8 = 0x3F; + matchData.mask.u8 = 0x3F; } } else if (attr_name == MATCH_ETHER_TYPE || attr_name == MATCH_L4_SRC_PORT || attr_name == MATCH_L4_DST_PORT) { - value.aclfield.data.u16 = to_uint(attr_value); - value.aclfield.mask.u16 = 0xFFFF; + matchData.data.u16 = to_uint(attr_value); + matchData.mask.u16 = 0xFFFF; } else if (attr_name == MATCH_VLAN_ID) { - value.aclfield.data.u16 = to_uint(attr_value); - value.aclfield.mask.u16 = 0xFFF; + matchData.data.u16 = to_uint(attr_value); + matchData.mask.u16 = 0xFFF; - if (value.aclfield.data.u16 < MIN_VLAN_ID || value.aclfield.data.u16 > MAX_VLAN_ID) + if (matchData.data.u16 < MIN_VLAN_ID || matchData.data.u16 > MAX_VLAN_ID) { SWSS_LOG_ERROR("Invalid VLAN ID: %s", attr_value.c_str()); return false; @@ -327,21 +327,21 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) /* Support both exact value match and value/mask match */ auto dscp_data = tokenize(attr_value, '/'); - value.aclfield.data.u8 = to_uint(dscp_data[0], 0, 0x3F); + matchData.data.u8 = to_uint(dscp_data[0], 0, 0x3F); if (dscp_data.size() == 2) { - value.aclfield.mask.u8 = to_uint(dscp_data[1], 0, 0x3F); + matchData.mask.u8 = to_uint(dscp_data[1], 0, 0x3F); } else { - value.aclfield.mask.u8 = 0x3F; + matchData.mask.u8 = 0x3F; } } else if (attr_name == MATCH_IP_PROTOCOL || attr_name == MATCH_NEXT_HEADER) { - value.aclfield.data.u8 = to_uint(attr_value); - value.aclfield.mask.u8 = 0xFF; + matchData.data.u8 = to_uint(attr_value); + matchData.mask.u8 = 0xFF; } else if (attr_name == MATCH_SRC_IP || attr_name == MATCH_DST_IP) { @@ -352,8 +352,8 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) SWSS_LOG_ERROR("IP type is not v4 type"); return false; } - value.aclfield.data.ip4 = ip.getIp().getV4Addr(); - value.aclfield.mask.ip4 = ip.getMask().getV4Addr(); + matchData.data.ip4 = ip.getIp().getV4Addr(); + matchData.mask.ip4 = ip.getMask().getV4Addr(); } else if (attr_name == MATCH_SRC_IPV6 || attr_name == MATCH_DST_IPV6) { @@ -363,8 +363,8 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) SWSS_LOG_ERROR("IP type is not v6 type"); return false; } - memcpy(value.aclfield.data.ip6, ip.getIp().getV6Addr(), 16); - memcpy(value.aclfield.mask.ip6, ip.getMask().getV6Addr(), 16); + memcpy(matchData.data.ip6, ip.getIp().getV6Addr(), 16); + memcpy(matchData.mask.ip6, ip.getMask().getV6Addr(), 16); } else if ((attr_name == MATCH_L4_SRC_PORT_RANGE) || (attr_name == MATCH_L4_DST_PORT_RANGE)) { @@ -392,30 +392,30 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) } else if (attr_name == MATCH_TC) { - value.aclfield.data.u8 = to_uint(attr_value); - value.aclfield.mask.u8 = 0xFF; + matchData.data.u8 = to_uint(attr_value); + matchData.mask.u8 = 0xFF; } else if (attr_name == MATCH_ICMP_TYPE || attr_name == MATCH_ICMP_CODE || attr_name == MATCH_ICMPV6_TYPE || attr_name == MATCH_ICMPV6_CODE) { - value.aclfield.data.u8 = to_uint(attr_value); - value.aclfield.mask.u8 = 0xFF; + matchData.data.u8 = to_uint(attr_value); + matchData.mask.u8 = 0xFF; } else if (attr_name == MATCH_TUNNEL_VNI) { - value.aclfield.data.u32 = to_uint(attr_value); - value.aclfield.mask.u32 = 0xFFFFFFFF; + matchData.data.u32 = to_uint(attr_value); + matchData.mask.u32 = 0xFFFFFFFF; } else if (attr_name == MATCH_INNER_ETHER_TYPE || attr_name == MATCH_INNER_L4_SRC_PORT || attr_name == MATCH_INNER_L4_DST_PORT) { - value.aclfield.data.u16 = to_uint(attr_value); - value.aclfield.mask.u16 = 0xFFFF; + matchData.data.u16 = to_uint(attr_value); + matchData.mask.u16 = 0xFFFF; } else if (attr_name == MATCH_INNER_IP_PROTOCOL) { - value.aclfield.data.u8 = to_uint(attr_value); - value.aclfield.mask.u8 = 0xFF; + matchData.data.u8 = to_uint(attr_value); + matchData.mask.u8 = 0xFF; } } catch (exception &e) @@ -438,7 +438,7 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) attr_name = MATCH_NEXT_HEADER; } - return setMatch(aclMatchLookup[attr_name], value); + return setMatch(aclMatchLookup[attr_name], matchData); } bool AclRule::validateAddAction(string attr_name, string attr_value) @@ -526,13 +526,13 @@ bool AclRule::create() } // store matches - for (auto it : matches) + for (auto it : m_matches) { rule_attrs.push_back(it.second.getSaiAttr()); } // store actions - for (auto it : actions) + for (auto it : m_actions) { attr = it.second.getSaiAttr(); rule_attrs.push_back(attr); @@ -621,7 +621,7 @@ void AclRule::updateInPorts() SWSS_LOG_ENTER(); sai_status_t status; - auto attr = matches[SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS].getSaiAttr(); + auto attr = m_matches[SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS].getSaiAttr(); attr.value.aclfield.enable = true; status = sai_acl_api->set_acl_entry_attribute(m_ruleOid, &attr); @@ -709,18 +709,18 @@ bool AclRule::updateMatches(const AclRule& updatedRule) // Diff by value to get new matches and updated matches // in a single set_difference pass. set_difference( - updatedRule.matches.begin(), - updatedRule.matches.end(), - matches.begin(), matches.end(), + updatedRule.m_matches.begin(), + updatedRule.m_matches.end(), + m_matches.begin(), m_matches.end(), back_inserter(matchesUpdated) ); // Diff by key only to get delete matches. Assuming that // deleted matches mean setting a match attribute to disabled state. set_difference( - matches.begin(), matches.end(), - updatedRule.matches.begin(), - updatedRule.matches.end(), + m_matches.begin(), m_matches.end(), + updatedRule.m_matches.begin(), + updatedRule.m_matches.end(), back_inserter(matchesDisabled), [](auto& oldMatch, auto& newMatch) { @@ -736,7 +736,7 @@ bool AclRule::updateMatches(const AclRule& updatedRule) { return false; } - matches.erase(static_cast(attr.id)); + m_matches.erase(static_cast(attr.id)); } for (const auto& attrPair: matchesUpdated) @@ -746,7 +746,7 @@ bool AclRule::updateMatches(const AclRule& updatedRule) { return false; } - matches[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + m_matches[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); } return true; @@ -760,18 +760,18 @@ bool AclRule::updateActions(const AclRule& updatedRule) // Diff by value to get new action and updated actions // in a single set_difference pass. set_difference( - updatedRule.actions.begin(), - updatedRule.actions.end(), - actions.begin(), actions.end(), + updatedRule.m_actions.begin(), + updatedRule.m_actions.end(), + m_actions.begin(), m_actions.end(), back_inserter(actionsUpdated) ); // Diff by key only to get delete actions. Assuming that // action matches mean setting a action attribute to disabled state. set_difference( - actions.begin(), actions.end(), - updatedRule.actions.begin(), - updatedRule.actions.end(), + m_actions.begin(), m_actions.end(), + updatedRule.m_actions.begin(), + updatedRule.m_actions.end(), back_inserter(actionsDisabled), [](auto& oldAction, auto& newAction) { @@ -787,7 +787,7 @@ bool AclRule::updateActions(const AclRule& updatedRule) { return false; } - actions.erase(static_cast(attr.id)); + m_actions.erase(static_cast(attr.id)); } for (const auto& attrPair: actionsUpdated) @@ -797,7 +797,7 @@ bool AclRule::updateActions(const AclRule& updatedRule) { return false; } - actions[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + m_actions[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); } return true; @@ -845,18 +845,20 @@ bool AclRule::setAction(sai_acl_entry_attr_t actionId, sai_acl_action_data_t act attr.id = actionId; attr.value.aclaction = actionData; - actions[actionId] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + m_actions[actionId] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); return true; } -bool AclRule::setMatch(sai_acl_entry_attr_t matchId, sai_attribute_value_t value) +bool AclRule::setMatch(sai_acl_entry_attr_t matchId, sai_acl_field_data_t matchData) { SaiAttr attribute(SAI_OBJECT_TYPE_ACL_ENTRY, sai_attribute_t{ .id = static_cast(matchId), - .value = value, + .value = { + .aclfield = matchData, + }, }); - matches[matchId] = attribute; + m_matches[matchId] = attribute; return true; } @@ -1329,7 +1331,7 @@ bool AclRuleL3::validate() { SWSS_LOG_ENTER(); - if (matches.size() == 0 || actions.size() != 1) + if (m_matches.size() == 0 || m_actions.size() != 1) { return false; } @@ -1503,7 +1505,7 @@ bool AclRuleMirror::validate() { SWSS_LOG_ENTER(); - if (matches.size() == 0 || m_sessionName.empty()) + if (m_matches.size() == 0 || m_sessionName.empty()) { return false; } @@ -1539,13 +1541,13 @@ bool AclRuleMirror::create() SWSS_LOG_THROW("Failed to get mirror session OID for session %s", m_sessionName.c_str()); } - for (auto& it: actions) + for (auto& it: m_actions) { auto attr = it.second.getSaiAttr(); attr.value.aclaction.enable = true; attr.value.aclaction.parameter.objlist.list = &oid; attr.value.aclaction.parameter.objlist.count = 1; - actions[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + m_actions[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); } if (!AclRule::create()) @@ -1633,7 +1635,7 @@ bool AclRuleMclag::validate() { SWSS_LOG_ENTER(); - if (matches.size() == 0) + if (m_matches.size() == 0) { return false; } @@ -2380,7 +2382,7 @@ bool AclRuleDTelFlowWatchListEntry::validate() return false; } - if (matches.size() == 0 || actions.size() == 0) + if (m_matches.size() == 0 || m_actions.size() == 0) { return false; } @@ -2540,7 +2542,7 @@ bool AclRuleDTelDropWatchListEntry::validate() return false; } - if (matches.size() == 0 || actions.size() == 0) + if (m_matches.size() == 0 || m_actions.size() == 0) { return false; } diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 1701cff473..27835e3eee 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -221,7 +221,7 @@ class AclRule virtual bool setPriority(const sai_uint32_t &value); virtual bool setAction(sai_acl_entry_attr_t actionId, sai_acl_action_data_t actionData); - virtual bool setMatch(sai_acl_entry_attr_t matchId, sai_attribute_value_t value); + virtual bool setMatch(sai_acl_entry_attr_t matchId, sai_acl_field_data_t matchData); virtual bool setAttribute(sai_attribute_t attr); @@ -239,10 +239,8 @@ class AclRule sai_object_id_t m_ruleOid; sai_object_id_t m_counterOid; uint32_t m_priority; - map m_matches; - map m_actions; - map actions; - map matches; + map m_actions; + map m_matches; string m_redirect_target_next_hop; string m_redirect_target_next_hop_group; diff --git a/orchagent/pbh/pbhrule.cpp b/orchagent/pbh/pbhrule.cpp index ab5057a81d..5fc6f7754e 100644 --- a/orchagent/pbh/pbhrule.cpp +++ b/orchagent/pbh/pbhrule.cpp @@ -11,15 +11,7 @@ bool AclRulePbh::validateAddPriority(const sai_uint32_t &value) { SWSS_LOG_ENTER(); - if ((value < m_minPriority) || (value > m_maxPriority)) - { - SWSS_LOG_ERROR("Failed to validate priority: invalid value %d", value); - return false; - } - - m_priority = value; - - return true; + return setPriority(value); } bool AclRulePbh::validateAddMatch(const sai_attribute_t &attr) @@ -52,9 +44,7 @@ bool AclRulePbh::validateAddMatch(const sai_attribute_t &attr) return false; } - m_matches[attrId] = attr.value; - - return true; + return setMatch(attrId, attr.value.aclfield); } bool AclRulePbh::validateAddAction(const sai_attribute_t &attr) @@ -83,15 +73,7 @@ bool AclRulePbh::validateAddAction(const sai_attribute_t &attr) return false; } - if (!AclRule::isActionSupported(attrId)) - { - SWSS_LOG_ERROR("Action %s is not supported by ASIC", attrName.c_str()); - return false; - } - - m_actions[attrId] = attr.value; - - return true; + return setAction(attrId, attr.value.aclaction); } bool AclRulePbh::validate() diff --git a/orchagent/saihelper.cpp b/orchagent/saihelper.cpp index 9f94e98fb2..490efb90b3 100644 --- a/orchagent/saihelper.cpp +++ b/orchagent/saihelper.cpp @@ -451,212 +451,3 @@ sai_status_t initSaiPhyApi(swss::gearbox_phy_t *phy) return status; } - -template -int compareList(T first, T second) -{ - if (first.count != second.count) - { - if (first.count < second.count) - { - return -1; - } - else - { - return 1; - } - } - return memcmp(first.list, second.list, first.count); -} - -int compareMacAddress(sai_mac_t first, sai_mac_t second) -{ - return memcmp(first, second, sizeof(sai_mac_t)); -} - -int compareIpV4Address(sai_ip4_t first, sai_ip4_t second) -{ - if (first == second) - { - return 0; - } - else if (first > second) - { - return 1; - } - return -1; -} - -int compareIpV6Address(sai_ip6_t first, sai_ip6_t second) -{ - return memcmp(first, second, sizeof(sai_ip6_t)); -} - -int compareIpAddress(sai_ip_address_t first, sai_ip_address_t second) -{ - if (first.addr_family != second.addr_family) - { - if (first.addr_family < second.addr_family) - { - return -1; - } - else - { - return 1; - } - } - switch (first.addr_family) - { - case SAI_IP_ADDR_FAMILY_IPV4: - return compareIpV4Address(first.addr.ip4, second.addr.ip4); - case SAI_IP_ADDR_FAMILY_IPV6: - return compareIpV6Address(first.addr.ip6, second.addr.ip6); - default: - SWSS_LOG_THROW("Unknown IP address famility %d", first.addr_family); - } - SWSS_LOG_THROW("BUG: Unexpectedly reached dead code block"); -} - -// helper function used in ACL rule attributes diff calculation. -bool compareAclAction(sai_acl_entry_attr_t id, sai_acl_action_data_t first, sai_acl_action_data_t second) -{ - auto meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, id); - if (!meta) - { - SWSS_LOG_THROW("SAI Bug: Failed to get metadata for SAI_OBJECT_TYPE_ACL_ENTRY"); - } - - if (first.enable < second.enable) - { - return true; - } - - switch(meta->attrvaluetype) - { - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_BOOL: - return first.parameter.booldata < second.parameter.booldata; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_UINT8: - return first.parameter.u8 < second.parameter.u8; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_INT8: - return first.parameter.s8 < second.parameter.s8; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_UINT16: - return first.parameter.u16 < second.parameter.u16; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_INT16: - return first.parameter.s16 < second.parameter.s16; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_UINT32: - return first.parameter.u32 < second.parameter.u32; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_INT32: - return first.parameter.s32 < second.parameter.s32; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_MAC: - return compareMacAddress(first.parameter.mac, second.parameter.mac) < 0; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_IPV4: - return compareIpV4Address(first.parameter.ip4, second.parameter.ip4) < 0; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_IPV6: - return compareIpV6Address(first.parameter.ip6, second.parameter.ip6) < 0; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_OBJECT_ID: - return first.parameter.oid < second.parameter.oid; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_OBJECT_LIST: - return compareList(first.parameter.objlist, second.parameter.objlist) < 0; - case SAI_ATTR_VALUE_TYPE_ACL_ACTION_DATA_IP_ADDRESS: - return compareIpAddress(first.parameter.ipaddr, second.parameter.ipaddr) < 0; - default: - SWSS_LOG_THROW("Unhandled ACL field type %d", meta->attrvaluetype); - } - - SWSS_LOG_THROW("BUG: Reached unexpected code block"); -} - -// helper function used in ACL rule attributes diff calculation. -bool compareAclField(sai_acl_entry_attr_t id, sai_acl_field_data_t first, sai_acl_field_data_t second) -{ - auto meta = sai_metadata_get_attr_metadata(SAI_OBJECT_TYPE_ACL_ENTRY, id); - if (!meta) - { - SWSS_LOG_THROW("SAI Bug: Failed to get metadata for SAI_OBJECT_TYPE_ACL_ENTRY"); - } - - if (first.enable < second.enable) - { - return true; - } - - switch(meta->attrvaluetype) - { - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_BOOL: - return first.data.booldata < second.data.booldata; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT8: - if (first.mask.u8 != second.mask.u8) - { - return first.mask.u8 < second.mask.u8; - } - return first.data.u8 < second.data.u8; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_INT8: - if (first.mask.s8 != second.mask.s8) - { - return first.mask.s8 < second.mask.s8; - } - return first.data.s8 < second.data.s8; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT16: - if (first.mask.u16 != second.mask.u16) - { - return first.mask.u16 < second.mask.u16; - } - return first.data.u16 < second.data.u16; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_INT16: - if (first.mask.s16 != second.mask.s16) - { - return first.mask.s16 < second.mask.s16; - } - return first.data.s16 < second.data.s16; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT32: - if (first.mask.u32 != second.mask.u32) - { - return first.mask.u32 < second.mask.u32; - } - return first.data.u32 < second.data.u32; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_INT32: - if (first.mask.s32 != second.mask.s32) - { - return first.mask.s32 < second.mask.s32; - } - return first.data.s32 < second.data.s32; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT64: - if (first.mask.u64 != second.mask.u64) - { - return first.mask.u64 < second.mask.u64; - } - return first.data.u64 < second.data.u64; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_MAC: - if (compareMacAddress(first.mask.mac, second.mask.mac) != 0) - { - return compareMacAddress(first.mask.mac, second.mask.mac) < 0; - } - return compareMacAddress(first.data.mac, second.data.mac) < 0; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_IPV4: - if (compareIpV4Address(first.mask.ip4, second.mask.ip4) != 0) - { - return compareIpV4Address(first.mask.ip4, second.mask.ip4) < 0; - } - return compareIpV4Address(first.data.ip4, second.data.ip4) < 0; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_IPV6: - if (compareIpV6Address(first.mask.ip6, second.mask.ip6) != 0) - { - return compareIpV6Address(first.mask.ip6, second.mask.ip6) < 0; - } - return compareIpV6Address(first.data.ip6, second.data.ip6) < 0; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_OBJECT_ID: - return first.data.oid < second.data.oid; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_OBJECT_LIST: - return compareList(first.data.objlist, second.data.objlist) < 0; - case SAI_ATTR_VALUE_TYPE_ACL_FIELD_DATA_UINT8_LIST: - if (compareList(first.mask.u8list, second.mask.u8list) != 0) - { - return compareList(first.mask.u8list, second.mask.u8list) < 0; - } - return compareList(first.data.u8list, second.data.u8list) < 0; - default: - SWSS_LOG_THROW("Unhandled ACL field type %d", meta->attrvaluetype); - } - - SWSS_LOG_THROW("BUG: Reached unexpected code block"); -} diff --git a/orchagent/saihelper.h b/orchagent/saihelper.h index b652a0a5df..450acf8b69 100644 --- a/orchagent/saihelper.h +++ b/orchagent/saihelper.h @@ -7,6 +7,3 @@ void initSaiApi(); void initSaiRedis(const std::string &record_location, const std::string &record_filename); sai_status_t initSaiPhyApi(swss::gearbox_phy_t *phy); - -bool compareAclAction(sai_acl_entry_attr_t id, sai_acl_action_data_t first, sai_acl_action_data_t second); -bool compareAclField(sai_acl_entry_attr_t id, sai_acl_field_data_t first, sai_acl_field_data_t second); diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp index 2a7bab8571..c087058684 100644 --- a/tests/mock_tests/aclorch_ut.cpp +++ b/tests/mock_tests/aclorch_ut.cpp @@ -1366,7 +1366,7 @@ namespace aclorch_test void disableMatch(sai_acl_entry_attr_t attr) { - matches.erase(attr); + m_matches.erase(attr); } }; diff --git a/tests/mock_tests/portal.h b/tests/mock_tests/portal.h index 238947ef79..6952ac718a 100644 --- a/tests/mock_tests/portal.h +++ b/tests/mock_tests/portal.h @@ -20,12 +20,12 @@ struct Portal static const map &getMatches(const AclRule *aclRule) { - return aclRule->matches; + return aclRule->m_matches; } static const map &getActions(const AclRule *aclRule) { - return aclRule->actions; + return aclRule->m_actions; } }; From f9bcfa2d5882adf387d920651ec11d6dcf3d2503 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Tue, 26 Oct 2021 16:55:02 +0300 Subject: [PATCH 04/16] intermediate commit Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 48 +++++++++++++++++++++++++++++++++++-------- orchagent/aclorch.h | 24 ++++++++++------------ 2 files changed, 51 insertions(+), 21 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index a6cc563d22..67181b38fb 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -227,7 +227,7 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return false; } - m_inPorts.clear(); + vector inPorts; for (auto alias : ports) { Port port; @@ -243,11 +243,11 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return false; } - m_inPorts.push_back(port.m_port_id); + inPorts.push_back(port.m_port_id); } - matchData.data.objlist.count = static_cast(m_inPorts.size()); - matchData.data.objlist.list = m_inPorts.data(); + matchData.data.objlist.count = static_cast(inPorts.size()); + matchData.data.objlist.list = inPorts.data(); } else if (attr_name == MATCH_OUT_PORTS) { @@ -258,7 +258,7 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return false; } - m_outPorts.clear(); + vector outPorts; for (auto alias : ports) { Port port; @@ -274,11 +274,11 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return false; } - m_outPorts.push_back(port.m_port_id); + outPorts.push_back(port.m_port_id); } - matchData.data.objlist.count = static_cast(m_outPorts.size()); - matchData.data.objlist.list = m_outPorts.data(); + matchData.data.objlist.count = static_cast(outPorts.size()); + matchData.data.objlist.list = outPorts.data(); } else if (attr_name == MATCH_IP_TYPE) { @@ -636,6 +636,12 @@ bool AclRule::update(const AclRule& updatedRule) { SWSS_LOG_ENTER(); + if (!m_rangeConfig.empty() || !updatedRule.m_rangeConfig.empty()) + { + SWSS_LOG_ERROR("Updating range matches is currently not implemented"); + return false; + } + if (!updateCounter(updatedRule)) { return false; @@ -1616,6 +1622,19 @@ void AclRuleMirror::onUpdate(SubjectType type, void *cntx) } } +bool AclRuleMirror::update(const AclRule& rule) +{ + auto mirrorRule = dynamic_cast(&rule); + if (!mirrorRule) + { + SWSS_LOG_ERROR("Cannot update mirror rule with a rule of a different type"); + return false; + } + + SWSS_LOG_ERROR("Updating mirror rule is currently not implemented"); + return false; +} + AclRuleMclag::AclRuleMclag(AclOrch *aclOrch, string rule, string table, acl_table_type_t type, bool createCounter) : AclRuleL3(aclOrch, rule, table, type, createCounter) { @@ -2502,6 +2521,19 @@ void AclRuleDTelFlowWatchListEntry::onUpdate(SubjectType type, void *cntx) } } +bool AclRuleDTelFlowWatchListEntry::update(const AclRule& rule) +{ + auto dtelDropWathcListRule = dynamic_cast(&rule); + if (!dtelDropWathcListRule) + { + SWSS_LOG_ERROR("Cannot update DTEL flow watch list rule with a rule of a different type"); + return false; + } + + SWSS_LOG_ERROR("Updating DTEL flow watch list rule is currently not implemented"); + return false; +} + AclRuleDTelDropWatchListEntry::AclRuleDTelDropWatchListEntry(AclOrch *aclOrch, DTelOrch *dtel, string rule, string table, acl_table_type_t type) : AclRule(aclOrch, rule, table, type), m_pDTelOrch(dtel) diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 27835e3eee..95cad32bf2 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -203,7 +203,15 @@ class AclRule vector getInPorts() { - return m_inPorts; + vector inPorts; + auto it = m_matches.find(SAI_ACL_ENTRY_ATTR_FIELD_IN_PORT); + if (it == m_matches.end()) + { + return inPorts; + } + auto objlist = it->second.getSaiAttr().value.objlist; + inPorts = vector(objlist.list, objlist.list + objlist.count); + return inPorts; } static shared_ptr makeShared(acl_table_type_t type, AclOrch *acl, MirrorOrch *mirror, DTelOrch *dtel, const string& rule, const string& table, const KeyOpFieldsValuesTuple&); @@ -244,9 +252,6 @@ class AclRule string m_redirect_target_next_hop; string m_redirect_target_next_hop_group; - vector m_inPorts; - vector m_outPorts; - vector m_rangeConfig; vector m_ranges; @@ -301,6 +306,7 @@ class AclRuleMirror: public AclRule void onUpdate(SubjectType, void *) override; AclRuleCounters getCounters(); + bool update(const AclRule& updatedRule) override; protected: bool m_state {false}; string m_sessionName; @@ -318,6 +324,7 @@ class AclRuleDTelFlowWatchListEntry: public AclRule bool remove(); void onUpdate(SubjectType, void *) override; + bool update(const AclRule& updatedRule) override; protected: DTelOrch *m_pDTelOrch; string m_intSessionId; @@ -332,7 +339,6 @@ class AclRuleDTelDropWatchListEntry: public AclRule bool validateAddAction(string attr_name, string attr_value); bool validate(); void onUpdate(SubjectType, void *) override; - protected: DTelOrch *m_pDTelOrch; }; @@ -443,14 +449,6 @@ class AclOrch : public Orch, public Observer bool updateAclTable(string table_id, AclTable &table); bool addAclRule(shared_ptr aclRule, string table_id); bool removeAclRule(string table_id, string rule_id); - - // Update ACL rule based on new configuration in updatedAclRule. - // This method calculates the diff between counter, priority, - // matches and actions and applies the difference to SAI. - // This API is limited to support actions and matches that - // hold a plain value, e.g: actions or matches of type - // SAI_ATTR_VALUE_TYPE_ACL_ACTION/FIELD_DATA_OBJECT_LIST - // are not supported. bool updateAclRule(shared_ptr updatedAclRule); bool updateAclRule(string table_id, string rule_id, string attr_name, void *data, bool oper); bool updateAclRule(string table_id, string rule_id, bool enableCounter); From 37a6d4dae328e09984fbb709feb81c2c32bb139e Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Tue, 26 Oct 2021 17:26:52 +0300 Subject: [PATCH 05/16] intermediate commit Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 24 ++++++++++++------------ orchagent/aclorch.h | 4 ++-- orchagent/saiattr.cpp | 30 +++++++++++++++--------------- orchagent/saiattr.h | 22 +++++++++++----------- tests/mock_tests/aclorch_ut.cpp | 3 +++ tests/mock_tests/portal.h | 4 ++-- 6 files changed, 45 insertions(+), 42 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 67181b38fb..fbcd55adcc 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -709,8 +709,8 @@ bool AclRule::updatePriority(const AclRule& updatedRule) bool AclRule::updateMatches(const AclRule& updatedRule) { - vector> matchesUpdated; - vector> matchesDisabled; + vector> matchesUpdated; + vector> matchesDisabled; // Diff by value to get new matches and updated matches // in a single set_difference pass. @@ -742,7 +742,7 @@ bool AclRule::updateMatches(const AclRule& updatedRule) { return false; } - m_matches.erase(static_cast(attr.id)); + m_matches.erase(attrPair.first); } for (const auto& attrPair: matchesUpdated) @@ -752,7 +752,7 @@ bool AclRule::updateMatches(const AclRule& updatedRule) { return false; } - m_matches[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + m_matches[attrPair.first] = attrPair.second; } return true; @@ -760,8 +760,8 @@ bool AclRule::updateMatches(const AclRule& updatedRule) bool AclRule::updateActions(const AclRule& updatedRule) { - vector> actionsUpdated; - vector> actionsDisabled; + vector> actionsUpdated; + vector> actionsDisabled; // Diff by value to get new action and updated actions // in a single set_difference pass. @@ -793,7 +793,7 @@ bool AclRule::updateActions(const AclRule& updatedRule) { return false; } - m_actions.erase(static_cast(attr.id)); + m_actions.erase(attrPair.first); } for (const auto& attrPair: actionsUpdated) @@ -803,7 +803,7 @@ bool AclRule::updateActions(const AclRule& updatedRule) { return false; } - m_actions[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + m_actions[attrPair.first] = attrPair.second; } return true; @@ -851,15 +851,15 @@ bool AclRule::setAction(sai_acl_entry_attr_t actionId, sai_acl_action_data_t act attr.id = actionId; attr.value.aclaction = actionData; - m_actions[actionId] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + m_actions[actionId] = SaiAttrWrapper(SAI_OBJECT_TYPE_ACL_ENTRY, attr); return true; } bool AclRule::setMatch(sai_acl_entry_attr_t matchId, sai_acl_field_data_t matchData) { - SaiAttr attribute(SAI_OBJECT_TYPE_ACL_ENTRY, sai_attribute_t{ - .id = static_cast(matchId), + SaiAttrWrapper attribute(SAI_OBJECT_TYPE_ACL_ENTRY, sai_attribute_t{ + .id = matchId, .value = { .aclfield = matchData, }, @@ -1553,7 +1553,7 @@ bool AclRuleMirror::create() attr.value.aclaction.enable = true; attr.value.aclaction.parameter.objlist.list = &oid; attr.value.aclaction.parameter.objlist.count = 1; - m_actions[static_cast(attr.id)] = SaiAttr(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + m_actions[it.first] = SaiAttrWrapper(SAI_OBJECT_TYPE_ACL_ENTRY, attr); } if (!AclRule::create()) diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 95cad32bf2..cfdae1988a 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -247,8 +247,8 @@ class AclRule sai_object_id_t m_ruleOid; sai_object_id_t m_counterOid; uint32_t m_priority; - map m_actions; - map m_matches; + map m_actions; + map m_matches; string m_redirect_target_next_hop; string m_redirect_target_next_hop_group; diff --git a/orchagent/saiattr.cpp b/orchagent/saiattr.cpp index a2771bdffd..1dabd063f1 100644 --- a/orchagent/saiattr.cpp +++ b/orchagent/saiattr.cpp @@ -5,7 +5,7 @@ #include -SaiAttr::SaiAttr(sai_object_type_t objectType, const sai_attribute_t& attr) +SaiAttrWrapper::SaiAttrWrapper(sai_object_type_t objectType, const sai_attribute_t& attr) { auto meta = sai_metadata_get_attr_metadata(objectType, attr.id); if (!meta) @@ -13,57 +13,57 @@ SaiAttr::SaiAttr(sai_object_type_t objectType, const sai_attribute_t& attr) SWSS_LOG_THROW("Failed to get attribute %d metadata", attr.id); } - initializeFrom(objectType, *meta, attr); + init(objectType, *meta, attr); } -SaiAttr::~SaiAttr() +SaiAttrWrapper::~SaiAttrWrapper() { sai_deserialize_free_attribute_value(m_meta->attrvaluetype, m_attr); } -SaiAttr::SaiAttr(const SaiAttr& other) +SaiAttrWrapper::SaiAttrWrapper(const SaiAttrWrapper& other) { - initializeFrom(other.m_objectType, *other.m_meta, other.m_attr); + init(other.m_objectType, *other.m_meta, other.m_attr); } -SaiAttr& SaiAttr::operator=(const SaiAttr& other) +SaiAttrWrapper& SaiAttrWrapper::operator=(const SaiAttrWrapper& other) { - initializeFrom(other.m_objectType, *other.m_meta, other.m_attr); + init(other.m_objectType, *other.m_meta, other.m_attr); return *this; } -SaiAttr::SaiAttr(const SaiAttr&& other) +SaiAttrWrapper::SaiAttrWrapper(const SaiAttrWrapper&& other) { swap(std::move(other)); } -SaiAttr& SaiAttr::operator=(const SaiAttr&& other) +SaiAttrWrapper& SaiAttrWrapper::operator=(const SaiAttrWrapper&& other) { swap(std::move(other)); return *this; } -bool SaiAttr::operator<(const SaiAttr& other) const +bool SaiAttrWrapper::operator<(const SaiAttrWrapper& other) const { return m_serializedAttr < other.m_serializedAttr; } -const sai_attribute_t& SaiAttr::getSaiAttr() const +const sai_attribute_t& SaiAttrWrapper::getSaiAttr() const { return m_attr; } -std::string SaiAttr::toString() const +std::string SaiAttrWrapper::toString() const { return m_serializedAttr; } -sai_attr_id_t SaiAttr::getAttrId() const +sai_attr_id_t SaiAttrWrapper::getAttrId() const { return m_attr.id; } -void SaiAttr::swap(const SaiAttr&& other) +void SaiAttrWrapper::swap(const SaiAttrWrapper&& other) { m_objectType = other.m_objectType; m_meta = other.m_meta; @@ -71,7 +71,7 @@ void SaiAttr::swap(const SaiAttr&& other) m_serializedAttr = other.m_serializedAttr; } -void SaiAttr::initializeFrom( +void SaiAttrWrapper::init( sai_object_type_t objectType, const sai_attr_metadata_t& meta, const sai_attribute_t& attr) diff --git a/orchagent/saiattr.h b/orchagent/saiattr.h index 3d6efcc88a..2ebbbd0974 100644 --- a/orchagent/saiattr.h +++ b/orchagent/saiattr.h @@ -8,19 +8,19 @@ extern "C" #include -class SaiAttr +class SaiAttrWrapper { public: - SaiAttr() = default; + SaiAttrWrapper() = default; - SaiAttr(sai_object_type_t objectType, const sai_attribute_t& attr); - SaiAttr(const SaiAttr& other); - SaiAttr(const SaiAttr&& other); - SaiAttr& operator=(const SaiAttr& other); - SaiAttr& operator=(const SaiAttr&& other); - virtual ~SaiAttr(); + SaiAttrWrapper(sai_object_type_t objectType, const sai_attribute_t& attr); + SaiAttrWrapper(const SaiAttrWrapper& other); + SaiAttrWrapper(const SaiAttrWrapper&& other); + SaiAttrWrapper& operator=(const SaiAttrWrapper& other); + SaiAttrWrapper& operator=(const SaiAttrWrapper&& other); + virtual ~SaiAttrWrapper(); - bool operator<(const SaiAttr& other) const; + bool operator<(const SaiAttrWrapper& other) const; const sai_attribute_t& getSaiAttr() const; std::string toString() const; @@ -28,11 +28,11 @@ class SaiAttr private: - void initializeFrom( + void init( sai_object_type_t objectType, const sai_attr_metadata_t& meta, const sai_attribute_t& attr); - void swap(const SaiAttr&& other); + void swap(const SaiAttrWrapper&& other); sai_object_type_t m_objectType; const sai_attr_metadata_t* m_meta; diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp index c087058684..28716a9119 100644 --- a/tests/mock_tests/aclorch_ut.cpp +++ b/tests/mock_tests/aclorch_ut.cpp @@ -1397,7 +1397,10 @@ namespace aclorch_test updatedRule2->disableMatch(SAI_ACL_ENTRY_ATTR_FIELD_DST_IP); ASSERT_TRUE(orch->m_aclOrch->updateAclRule(updatedRule2)); ASSERT_TRUE(validateAclRuleCounter(*orch->m_aclOrch->getAclRule(acl_table_id, acl_rule_id), false)); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_PRIORITY), "900"); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_FIELD_SRC_IP), "2.2.2.2&mask:255.255.255.0"); ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_FIELD_DST_IP), "disabled"); + ASSERT_EQ(getAclRuleSaiAttribute(*rule, SAI_ACL_ENTRY_ATTR_ACTION_PACKET_ACTION), "SAI_PACKET_ACTION_DROP"); auto updatedRule3 = make_shared(*updatedRule2); updatedRule3->setCounterEnabled(true); diff --git a/tests/mock_tests/portal.h b/tests/mock_tests/portal.h index 6952ac718a..c2438e8e1c 100644 --- a/tests/mock_tests/portal.h +++ b/tests/mock_tests/portal.h @@ -18,12 +18,12 @@ struct Portal return aclRule->m_ruleOid; } - static const map &getMatches(const AclRule *aclRule) + static const map &getMatches(const AclRule *aclRule) { return aclRule->m_matches; } - static const map &getActions(const AclRule *aclRule) + static const map &getActions(const AclRule *aclRule) { return aclRule->m_actions; } From da587bc334ccd6259fc5893ab36f592eb94d939a Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Tue, 26 Oct 2021 17:35:12 +0300 Subject: [PATCH 06/16] intermediate commit Signed-off-by: Stepan Blyshchak --- orchagent/saiattr.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/saiattr.cpp b/orchagent/saiattr.cpp index 1dabd063f1..3e282e354e 100644 --- a/orchagent/saiattr.cpp +++ b/orchagent/saiattr.cpp @@ -85,4 +85,4 @@ void SaiAttrWrapper::init( // deserialize to actually preform a deep copy of attr // and attribute value's dynamically allocated lists. sai_deserialize_attr_value(m_serializedAttr, *m_meta, m_attr); -} \ No newline at end of file +} From 902853dcf03c92571405f3ee6452a1be28488d13 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Wed, 27 Oct 2021 17:34:57 +0300 Subject: [PATCH 07/16] internal review Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 38 ++++++++++++++++++++++++++++---------- orchagent/aclorch.h | 13 +------------ orchagent/saiattr.h | 6 +++--- 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index fbcd55adcc..2ad133c95d 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -752,7 +752,7 @@ bool AclRule::updateMatches(const AclRule& updatedRule) { return false; } - m_matches[attrPair.first] = attrPair.second; + setMatch(attrPair.first, attr.value.aclfield); } return true; @@ -803,7 +803,7 @@ bool AclRule::updateActions(const AclRule& updatedRule) { return false; } - m_actions[attrPair.first] = attrPair.second; + setAction(attrPair.first, attr.value.aclaction); } return true; @@ -858,13 +858,12 @@ bool AclRule::setAction(sai_acl_entry_attr_t actionId, sai_acl_action_data_t act bool AclRule::setMatch(sai_acl_entry_attr_t matchId, sai_acl_field_data_t matchData) { - SaiAttrWrapper attribute(SAI_OBJECT_TYPE_ACL_ENTRY, sai_attribute_t{ - .id = matchId, - .value = { - .aclfield = matchData, - }, - }); - m_matches[matchId] = attribute; + sai_attribute_t attr; + attr.id = matchId; + attr.value.aclfield = matchData; + + m_matches[matchId] = SaiAttrWrapper(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + return true; } @@ -910,6 +909,24 @@ AclRuleCounters AclRule::getCounters() return AclRuleCounters(counter_attr[0].value.u64, counter_attr[1].value.u64); } +vector AclRule::getInPorts() const +{ + vector inPorts; + auto it = m_matches.find(SAI_ACL_ENTRY_ATTR_FIELD_IN_PORT); + if (it == m_matches.end()) + { + return inPorts; + } + auto attr = it->second.getSaiAttr(); + if (!attr.value.aclfield.enable) + { + return inPorts; + } + auto objlist = attr.value.aclfield.data.objlist; + inPorts = vector(objlist.list, objlist.list + objlist.count); + return inPorts; +} + shared_ptr AclRule::makeShared(acl_table_type_t type, AclOrch *acl, MirrorOrch *mirror, DTelOrch *dtel, const string& rule, const string& table, const KeyOpFieldsValuesTuple& data) { string action; @@ -1553,7 +1570,7 @@ bool AclRuleMirror::create() attr.value.aclaction.enable = true; attr.value.aclaction.parameter.objlist.list = &oid; attr.value.aclaction.parameter.objlist.count = 1; - m_actions[it.first] = SaiAttrWrapper(SAI_OBJECT_TYPE_ACL_ENTRY, attr); + setAction(it.first, attr.value.aclaction); } if (!AclRule::create()) @@ -3616,6 +3633,7 @@ bool AclOrch::updateAclRule(string table_id, string rule_id, bool enableCounter) bool AclOrch::updateAclRule(shared_ptr updatedRule) { + SWSS_LOG_ENTER(); auto tableId = updatedRule->getTableId(); sai_object_id_t tableOid = getTableById(tableId); diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index cfdae1988a..d0db47f5c3 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -201,18 +201,7 @@ class AclRule return m_counterOid; } - vector getInPorts() - { - vector inPorts; - auto it = m_matches.find(SAI_ACL_ENTRY_ATTR_FIELD_IN_PORT); - if (it == m_matches.end()) - { - return inPorts; - } - auto objlist = it->second.getSaiAttr().value.objlist; - inPorts = vector(objlist.list, objlist.list + objlist.count); - return inPorts; - } + vector getInPorts() const; static shared_ptr makeShared(acl_table_type_t type, AclOrch *acl, MirrorOrch *mirror, DTelOrch *dtel, const string& rule, const string& table, const KeyOpFieldsValuesTuple&); virtual ~AclRule() {} diff --git a/orchagent/saiattr.h b/orchagent/saiattr.h index 2ebbbd0974..9b9e4b8ce0 100644 --- a/orchagent/saiattr.h +++ b/orchagent/saiattr.h @@ -34,8 +34,8 @@ class SaiAttrWrapper const sai_attribute_t& attr); void swap(const SaiAttrWrapper&& other); - sai_object_type_t m_objectType; - const sai_attr_metadata_t* m_meta; - sai_attribute_t m_attr; + sai_object_type_t m_objectType {SAI_OBJECT_TYPE_NULL}; + const sai_attr_metadata_t* m_meta {nullptr}; + sai_attribute_t m_attr {}; std::string m_serializedAttr; }; From 0b219aadb3536270900247798ead2bf8097bd10b Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 10 Nov 2021 10:08:45 +0000 Subject: [PATCH 08/16] fix issues found during vs tests Signed-off-by: Stepan Blyschak --- orchagent/aclorch.cpp | 24 +++++++++++++----------- orchagent/saiattr.cpp | 13 +++++++++---- orchagent/saiattr.h | 6 +++--- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 2ad133c95d..6e16a04c41 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -209,7 +209,10 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) { SWSS_LOG_ENTER(); - sai_acl_field_data_t matchData; + sai_acl_field_data_t matchData{}; + vector inPorts; + vector outPorts; + matchData.enable = true; try @@ -227,7 +230,6 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return false; } - vector inPorts; for (auto alias : ports) { Port port; @@ -258,7 +260,6 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return false; } - vector outPorts; for (auto alias : ports) { Port port; @@ -375,7 +376,7 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return false; } - rangeConfig.rangeType = aclRangeTypeLookup[attr_value]; + rangeConfig.rangeType = aclRangeTypeLookup[attr_name]; // check boundaries if ((rangeConfig.min > USHRT_MAX) || @@ -526,13 +527,14 @@ bool AclRule::create() } // store matches - for (auto it : m_matches) + for (auto& it : m_matches) { - rule_attrs.push_back(it.second.getSaiAttr()); + attr = it.second.getSaiAttr(); + rule_attrs.push_back(attr); } // store actions - for (auto it : m_actions) + for (auto& it : m_actions) { attr = it.second.getSaiAttr(); rule_attrs.push_back(attr); @@ -1354,7 +1356,7 @@ bool AclRuleL3::validate() { SWSS_LOG_ENTER(); - if (m_matches.size() == 0 || m_actions.size() != 1) + if ((m_rangeConfig.empty() && m_matches.empty()) || m_actions.size() != 1) { return false; } @@ -1528,7 +1530,7 @@ bool AclRuleMirror::validate() { SWSS_LOG_ENTER(); - if (m_matches.size() == 0 || m_sessionName.empty()) + if ((m_rangeConfig.empty() && m_matches.empty()) || m_sessionName.empty()) { return false; } @@ -2418,7 +2420,7 @@ bool AclRuleDTelFlowWatchListEntry::validate() return false; } - if (m_matches.size() == 0 || m_actions.size() == 0) + if ((m_rangeConfig.empty() && m_matches.empty()) || m_actions.size() == 0) { return false; } @@ -2591,7 +2593,7 @@ bool AclRuleDTelDropWatchListEntry::validate() return false; } - if (m_matches.size() == 0 || m_actions.size() == 0) + if ((m_rangeConfig.empty() && m_matches.empty()) || m_actions.size() == 0) { return false; } diff --git a/orchagent/saiattr.cpp b/orchagent/saiattr.cpp index 3e282e354e..42fe2d57d0 100644 --- a/orchagent/saiattr.cpp +++ b/orchagent/saiattr.cpp @@ -18,7 +18,10 @@ SaiAttrWrapper::SaiAttrWrapper(sai_object_type_t objectType, const sai_attribute SaiAttrWrapper::~SaiAttrWrapper() { - sai_deserialize_free_attribute_value(m_meta->attrvaluetype, m_attr); + if (m_meta) + { + sai_deserialize_free_attribute_value(m_meta->attrvaluetype, m_attr); + } } SaiAttrWrapper::SaiAttrWrapper(const SaiAttrWrapper& other) @@ -32,12 +35,12 @@ SaiAttrWrapper& SaiAttrWrapper::operator=(const SaiAttrWrapper& other) return *this; } -SaiAttrWrapper::SaiAttrWrapper(const SaiAttrWrapper&& other) +SaiAttrWrapper::SaiAttrWrapper(SaiAttrWrapper&& other) { swap(std::move(other)); } -SaiAttrWrapper& SaiAttrWrapper::operator=(const SaiAttrWrapper&& other) +SaiAttrWrapper& SaiAttrWrapper::operator=(SaiAttrWrapper&& other) { swap(std::move(other)); return *this; @@ -63,12 +66,14 @@ sai_attr_id_t SaiAttrWrapper::getAttrId() const return m_attr.id; } -void SaiAttrWrapper::swap(const SaiAttrWrapper&& other) +void SaiAttrWrapper::swap(SaiAttrWrapper&& other) { m_objectType = other.m_objectType; m_meta = other.m_meta; m_attr = other.m_attr; m_serializedAttr = other.m_serializedAttr; + other.m_attr = sai_attribute_t{}; + other.m_serializedAttr.clear(); } void SaiAttrWrapper::init( diff --git a/orchagent/saiattr.h b/orchagent/saiattr.h index 9b9e4b8ce0..c4fef0ba0d 100644 --- a/orchagent/saiattr.h +++ b/orchagent/saiattr.h @@ -15,9 +15,9 @@ class SaiAttrWrapper SaiAttrWrapper(sai_object_type_t objectType, const sai_attribute_t& attr); SaiAttrWrapper(const SaiAttrWrapper& other); - SaiAttrWrapper(const SaiAttrWrapper&& other); + SaiAttrWrapper(SaiAttrWrapper&& other); SaiAttrWrapper& operator=(const SaiAttrWrapper& other); - SaiAttrWrapper& operator=(const SaiAttrWrapper&& other); + SaiAttrWrapper& operator=(SaiAttrWrapper&& other); virtual ~SaiAttrWrapper(); bool operator<(const SaiAttrWrapper& other) const; @@ -32,7 +32,7 @@ class SaiAttrWrapper sai_object_type_t objectType, const sai_attr_metadata_t& meta, const sai_attribute_t& attr); - void swap(const SaiAttrWrapper&& other); + void swap(SaiAttrWrapper&& other); sai_object_type_t m_objectType {SAI_OBJECT_TYPE_NULL}; const sai_attr_metadata_t* m_meta {nullptr}; From c1287d39c32a1810f25aaff1ad03e50cee979e5d Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 10 Nov 2021 18:48:47 +0200 Subject: [PATCH 09/16] fix issue observed during mux test: change IN_PORT to IN_PORTS 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 2ad133c95d..f5d27e917d 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -912,7 +912,7 @@ AclRuleCounters AclRule::getCounters() vector AclRule::getInPorts() const { vector inPorts; - auto it = m_matches.find(SAI_ACL_ENTRY_ATTR_FIELD_IN_PORT); + auto it = m_matches.find(SAI_ACL_ENTRY_ATTR_FIELD_IN_PORTS); if (it == m_matches.end()) { return inPorts; From ff570c016c27e00f9a75875640d490e4df8babce Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 10 Nov 2021 19:19:33 +0200 Subject: [PATCH 10/16] fix indentation 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 da5225aec8..c341c5df94 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -529,7 +529,7 @@ bool AclRule::create() // store matches for (auto& it : m_matches) { - attr = it.second.getSaiAttr(); + attr = it.second.getSaiAttr(); rule_attrs.push_back(attr); } From b6632ec765f86f9b7bfae813303ce0bea4c9d16a Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 10 Nov 2021 19:20:19 +0200 Subject: [PATCH 11/16] remove redundant include Signed-off-by: Stepan Blyschak --- orchagent/saiattr.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/orchagent/saiattr.cpp b/orchagent/saiattr.cpp index 42fe2d57d0..1c24489ed5 100644 --- a/orchagent/saiattr.cpp +++ b/orchagent/saiattr.cpp @@ -3,8 +3,6 @@ #include #include -#include - SaiAttrWrapper::SaiAttrWrapper(sai_object_type_t objectType, const sai_attribute_t& attr) { auto meta = sai_metadata_get_attr_metadata(objectType, attr.id); From ffea5c0fa7e86575b9c4056386c8ee912df933e1 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 11 Nov 2021 17:19:33 +0200 Subject: [PATCH 12/16] define udpate for flow watch list entry acl Signed-off-by: Stepan Blyschak --- orchagent/aclorch.h | 1 + 1 file changed, 1 insertion(+) diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 15dd1197d9..76ef87a8ab 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -301,6 +301,7 @@ class AclRuleDTelFlowWatchListEntry: public AclRule bool activate(); bool deactivate(); + bool update(const AclRule& updatedRule) override; protected: DTelOrch *m_pDTelOrch; string m_intSessionId; From eacb0c859ed3c917b0d318029e929bb2655f9c69 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Thu, 18 Nov 2021 11:19:47 +0200 Subject: [PATCH 13/16] [aclorch] make AclRule::validateAddAction pure virtual Signed-off-by: Stepan Blyshchak --- orchagent/aclorch.cpp | 5 ----- orchagent/aclorch.h | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 898edc432c..e3b279a482 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -449,11 +449,6 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return setMatch(aclMatchLookup[attr_name], matchData); } -bool AclRule::validateAddAction(string attr_name, string attr_value) -{ - return false; -} - bool AclRule::processIpType(string type, sai_uint32_t &ip_type) { SWSS_LOG_ENTER(); diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 76ef87a8ab..9b1df18570 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -140,7 +140,7 @@ class AclRule AclRule(AclOrch *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); + virtual bool validateAddAction(string attr_name, string attr_value) = 0; 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) From 5b26cbca409e34db42e6728304de476ff1a61372 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Thu, 18 Nov 2021 14:38:20 +0200 Subject: [PATCH 14/16] Revert "[aclorch] make AclRule::validateAddAction pure virtual" This reverts commit eacb0c859ed3c917b0d318029e929bb2655f9c69. --- orchagent/aclorch.cpp | 5 +++++ orchagent/aclorch.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index e3b279a482..898edc432c 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -449,6 +449,11 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return setMatch(aclMatchLookup[attr_name], matchData); } +bool AclRule::validateAddAction(string attr_name, string attr_value) +{ + return false; +} + bool AclRule::processIpType(string type, sai_uint32_t &ip_type) { SWSS_LOG_ENTER(); diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 9b1df18570..76ef87a8ab 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -140,7 +140,7 @@ class AclRule AclRule(AclOrch *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) From efadc50f101f450a0527ba9863dc1e62facb663e Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Thu, 18 Nov 2021 14:51:24 +0200 Subject: [PATCH 15/16] impl AclRule::validateAddAction on PbhRule Signed-off-by: Stepan Blyshchak --- orchagent/pbh/pbhrule.cpp | 5 +++++ orchagent/pbh/pbhrule.h | 1 + 2 files changed, 6 insertions(+) diff --git a/orchagent/pbh/pbhrule.cpp b/orchagent/pbh/pbhrule.cpp index 5fc6f7754e..d4e2218cf0 100644 --- a/orchagent/pbh/pbhrule.cpp +++ b/orchagent/pbh/pbhrule.cpp @@ -93,3 +93,8 @@ void AclRulePbh::onUpdate(SubjectType, void *) { // Do nothing } + +bool AclRulePbh::validateAddAction(string attr_name, string attr_value) +{ + SWSS_LOG_THROW("This API should not be used on PbhRule"); +} diff --git a/orchagent/pbh/pbhrule.h b/orchagent/pbh/pbhrule.h index 2ea38a2c0a..9e661761c4 100644 --- a/orchagent/pbh/pbhrule.h +++ b/orchagent/pbh/pbhrule.h @@ -12,4 +12,5 @@ class AclRulePbh: public AclRule bool validateAddAction(const sai_attribute_t &attr); bool validate() override; void onUpdate(SubjectType, void *) override; + bool validateAddAction(string attr_name, string attr_value) override; }; From 691019e6557e625ccba1626bde151c0d85c93fe2 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak Date: Thu, 18 Nov 2021 14:59:43 +0200 Subject: [PATCH 16/16] Revert "Revert "[aclorch] make AclRule::validateAddAction pure virtual"" This reverts commit 5b26cbca409e34db42e6728304de476ff1a61372. --- orchagent/aclorch.cpp | 5 ----- orchagent/aclorch.h | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 898edc432c..e3b279a482 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -449,11 +449,6 @@ bool AclRule::validateAddMatch(string attr_name, string attr_value) return setMatch(aclMatchLookup[attr_name], matchData); } -bool AclRule::validateAddAction(string attr_name, string attr_value) -{ - return false; -} - bool AclRule::processIpType(string type, sai_uint32_t &ip_type) { SWSS_LOG_ENTER(); diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 76ef87a8ab..9b1df18570 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -140,7 +140,7 @@ class AclRule AclRule(AclOrch *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); + virtual bool validateAddAction(string attr_name, string attr_value) = 0; 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)