From 7657ab1d7cfe509a9a31b8ce15693a4c8acdb41a Mon Sep 17 00:00:00 2001 From: bingwang Date: Sat, 4 Feb 2023 00:48:20 +0000 Subject: [PATCH 1/7] Add status for ACL_TABLE and ACL_RULE in STATE_DB --- orchagent/aclorch.cpp | 65 +++++++++++++++++++++++++++++++++++++++++++ orchagent/aclorch.h | 9 ++++++ 2 files changed, 74 insertions(+) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 5be81efd79..52d95af2cc 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -3508,6 +3508,8 @@ AclOrch::AclOrch(vector& 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), @@ -4331,21 +4333,30 @@ void AclOrch::doAclTableTask(Consumer &consumer) { SWSS_LOG_NOTICE("Successfully updated existing ACL table %s", table_id.c_str()); + setAclTableStatus(table_id, true); 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, false); it++; } } else { if (addAclTable(newTable)) + { + setAclTableStatus(table_id, true); it = consumer.m_toSync.erase(it); + } else + { + setAclTableStatus(table_id, false); it++; + } } } else @@ -4358,7 +4369,10 @@ void AclOrch::doAclTableTask(Consumer &consumer) else if (op == DEL_COMMAND) { if (removeAclTable(table_id)) + { + removeAclTableStatus(table_id); it = consumer.m_toSync.erase(it); + } else it++; } @@ -4500,9 +4514,15 @@ void AclOrch::doAclRuleTask(Consumer &consumer) if (bAllAttributesOk && newRule->validate()) { if (addAclRule(newRule, table_id)) + { + setAclRuleStatus(rule_id, true); it = consumer.m_toSync.erase(it); + } else + { + setAclRuleStatus(rule_id, false); it++; + } } else { @@ -4513,7 +4533,10 @@ void AclOrch::doAclRuleTask(Consumer &consumer) else if (op == DEL_COMMAND) { if (removeAclRule(table_id, rule_id)) + { + removeAclRuleStatus(rule_id); it = consumer.m_toSync.erase(it); + } else it++; } @@ -4873,3 +4896,45 @@ 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, bool active) +{ + vector fvVector; + if (active) + { + fvVector.emplace_back("Status", "Active"); + } + else + { + fvVector.emplace_back("Status", "Inactive"); + } + 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 rule_name, book active) +{ + vector fvVector; + if (active) + { + fvVector.emplace_back("Status", "Active"); + } + else + { + fvVector.emplace_back("Status", "Inactive"); + } + m_aclRuleStateTable.set(rule_name, fvVector); +} + +// Remove the status record of given ACL rule from STATE_DB +void AclOrch::removeAclRuleStatus(string rule_name) +{ + m_aclRuleStateTable.del(rule_name); +} \ No newline at end of file diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index c62a68991a..8fe07c5ecb 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -553,6 +553,12 @@ class AclOrch : public Orch, public Observer string generateAclRuleIdentifierInCountersDb(const AclRule& rule) const; + void setAclTableStatus(string table_name, bool active); + void setAclRuleStatus(string rule_name, bool active); + + void removeAclTableStatus(string table_name); + void removeAclRuleStatus(string rule_name); + map m_AclTables; // TODO: Move all ACL tables into one map: name -> instance map m_ctrlAclTables; @@ -563,6 +569,9 @@ class AclOrch : public Orch, public Observer Table m_aclStageCapabilityTable; + Table m_aclTableStateTable; + Table m_aclRuleStateTable; + map m_mirrorTableId; map m_mirrorV6TableId; From 1595ec079114b659ffcfd980c40ddcba403513f7 Mon Sep 17 00:00:00 2001 From: bingwang Date: Mon, 6 Feb 2023 19:01:27 +0000 Subject: [PATCH 2/7] Add status for ACL_TABLE and ACL_RULE in STATE_DB --- orchagent/aclorch.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 52d95af2cc..22416d20d7 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -3509,7 +3509,7 @@ AclOrch::AclOrch(vector& connectors, DBConnector* stateDb, Switc 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_aclRuleStateTable(stateDb, STATE_ACL_RULE_TABLE_NAME), m_switchOrch(switchOrch), m_mirrorOrch(mirrorOrch), m_neighOrch(neighOrch), @@ -4362,6 +4362,8 @@ void AclOrch::doAclTableTask(Consumer &consumer) else { it = consumer.m_toSync.erase(it); + // Mark the ACL table as inactive if the configuration is invalid + setAclTableStatus(table_id, false); SWSS_LOG_ERROR("Failed to create ACL table %s, invalid configuration", table_id.c_str()); } @@ -4527,6 +4529,8 @@ void AclOrch::doAclRuleTask(Consumer &consumer) else { it = consumer.m_toSync.erase(it); + // Mark the rule inactive if the configuration is invalid + setAclRuleStatus(rule_id, false); SWSS_LOG_ERROR("Failed to create ACL rule. Rule configuration is invalid"); } } @@ -4919,7 +4923,7 @@ void AclOrch::removeAclTableStatus(string table_name) } // Set the status of ACL rule in STATE_DB -void AclOrch::setAclRuleStatus(string rule_name, book active) +void AclOrch::setAclRuleStatus(string rule_name, bool active) { vector fvVector; if (active) From 052f80b60d267c86e5403f3fab8b628915be99a1 Mon Sep 17 00:00:00 2001 From: bingwang Date: Thu, 9 Feb 2023 17:32:13 -0800 Subject: [PATCH 3/7] Fix key issue --- orchagent/aclorch.cpp | 26 +++++++++++++------------- orchagent/aclorch.h | 4 ++-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 22416d20d7..85263ee1c1 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -4517,12 +4517,12 @@ void AclOrch::doAclRuleTask(Consumer &consumer) { if (addAclRule(newRule, table_id)) { - setAclRuleStatus(rule_id, true); + setAclRuleStatus(table_id, rule_id, true); it = consumer.m_toSync.erase(it); } else { - setAclRuleStatus(rule_id, false); + setAclRuleStatus(table_id, rule_id, false); it++; } } @@ -4530,7 +4530,7 @@ void AclOrch::doAclRuleTask(Consumer &consumer) { it = consumer.m_toSync.erase(it); // Mark the rule inactive if the configuration is invalid - setAclRuleStatus(rule_id, false); + setAclRuleStatus(table_id, rule_id, false); SWSS_LOG_ERROR("Failed to create ACL rule. Rule configuration is invalid"); } } @@ -4538,7 +4538,7 @@ void AclOrch::doAclRuleTask(Consumer &consumer) { if (removeAclRule(table_id, rule_id)) { - removeAclRuleStatus(rule_id); + removeAclRuleStatus(table_id, rule_id); it = consumer.m_toSync.erase(it); } else @@ -4907,11 +4907,11 @@ void AclOrch::setAclTableStatus(string table_name, bool active) vector fvVector; if (active) { - fvVector.emplace_back("Status", "Active"); + fvVector.emplace_back("status", "Active"); } else { - fvVector.emplace_back("Status", "Inactive"); + fvVector.emplace_back("status", "Inactive"); } m_aclTableStateTable.set(table_name, fvVector); } @@ -4923,22 +4923,22 @@ void AclOrch::removeAclTableStatus(string table_name) } // Set the status of ACL rule in STATE_DB -void AclOrch::setAclRuleStatus(string rule_name, bool active) +void AclOrch::setAclRuleStatus(string table_name, string rule_name, bool active) { vector fvVector; if (active) { - fvVector.emplace_back("Status", "Active"); + fvVector.emplace_back("status", "Active"); } else { - fvVector.emplace_back("Status", "Inactive"); + fvVector.emplace_back("status", "Inactive"); } - m_aclRuleStateTable.set(rule_name, fvVector); + m_aclRuleStateTable.set(table_name + string("|") + rule_name, fvVector); } // Remove the status record of given ACL rule from STATE_DB -void AclOrch::removeAclRuleStatus(string rule_name) +void AclOrch::removeAclRuleStatus(string table_name, string rule_name) { - m_aclRuleStateTable.del(rule_name); -} \ No newline at end of file + m_aclRuleStateTable.del(table_name + string("|") + rule_name); +} diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 8fe07c5ecb..df04ce0574 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -554,10 +554,10 @@ class AclOrch : public Orch, public Observer string generateAclRuleIdentifierInCountersDb(const AclRule& rule) const; void setAclTableStatus(string table_name, bool active); - void setAclRuleStatus(string rule_name, bool active); + void setAclRuleStatus(string table_name, string rule_name, bool active); void removeAclTableStatus(string table_name); - void removeAclRuleStatus(string rule_name); + void removeAclRuleStatus(string table_name, string rule_name); map m_AclTables; // TODO: Move all ACL tables into one map: name -> instance From 8acf96445e5bc1e530f239ca5da8e2d63fae329c Mon Sep 17 00:00:00 2001 From: bingwang Date: Mon, 13 Feb 2023 07:16:20 +0000 Subject: [PATCH 4/7] Fix UT --- tests/dvslib/dvs_acl.py | 45 ++++++++++++++- tests/test_acl.py | 123 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 162 insertions(+), 6 deletions(-) diff --git a/tests/dvslib/dvs_acl.py b/tests/dvslib/dvs_acl.py index a4801891cd..4315da3798 100644 --- a/tests/dvslib/dvs_acl.py +++ b/tests/dvslib/dvs_acl.py @@ -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.""" @@ -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" @@ -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) + diff --git a/tests/test_acl.py b/tests/test_acl.py index 618f6eda6f..bacf0059ee 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -100,9 +100,13 @@ def test_AclTableCreationDeletion(self, dvs_acl): 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, L3_BIND_PORTS, 1) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_table_status(L3_TABLE_NAME, "Active") finally: dvs_acl.remove_acl_table(L3_TABLE_NAME) dvs_acl.verify_acl_table_count(0) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_table_status(L3_TABLE_NAME, None) def test_AclRuleL4SrcPort(self, dvs_acl, l3_acl_table): config_qualifiers = {"L4_SRC_PORT": "65000"} @@ -112,8 +116,12 @@ def test_AclRuleL4SrcPort(self, dvs_acl, l3_acl_table): dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_AclRuleIpProtocol(self, dvs_acl, l3_acl_table): @@ -124,8 +132,12 @@ def test_AclRuleIpProtocol(self, dvs_acl, l3_acl_table): dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_AclRuleTCPProtocolAppendedForTCPFlags(self, dvs_acl, l3_acl_table): @@ -141,8 +153,12 @@ def test_AclRuleTCPProtocolAppendedForTCPFlags(self, dvs_acl, l3_acl_table): } dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_AclRuleNextHeader(self, dvs_acl, l3_acl_table): @@ -150,9 +166,13 @@ def test_AclRuleNextHeader(self, dvs_acl, l3_acl_table): # Shouldn't allow NEXT_HEADER on vanilla L3 tables. dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Inactive") dvs_acl.verify_no_acl_rules() dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleNextHeaderAppendedForTCPFlags(self, dvs_acl, l3v6_acl_table): @@ -169,8 +189,12 @@ def test_V6AclRuleNextHeaderAppendedForTCPFlags(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_AclRuleInPorts(self, dvs_acl, mirror_acl_table): @@ -187,9 +211,13 @@ def test_AclRuleInPorts(self, dvs_acl, mirror_acl_table): } dvs_acl.create_acl_rule(MIRROR_TABLE_NAME, MIRROR_RULE_NAME, config_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(MIRROR_TABLE_NAME, MIRROR_RULE_NAME, "Active") dvs_acl.verify_acl_rule(expected_sai_qualifiers) dvs_acl.remove_acl_rule(MIRROR_TABLE_NAME, MIRROR_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(MIRROR_TABLE_NAME, MIRROR_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_AclRuleOutPorts(self, dvs_acl, mclag_acl_table): @@ -207,8 +235,12 @@ def test_AclRuleOutPorts(self, dvs_acl, mclag_acl_table): dvs_acl.create_acl_rule(MCLAG_TABLE_NAME, MCLAG_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(MCLAG_TABLE_NAME, MCLAG_RULE_NAME, "Active") dvs_acl.remove_acl_rule(MCLAG_TABLE_NAME, MCLAG_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(MCLAG_TABLE_NAME, MCLAG_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_AclRuleInPortsNonExistingInterface(self, dvs_acl, mirror_acl_table): @@ -220,9 +252,12 @@ def test_AclRuleInPortsNonExistingInterface(self, dvs_acl, mirror_acl_table): } dvs_acl.create_acl_rule(MIRROR_TABLE_NAME, MIRROR_RULE_NAME, config_qualifiers) - + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(MIRROR_TABLE_NAME, MIRROR_RULE_NAME, "Inactive") dvs_acl.verify_no_acl_rules() dvs_acl.remove_acl_rule(MIRROR_TABLE_NAME, MIRROR_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(MIRROR_TABLE_NAME, MIRROR_RULE_NAME, None) def test_AclRuleOutPortsNonExistingInterface(self, dvs_acl, mclag_acl_table): """ @@ -233,9 +268,12 @@ def test_AclRuleOutPortsNonExistingInterface(self, dvs_acl, mclag_acl_table): } dvs_acl.create_acl_rule(MCLAG_TABLE_NAME, MCLAG_RULE_NAME, config_qualifiers) - + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(MCLAG_TABLE_NAME, MCLAG_RULE_NAME, "Inactive") dvs_acl.verify_no_acl_rules() dvs_acl.remove_acl_rule(MCLAG_TABLE_NAME, MCLAG_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(MCLAG_TABLE_NAME, MCLAG_RULE_NAME, None) def test_AclRuleVlanId(self, dvs_acl, l3_acl_table): config_qualifiers = {"VLAN_ID": "100"} @@ -244,9 +282,13 @@ def test_AclRuleVlanId(self, dvs_acl, l3_acl_table): } dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Active") dvs_acl.verify_acl_rule(expected_sai_qualifiers) dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclTableCreationDeletion(self, dvs_acl): @@ -259,8 +301,12 @@ def test_V6AclTableCreationDeletion(self, dvs_acl): acl_table_group_ids = dvs_acl.get_acl_table_group_ids(len(L3V6_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, L3V6_BIND_PORTS, 1) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_table_status(L3V6_TABLE_NAME, "Active") finally: dvs_acl.remove_acl_table(L3V6_TABLE_NAME) + # Verify the STATE_DB entry is cleared + dvs_acl.verify_acl_table_status(L3V6_TABLE_NAME, None) dvs_acl.verify_acl_table_count(0) def test_V6AclRuleIPv6Any(self, dvs_acl, l3v6_acl_table): @@ -270,9 +316,13 @@ def test_V6AclRuleIPv6Any(self, dvs_acl, l3v6_acl_table): } dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.verify_acl_rule(expected_sai_qualifiers) dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleIPv6AnyDrop(self, dvs_acl, l3v6_acl_table): @@ -286,8 +336,12 @@ def test_V6AclRuleIPv6AnyDrop(self, dvs_acl, l3v6_acl_table): config_qualifiers, action="DROP") dvs_acl.verify_acl_rule(expected_sai_qualifiers, action="DROP") + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() # This test validates that backwards compatibility works as expected, it should @@ -300,8 +354,12 @@ def test_V6AclRuleIpProtocol(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleNextHeader(self, dvs_acl, l3v6_acl_table): @@ -312,8 +370,12 @@ def test_V6AclRuleNextHeader(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleSrcIPv6(self, dvs_acl, l3v6_acl_table): @@ -325,8 +387,12 @@ def test_V6AclRuleSrcIPv6(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleDstIPv6(self, dvs_acl, l3v6_acl_table): @@ -337,8 +403,12 @@ def test_V6AclRuleDstIPv6(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleL4SrcPort(self, dvs_acl, l3v6_acl_table): @@ -349,8 +419,12 @@ def test_V6AclRuleL4SrcPort(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleL4DstPort(self, dvs_acl, l3v6_acl_table): @@ -361,8 +435,12 @@ def test_V6AclRuleL4DstPort(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleL4SrcPortRange(self, dvs_acl, l3v6_acl_table): @@ -373,8 +451,12 @@ def test_V6AclRuleL4SrcPortRange(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleL4DstPortRange(self, dvs_acl, l3v6_acl_table): @@ -385,8 +467,12 @@ def test_V6AclRuleL4DstPortRange(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_V6AclRuleVlanId(self, dvs_acl, l3v6_acl_table): @@ -397,8 +483,12 @@ def test_V6AclRuleVlanId(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_InsertAclRuleBetweenPriorities(self, dvs_acl, l3_acl_table): @@ -430,6 +520,8 @@ def test_InsertAclRuleBetweenPriorities(self, dvs_acl, l3_acl_table): f"PRIORITY_TEST_RULE_{rule}", config_qualifiers[rule], action=config_actions[rule], priority=rule) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, f"PRIORITY_TEST_RULE_{rule}", "Active") dvs_acl.verify_acl_rule_set(rule_priorities, config_actions, expected_sai_qualifiers) @@ -447,9 +539,12 @@ def test_InsertAclRuleBetweenPriorities(self, dvs_acl, l3_acl_table): action="DROP", priority=odd_priority) dvs_acl.verify_acl_rule_set(rule_priorities, config_actions, expected_sai_qualifiers) - + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, f"PRIORITY_TEST_RULE_{odd_priority}", "Active") for rule in rule_priorities: dvs_acl.remove_acl_rule(L3_TABLE_NAME, f"PRIORITY_TEST_RULE_{rule}") + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, f"PRIORITY_TEST_RULE_{rule}", None) dvs_acl.verify_no_acl_rules() def test_RulesWithDiffMaskLengths(self, dvs_acl, l3_acl_table): @@ -488,10 +583,14 @@ def test_RulesWithDiffMaskLengths(self, dvs_acl, l3_acl_table): config_qualifiers[rule], action=config_actions[rule], priority=rule) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, f"MASK_TEST_RULE_{rule}", "Active") dvs_acl.verify_acl_rule_set(rule_priorities, config_actions, expected_sai_qualifiers) for rule in rule_priorities: dvs_acl.remove_acl_rule(L3_TABLE_NAME, f"MASK_TEST_RULE_{rule}") + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, f"MASK_TEST_RULE_{rule}", None) dvs_acl.verify_no_acl_rules() def test_AclRuleIcmp(self, dvs_acl, l3_acl_table): @@ -507,8 +606,12 @@ def test_AclRuleIcmp(self, dvs_acl, l3_acl_table): dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) dvs_acl.verify_no_acl_rules() dvs_acl.remove_acl_table(L3_TABLE_NAME) @@ -527,8 +630,12 @@ def test_AclRuleIcmpV6(self, dvs_acl, l3v6_acl_table): dvs_acl.create_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME, config_qualifiers) dvs_acl.verify_acl_rule(expected_sai_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3V6_TABLE_NAME, L3V6_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3V6_TABLE_NAME, L3V6_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_AclRuleRedirect(self, dvs, dvs_acl, l3_acl_table, setup_teardown_neighbor): @@ -546,8 +653,11 @@ def test_AclRuleRedirect(self, dvs, dvs_acl, l3_acl_table, setup_teardown_neighb next_hop_id = setup_teardown_neighbor dvs_acl.verify_redirect_acl_rule(expected_sai_qualifiers, next_hop_id, priority="20") - + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) dvs_acl.verify_no_acl_rules() dvs_acl.create_redirect_acl_rule(L3_TABLE_NAME, @@ -558,8 +668,11 @@ def test_AclRuleRedirect(self, dvs, dvs_acl, l3_acl_table, setup_teardown_neighb intf_id = dvs.asic_db.port_name_map["Ethernet4"] dvs_acl.verify_redirect_acl_rule(expected_sai_qualifiers, intf_id, priority="20") - + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, "Active") dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, L3_RULE_NAME, None) dvs_acl.verify_no_acl_rules() def test_AclTableMandatoryMatchFields(self, dvs, pfcwd_acl_table): From 6f9633a9d7915bd2dfdd41578c87830f1538ae09 Mon Sep 17 00:00:00 2001 From: bingwang Date: Wed, 1 Mar 2023 00:58:16 +0000 Subject: [PATCH 5/7] Support pending creation/removal status --- orchagent/aclorch.cpp | 54 ++++++++++++++++++++++--------------------- orchagent/aclorch.h | 12 ++++++++-- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 85263ee1c1..8b0572aa9d 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -415,6 +415,14 @@ static map aclCounterLookup = {SAI_ACL_COUNTER_ATTR_ENABLE_PACKET_COUNT, SAI_ACL_COUNTER_ATTR_PACKETS}, }; +static map 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)) @@ -4333,7 +4341,7 @@ void AclOrch::doAclTableTask(Consumer &consumer) { SWSS_LOG_NOTICE("Successfully updated existing ACL table %s", table_id.c_str()); - setAclTableStatus(table_id, true); + setAclTableStatus(table_id, AclObjectStatus::ACTIVE); it = consumer.m_toSync.erase(it); } else @@ -4341,7 +4349,7 @@ void AclOrch::doAclTableTask(Consumer &consumer) 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, false); + setAclTableStatus(table_id, AclObjectStatus::INACTIVE); it++; } } @@ -4349,12 +4357,12 @@ void AclOrch::doAclTableTask(Consumer &consumer) { if (addAclTable(newTable)) { - setAclTableStatus(table_id, true); + setAclTableStatus(table_id, AclObjectStatus::ACTIVE); it = consumer.m_toSync.erase(it); } else { - setAclTableStatus(table_id, false); + setAclTableStatus(table_id, AclObjectStatus::PENDING_CREATION); it++; } } @@ -4363,7 +4371,7 @@ void AclOrch::doAclTableTask(Consumer &consumer) { it = consumer.m_toSync.erase(it); // Mark the ACL table as inactive if the configuration is invalid - setAclTableStatus(table_id, false); + setAclTableStatus(table_id, AclObjectStatus::INACTIVE); SWSS_LOG_ERROR("Failed to create ACL table %s, invalid configuration", table_id.c_str()); } @@ -4376,7 +4384,11 @@ void AclOrch::doAclTableTask(Consumer &consumer) 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++; + } } else { @@ -4517,12 +4529,12 @@ void AclOrch::doAclRuleTask(Consumer &consumer) { if (addAclRule(newRule, table_id)) { - setAclRuleStatus(table_id, rule_id, true); + setAclRuleStatus(table_id, rule_id, AclObjectStatus::ACTIVE); it = consumer.m_toSync.erase(it); } else { - setAclRuleStatus(table_id, rule_id, false); + setAclRuleStatus(table_id, rule_id, AclObjectStatus::PENDING_CREATION); it++; } } @@ -4530,7 +4542,7 @@ void AclOrch::doAclRuleTask(Consumer &consumer) { it = consumer.m_toSync.erase(it); // Mark the rule inactive if the configuration is invalid - setAclRuleStatus(table_id, rule_id, false); + setAclRuleStatus(table_id, rule_id, AclObjectStatus::INACTIVE); SWSS_LOG_ERROR("Failed to create ACL rule. Rule configuration is invalid"); } } @@ -4542,7 +4554,11 @@ void AclOrch::doAclRuleTask(Consumer &consumer) 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 { @@ -4902,17 +4918,10 @@ bool AclOrch::getAclBindPortId(Port &port, sai_object_id_t &port_id) } // Set the status of ACL table in STATE_DB -void AclOrch::setAclTableStatus(string table_name, bool active) +void AclOrch::setAclTableStatus(string table_name, AclObjectStatus status) { vector fvVector; - if (active) - { - fvVector.emplace_back("status", "Active"); - } - else - { - fvVector.emplace_back("status", "Inactive"); - } + fvVector.emplace_back("status", aclObjectStatusLookup[status]); m_aclTableStateTable.set(table_name, fvVector); } @@ -4923,17 +4932,10 @@ void AclOrch::removeAclTableStatus(string table_name) } // Set the status of ACL rule in STATE_DB -void AclOrch::setAclRuleStatus(string table_name, string rule_name, bool active) +void AclOrch::setAclRuleStatus(string table_name, string rule_name, AclObjectStatus status) { vector fvVector; - if (active) - { - fvVector.emplace_back("status", "Active"); - } - else - { - fvVector.emplace_back("status", "Inactive"); - } + fvVector.emplace_back("status", aclObjectStatusLookup[status]); m_aclRuleStateTable.set(table_name + string("|") + rule_name, fvVector); } diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index df04ce0574..129eb3c6c6 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -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 actionList; @@ -553,8 +561,8 @@ class AclOrch : public Orch, public Observer string generateAclRuleIdentifierInCountersDb(const AclRule& rule) const; - void setAclTableStatus(string table_name, bool active); - void setAclRuleStatus(string table_name, string rule_name, bool active); + 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); From 08b4583882a10e1302509a20ea50e55c273a1416 Mon Sep 17 00:00:00 2001 From: bingwang Date: Thu, 2 Mar 2023 00:41:01 +0000 Subject: [PATCH 6/7] Clear STATE_DB at aclorch init --- orchagent/aclorch.cpp | 28 ++++++++++++++++++++++++++++ orchagent/aclorch.h | 3 +++ 2 files changed, 31 insertions(+) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 8b0572aa9d..9eafa6e3cd 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -3014,6 +3014,10 @@ void AclOrch::init(vector& 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 @@ -4944,3 +4948,27 @@ 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 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 keys; + m_aclRuleStateTable.getKeys(keys); + for (auto key : keys) + { + m_aclRuleStateTable.del(key); + } +} + diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h index 129eb3c6c6..cfd92f09df 100644 --- a/orchagent/aclorch.h +++ b/orchagent/aclorch.h @@ -567,6 +567,9 @@ class AclOrch : public Orch, public Observer void removeAclTableStatus(string table_name); void removeAclRuleStatus(string table_name, string rule_name); + void removeAllAclTableStatus(); + void removeAllAclRuleStatus(); + map m_AclTables; // TODO: Move all ACL tables into one map: name -> instance map m_ctrlAclTables; From 40391178a76fc38a4f1334a14432a8f9f0c90aa1 Mon Sep 17 00:00:00 2001 From: bingwang Date: Wed, 8 Mar 2023 01:30:07 +0000 Subject: [PATCH 7/7] Update test --- orchagent/aclorch.cpp | 5 +++-- tests/test_acl.py | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 9eafa6e3cd..004a9e174c 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -4345,6 +4345,7 @@ void AclOrch::doAclTableTask(Consumer &consumer) { SWSS_LOG_NOTICE("Successfully updated existing ACL table %s", table_id.c_str()); + // Mark ACL table as ACTIVE setAclTableStatus(table_id, AclObjectStatus::ACTIVE); it = consumer.m_toSync.erase(it); } @@ -4352,8 +4353,6 @@ void AclOrch::doAclTableTask(Consumer &consumer) { 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++; } } @@ -4361,6 +4360,7 @@ void AclOrch::doAclTableTask(Consumer &consumer) { if (addAclTable(newTable)) { + // Mark ACL table as ACTIVE setAclTableStatus(table_id, AclObjectStatus::ACTIVE); it = consumer.m_toSync.erase(it); } @@ -4384,6 +4384,7 @@ void AclOrch::doAclTableTask(Consumer &consumer) { if (removeAclTable(table_id)) { + // Remove ACL table status from STATE_DB removeAclTableStatus(table_id); it = consumer.m_toSync.erase(it); } diff --git a/tests/test_acl.py b/tests/test_acl.py index bacf0059ee..d0bad2c509 100644 --- a/tests/test_acl.py +++ b/tests/test_acl.py @@ -108,6 +108,29 @@ def test_AclTableCreationDeletion(self, dvs_acl): # Verify the STATE_DB entry is removed dvs_acl.verify_acl_table_status(L3_TABLE_NAME, None) + def test_InvalidAclTableCreationDeletion(self, dvs_acl): + try: + dvs_acl.create_acl_table("INVALID_ACL_TABLE", L3_TABLE_TYPE, "dummy_port", "invalid_stage") + # Verify status is written into STATE_DB + dvs_acl.verify_acl_table_status("INVALID_ACL_TABLE", "Inactive") + finally: + dvs_acl.remove_acl_table("INVALID_ACL_TABLE") + dvs_acl.verify_acl_table_count(0) + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_table_status("INVALID_ACL_TABLE", None) + + def test_InvalidAclRuleCreation(self, dvs_acl, l3_acl_table): + config_qualifiers = {"INVALID_QUALIFIER": "TEST"} + + dvs_acl.create_acl_rule(L3_TABLE_NAME, "INVALID_RULE", config_qualifiers) + # Verify status is written into STATE_DB + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, "INVALID_RULE", "Inactive") + + dvs_acl.remove_acl_rule(L3_TABLE_NAME, "INVALID_RULE") + # Verify the STATE_DB entry is removed + dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, "INVALID_RULE", None) + dvs_acl.verify_no_acl_rules() + def test_AclRuleL4SrcPort(self, dvs_acl, l3_acl_table): config_qualifiers = {"L4_SRC_PORT": "65000"} expected_sai_qualifiers = {