From 1ed0b4bedae40ca45627f33949219b7cc912217d Mon Sep 17 00:00:00 2001
From: Junhua Zhai <junhua.zhai@outlook.com>
Date: Fri, 24 Jun 2022 10:37:56 +0000
Subject: [PATCH] [macsec] Refactor the logic of macsec name map (#2348)

* Add/remove macsec name map w/o gearbox correctly
* Add macsec counter unit test
---
 orchagent/macsecorch.cpp | 17 ++-----------
 tests/test_macsec.py     | 53 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/orchagent/macsecorch.cpp b/orchagent/macsecorch.cpp
index ac56fcbd5d0d..dc2c9d7b4318 100644
--- a/orchagent/macsecorch.cpp
+++ b/orchagent/macsecorch.cpp
@@ -2340,10 +2340,6 @@ void MACsecOrch::installCounter(
     sai_object_id_t obj_id,
     const std::vector<std::string> &stats)
 {
-    FieldValueTuple tuple(obj_name, sai_serialize_object_id(obj_id));
-    vector<FieldValueTuple> fields;
-    fields.push_back(tuple);
-
     std::unordered_set<std::string> counter_stats;
     for (const auto &stat : stats)
     {
@@ -2353,12 +2349,11 @@ void MACsecOrch::installCounter(
     {
         case CounterType::MACSEC_SA_ATTR:
             MACsecSaAttrStatManager(ctx).setCounterIdList(obj_id, counter_type, counter_stats);
-            MACsecCountersMap(ctx).set("", fields);
             break;
 
         case CounterType::MACSEC_SA:
             MACsecSaStatManager(ctx).setCounterIdList(obj_id, counter_type, counter_stats);
-            MACsecCountersMap(ctx).set("", fields);
+            MACsecCountersMap(ctx).hset("", obj_name, sai_serialize_object_id(obj_id));
             break;
 
         case CounterType::MACSEC_FLOW:
@@ -2383,19 +2378,11 @@ void MACsecOrch::uninstallCounter(
     {
         case CounterType::MACSEC_SA_ATTR:
             MACsecSaAttrStatManager(ctx).clearCounterIdList(obj_id);
-            m_counter_db.hdel(COUNTERS_MACSEC_NAME_MAP, obj_name);
             break;
 
         case CounterType::MACSEC_SA:
             MACsecSaStatManager(ctx).clearCounterIdList(obj_id);
-            if (direction == SAI_MACSEC_DIRECTION_EGRESS)
-            {
-                m_counter_db.hdel(COUNTERS_MACSEC_SA_TX_NAME_MAP, obj_name);
-            }
-            else
-            {
-                m_counter_db.hdel(COUNTERS_MACSEC_SA_RX_NAME_MAP, obj_name);
-            }
+            MACsecCountersMap(ctx).hdel("", obj_name);
             break;
 
         case CounterType::MACSEC_FLOW:
diff --git a/tests/test_macsec.py b/tests/test_macsec.py
index 61c90d84a859..f2f8e8843edc 100644
--- a/tests/test_macsec.py
+++ b/tests/test_macsec.py
@@ -1,9 +1,11 @@
 from swsscommon import swsscommon
+from swsscommon.swsscommon import CounterTable, MacsecCounter
 import conftest
 
 import functools
 import typing
 import re
+import time
 
 
 def to_string(value):
@@ -389,6 +391,21 @@ def get_macsec_sa(
         print(info.group(0))
         return info.group(0)
 
+    @macsec_sa()
+    def get_macsec_xpn_counter(
+            self,
+            sai: str) -> int:
+        counter_table = CounterTable(self.dvs.get_counters_db().db_connection)
+        for i in range(3):
+            r, value = counter_table.hget(
+                MacsecCounter(),
+                sai,
+                "SAI_MACSEC_SA_ATTR_CURRENT_XPN")
+            if r: return int(value)
+            time.sleep(1) # wait a moment for polling counter
+
+        return None
+
 
 class TestMACsec(object):
     def init_macsec(
@@ -658,6 +675,18 @@ def test_macsec_term_orch(self, dvs: conftest.DockerVirtualSwitch, testlog):
                 peer_mac_address,
                 macsec_port_identifier,
                 0))
+        assert(
+            inspector.get_macsec_xpn_counter(
+                port_name,
+                local_mac_address,
+                macsec_port_identifier,
+                0) == packet_number)
+        assert(
+            inspector.get_macsec_xpn_counter(
+                port_name,
+                peer_mac_address,
+                macsec_port_identifier,
+                0) == packet_number)
         self.rekey_macsec(
             wpa,
             port_name,
@@ -683,6 +712,18 @@ def test_macsec_term_orch(self, dvs: conftest.DockerVirtualSwitch, testlog):
                 peer_mac_address,
                 macsec_port_identifier,
                 1))
+        assert(
+            inspector.get_macsec_xpn_counter(
+                port_name,
+                local_mac_address,
+                macsec_port_identifier,
+                1) == packet_number)
+        assert(
+            inspector.get_macsec_xpn_counter(
+                port_name,
+                peer_mac_address,
+                macsec_port_identifier,
+                1) == packet_number)
         assert(
             not inspector.get_macsec_sa(
                 macsec_port,
@@ -695,6 +736,18 @@ def test_macsec_term_orch(self, dvs: conftest.DockerVirtualSwitch, testlog):
                 peer_mac_address,
                 macsec_port_identifier,
                 0))
+        assert(
+            not inspector.get_macsec_xpn_counter(
+                port_name,
+                local_mac_address,
+                macsec_port_identifier,
+                0) == packet_number)
+        assert(
+            not inspector.get_macsec_xpn_counter(
+                port_name,
+                peer_mac_address,
+                macsec_port_identifier,
+                0) == packet_number)
         # Exit MACsec port
         self.deinit_macsec(
             wpa,