From a7bcfe2c91ad2a657903dacd92f9ee83804ee474 Mon Sep 17 00:00:00 2001 From: Vivek Date: Sat, 9 Dec 2023 20:55:33 -0800 Subject: [PATCH] [copporch] Add safeguard during policer attribute update (#2977) Add safeguard during policer attribute update in copporch when the config is trying to change CREATE_AND_UPDATE attributes by not allowing CREATE_ONLY attributes to pass to SAI --- orchagent/copporch.cpp | 67 ++++++++++++++++++++++---- orchagent/copporch.h | 14 +++++- tests/mock_tests/copporch_ut.cpp | 80 +++++++++++++++++++++++++++++++- tests/mock_tests/portal.h | 5 ++ 4 files changed, 155 insertions(+), 11 deletions(-) diff --git a/orchagent/copporch.cpp b/orchagent/copporch.cpp index 8a58ae73a0..9d597c601f 100644 --- a/orchagent/copporch.cpp +++ b/orchagent/copporch.cpp @@ -369,7 +369,7 @@ bool CoppOrch::removePolicer(string trap_group_name) sai_attribute_t attr; sai_status_t sai_status; - sai_object_id_t policer_id = getPolicer(trap_group_name); + sai_object_id_t policer_id = getPolicer(trap_group_name).policer_id; if (SAI_NULL_OBJECT_ID == policer_id) { @@ -407,21 +407,21 @@ bool CoppOrch::removePolicer(string trap_group_name) return true; } -sai_object_id_t CoppOrch::getPolicer(string trap_group_name) +policer_object CoppOrch::getPolicer(string trap_group_name) { SWSS_LOG_ENTER(); SWSS_LOG_DEBUG("trap group name:%s:", trap_group_name.c_str()); if (m_trap_group_map.find(trap_group_name) == m_trap_group_map.end()) { - return SAI_NULL_OBJECT_ID; + return policer_object(); } SWSS_LOG_DEBUG("trap group id:%" PRIx64, m_trap_group_map[trap_group_name]); if (m_trap_group_policer_map.find(m_trap_group_map[trap_group_name]) == m_trap_group_policer_map.end()) { - return SAI_NULL_OBJECT_ID; + return policer_object(); } - SWSS_LOG_DEBUG("trap group policer id:%" PRIx64, m_trap_group_policer_map[m_trap_group_map[trap_group_name]]); + SWSS_LOG_DEBUG("trap group policer id:%" PRIx64, m_trap_group_policer_map[m_trap_group_map[trap_group_name]].policer_id); return m_trap_group_policer_map[m_trap_group_map[trap_group_name]]; } @@ -460,8 +460,28 @@ bool CoppOrch::createPolicer(string trap_group_name, vector &po } } + policer_object obj; + obj.policer_id = policer_id; + /* Save the CREATE_ONLY attributes for future use */ + for (sai_uint32_t ind = 0; ind < policer_attribs.size(); ind++) + { + auto attr = policer_attribs[ind]; + if(attr.id == SAI_POLICER_ATTR_METER_TYPE) + { + obj.meter = (sai_meter_type_t)attr.value.s32; + } + else if(attr.id == SAI_POLICER_ATTR_MODE) + { + obj.mode = (sai_policer_mode_t)attr.value.s32; + } + else if(attr.id == SAI_POLICER_ATTR_COLOR_SOURCE) + { + obj.color = (sai_policer_color_source_t)attr.value.s32; + } + } + SWSS_LOG_NOTICE("Bind policer to trap group %s:", trap_group_name.c_str()); - m_trap_group_policer_map[m_trap_group_map[trap_group_name]] = policer_id; + m_trap_group_policer_map[m_trap_group_map[trap_group_name]] = obj; return true; } @@ -1107,12 +1127,14 @@ bool CoppOrch::getAttribsFromTrapGroup (vector &fv_tuple, bool CoppOrch::trapGroupUpdatePolicer (string trap_group_name, vector &policer_attribs) { - sai_object_id_t policer_id = getPolicer(trap_group_name); - if (m_trap_group_map.find(trap_group_name) == m_trap_group_map.end()) { return false; } + + auto policer_object = getPolicer(trap_group_name); + auto policer_id = policer_object.policer_id; + if (SAI_NULL_OBJECT_ID == policer_id) { SWSS_LOG_WARN("Creating policer for existing Trap group: %" PRIx64 " (name:%s).", @@ -1128,6 +1150,35 @@ bool CoppOrch::trapGroupUpdatePolicer (string trap_group_name, for (sai_uint32_t ind = 0; ind < policer_attribs.size(); ind++) { auto policer_attr = policer_attribs[ind]; + /* + Updating the CREATE_ONLY attributes of the policer will cause a crash + If modified, throw an error log and proceed with changeable attributes + */ + if(policer_attr.id == SAI_POLICER_ATTR_METER_TYPE) + { + if (policer_object.meter != (sai_meter_type_t)policer_attr.value.s32) + { + SWSS_LOG_ERROR("Trying to modify policer attribute: (meter), trap group: (%s)", trap_group_name.c_str()); + } + continue; + } + else if(policer_attr.id == SAI_POLICER_ATTR_MODE) + { + if (policer_object.mode != (sai_policer_mode_t)policer_attr.value.s32) + { + SWSS_LOG_ERROR("Trying to modify policer attribute: (mode), trap group: (%s)", trap_group_name.c_str()); + } + continue; + } + else if(policer_attr.id == SAI_POLICER_ATTR_COLOR_SOURCE) + { + if (policer_object.color != (sai_policer_color_source_t)policer_attr.value.s32) + { + SWSS_LOG_ERROR("Trying to modify policer attribute: (color), trap group: (%s)", trap_group_name.c_str()); + } + continue; + } + sai_status_t sai_status = sai_policer_api->set_policer_attribute(policer_id, &policer_attr); if (sai_status != SAI_STATUS_SUCCESS) diff --git a/orchagent/copporch.h b/orchagent/copporch.h index d774db64ba..c8f956b6d7 100644 --- a/orchagent/copporch.h +++ b/orchagent/copporch.h @@ -46,8 +46,18 @@ struct copp_trap_objects sai_hostif_trap_type_t trap_type; }; +struct policer_object +{ + sai_object_id_t policer_id; + sai_meter_type_t meter; + sai_policer_mode_t mode; + sai_policer_color_source_t color; + + policer_object() : policer_id(SAI_NULL_OBJECT_ID) {} +}; + /* TrapGroupPolicerTable: trap group ID, policer ID */ -typedef std::map TrapGroupPolicerTable; +typedef std::map TrapGroupPolicerTable; /* TrapIdTrapObjectsTable: trap ID, copp trap objects */ typedef std::map TrapIdTrapObjectsTable; /* TrapGroupHostIfMap: trap group ID, host interface ID */ @@ -113,7 +123,7 @@ class CoppOrch : public Orch bool createPolicer(std::string trap_group, std::vector &policer_attribs); bool removePolicer(std::string trap_group_name); - sai_object_id_t getPolicer(std::string trap_group_name); + policer_object getPolicer(std::string trap_group_name); bool createGenetlinkHostIf(std::string trap_group_name, std::vector &hostif_attribs); bool removeGenetlinkHostIf(std::string trap_group_name); diff --git a/tests/mock_tests/copporch_ut.cpp b/tests/mock_tests/copporch_ut.cpp index fa7c360f01..9f94e58634 100644 --- a/tests/mock_tests/copporch_ut.cpp +++ b/tests/mock_tests/copporch_ut.cpp @@ -33,6 +33,18 @@ namespace copporch_test static_cast(this->coppOrch.get())->doTask(*consumer); } + task_process_status doProcessCoppRule(const std::deque &entries) + { + // ConsumerStateTable is used for APP DB + auto consumer = std::unique_ptr(new Consumer( + new ConsumerStateTable(this->appDb.get(), APP_COPP_TABLE_NAME, 1, 1), + this->coppOrch.get(), APP_COPP_TABLE_NAME + )); + + consumer->addToSync(entries); + return Portal::CoppOrchInternal::processCoppRule(*coppOrch, *consumer); + } + CoppOrch& get() { return *coppOrch; @@ -322,7 +334,7 @@ namespace copporch_test } } - TEST_F(CoppOrchTest, TrapGroupWithPolicer_AddRemove) + TEST_F(CoppOrchTest, TrapGroupWithPolicer_AddUpdateRemove) { const std::string trapGroupName = "queue4_group2"; @@ -341,6 +353,7 @@ namespace copporch_test { copp_queue_field, "4" }, { copp_policer_meter_type_field, "packets" }, { copp_policer_mode_field, "sr_tcm" }, + { copp_policer_color_field, "aware" }, { copp_policer_cir_field, "600" }, { copp_policer_cbs_field, "600" }, { copp_policer_action_red_field, "drop" } @@ -358,8 +371,27 @@ namespace copporch_test const auto &trapGroupOid = cit1->second; const auto &cit2 = trapGroupPolicerMap.find(trapGroupOid); EXPECT_TRUE(cit2 != trapGroupPolicerMap.end()); + EXPECT_TRUE(cit2->second.meter == SAI_METER_TYPE_PACKETS); + EXPECT_TRUE(cit2->second.mode == SAI_POLICER_MODE_SR_TCM); + + /* Update the non create only attributes */ + auto tableKofvt2 = std::deque( + { + { + trapGroupName, + SET_COMMAND, + { + { copp_policer_cir_field, "1000" }, + { copp_policer_cbs_field, "1000" }, + { copp_policer_action_red_field, "drop" } + } + } + } + ); + ASSERT_EQ(coppOrch.doProcessCoppRule(tableKofvt2), task_process_status::task_success); } + // Delete CoPP Trap Group { auto tableKofvt = std::deque( @@ -376,6 +408,52 @@ namespace copporch_test } } + TEST_F(CoppOrchTest, TrapGroupWithPolicer_nothrowExec) + { + const std::string trapGroupName = "queue4_group2"; + + MockCoppOrch coppOrch; + + { + // Create CoPP Trap Group + auto tableKofvt = std::deque( + { + { + trapGroupName, + SET_COMMAND, + { + { copp_trap_action_field, "copy" }, + { copp_trap_priority_field, "4" }, + { copp_queue_field, "4" }, + { copp_policer_meter_type_field, "packets" }, + { copp_policer_mode_field, "sr_tcm" }, + { copp_policer_cir_field, "600" }, + { copp_policer_cbs_field, "600" }, + { copp_policer_action_red_field, "drop" } + } + } + } + ); + coppOrch.doCoppTableTask(tableKofvt); + + // Update create-only Policer Attributes + auto tableKofvt2 = std::deque( + { + { + trapGroupName, + SET_COMMAND, + { + { copp_policer_meter_type_field, "bytes" }, + { copp_policer_mode_field, "tr_tcm" }, + { copp_policer_color_field, "blind" }, + } + } + } + ); + EXPECT_NO_THROW(coppOrch.doProcessCoppRule(tableKofvt2)); + } + } + TEST_F(CoppOrchTest, Trap_AddRemove) { const std::string trapGroupName = "queue4_group1"; diff --git a/tests/mock_tests/portal.h b/tests/mock_tests/portal.h index 8f0c4ab2db..df73f65cc0 100644 --- a/tests/mock_tests/portal.h +++ b/tests/mock_tests/portal.h @@ -81,6 +81,11 @@ struct Portal obj.getTrapIdsFromTrapGroup(trapGroupOid, trapIdList); return trapIdList; } + + static task_process_status processCoppRule(CoppOrch &obj, Consumer& processCoppRule) + { + return obj.processCoppRule(processCoppRule); + } }; struct SflowOrchInternal