[teammgr]: Waiting MACsec ready before doLagMemberTask (#2286)

Signed-off-by: Ze Gan ganze718@gmail.com, Judy Joseph jujoseph@microsoft.com

What I did
If a member of portchannel has macsec profile attached in config, enable MACsec on the port before it's been added as a member of portchannel.

Why I did it
Due to some hardware limitation, cannot enable MACsec on a member of portchannel.
So we enable the macsec on interface first and then add it as part of portchannel.

Note: This is a work around which will be removed when h/w supports it future releases.

The approach taken in this PR is

In the teamdMgr when an interface is added as part of the LAG, we wait for the macsecPort creation done in SAI and Ingress SA creation complete (if macsec is enabled on the interface)
The above takes care of config reload, reboot scenario's where we cannot guarantee the sequence of macsec attach to interface, add interface as part of portchannel.

If we do a manual removal of port from portchannel, or remove macsec config from the interface, Please follow this steps

First remove the portchannel member out of portchannel
Remove the macsec profile attached to interface.

How I verified it

Verified with config reload, reboot with the macsec profile attached to portchannel member interfaces.
Verified case when SAK rekey is enabled on macsec on portchannel members
Verified case when member interface link flaps

Author: Pterosaur

cfgmgr/teammgr.cpp

@@ -33,7 +33,8 @@ TeamMgr::TeamMgr(DBConnector *confDb, DBConnector *applDb, DBConnector *statDb,
     m_appPortTable(applDb, APP_PORT_TABLE_NAME),
     m_appLagTable(applDb, APP_LAG_TABLE_NAME),
     m_statePortTable(statDb, STATE_PORT_TABLE_NAME),
-    m_stateLagTable(statDb, STATE_LAG_TABLE_NAME)
+    m_stateLagTable(statDb, STATE_LAG_TABLE_NAME),
+    m_stateMACsecIngressSATable(statDb, STATE_MACSEC_INGRESS_SA_TABLE_NAME)
 {
     SWSS_LOG_ENTER();
 
@@ -98,6 +99,51 @@ bool TeamMgr::isLagStateOk(const string &alias)
     return true;
 }
 
+bool TeamMgr::isMACsecAttached(const std::string &port)
+{
+    SWSS_LOG_ENTER();
+
+    vector<FieldValueTuple> temp;
+
+    if (!m_cfgPortTable.get(port, temp))
+    {
+        SWSS_LOG_INFO("Port %s is not ready", port.c_str());
+        return false;
+    }
+
+    auto macsec_opt = swss::fvsGetValue(temp, "macsec", true);
+    if (!macsec_opt || macsec_opt->empty())
+    {
+        SWSS_LOG_INFO("MACsec isn't setted on the port %s", port.c_str());
+        return false;
+    }
+
+    return true;
+}
+
+bool TeamMgr::isMACsecIngressSAOk(const std::string &port)
+{
+    SWSS_LOG_ENTER();
+
+    vector<string> keys;
+    m_stateMACsecIngressSATable.getKeys(keys);
+
+    for (auto key: keys)
+    {
+        auto tokens = tokenize(key, state_db_key_delimiter);
+        auto interface = tokens[0];
+
+        if (port == interface)
+        {
+            SWSS_LOG_NOTICE(" MACsec is ready on the port %s", port.c_str());
+            return true;
+        }
+    }
+
+    SWSS_LOG_INFO("MACsec is NOT ready on the port %s", port.c_str());
+    return false;
+}
+
 void TeamMgr::doTask(Consumer &consumer)
 {
     SWSS_LOG_ENTER();
@@ -309,7 +355,11 @@ void TeamMgr::doLagMemberTask(Consumer &consumer)
                 it++;
                 continue;
             }
-
+            if (isMACsecAttached(member) && !isMACsecIngressSAOk(member))
+            {
+                it++;
+                continue;
+            }
             if (addLagMember(lag, member) == task_need_retry)
             {
                 it++;
@@ -400,6 +450,13 @@ void TeamMgr::doPortUpdateTask(Consumer &consumer)
             string lag;
             if (findPortMaster(lag, alias))
             {
+                if (isMACsecAttached(alias) && !isMACsecIngressSAOk(alias))
+                {
+                    it++;
+                    SWSS_LOG_INFO("MACsec is NOT ready on the port %s", alias.c_str());
+                    continue;
+                }
+
                 if (addLagMember(lag, alias) == task_need_retry)
                 {
                     it++;

cfgmgr/teammgr.h

@@ -27,6 +27,7 @@ class TeamMgr : public Orch
     Table m_cfgLagMemberTable;
     Table m_statePortTable;
     Table m_stateLagTable;
+    Table m_stateMACsecIngressSATable;
 
     ProducerStateTable m_appPortTable;
     ProducerStateTable m_appLagTable;
@@ -55,6 +56,8 @@ class TeamMgr : public Orch
     bool checkPortIffUp(const std::string &);
     bool isPortStateOk(const std::string&);
     bool isLagStateOk(const std::string&);
+    bool isMACsecAttached(const std::string &);
+    bool isMACsecIngressSAOk(const std::string &);
     uint16_t generateLacpKey(const std::string&);
 };
 

tests/test_macsec.py

@@ -2,6 +2,7 @@
 from swsscommon.swsscommon import CounterTable, MacsecCounter
 import conftest
 
+import time
 import functools
 import typing
 import re
@@ -89,6 +90,12 @@ def convert_key(self, key: str):
                 StateDBTable.SEPARATOR))
 
 
+class ConfigTable(Table):
+
+    def __init__(self, dvs: conftest.DockerVirtualSwitch, table_name: str):
+        super(ConfigTable, self).__init__(dvs.get_config_db(), table_name)
+
+
 def gen_sci(macsec_system_identifier: str, macsec_port_identifier: int) -> str:
     macsec_system_identifier = macsec_system_identifier.translate(
         str.maketrans("", "", ":.-"))
@@ -808,6 +815,87 @@ def test_macsec_attribute_change(self, dvs: conftest.DockerVirtualSwitch, testlo
             macsec_port_identifier,
             0)
 
+    def test_macsec_with_portchannel(self, dvs: conftest.DockerVirtualSwitch, testlog):
+
+        # Set MACsec enabled on Ethernet0
+        ConfigTable(dvs, "PORT")["Ethernet0"] = {"macsec" : "test"}
+        StateDBTable(dvs, "FEATURE")["macsec"] = {"state": "enabled"}
+
+        # Setup Port-channel
+        ConfigTable(dvs, "PORTCHANNEL")["PortChannel001"] = {"admin": "up", "mtu": "9100", "oper_status": "up"}
+        time.sleep(1)
+
+        # create port channel member
+        ConfigTable(dvs, "PORTCHANNEL_MEMBER")["PortChannel001|Ethernet0"] = {"NULL": "NULL"}
+        ConfigTable(dvs, "PORTCHANNEL_INTERFACE")["PortChannel001"] = {"NULL": "NULL"}
+        ConfigTable(dvs, "PORTCHANNEL_INTERFACE")["PortChannel001|40.0.0.0/31"] = {"NULL": "NULL"}
+        time.sleep(3)
+
+        # Check Portchannel member in ASIC db that shouldn't been created before MACsec enabled
+        lagmtbl = swsscommon.Table(swsscommon.DBConnector(1, dvs.redis_sock, 0), "ASIC_STATE:SAI_OBJECT_TYPE_LAG_MEMBER")
+        lagms = lagmtbl.getKeys()
+        assert len(lagms) == 0
+
+        # Create MACsec session
+        port_name = "Ethernet0"
+        local_mac_address = "00-15-5D-78-FF-C1"
+        peer_mac_address = "00-15-5D-78-FF-C2"
+        macsec_port_identifier = 1
+        macsec_port = "macsec_eth1"
+        sak = "0" * 32
+        auth_key = "0" * 32
+        packet_number = 1
+        ssci = 1
+        salt = "0" * 24
+
+        wpa = WPASupplicantMock(dvs)
+        inspector = MACsecInspector(dvs)
+
+        self.init_macsec(
+            wpa,
+            port_name,
+            local_mac_address,
+            macsec_port_identifier)
+        self.establish_macsec(
+            wpa,
+            port_name,
+            local_mac_address,
+            peer_mac_address,
+            macsec_port_identifier,
+            0,
+            sak,
+            packet_number,
+            auth_key,
+            ssci,
+            salt)
+        time.sleep(3)
+
+        # Check Portchannel member in ASIC db that should been created after MACsec enabled
+        lagmtbl = swsscommon.Table(swsscommon.DBConnector(1, dvs.redis_sock, 0), "ASIC_STATE:SAI_OBJECT_TYPE_LAG_MEMBER")
+        lagms = lagmtbl.getKeys()
+        assert len(lagms) == 1
+
+        self.deinit_macsec(
+            wpa,
+            inspector,
+            port_name,
+            macsec_port,
+            local_mac_address,
+            peer_mac_address,
+            macsec_port_identifier,
+            0)
+
+        # remove port channel member
+        del ConfigTable(dvs, "PORTCHANNEL_INTERFACE")["PortChannel001"]
+        del ConfigTable(dvs, "PORTCHANNEL_INTERFACE")["PortChannel001|40.0.0.0/31"]
+        del ConfigTable(dvs, "PORTCHANNEL_MEMBER")["PortChannel001|Ethernet0"]
+
+        # remove port channel
+        del ConfigTable(dvs, "PORTCHANNEL")["PortChannel001"]
+
+        # Clear MACsec enabled on Ethernet0
+        ConfigTable(dvs, "PORT")["Ethernet0"] = {"macsec" : ""}
+
 
 # Add Dummy always-pass test at end as workaroud
 # for issue when Flaky fail on final test it invokes module tear-down