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

bingwang-ms · yxieca · commit 95d3f6d139d3 · 2023-03-08T17:35:53.000Z
* Add status for ACL_TABLE and ACL_RULE in STATE_DB
diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp
@@ -380,6 +380,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))
@@ -2911,6 +2919,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
@@ -3416,6 +3428,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),
@@ -4239,6 +4253,8 @@ 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);
                     }
                     else
@@ -4251,24 +4267,41 @@ void AclOrch::doAclTableTask(Consumer &consumer)
                 else
                 {
                     if (addAclTable(newTable))
+                    {
+                        // Mark ACL table as ACTIVE
+                        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))
+            {
+                // Remove ACL table status from STATE_DB
+                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++;
+            }
         }
         else
         {
@@ -4408,22 +4441,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
         {
@@ -4781,3 +4829,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);
+    }
+}
+
diff --git a/orchagent/aclorch.h b/orchagent/aclorch.h
@@ -99,6 +99,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;
@@ -549,6 +557,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;
@@ -559,6 +576,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;
 
diff --git 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
