Skip to content

Commit

Permalink
[ACL] Support stage particular match fields (sonic-net#2341)
Browse files Browse the repository at this point in the history
What I did
This PR is to fix ACL table creation failure for certain types.
We saw PFCWD table failed to be created at EGRESS stage. The error logs are

Jun 21 07:00:03.409283 str2-7050cx3-acs-08 ERR syncd#syncd: [none] SAI_API_ACL:_brcm_sai_create_acl_table:11205 field group config create failed with error Feature unavailable (0xfffffff0).
Jun 21 07:00:03.409738 str2-7050cx3-acs-08 ERR syncd#syncd: [none] SAI_API_ACL:brcm_sai_create_acl_table:298 create table entry failed with error -2.
Jun 21 07:00:03.409738 str2-7050cx3-acs-08 ERR syncd#syncd: :- sendApiResponse: api SAI_COMMON_API_CREATE failed in syncd mode: SAI_STATUS_NOT_SUPPORTED
Jun 21 07:00:03.409780 str2-7050cx3-acs-08 ERR syncd#syncd: :- processQuadEvent: attr: SAI_ACL_TABLE_ATTR_ACL_BIND_POINT_TYPE_LIST: 1:SAI_ACL_BIND_POINT_TYPE_PORT
Jun 21 07:00:03.409820 str2-7050cx3-acs-08 ERR syncd#syncd: :- processQuadEvent: attr: SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS: true
Jun 21 07:00:03.409820 str2-7050cx3-acs-08 ERR syncd#syncd: :- processQuadEvent: attr: SAI_ACL_TABLE_ATTR_FIELD_TC: true
Jun 21 07:00:03.410144 str2-7050cx3-acs-08 ERR syncd#syncd: :- processQuadEvent: attr: SAI_ACL_TABLE_ATTR_ACL_ACTION_TYPE_LIST: 2:SAI_ACL_ACTION_TYPE_PACKET_ACTION,SAI_ACL_ACTION_TYPE_COUNTER
Jun 21 07:00:03.410144 str2-7050cx3-acs-08 ERR syncd#syncd: :- processQuadEvent: attr: SAI_ACL_TABLE_ATTR_ACL_STAGE: SAI_ACL_STAGE_EGRESS
Jun 21 07:00:03.410144 str2-7050cx3-acs-08 ERR swss#orchagent: :- create: create status: SAI_STATUS_NOT_SUPPORTED
Jun 21 07:00:03.410144 str2-7050cx3-acs-08 ERR swss#orchagent: :- addAclTable: Failed to create ACL table pfcwd_egress
The root cause for the issue is SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS is not supported at EGRESS stage.

This PR addressed the issue by adding match field according to the stage.
For ACL type TABLE_TYPE_PFCWD and TABLE_TYPE_DROP at INGRESS stage, the match field SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS is added, while for EGRESS the field is not added.

Why I did it
To fix ACL table creation issue.

How I verified it

Verified by vstest
test_acl.py::TestAcl::test_AclTableMandatoryMatchFields[ingress] PASSED                                                                                                                         [ 87%]
test_acl.py::TestAcl::test_AclTableMandatoryMatchFields[egress] PASSED                                                                                                                          [ 90%]
Verified by building a new image and run on a TD3 device.

Signed-off-by: bingwang <wang.bing@microsoft.com>
  • Loading branch information
bingwang-ms committed Jun 22, 2022
1 parent efb4530 commit 6e0fc85
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 4 deletions.
70 changes: 68 additions & 2 deletions orchagent/aclorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,38 @@ static acl_table_action_list_lookup_t defaultAclActionList =
}
};

// The match fields for certain ACL table type are not exactly the same between INGRESS and EGRESS.
// For example, we can only match IN_PORT for PFCWD table type at INGRESS.
// Hence we need to specify stage particular matching fields in stageMandatoryMatchFields
static acl_table_match_field_lookup_t stageMandatoryMatchFields =
{
{
// TABLE_TYPE_PFCWD
TABLE_TYPE_PFCWD,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS
}
}
}
},
{
// TABLE_TYPE_DROP
TABLE_TYPE_DROP,
{
{
ACL_STAGE_INGRESS,
{
SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS
}
}
}
}

};

static acl_ip_type_lookup_t aclIpTypeLookup =
{
{ IP_TYPE_ANY, SAI_ACL_IP_TYPE_ANY },
Expand Down Expand Up @@ -477,6 +509,12 @@ bool AclTableType::addAction(sai_acl_action_type_t action)
return true;
}

bool AclTableType::addMatch(shared_ptr<AclTableMatchInterface> match)
{
m_matches.emplace(match->getId(), match);
return true;
}

AclTableTypeBuilder& AclTableTypeBuilder::withName(string name)
{
m_tableType.m_name = name;
Expand Down Expand Up @@ -2020,6 +2058,34 @@ bool AclTable::addMandatoryActions()
return true;
}

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

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

if (stageMandatoryMatchFields.count(type.getName()) != 0)
{
auto &fields_for_stage = stageMandatoryMatchFields[type.getName()];
if (fields_for_stage.count(stage) != 0)
{
// Add the stage particular matching fields
for (auto match : fields_for_stage[stage])
{
type.addMatch(make_shared<AclTableMatch>(match));
SWSS_LOG_INFO("Added mandatory match field %s for table type %s stage %d",
sai_serialize_enum(match, &sai_metadata_enum_sai_acl_table_attr_t).c_str(),
type.getName().c_str(), stage);
}
}
}

return true;
}

bool AclTable::validateAddType(const AclTableType &tableType)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -2983,15 +3049,13 @@ void AclOrch::initDefaultTableTypes()
builder.withName(TABLE_TYPE_PFCWD)
.withBindPointType(SAI_ACL_BIND_POINT_TYPE_PORT)
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_TC))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS))
.build()
);

addAclTableType(
builder.withName(TABLE_TYPE_DROP)
.withBindPointType(SAI_ACL_BIND_POINT_TYPE_PORT)
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_TC))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS))
.build()
);

Expand Down Expand Up @@ -4108,6 +4172,8 @@ void AclOrch::doAclTableTask(Consumer &consumer)

newTable.validateAddType(*tableType);

newTable.addStageMandatoryMatchFields();

newTable.addMandatoryActions();

// validate and create/update ACL Table
Expand Down
7 changes: 7 additions & 0 deletions orchagent/aclorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ typedef map<sai_acl_action_type_t, set<int32_t>> acl_action_enum_values_capabili
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;

typedef map<acl_stage_type_t, set<sai_acl_table_attr_t> > acl_stage_match_field_t;
typedef map<string, acl_stage_match_field_t> acl_table_match_field_lookup_t;

class AclRule;

class AclTableMatchInterface
Expand Down Expand Up @@ -160,6 +163,7 @@ class AclTableType
const set<sai_acl_action_type_t>& getActions() const;

bool addAction(sai_acl_action_type_t action);
bool addMatch(shared_ptr<AclTableMatchInterface> match);

private:
friend class AclTableTypeBuilder;
Expand Down Expand Up @@ -384,6 +388,9 @@ class AclTable
// Add actions to ACL table if mandatory action list is required on table creation.
bool addMandatoryActions();

// Add stage mandatory matching fields to ACL table
bool addStageMandatoryMatchFields();

// 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
31 changes: 29 additions & 2 deletions tests/test_acl.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import pytest
from requests import request

L3_TABLE_TYPE = "L3"
L3_TABLE_NAME = "L3_TEST"
Expand All @@ -20,6 +21,9 @@
MIRROR_BIND_PORTS = ["Ethernet0", "Ethernet4", "Ethernet8", "Ethernet12"]
MIRROR_RULE_NAME = "MIRROR_TEST_RULE"

PFCWD_TABLE_TYPE = "PFCWD"
PFCWD_TABLE_NAME = "PFCWD_TEST"
PFCWD_BIND_PORTS = ["Ethernet0", "Ethernet4", "Ethernet8", "Ethernet12"]
class TestAcl:
@pytest.yield_fixture
def l3_acl_table(self, dvs_acl):
Expand Down Expand Up @@ -59,6 +63,15 @@ def mirror_acl_table(self, dvs_acl):
dvs_acl.remove_acl_table(MIRROR_TABLE_NAME)
dvs_acl.verify_acl_table_count(0)

@pytest.fixture(params=['ingress', 'egress'])
def pfcwd_acl_table(self, dvs_acl, request):
try:
dvs_acl.create_acl_table(PFCWD_TABLE_NAME, PFCWD_TABLE_TYPE, PFCWD_BIND_PORTS, request.param)
yield dvs_acl.get_acl_table_ids(1)[0], request.param
finally:
dvs_acl.remove_acl_table(PFCWD_TABLE_NAME)
dvs_acl.verify_acl_table_count(0)

@pytest.yield_fixture
def setup_teardown_neighbor(self, dvs):
try:
Expand Down Expand Up @@ -548,8 +561,22 @@ def test_AclRuleRedirect(self, dvs, dvs_acl, l3_acl_table, setup_teardown_neighb

dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME)
dvs_acl.verify_no_acl_rules()



def test_AclTableMandatoryMatchFields(self, dvs, pfcwd_acl_table):
"""
The test case is to verify stage particular matching fields is applied
"""
table_oid, stage = pfcwd_acl_table
match_in_ports = False
entry = dvs.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE", table_oid)
for k, v in entry.items():
if k == "SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS" and v == "true":
match_in_ports = True

if stage == "ingress":
assert match_in_ports
else:
assert not match_in_ports
class TestAclCrmUtilization:
@pytest.fixture(scope="class", autouse=True)
def configure_crm_polling_interval_for_test(self, dvs):
Expand Down

0 comments on commit 6e0fc85

Please sign in to comment.