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

Combine v4 and v6 L3 ACL rules on optimized platforms #1267 #2735

Merged
merged 2 commits into from
May 25, 2023
Merged

Combine v4 and v6 L3 ACL rules on optimized platforms #1267 #2735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4062e8
Combine v4 and v6 L3 ACL rules on optimized platforms #1267
rck-innovium Apr 14, 2023
456bbfe
Address review comments
rck-innovium May 3, 2023
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
124 changes: 124 additions & 0 deletions orchagent/aclorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ extern CrmOrch *gCrmOrch;

#define STATE_DB_ACL_ACTION_FIELD_IS_ACTION_LIST_MANDATORY "is_action_list_mandatory"
#define STATE_DB_ACL_ACTION_FIELD_ACTION_LIST "action_list"
#define STATE_DB_ACL_L3V4V6_SUPPORTED "supported_L3V4V6"
#define COUNTERS_ACL_COUNTER_RULE_MAP "ACL_COUNTER_RULE_MAP"

#define ACL_COUNTER_DEFAULT_POLLING_INTERVAL_MS 10000 // ms
Expand Down Expand Up @@ -202,6 +203,26 @@ static acl_table_action_list_lookup_t defaultAclActionList =
}
}
},
{
// L3V4V6
TABLE_TYPE_L3V4V6,
{
{
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,
Expand Down Expand Up @@ -2292,6 +2313,19 @@ bool AclTable::validate()
return false;
}

if (type.getName() == TABLE_TYPE_L3V4V6)
{
if (!m_pAclOrch->isAclL3V4V6TableSupported(stage))
{

SWSS_LOG_ERROR("Table %s: table type %s in stage %d not supported on this platform.",
id.c_str(), type.getName().c_str(), stage);
return false;
}
}



if (m_pAclOrch->isAclActionListMandatoryOnTableCreation(stage))
{
if (type.getActions().empty())
Expand Down Expand Up @@ -3049,11 +3083,36 @@ void AclOrch::init(vector<TableConnector>& connectors, PortsOrch *portOrch, Mirr
};
}

if ( platform == MRVL_PLATFORM_SUBSTRING ||
platform == INVM_PLATFORM_SUBSTRING ||
platform == VS_PLATFORM_SUBSTRING)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change VS platform to combined? Does it impact any current acl tests? @bingwang-ms

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 change to VS makes no difference. AFAIK, We don't run acl test on VS. But even though, I think it doesn't make sense to change the behavior of VS. I suggest removing the change to VS_PLATFORM. @rck-innovium

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 

After removing the VS platform, I ran the tests that create ACL tables of the new L3V4V6 type and saw that the ACL table creation fails. 

marvell@cpss-testvm2:~/rck/sonic-dbg/sonic-buildimage/src/sonic-swss/tests$ sudo pytest -v  test_acl_l3v4v6.py
[sudo] password for marvell:
============================================================================================================================ test session starts =============================================================================================================================
:
:
test_acl_l3v4v6.py::TestAcl::test_L3V4V6AclTableCreationDeletion FAILED                                                                                                                                                                                                [ 20%]
:
:
=================================================================================================================================== ERRORS ===================================================================================================================================
________________________________________________________________________________________________________ ERROR at setup of TestAcl.test_ValidAclRuleCreation_sip_dip _________________________________________________________________________________________________________

self = <test_acl_l3v4v6.TestAcl object at 0x7f8141afba60>, dvs_acl = <dvslib.dvs_acl.DVSAcl object at 0x7f8141a564c0>

    @pytest.fixture
    def l3v4v6_acl_table(self, dvs_acl):
        try:
            dvs_acl.create_acl_table(L3V4V6_TABLE_NAME,
                                     L3V4V6_TABLE_TYPE,
                                     L3V4V6_BIND_PORTS)
>           yield dvs_acl.get_acl_table_ids(1)[0]

test_acl_l3v4v6.py:16:

The reason for the error is that the VS platform does NOT support the new ACL table type L3V4V6.

May 13 14:49:13.301367 e01c265b5ca5 ERR #orchagent: :- validate: Table L3_V4V6_TEST: table type L3V4V6 in stage 1 not supported on this platform.
May 13 14:49:13.301645 e01c265b5ca5 ERR #orchagent: :- doAclTableTask: Failed to create ACL table L3_V4V6_TEST, invalid configuration

@prsunny 

This change does not affect any other existing ACL  features since this capability check is used only for the newly introduced ACL table type L3V4V6. Also, I verified that all the current swss ACL tests pass as well as PTF test_acl.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rck-innovium Probably we can mock the platform by setting DVS_FAKE_PLATFORM. Please search in existing test code.
Thanks

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 

I was not able to find DVS_FAKE_PLATFORM in existing swss test code

marvell@cpss-testvm2:/rck/sonic-dbg/sonic-buildimage/src/sonic-swss$ grep  -ir DVS_FAKE_PLATFORM *
marvell@cpss-testvm2:/rck/sonic-dbg/sonic-buildimage/src/sonic-swss$ grep  -r FAKE_PLATFORM *
marvell@cpss-testvm2:/rck/sonic-dbg/sonic-buildimage/src/sonic-swss$ cd ..
marvell@cpss-testvm2:/rck/sonic-dbg/sonic-buildimage/src$ grep  -r DVS_FAKE_PLATFORM *
marvell@cpss-testvm2:~/rck/sonic-dbg/sonic-buildimage/src$

Copy link
Contributor

Choose a reason for hiding this comment

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

@rck-innovium Sorry for the misleading. I just realize that the DVS_FAKE_PLATFORM has been removed from the latest testing code. I don't know if we have other method to mock the platform string. @prsunny Any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

E.g:

DVS_FAKE_PLATFORM = "marvell"
fakeplatform = getattr(request.module, "DVS_FAKE_PLATFORM", None)
dvs = DockerVirtualSwitch(name, keeptb, fakeplatform)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsunny @bingwang-ms

DVS_FAKE_PLATFORM has been removed from the entire SONiC codebase and in SWSS it has been replaced with DVS_ENV.

I digged through the past versions of SONiC to find that Nvidia has changed DVS_FAKE_PLATFORM to DVS_ENV = ["HWSKU=Mellanox-SN2700"]. The reason I presume is to support mimicking at a deeper HwSKU level instead of just the platform.

However, I see in today’s swss/tests, only Mellanox-SN2700 SKU is being faked. In fact, I ran into several issues when faking Marvell and Innovium platforms.
I have raised PR #2785 to track the issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsunny
Coming back to the original question, the new checks do NOT affect any other feature since this capability check is only used by the new L3V4V6 ACL table feature.
So adding VS platform does not have any impact on any of the existing feature.

{
m_L3V4V6Capability =
{
{ACL_STAGE_INGRESS, true},
{ACL_STAGE_EGRESS, true},
};
}
else
{
m_L3V4V6Capability =
{
{ACL_STAGE_INGRESS, false},
{ACL_STAGE_EGRESS, false},
};

}


SWSS_LOG_NOTICE("%s switch capability:", platform.c_str());
SWSS_LOG_NOTICE(" TABLE_TYPE_MIRROR: %s",
m_mirrorTableCapabilities[TABLE_TYPE_MIRROR] ? "yes" : "no");
SWSS_LOG_NOTICE(" TABLE_TYPE_MIRRORV6: %s",
m_mirrorTableCapabilities[TABLE_TYPE_MIRRORV6] ? "yes" : "no");
SWSS_LOG_NOTICE(" TABLE_TYPE_L3V4V6: Ingress [%s], Egress [%s]",
m_L3V4V6Capability[ACL_STAGE_INGRESS] ? "yes" : "no",
m_L3V4V6Capability[ACL_STAGE_EGRESS] ? "yes" : "no");


// In Mellanox platform, V4 and V6 rules are stored in different tables
// In Broadcom DNX platform also, V4 and V6 rules are stored in different tables
Expand Down Expand Up @@ -3168,6 +3227,31 @@ void AclOrch::initDefaultTableTypes()
.build()
);


addAclTableType(
builder.withName(TABLE_TYPE_L3V4V6)
.withBindPointType(SAI_ACL_BIND_POINT_TYPE_PORT)
.withBindPointType(SAI_ACL_BIND_POINT_TYPE_LAG)
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ETHER_TYPE))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_OUTER_VLAN_ID))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ACL_IP_TYPE))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_SRC_IP))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_DST_IP))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ICMP_TYPE))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ICMP_CODE))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_IP_PROTOCOL))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_SRC_IPV6))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_DST_IPV6))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ICMPV6_CODE))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_ICMPV6_TYPE))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_IPV6_NEXT_HEADER))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_L4_SRC_PORT))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_L4_DST_PORT))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_TCP_FLAGS))
.withMatch(make_shared<AclTableMatch>(SAI_ACL_TABLE_ATTR_FIELD_IN_PORTS))
.build()
);
rck-innovium marked this conversation as resolved.
Show resolved Hide resolved

addAclTableType(
builder.withName(TABLE_TYPE_MCLAG)
.withBindPointType(SAI_ACL_BIND_POINT_TYPE_PORT)
Expand Down Expand Up @@ -3408,10 +3492,21 @@ void AclOrch::putAclActionCapabilityInDB(acl_stage_type_t stage)
}
}


is_action_list_mandatory_stream << boolalpha << capabilities.isActionListMandatoryOnTableCreation;

fvVector.emplace_back(STATE_DB_ACL_ACTION_FIELD_IS_ACTION_LIST_MANDATORY, is_action_list_mandatory_stream.str());
fvVector.emplace_back(STATE_DB_ACL_ACTION_FIELD_ACTION_LIST, acl_action_value_stream.str());

for (auto const& it : m_L3V4V6Capability)
{
string value = it.second ? "true" : "false";
if (it.first == stage)
{
fvVector.emplace_back(STATE_DB_ACL_L3V4V6_SUPPORTED, value);
}
}

m_aclStageCapabilityTable.set(stage_str, fvVector);
}

Expand Down Expand Up @@ -4219,6 +4314,16 @@ bool AclOrch::isAclActionListMandatoryOnTableCreation(acl_stage_type_t stage) co
return it->second.isActionListMandatoryOnTableCreation;
}

bool AclOrch::isAclL3V4V6TableSupported(acl_stage_type_t stage) const
{
const auto& it = m_L3V4V6Capability.find(stage);
if (it == m_L3V4V6Capability.cend())
{
return false;
}
return it->second;
}

bool AclOrch::isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t action) const
{
const auto& it = m_aclCapabilities.find(stage);
Expand Down Expand Up @@ -4472,6 +4577,8 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
}
bool bHasTCPFlag = false;
bool bHasIPProtocol = false;
bool bHasIPV4 = false;
bool bHasIPV6 = false;
for (const auto& itr : kfvFieldsValues(t))
{
string attr_name = to_upper(fvField(itr));
Expand All @@ -4482,6 +4589,14 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
{
bHasTCPFlag = true;
}
if (attr_name == MATCH_SRC_IP || attr_name == MATCH_DST_IP)
{
bHasIPV4 = true;
}
if (attr_name == MATCH_SRC_IPV6 || attr_name == MATCH_DST_IPV6)
{
bHasIPV6 = true;
}
if (attr_name == MATCH_IP_PROTOCOL || attr_name == MATCH_NEXT_HEADER)
{
bHasIPProtocol = true;
Expand Down Expand Up @@ -4530,6 +4645,15 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
}
}

if (bHasIPV4 && bHasIPV6)
{
if (type == TABLE_TYPE_L3V4V6)
{
SWSS_LOG_ERROR("Rule '%s' is invalid since it has both v4 and v6 matchfields.", rule_id.c_str());
bAllAttributesOk = false;
}
}

// validate and create ACL rule
if (bAllAttributesOk && newRule->validate())
{
Expand Down
2 changes: 2 additions & 0 deletions orchagent/aclorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,12 +502,14 @@ class AclOrch : public Orch, public Observer
bool isAclMirrorV6Supported() const;
bool isAclMirrorV4Supported() const;
bool isAclMirrorTableSupported(string type) const;
bool isAclL3V4V6TableSupported(acl_stage_type_t stage) const;
bool isAclActionListMandatoryOnTableCreation(acl_stage_type_t stage) const;
bool isAclActionSupported(acl_stage_type_t stage, sai_acl_action_type_t action) const;
bool isAclActionEnumValueSupported(sai_acl_action_type_t action, sai_acl_action_parameter_t param) const;

bool m_isCombinedMirrorV6Table = true;
map<string, bool> m_mirrorTableCapabilities;
map<acl_stage_type_t, bool> m_L3V4V6Capability;

void registerFlexCounter(const AclRule& rule);
void deregisterFlexCounter(const AclRule& rule);
Expand Down
1 change: 1 addition & 0 deletions orchagent/acltable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ extern "C" {

#define TABLE_TYPE_L3 "L3"
#define TABLE_TYPE_L3V6 "L3V6"
#define TABLE_TYPE_L3V4V6 "L3V4V6"
#define TABLE_TYPE_MIRROR "MIRROR"
#define TABLE_TYPE_MIRRORV6 "MIRRORV6"
#define TABLE_TYPE_MIRROR_DSCP "MIRROR_DSCP"
Expand Down
99 changes: 99 additions & 0 deletions tests/test_acl_l3v4v6.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import pytest
from requests import request

L3V4V6_TABLE_TYPE = "L3V4V6"
L3V4V6_TABLE_NAME = "L3_V4V6_TEST"
L3V4V6_BIND_PORTS = ["Ethernet0", "Ethernet4", "Ethernet8"]
L3V4V6_RULE_NAME = "L3V4V6_TEST_RULE"

class TestAcl:
@pytest.fixture
def l3v4v6_acl_table(self, dvs_acl):
try:
dvs_acl.create_acl_table(L3V4V6_TABLE_NAME,
L3V4V6_TABLE_TYPE,
L3V4V6_BIND_PORTS)
yield dvs_acl.get_acl_table_ids(1)[0]
finally:
dvs_acl.remove_acl_table(L3V4V6_TABLE_NAME)
dvs_acl.verify_acl_table_count(0)

@pytest.fixture
def setup_teardown_neighbor(self, dvs):
try:
# NOTE: set_interface_status has a dependency on cdb within dvs,
# so we still need to setup the db. This should be refactored.
dvs.setup_db()

# Bring up an IP interface with a neighbor
dvs.set_interface_status("Ethernet4", "up")
dvs.add_ip_address("Ethernet4", "10.0.0.1/24")
dvs.add_neighbor("Ethernet4", "10.0.0.2", "00:01:02:03:04:05")

yield dvs.get_asic_db().wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP", 1)[0]
finally:
# Clean up the IP interface and neighbor
dvs.remove_neighbor("Ethernet4", "10.0.0.2")
dvs.remove_ip_address("Ethernet4", "10.0.0.1/24")
dvs.set_interface_status("Ethernet4", "down")

def test_L3V4V6AclTableCreationDeletion(self, dvs_acl):
try:
dvs_acl.create_acl_table(L3V4V6_TABLE_NAME, L3V4V6_TABLE_TYPE, L3V4V6_BIND_PORTS)

acl_table_id = dvs_acl.get_acl_table_ids(1)[0]
acl_table_group_ids = dvs_acl.get_acl_table_group_ids(len(L3V4V6_BIND_PORTS))

dvs_acl.verify_acl_table_group_members(acl_table_id, acl_table_group_ids, 1)
dvs_acl.verify_acl_table_port_binding(acl_table_id, L3V4V6_BIND_PORTS, 1)
# Verify status is written into STATE_DB
dvs_acl.verify_acl_table_status(L3V4V6_TABLE_NAME, "Active")
finally:
dvs_acl.remove_acl_table(L3V4V6_TABLE_NAME)
dvs_acl.verify_acl_table_count(0)
# Verify the STATE_DB entry is removed
dvs_acl.verify_acl_table_status(L3V4V6_TABLE_NAME, None)

def test_ValidAclRuleCreation_sip_dip(self, dvs_acl, l3v4v6_acl_table):
config_qualifiers = {"DST_IP": "20.0.0.1/32",
"SRC_IP": "10.0.0.0/32"};

dvs_acl.create_acl_rule(L3V4V6_TABLE_NAME, "VALID_RULE", config_qualifiers)
# Verify status is written into STATE_DB
dvs_acl.verify_acl_rule_status(L3V4V6_TABLE_NAME, "VALID_RULE", "Active")

dvs_acl.remove_acl_rule(L3V4V6_TABLE_NAME, "VALID_RULE")
# Verify the STATE_DB entry is removed
dvs_acl.verify_acl_rule_status(L3V4V6_TABLE_NAME, "VALID_RULE", None)
dvs_acl.verify_no_acl_rules()

def test_InvalidAclRuleCreation_sip_sipv6(self, dvs_acl, l3v4v6_acl_table):
config_qualifiers = {"SRC_IPV6": "2777::0/64",
"SRC_IP": "10.0.0.0/32"};

dvs_acl.create_acl_rule(L3V4V6_TABLE_NAME, "INVALID_RULE", config_qualifiers)
# Verify status is written into STATE_DB
dvs_acl.verify_acl_rule_status(L3V4V6_TABLE_NAME, "INVALID_RULE", "Inactive")

dvs_acl.remove_acl_rule(L3V4V6_TABLE_NAME, "INVALID_RULE")
# Verify the STATE_DB entry is removed
dvs_acl.verify_acl_rule_status(L3V4V6_TABLE_NAME, "INVALID_RULE", None)
dvs_acl.verify_no_acl_rules()

def test_InvalidAclRuleCreation_dip_sipv6(self, dvs_acl, l3v4v6_acl_table):
config_qualifiers = {"SRC_IPV6": "2777::0/64",
"DST_IP": "10.0.0.0/32"};

dvs_acl.create_acl_rule(L3V4V6_TABLE_NAME, "INVALID_RULE", config_qualifiers)
# Verify status is written into STATE_DB
dvs_acl.verify_acl_rule_status(L3V4V6_TABLE_NAME, "INVALID_RULE", "Inactive")

dvs_acl.remove_acl_rule(L3V4V6_TABLE_NAME, "INVALID_RULE")
# Verify the STATE_DB entry is removed
dvs_acl.verify_acl_rule_status(L3V4V6_TABLE_NAME, "INVALID_RULE", None)
dvs_acl.verify_no_acl_rules()

# Add Dummy always-pass test at end as workaroud
# for issue when Flaky fail on final test it invokes module tear-down before retrying
def test_nonflaky_dummy():
pass