[pbh]: Fix review comments.

Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
sonic-net · Aug 23, 2021 · 08743a1 · 08743a1
1 parent 5b5a237
commit 08743a1
Show file tree

Hide file tree

Showing 2 changed files with 138 additions and 0 deletions.
diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp
@@ -766,6 +766,11 @@ bool AclRule::enableCounter()
 {
     SWSS_LOG_ENTER();
 
+    if (m_counterOid != SAI_NULL_OBJECT_ID)
+    {
+        return true;
+    }
+
     if (m_ruleOid == SAI_NULL_OBJECT_ID)
     {
         SWSS_LOG_ERROR("ACL rule %s doesn't exist in ACL table %s", m_id.c_str(), m_tableId.c_str());
@@ -787,6 +792,7 @@ bool AclRule::enableCounter()
     if (status != SAI_STATUS_SUCCESS)
     {
         SWSS_LOG_ERROR("Failed to enable counter for ACL rule %s in ACL table %s", m_id.c_str(), m_tableId.c_str());
+        removeCounter();
         return false;
     }
 
@@ -797,6 +803,11 @@ bool AclRule::disableCounter()
 {
     SWSS_LOG_ENTER();
 
+    if (m_counterOid == SAI_NULL_OBJECT_ID)
+    {
+        return true;
+    }
+
     if (m_ruleOid == SAI_NULL_OBJECT_ID)
     {
         SWSS_LOG_ERROR("ACL rule %s doesn't exist in ACL table %s", m_id.c_str(), m_tableId.c_str());

diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp
@@ -937,6 +937,35 @@ namespace aclorch_test
             }
             return true;
         }
+
+        bool validateAclRuleCounter(const AclRule &rule, bool enabled)
+        {
+            auto ruleOid = Portal::AclRuleInternal::getRuleOid(&rule);
+
+            sai_attribute_t attr;
+            attr.id = SAI_ACL_ENTRY_ATTR_ACTION_COUNTER;
+
+            auto status = sai_acl_api->get_acl_entry_attribute(ruleOid, 1, &attr);
+            if (status != SAI_STATUS_SUCCESS)
+            {
+                return false;
+            }
+
+            auto &aclEnable = attr.value.aclaction.enable;
+            auto &aclOid = attr.value.aclaction.parameter.oid;
+
+            if (enabled)
+            {
+                if (aclEnable && aclOid != SAI_NULL_OBJECT_ID)
+                {
+                    return true;
+                }
+
+                return false;
+            }
+
+            return !aclEnable && aclOid == SAI_NULL_OBJECT_ID;
+        }
     };
 
     map<string, string> AclOrchTest::gProfileMap;
@@ -1180,4 +1209,102 @@ namespace aclorch_test
         }
     }
 
+    // When received ACL table/rule SET_COMMAND, orchagent can create corresponding ACL table/rule
+    // When received ACL table/rule DEL_COMMAND, orchagent can delete corresponding ACL table/rule
+    //
+    // Verify ACL rule counter enable/disable
+    //
+    TEST_F(AclOrchTest, AclRule_Counter_Configuration)
+    {
+        string tableId = "acl_table_1";
+        string ruleId = "acl_rule_1";
+
+        auto orch = createAclOrch();
+
+        // add acl table ...
+
+        auto kvfAclTable = deque<KeyOpFieldsValuesTuple>({{
+            tableId,
+            SET_COMMAND,
+            {
+                { ACL_TABLE_DESCRIPTION, "L3 table" },
+                { ACL_TABLE_TYPE, TABLE_TYPE_L3 },
+                { ACL_TABLE_STAGE, STAGE_INGRESS },
+                { ACL_TABLE_PORTS, "1,2" }
+            }
+        }});
+
+        orch->doAclTableTask(kvfAclTable);
+
+        // validate acl table add ...
+
+        auto tableOid = orch->getTableById(tableId);
+        ASSERT_NE(tableOid, SAI_NULL_OBJECT_ID);
+
+        auto tableIt = orch->getAclTables().find(tableOid);
+        ASSERT_NE(tableIt, orch->getAclTables().end());
+
+        // add acl rule ...
+
+        auto kvfAclRule = deque<KeyOpFieldsValuesTuple>({{
+            tableId + "|" + ruleId,
+            SET_COMMAND,
+            {
+                { ACTION_PACKET_ACTION, PACKET_ACTION_FORWARD },
+                { MATCH_SRC_IP, "1.2.3.4" },
+                { MATCH_DST_IP, "4.3.2.1" }
+            }
+        }});
+
+        orch->doAclRuleTask(kvfAclRule);
+
+        // validate acl rule add ...
+
+        auto ruleIt = tableIt->second.rules.find(ruleId);
+        ASSERT_NE(ruleIt, tableIt->second.rules.end());
+
+        auto &tableObj = tableIt->second;
+        auto &ruleObj = ruleIt->second;
+
+        // validate acl counter disabled ...
+
+        ASSERT_TRUE(ruleObj->disableCounter());
+        ASSERT_TRUE(validateAclRuleCounter(*ruleObj, false));
+
+        // validate acl counter enabled ...
+
+        ASSERT_TRUE(ruleObj->enableCounter());
+        ASSERT_TRUE(validateAclRuleCounter(*ruleObj, true));
+
+        // delete acl rule ...
+
+        kvfAclRule = deque<KeyOpFieldsValuesTuple>({{
+            tableId + "|" + ruleId,
+            DEL_COMMAND,
+            {}
+        }});
+
+        orch->doAclRuleTask(kvfAclRule);
+
+        // validate acl rule delete ...
+
+        ruleIt = tableObj.rules.find(ruleId);
+        ASSERT_EQ(ruleIt, tableObj.rules.end());
+
+        // delete acl table ...
+
+        kvfAclTable = deque<KeyOpFieldsValuesTuple>({{
+            tableId,
+            DEL_COMMAND,
+            {}
+        }});
+
+        orch->doAclTableTask(kvfAclTable);
+
+        // validate acl table delete ...
+
+        tableIt = orch->getAclTables().find(tableOid);
+        ASSERT_EQ(tableIt, orch->getAclTables().end());
+    }
+
 } // namespace nsAclOrchTest