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.

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

[ACL] Write ACL table/rule creation status into STATE_DB #2662

Merged
merged 7 commits into from
Mar 8, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
99 changes: 99 additions & 0 deletions orchagent/aclorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,14 @@ static map<sai_acl_counter_attr_t, sai_acl_counter_attr_t> aclCounterLookup =
{SAI_ACL_COUNTER_ATTR_ENABLE_PACKET_COUNT, SAI_ACL_COUNTER_ATTR_PACKETS},
};

static map<AclObjectStatus, string> aclObjectStatusLookup =
{
{AclObjectStatus::ACTIVE, "Active"},
{AclObjectStatus::INACTIVE, "Inactive"},
{AclObjectStatus::PENDING_CREATION, "Pending creation"},
{AclObjectStatus::PENDING_REMOVAL, "Pending removal"}
};

static sai_acl_table_attr_t AclEntryFieldToAclTableField(sai_acl_entry_attr_t attr)
{
if (!IS_ATTR_ID_IN_RANGE(attr, ACL_ENTRY, FIELD))
Expand Down Expand Up @@ -3006,6 +3014,10 @@ void AclOrch::init(vector<TableConnector>& connectors, PortsOrch *portOrch, Mirr
{
SWSS_LOG_ENTER();

// Clear ACL_TABLE and ACL_RULE status from STATE_DB
removeAllAclTableStatus();
removeAllAclRuleStatus();

// TODO: Query SAI to get mirror table capabilities
// Right now, verified platforms that support mirroring IPv6 packets are
// Broadcom and Mellanox. Virtual switch is also supported for testing
Expand Down Expand Up @@ -3508,6 +3520,8 @@ AclOrch::AclOrch(vector<TableConnector>& connectors, DBConnector* stateDb, Switc
PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch, DTelOrch *dtelOrch) :
Orch(connectors),
m_aclStageCapabilityTable(stateDb, STATE_ACL_STAGE_CAPABILITY_TABLE_NAME),
m_aclTableStateTable(stateDb, STATE_ACL_TABLE_TABLE_NAME),
m_aclRuleStateTable(stateDb, STATE_ACL_RULE_TABLE_NAME),
m_switchOrch(switchOrch),
m_mirrorOrch(mirrorOrch),
m_neighOrch(neighOrch),
Expand Down Expand Up @@ -4331,36 +4345,54 @@ void AclOrch::doAclTableTask(Consumer &consumer)
{
SWSS_LOG_NOTICE("Successfully updated existing ACL table %s",
table_id.c_str());
setAclTableStatus(table_id, AclObjectStatus::ACTIVE);
it = consumer.m_toSync.erase(it);
}
else
{
SWSS_LOG_ERROR("Failed to update existing ACL table %s",
table_id.c_str());
// For now, updateAclTable always return true. So we should never reach here
setAclTableStatus(table_id, AclObjectStatus::INACTIVE);
it++;
}
}
else
{
if (addAclTable(newTable))
{
setAclTableStatus(table_id, AclObjectStatus::ACTIVE);
it = consumer.m_toSync.erase(it);
}
else
{
setAclTableStatus(table_id, AclObjectStatus::PENDING_CREATION);
it++;
}
}
}
else
{
it = consumer.m_toSync.erase(it);
// Mark the ACL table as inactive if the configuration is invalid
setAclTableStatus(table_id, AclObjectStatus::INACTIVE);
SWSS_LOG_ERROR("Failed to create ACL table %s, invalid configuration",
table_id.c_str());
}
}
else if (op == DEL_COMMAND)
{
if (removeAclTable(table_id))
{
removeAclTableStatus(table_id);
it = consumer.m_toSync.erase(it);
}
else
{
// Set the status of ACL_TABLE to pending removal if removeAclTable returns error
setAclTableStatus(table_id, AclObjectStatus::PENDING_REMOVAL);
it++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to handle here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added a Pending removal status for this case.

Copy link
Contributor Author

@bingwang-ms bingwang-ms Mar 1, 2023

Choose a reason for hiding this comment

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

Also I added a Pending removal status for the ACL rules that are pending to be removed.

}
}
else
{
Expand Down Expand Up @@ -4500,22 +4532,37 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
if (bAllAttributesOk && newRule->validate())
{
if (addAclRule(newRule, table_id))
{
setAclRuleStatus(table_id, rule_id, AclObjectStatus::ACTIVE);
it = consumer.m_toSync.erase(it);
}
else
{
setAclRuleStatus(table_id, rule_id, AclObjectStatus::PENDING_CREATION);
it++;
}
}
else
{
it = consumer.m_toSync.erase(it);
// Mark the rule inactive if the configuration is invalid
setAclRuleStatus(table_id, rule_id, AclObjectStatus::INACTIVE);
SWSS_LOG_ERROR("Failed to create ACL rule. Rule configuration is invalid");
}
}
else if (op == DEL_COMMAND)
{
if (removeAclRule(table_id, rule_id))
{
removeAclRuleStatus(table_id, rule_id);
it = consumer.m_toSync.erase(it);
}
else
{
// Mark pending removal status if removeAclRule returns error
setAclRuleStatus(table_id, rule_id, AclObjectStatus::PENDING_REMOVAL);
it++;
}
}
else
{
Expand Down Expand Up @@ -4873,3 +4920,55 @@ bool AclOrch::getAclBindPortId(Port &port, sai_object_id_t &port_id)

return true;
}

// Set the status of ACL table in STATE_DB
void AclOrch::setAclTableStatus(string table_name, AclObjectStatus status)
{
vector<FieldValueTuple> fvVector;
fvVector.emplace_back("status", aclObjectStatusLookup[status]);
m_aclTableStateTable.set(table_name, fvVector);
}

// Remove the status record of given ACL table from STATE_DB
void AclOrch::removeAclTableStatus(string table_name)
{
m_aclTableStateTable.del(table_name);
}

// Set the status of ACL rule in STATE_DB
void AclOrch::setAclRuleStatus(string table_name, string rule_name, AclObjectStatus status)
{
vector<FieldValueTuple> fvVector;
fvVector.emplace_back("status", aclObjectStatusLookup[status]);
m_aclRuleStateTable.set(table_name + string("|") + rule_name, fvVector);
}

// Remove the status record of given ACL rule from STATE_DB
void AclOrch::removeAclRuleStatus(string table_name, string rule_name)
{
m_aclRuleStateTable.del(table_name + string("|") + rule_name);
}

// Remove all ACL table status from STATE_DB
void AclOrch::removeAllAclTableStatus()
{
vector<string> keys;
m_aclTableStateTable.getKeys(keys);

for (auto key : keys)
{
m_aclTableStateTable.del(key);
}
}

// Remove all ACL rule status from STATE_DB
void AclOrch::removeAllAclRuleStatus()
{
vector<string> keys;
m_aclRuleStateTable.getKeys(keys);
for (auto key : keys)
{
m_aclRuleStateTable.del(key);
}
}

20 changes: 20 additions & 0 deletions orchagent/aclorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,14 @@

#define ACL_COUNTER_FLEX_COUNTER_GROUP "ACL_STAT_COUNTER"

enum AclObjectStatus
{
ACTIVE = 0,
INACTIVE,
PENDING_CREATION,
PENDING_REMOVAL
};

struct AclActionCapabilities
{
set<sai_acl_action_type_t> actionList;
Expand Down Expand Up @@ -553,6 +561,15 @@ class AclOrch : public Orch, public Observer

string generateAclRuleIdentifierInCountersDb(const AclRule& rule) const;

void setAclTableStatus(string table_name, AclObjectStatus status);
void setAclRuleStatus(string table_name, string rule_name, AclObjectStatus status);

void removeAclTableStatus(string table_name);
void removeAclRuleStatus(string table_name, string rule_name);

void removeAllAclTableStatus();
void removeAllAclRuleStatus();

map<sai_object_id_t, AclTable> m_AclTables;
// TODO: Move all ACL tables into one map: name -> instance
map<string, AclTable> m_ctrlAclTables;
Expand All @@ -563,6 +580,9 @@ class AclOrch : public Orch, public Observer

Table m_aclStageCapabilityTable;

Table m_aclTableStateTable;
Table m_aclRuleStateTable;

map<acl_stage_type_t, string> m_mirrorTableId;
map<acl_stage_type_t, string> m_mirrorV6TableId;

Expand Down
45 changes: 44 additions & 1 deletion tests/dvslib/dvs_acl.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""Utilities for interacting with ACLs when writing VS tests."""
from typing import Callable, Dict, List

from swsscommon import swsscommon

class DVSAcl:
"""Manage ACL tables and rules on the virtual switch."""
Expand All @@ -18,6 +18,9 @@ class DVSAcl:
ADB_ACL_GROUP_MEMBER_TABLE_NAME = "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE_GROUP_MEMBER"
ADB_ACL_COUNTER_TABLE_NAME = "ASIC_STATE:SAI_OBJECT_TYPE_ACL_COUNTER"

STATE_DB_ACL_TABLE_TABLE_NAME = "ACL_TABLE_TABLE"
STATE_DB_ACL_RULE_TABLE_NAME = "ACL_RULE_TABLE"

ADB_ACL_STAGE_LOOKUP = {
"ingress": "SAI_ACL_STAGE_INGRESS",
"egress": "SAI_ACL_STAGE_EGRESS"
Expand Down Expand Up @@ -740,3 +743,43 @@ def _check_acl_entry_counters_map(self, acl_entry_oid: str):
rule_to_counter_map = self.counters_db.get_entry("ACL_COUNTER_RULE_MAP", "")
counter_to_rule_map = {v: k for k, v in rule_to_counter_map.items()}
assert counter_oid in counter_to_rule_map

def verify_acl_table_status(
self,
acl_table_name,
expected_status
) -> None:
"""Verify that the STATE_DB status of ACL table is as expected.

Args:
acl_table_name: The name of ACL table to check
expected_status: The expected status in STATE_DB
"""
if expected_status:
fvs = self.state_db.wait_for_entry(self.STATE_DB_ACL_TABLE_TABLE_NAME, acl_table_name)
assert len(fvs) > 0
assert (fvs['status'] == expected_status)
else:
self.state_db.wait_for_deleted_entry(self.STATE_DB_ACL_TABLE_TABLE_NAME, acl_table_name)

def verify_acl_rule_status(
self,
acl_table_name,
acl_rule_name,
expected_status
) -> None:
"""Verify that the STATE_DB status of ACL rule is as expected.

Args:
acl_table_name: The name of ACL table to check
acl_rule_name: The name of ACL rule to check
expected_status: The expected status in STATE_DB
"""
key = acl_table_name + "|" + acl_rule_name
if expected_status:
fvs = self.state_db.wait_for_entry(self.STATE_DB_ACL_RULE_TABLE_NAME, key)
assert len(fvs) > 0
assert (fvs['status'] == expected_status)
else:
self.state_db.wait_for_deleted_entry(self.STATE_DB_ACL_TABLE_TABLE_NAME, key)

Loading