From 1f13d1a5888af59817811a69e4f1aa6077a11559 Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Wed, 6 Jul 2022 12:11:51 +0800 Subject: [PATCH] [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 --- cfgmgr/teammgr.cpp | 61 +++++++++++++++++++++++++++++- cfgmgr/teammgr.h | 3 ++ tests/test_macsec.py | 88 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 2 deletions(-) diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index ad8572e07b..31f911741c 100644 --- a/cfgmgr/teammgr.cpp +++ b/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 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 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++; diff --git a/cfgmgr/teammgr.h b/cfgmgr/teammgr.h index c1b5d525c0..db87fdd1f4 100644 --- a/cfgmgr/teammgr.h +++ b/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&); }; diff --git a/tests/test_macsec.py b/tests/test_macsec.py index f2f8e8843e..9dc5a4ed53 100644 --- a/tests/test_macsec.py +++ b/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