Skip to content

Commit

Permalink
[copporch] Add safeguard during policer attribute update (#2977)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@vivekrnv @mssonicbld
vivekrnv authored and mssonicbld committed Dec 20, 2023
1 parent 1377bb4 commit a7bcfe2
Show file tree
Hide file tree
Showing 4 changed files with 155 additions and 11 deletions.
67 changes: 59 additions & 8 deletions orchagent/copporch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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]];
}

Expand Down Expand Up @@ -460,8 +460,28 @@ bool CoppOrch::createPolicer(string trap_group_name, vector<sai_attribute_t> &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;
}

Expand Down Expand Up @@ -1107,12 +1127,14 @@ bool CoppOrch::getAttribsFromTrapGroup (vector<FieldValueTuple> &fv_tuple,
bool CoppOrch::trapGroupUpdatePolicer (string trap_group_name,
vector<sai_attribute_t> &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).",
Expand All @@ -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)
Expand Down
14 changes: 12 additions & 2 deletions orchagent/copporch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<sai_object_id_t, sai_object_id_t> TrapGroupPolicerTable;
typedef std::map<sai_object_id_t, policer_object> TrapGroupPolicerTable;
/* TrapIdTrapObjectsTable: trap ID, copp trap objects */
typedef std::map<sai_hostif_trap_type_t, copp_trap_objects> TrapIdTrapObjectsTable;
/* TrapGroupHostIfMap: trap group ID, host interface ID */
Expand Down Expand Up @@ -113,7 +123,7 @@ class CoppOrch : public Orch
bool createPolicer(std::string trap_group, std::vector<sai_attribute_t> &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<sai_attribute_t> &hostif_attribs);
bool removeGenetlinkHostIf(std::string trap_group_name);
Expand Down
80 changes: 79 additions & 1 deletion tests/mock_tests/copporch_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ namespace copporch_test
static_cast<Orch*>(this->coppOrch.get())->doTask(*consumer);
}

task_process_status doProcessCoppRule(const std::deque<KeyOpFieldsValuesTuple> &entries)
{
// ConsumerStateTable is used for APP DB
auto consumer = std::unique_ptr<Consumer>(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;
Expand Down Expand Up @@ -322,7 +334,7 @@ namespace copporch_test
}
}

TEST_F(CoppOrchTest, TrapGroupWithPolicer_AddRemove)
TEST_F(CoppOrchTest, TrapGroupWithPolicer_AddUpdateRemove)
{
const std::string trapGroupName = "queue4_group2";

Expand All @@ -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" }
Expand All @@ -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<KeyOpFieldsValuesTuple>(
{
{
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<KeyOpFieldsValuesTuple>(
Expand All @@ -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<KeyOpFieldsValuesTuple>(
{
{
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<KeyOpFieldsValuesTuple>(
{
{
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";
Expand Down
5 changes: 5 additions & 0 deletions tests/mock_tests/portal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit a7bcfe2

Please sign in to comment.