Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add mandatory ACL actions when creating mirror ACL table #2205

Closed
wants to merge 3 commits into from
Closed

Add mandatory ACL actions when creating mirror ACL table #2205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f38e3d
Add mandatory ACL actions when creating mirror ACL table
ysmanman Mar 28, 2022
a4fb759
Add acl action to TABLE_TYPE_MIRROR_DSCP and also fix typo
ysmanman Apr 23, 2022
9b1b9bb
Add test coverage.
ysmanman May 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions orchagent/aclorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,12 @@ const set<sai_acl_action_type_t>& AclTableType::getActions() const
return m_aclAcitons;
}

bool AclTableType::addAction(sai_acl_action_type_t action)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AclTableType was designed to be an immutable data structure, so you cannot change it after it has been created. This is due to the fact that matches, actions, bind point types are CREATE_ONLY SAI attributes. Adding mutable methods breaks this invariant. An object of type AclTableType may have been used to create tables already and changing the definition of a table type may cause divergence between software and hardware state.
Instead, a builder AclTableTypeBuilder is used to create AclTableTypes.
Why not adding neccessary actions in initDefaultTableTypes - https://github.com/Azure/sonic-swss/blob/master/orchagent/aclorch.cpp#L2895 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AclTableType was designed to be an immutable data structure, so you cannot change it after it has been created. This is due to the fact that matches, actions, bind point types are CREATE_ONLY SAI attributes. Adding mutable methods breaks this invariant. An object of type AclTableType may have been used to create tables already and changing the definition of a table type may cause divergence between software and hardware state. Instead, a builder AclTableTypeBuilder is used to create AclTableTypes. Why not adding neccessary actions in initDefaultTableTypes - https://github.com/Azure/sonic-swss/blob/master/orchagent/aclorch.cpp#L2895 ?

We also saw similar issue on broadcom paltform. Please see sonic-net/sonic-buildimage#10425.
We may have two options to workaround this issue

  1. Hardcode is_mandatory to False for the general ACL type, such as L3, L3V6
  2. Add default action list for these ACL tables
    What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see, so the issue may be due to SAI telling that action list is mandatory even if it is not (because it worked without actions before ACL table types changes). In this case, hardcoding is_mandatory to be False for the types we know should work without explicitly passing action list should be a simpler workaround. Put a comment that this hardcode needs to be removed once SAI is fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on our communication with broadcom, action list is mandatory when creating mirror acl table. Otherwise, SAI acl table creation will fail. Can you elaborate how would hardcoding is_mandatory to be False work around the SAI limitation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the easiest change may be bypass the below check for the known ACL types
https://github.com/Azure/sonic-swss/blob/bbbd5f44f2c55808785672177e44527f635204d6/orchagent/aclorch.cpp#L1881-L1888

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stepanblyschak Mirroring probably worked in fixed box before. My PR is to get mirroring work in broadcom VOQ chassis.

Copy link
Contributor

@bingwang-ms bingwang-ms May 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ysmanman
Thanks for the update. Could you clarify why SAI call will fail if we bypass this check? Before this change, there is no check for action list existance. Is it a new feature of SAI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bingwang-ms @stepanblyschak Let me provide some context of this PR. We were testing mirroring in broadcom DNX or VOQ chassis with broadcom SAI 5.2 and observed orchagent failed to add ACL rule in mirroring ACL table. The failure is because the ACL actions is not provided at the time of mirror ACL table creation. Broadcom DNX devices need ACL action list at the time of ACL table creation. To address this issue, this PR adds required ACL actions to mirroring type ACL table if the action list is mandatory on table creation.

Copy link
Contributor

@bingwang-ms bingwang-ms May 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ysmanman .
I saw similar issues when testing on Broadcom XGS devices. Please see sonic-net/sonic-buildimage#10425.
Per your clarification, the action_list for ACL table is a must when we creating ACL table now? If that's the case, my by-pass solution will not work. The solution in this PR makes sense. We have to do similar change for other ACL table types (L3, L3V6, and etc.)

Copy link
Contributor

@stepanblyschak stepanblyschak May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ysmanman @bingwang-ms Thanks for explanation. This PR makes sense. I noticed you do not modify the acl table type in m_aclTableTypes but a copy in AclTable. That is fine to me. You can mark this as resolved

{
m_aclAcitons.insert(action);
return true;
}

AclTableTypeBuilder& AclTableTypeBuilder::withName(string name)
{
m_tableType.m_name = name;
Expand Down Expand Up @@ -1801,6 +1807,51 @@ AclTable::AclTable(AclOrch *pAclOrch) noexcept : m_pAclOrch(pAclOrch)

}

bool AclTable::addMandatoryActions()
{
SWSS_LOG_ENTER();

if (stage == ACL_STAGE_UNKNOWN)
{
return false;
}

if (!m_pAclOrch->isAclActionListMandatoryOnTableCreation(stage))
{
// No op if action list is not mandatory on table creation.
return true;
}

if (type.getName() == TABLE_TYPE_MIRROR || type.getName() == TABLE_TYPE_MIRRORV6 ||
type.getName() == TABLE_TYPE_MIRROR_DSCP )
{

sai_acl_action_type_t acl_action = SAI_ACL_ACTION_TYPE_COUNTER;
if (m_pAclOrch->isAclActionSupported(stage, acl_action))
{
SWSS_LOG_INFO("Add counter acl action");
type.addAction(acl_action);
}

if (stage == ACL_STAGE_INGRESS)
{
acl_action = SAI_ACL_ACTION_TYPE_MIRROR_INGRESS;
}
else
{
acl_action = SAI_ACL_ACTION_TYPE_MIRROR_EGRESS;
}

if (m_pAclOrch->isAclActionSupported(stage, acl_action))
{
SWSS_LOG_INFO("Add ingress/egress acl action");
type.addAction(acl_action);
}
}

return true;
}

bool AclTable::validateAddType(const AclTableType &tableType)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -3939,6 +3990,8 @@ void AclOrch::doAclTableTask(Consumer &consumer)

newTable.validateAddType(*tableType);

newTable.addMandatoryActions();

// validate and create/update ACL Table
if (bAllAttributesOk && newTable.validate())
{
Expand Down
5 changes: 5 additions & 0 deletions orchagent/aclorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ class AclTableType
const set<sai_acl_range_type_t>& getRangeTypes() const;
const set<sai_acl_action_type_t>& getActions() const;

bool addAction(sai_acl_action_type_t action);

private:
friend class AclTableTypeBuilder;

Expand Down Expand Up @@ -387,6 +389,9 @@ class AclTable
bool validate();
bool create();

// Add actions to ACL table if mandatory action list is required on table creation.
bool addMandatoryActions();

// validate AclRule match attribute against rule and table configuration
bool validateAclRuleMatch(sai_acl_entry_attr_t matchId, const AclRule& rule) const;
// validate AclRule action attribute against rule and table configuration
Expand Down
92 changes: 92 additions & 0 deletions tests/mock_tests/aclorch_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1743,4 +1743,96 @@ namespace aclorch_test
// try to delete non existing acl rule
ASSERT_TRUE(orch->m_aclOrch->removeAclRule(tableId, ruleId));
}

sai_switch_api_t *old_sai_switch_api;

// The following function is used to override SAI API get_switch_attribute to request passing
// mandatory ACL actions to SAI when creating mirror ACL table.
sai_status_t getSwitchAttribute(_In_ sai_object_id_t switch_id,_In_ uint32_t attr_count,
_Inout_ sai_attribute_t *attr_list)
{
if (attr_count == 1)
{
switch(attr_list[0].id)
{
case SAI_SWITCH_ATTR_MAX_ACL_ACTION_COUNT:
attr_list[0].value.u32 = 2;
return SAI_STATUS_SUCCESS;
case SAI_SWITCH_ATTR_ACL_STAGE_INGRESS:
case SAI_SWITCH_ATTR_ACL_STAGE_EGRESS:
attr_list[0].value.aclcapability.action_list.count = 2;
attr_list[0].value.aclcapability.action_list.list[0]= SAI_ACL_ACTION_TYPE_COUNTER;
attr_list[0].value.aclcapability.action_list.list[1]=
attr_list[0].id == SAI_SWITCH_ATTR_ACL_STAGE_INGRESS ?
SAI_ACL_ACTION_TYPE_MIRROR_INGRESS : SAI_ACL_ACTION_TYPE_MIRROR_EGRESS;
attr_list[0].value.aclcapability.is_action_list_mandatory = true;
return SAI_STATUS_SUCCESS;
}
}
return old_sai_switch_api->get_switch_attribute(switch_id, attr_count, attr_list);
}

TEST_F(AclOrchTest, AclTableCreationWithMandatoryActions)
{
// Override SAI API get_switch_attribute to request passing mandatory ACL actions to SAI
// when creating mirror ACL table.
old_sai_switch_api = sai_switch_api;
sai_switch_api_t new_sai_switch_api = *sai_switch_api;
sai_switch_api = &new_sai_switch_api;
sai_switch_api->get_switch_attribute = getSwitchAttribute;

// Set platform env to enable support of MIRRORV6 ACL table.
bool unset_platform_env = false;
if (!getenv("platform"))
{
setenv("platform", VS_PLATFORM_SUBSTRING, 0);
unset_platform_env = true;
}

auto orch = createAclOrch();

for (const auto &acl_table_type : { TABLE_TYPE_MIRROR, TABLE_TYPE_MIRRORV6, TABLE_TYPE_MIRROR_DSCP })
{
for (const auto &acl_table_stage : { STAGE_INGRESS, STAGE_EGRESS })
{
// Create ACL table.
string acl_table_id = "mirror_acl_table";
auto kvfAclTable = deque<KeyOpFieldsValuesTuple>(
{ { acl_table_id,
SET_COMMAND,
{ { ACL_TABLE_DESCRIPTION, acl_table_type },
{ ACL_TABLE_TYPE, acl_table_type },
{ ACL_TABLE_STAGE, acl_table_stage },
{ ACL_TABLE_PORTS, "1,2" } } } });
orch->doAclTableTask(kvfAclTable);
auto acl_table = orch->getAclTable(acl_table_id);
ASSERT_NE(acl_table, nullptr);

// Verify mandaotry ACL actions has been added.
auto acl_actions = acl_table->type.getActions();
ASSERT_NE(acl_actions.find(SAI_ACL_ACTION_TYPE_COUNTER), acl_actions.end());
sai_acl_action_type_t action = strcmp(acl_table_stage, STAGE_INGRESS) == 0 ?
SAI_ACL_ACTION_TYPE_MIRROR_INGRESS : SAI_ACL_ACTION_TYPE_MIRROR_EGRESS;
ASSERT_NE(acl_actions.find(action), acl_actions.end());

// Delete ACL table.
kvfAclTable = deque<KeyOpFieldsValuesTuple>(
{ { acl_table_id,
DEL_COMMAND,
{} } });
orch->doAclTableTask(kvfAclTable);
acl_table = orch->getAclTable(acl_table_id);
ASSERT_EQ(acl_table, nullptr);
}
}

// Unset platform env.
if (unset_platform_env)
{
unsetenv("platform");
}

// Restore sai_switch_api.
sai_switch_api = old_sai_switch_api;
}
} // namespace nsAclOrchTest