Skip to content

Commit

Permalink
[ACL] Add default action_list for default ACL table type (sonic-net#2298
Browse files Browse the repository at this point in the history 
)

What I did
This PR is derived from sonic-net#2205
Fix sonic-net/sonic-buildimage#10425

We were seeing ACL table creation failure on some platform because action_list is mandatory, while the action_list is not provided by aclorch.

Apr  1 01:24:11.702608 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table DATAACL is mandatory
Apr  1 01:24:11.702608 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table DATAACL, invalid configuration
Apr  1 01:24:11.702741 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table EVERFLOW is mandatory
Apr  1 01:24:11.702741 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table EVERFLOW, invalid configuration
Apr  1 01:24:11.702926 str2-7050cx3-acs-03 ERR swss#orchagent: :- validate: Action list for table EVERFLOWV6 is mandatory
Apr  1 01:24:11.702926 str2-7050cx3-acs-03 ERR swss#orchagent: :- doAclTableTask: Failed to create ACL table EVERFLOWV6, invalid configuration
This PR fixed the issue by adding default action_list to the default ACL table type if not present.

Why I did it
Fix the ACL table creation issue.

How I verified it
Verified by running test_acl and test_everflow on Broadcom TD3 platform

Signed-off-by: bingwang <wang.bing@microsoft.com>
Co-authored-by: syuan <syuan@arista.com>
  • Loading branch information
@bingwang-ms @ysmanman @preetham-singh
2 people authored and preetham-singh committed Aug 6, 2022
1 parent 2c1b99f commit d2a25ab
Show file tree
Hide file tree
Showing 3 changed files with 323 additions and 0 deletions.
223 changes: 223 additions & 0 deletions orchagent/aclorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,176 @@ static const acl_capabilities_t defaultAclActionsSupported =
}
};

static acl_table_action_list_lookup_t defaultAclActionList =
{
{
// L3
TABLE_TYPE_L3,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION,
SAI_ACL_ACTION_TYPE_REDIRECT
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION,
SAI_ACL_ACTION_TYPE_REDIRECT
}
}
}
},
{
// L3V6
TABLE_TYPE_L3V6,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION,
SAI_ACL_ACTION_TYPE_REDIRECT
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION,
SAI_ACL_ACTION_TYPE_REDIRECT
}
}
}
},
{
// MIRROR
TABLE_TYPE_MIRROR,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_MIRROR_INGRESS
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_MIRROR_EGRESS
}
}
}
},
{
// MIRRORV6
TABLE_TYPE_MIRRORV6,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_MIRROR_INGRESS
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_MIRROR_EGRESS
}
}
}
},
{
// MIRROR_DSCP
TABLE_TYPE_MIRROR_DSCP,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_MIRROR_INGRESS
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_MIRROR_EGRESS
}
}
}
},
{
// TABLE_TYPE_PFCWD
TABLE_TYPE_PFCWD,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION
}
}
}
},
{
// MCLAG
TABLE_TYPE_MCLAG,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION
}
}
}
},
{
// MUX
TABLE_TYPE_MUX,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION
}
}
}
},
{
// DROP
TABLE_TYPE_DROP,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION
}
},
{
ACL_STAGE_EGRESS,
{
SAI_ACL_ACTION_TYPE_PACKET_ACTION
}
}
}
}
};

static acl_ip_type_lookup_t aclIpTypeLookup =
{
{ IP_TYPE_ANY, SAI_ACL_IP_TYPE_ANY },
Expand Down Expand Up @@ -301,6 +471,12 @@ const set<sai_acl_action_type_t>& AclTableType::getActions() const
return m_aclAcitons;
}

bool AclTableType::addAction(sai_acl_action_type_t action)
{
m_aclAcitons.insert(action);
return true;
}

AclTableTypeBuilder& AclTableTypeBuilder::withName(string name)
{
m_tableType.m_name = name;
Expand Down Expand Up @@ -1808,6 +1984,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.getActions().empty())
{
// No change if action_list is provided
return true;
}

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 (defaultAclActionList.count(type.getName()) != 0)
{
// Add the default action list
for (auto action : defaultAclActionList[type.getName()][stage])
{
if (m_pAclOrch->isAclActionSupported(stage, acl_action))
{
SWSS_LOG_INFO("Added default action for table type %s stage %s",
type.getName().c_str(),
((stage == ACL_STAGE_INGRESS)? "INGRESS":"EGRESS"));
type.addAction(action);
}
}
}

return true;
}

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

newTable.validateAddType(*tableType);

newTable.addMandatoryActions();

// validate and create/update ACL Table
if (bAllAttributesOk && newTable.validate())
{
Expand Down
8 changes: 8 additions & 0 deletions orchagent/aclorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ typedef tuple<sai_acl_range_type_t, int, int> acl_range_properties_t;
typedef map<acl_stage_type_t, AclActionCapabilities> acl_capabilities_t;
typedef map<sai_acl_action_type_t, set<int32_t>> acl_action_enum_values_capabilities_t;

typedef map<acl_stage_type_t, set<sai_acl_action_type_t> > acl_stage_action_list_t;
typedef map<string, acl_stage_action_list_t> acl_table_action_list_lookup_t;

class AclRule;

class AclTableMatchInterface
Expand Down Expand Up @@ -156,6 +159,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 +392,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
Original file line number Diff line number Diff line change
Expand Up @@ -1755,4 +1755,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

0 comments on commit d2a25ab

Please sign in to comment.