From e23cbf3e3aab467a54cf3821f5e554395bf0e64e Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Mon, 23 May 2022 11:02:43 +0000 Subject: [PATCH 1/6] [teammgr]: Waiting MACsec ready before doLagMemberTask Signed-off-by: Ze Gan --- cfgmgr/teammgr.cpp | 46 +++++++++++++++++++++++++++++++++++++++++++++- cfgmgr/teammgr.h | 3 +++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index ad8572e07b..c53228120e 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_stateMACsecPortTable(statDb, STATE_MACSEC_PORT_TABLE_NAME) { SWSS_LOG_ENTER(); @@ -98,6 +99,43 @@ bool TeamMgr::isLagStateOk(const string &alias) return true; } +bool TeamMgr::isMACsecSetted(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::isMACsecStateOk(const std::string &port) +{ + SWSS_LOG_ENTER(); + + vector temp; + + if (!m_stateMACsecPortTable.get(port, temp)) + { + SWSS_LOG_INFO("MACsec isn't ready on the port %s", port.c_str()); + return false; + } + + return true; +} + void TeamMgr::doTask(Consumer &consumer) { SWSS_LOG_ENTER(); @@ -310,6 +348,12 @@ void TeamMgr::doLagMemberTask(Consumer &consumer) continue; } + if (isMACsecSetted(member) && !isMACsecStateOk(member)) + { + it++; + continue; + } + if (addLagMember(lag, member) == task_need_retry) { it++; diff --git a/cfgmgr/teammgr.h b/cfgmgr/teammgr.h index c1b5d525c0..8ce81a3e1a 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_stateMACsecPortTable; 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 isMACsecSetted(const std::string &); + bool isMACsecStateOk(const std::string &); uint16_t generateLacpKey(const std::string&); }; From b26b30545be55b44765ddf9e999fb1bae119cda8 Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Tue, 24 May 2022 11:44:31 +0000 Subject: [PATCH 2/6] Add test Signed-off-by: Ze Gan --- tests/test_macsec.py | 75 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/test_macsec.py b/tests/test_macsec.py index 61c90d84a8..ef60a4b3d6 100644 --- a/tests/test_macsec.py +++ b/tests/test_macsec.py @@ -1,6 +1,7 @@ from swsscommon import swsscommon import conftest +import time import functools import typing import re @@ -87,6 +88,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("", "", ":.-")) @@ -755,6 +762,74 @@ 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"} + + # 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) # Add Dummy always-pass test at end as workaroud # for issue when Flaky fail on final test it invokes module tear-down From c24d7d60375d685e847acfb30565d6461a566fa5 Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Wed, 25 May 2022 06:08:26 +0000 Subject: [PATCH 3/6] Add feature check Signed-off-by: Ze Gan --- cfgmgr/teammgr.cpp | 33 +++++++++++++++++++++++++++++---- cfgmgr/teammgr.h | 2 ++ tests/test_macsec.py | 15 ++++++++++++++- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index c53228120e..be9b3ebe34 100644 --- a/cfgmgr/teammgr.cpp +++ b/cfgmgr/teammgr.cpp @@ -34,7 +34,8 @@ TeamMgr::TeamMgr(DBConnector *confDb, DBConnector *applDb, DBConnector *statDb, m_appLagTable(applDb, APP_LAG_TABLE_NAME), m_statePortTable(statDb, STATE_PORT_TABLE_NAME), m_stateLagTable(statDb, STATE_LAG_TABLE_NAME), - m_stateMACsecPortTable(statDb, STATE_MACSEC_PORT_TABLE_NAME) + m_stateMACsecPortTable(statDb, STATE_MACSEC_PORT_TABLE_NAME), + m_stateFeatureTable(statDb, "FEATURE") { SWSS_LOG_ENTER(); @@ -99,6 +100,27 @@ bool TeamMgr::isLagStateOk(const string &alias) return true; } +bool TeamMgr::isMACsecFeatureEnabled() +{ + SWSS_LOG_ENTER(); + + vector temp; + if (!m_stateFeatureTable.get("macsec", temp)) + { + return false; + } + + auto opt = swss::fvsGetValue(temp, "state", true); + + if (!opt || *opt != "enabled") + { + SWSS_LOG_INFO("MACsec feature isn't enabled"); + return false; + } + + return true; +} + bool TeamMgr::isMACsecSetted(const std::string &port) { SWSS_LOG_ENTER(); @@ -348,10 +370,13 @@ void TeamMgr::doLagMemberTask(Consumer &consumer) continue; } - if (isMACsecSetted(member) && !isMACsecStateOk(member)) + if (isMACsecFeatureEnabled()) { - it++; - continue; + if (isMACsecSetted(member) && !isMACsecStateOk(member)) + { + it++; + continue; + } } if (addLagMember(lag, member) == task_need_retry) diff --git a/cfgmgr/teammgr.h b/cfgmgr/teammgr.h index 8ce81a3e1a..8ce1db776a 100644 --- a/cfgmgr/teammgr.h +++ b/cfgmgr/teammgr.h @@ -28,6 +28,7 @@ class TeamMgr : public Orch Table m_statePortTable; Table m_stateLagTable; Table m_stateMACsecPortTable; + Table m_stateFeatureTable; ProducerStateTable m_appPortTable; ProducerStateTable m_appLagTable; @@ -56,6 +57,7 @@ class TeamMgr : public Orch bool checkPortIffUp(const std::string &); bool isPortStateOk(const std::string&); bool isLagStateOk(const std::string&); + bool isMACsecFeatureEnabled(); bool isMACsecSetted(const std::string &); bool isMACsecStateOk(const std::string &); uint16_t generateLacpKey(const std::string&); diff --git a/tests/test_macsec.py b/tests/test_macsec.py index ef60a4b3d6..3b95b3ab68 100644 --- a/tests/test_macsec.py +++ b/tests/test_macsec.py @@ -765,7 +765,8 @@ def test_macsec_attribute_change(self, dvs: conftest.DockerVirtualSwitch, testlo def test_macsec_with_portchannel(self, dvs: conftest.DockerVirtualSwitch, testlog): # Set MACsec enabled on Ethernet0 - ConfigTable(dvs, "PORT")["Ethernet0"] = {"macsec": "test"} + 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"} @@ -831,6 +832,18 @@ def test_macsec_with_portchannel(self, dvs: conftest.DockerVirtualSwitch, testlo 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 # before retrying From 4f12fddf9b0768c2866c139879542cfe5a0a0dea Mon Sep 17 00:00:00 2001 From: Judy Joseph Date: Tue, 14 Jun 2022 12:42:32 -0700 Subject: [PATCH 4/6] Update to wait till the Ingress SA entry is created in the STATEDB. Earlier we were checking for Macsec Port creation. --- cfgmgr/teammgr.cpp | 37 ++++++++++++++++++++++++++++--------- cfgmgr/teammgr.h | 6 +++--- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index be9b3ebe34..ebe28b2401 100644 --- a/cfgmgr/teammgr.cpp +++ b/cfgmgr/teammgr.cpp @@ -34,7 +34,7 @@ TeamMgr::TeamMgr(DBConnector *confDb, DBConnector *applDb, DBConnector *statDb, m_appLagTable(applDb, APP_LAG_TABLE_NAME), m_statePortTable(statDb, STATE_PORT_TABLE_NAME), m_stateLagTable(statDb, STATE_LAG_TABLE_NAME), - m_stateMACsecPortTable(statDb, STATE_MACSEC_PORT_TABLE_NAME), + m_stateMACsecIngressSATable(statDb, STATE_MACSEC_INGRESS_SA_TABLE_NAME), m_stateFeatureTable(statDb, "FEATURE") { SWSS_LOG_ENTER(); @@ -121,7 +121,7 @@ bool TeamMgr::isMACsecFeatureEnabled() return true; } -bool TeamMgr::isMACsecSetted(const std::string &port) +bool TeamMgr::isMACsecAttached(const std::string &port) { SWSS_LOG_ENTER(); @@ -143,19 +143,27 @@ bool TeamMgr::isMACsecSetted(const std::string &port) return true; } -bool TeamMgr::isMACsecStateOk(const std::string &port) +bool TeamMgr::isMACsecIngressSAOk(const std::string &port) { SWSS_LOG_ENTER(); - vector temp; + vector keys; + m_stateMACsecIngressSATable.getKeys(keys); - if (!m_stateMACsecPortTable.get(port, temp)) + for (auto key: keys) { - SWSS_LOG_INFO("MACsec isn't ready on the port %s", port.c_str()); - return false; + 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; + } } - return true; + SWSS_LOG_INFO("MACsec is NOT ready on the port %s", port.c_str()); + return false; } void TeamMgr::doTask(Consumer &consumer) @@ -372,7 +380,7 @@ void TeamMgr::doLagMemberTask(Consumer &consumer) if (isMACsecFeatureEnabled()) { - if (isMACsecSetted(member) && !isMACsecStateOk(member)) + if (isMACsecAttached(member) && !isMACsecIngressSAOk(member)) { it++; continue; @@ -469,6 +477,17 @@ void TeamMgr::doPortUpdateTask(Consumer &consumer) string lag; if (findPortMaster(lag, alias)) { + if (isMACsecFeatureEnabled()) + { + if (isMACsecAttached(alias) && !isMACsecIngressSAOk(alias)) + { + it++; + SWSS_LOG_INFO("MACsec is NOT ready on the port %s", alias.c_str()); + continue; + } + SWSS_LOG_NOTICE("MACsec is ready on the port %s", alias.c_str()); + } + if (addLagMember(lag, alias) == task_need_retry) { it++; diff --git a/cfgmgr/teammgr.h b/cfgmgr/teammgr.h index 8ce1db776a..5529ccf0a1 100644 --- a/cfgmgr/teammgr.h +++ b/cfgmgr/teammgr.h @@ -27,7 +27,7 @@ class TeamMgr : public Orch Table m_cfgLagMemberTable; Table m_statePortTable; Table m_stateLagTable; - Table m_stateMACsecPortTable; + Table m_stateMACsecIngressSATable; Table m_stateFeatureTable; ProducerStateTable m_appPortTable; @@ -58,8 +58,8 @@ class TeamMgr : public Orch bool isPortStateOk(const std::string&); bool isLagStateOk(const std::string&); bool isMACsecFeatureEnabled(); - bool isMACsecSetted(const std::string &); - bool isMACsecStateOk(const std::string &); + bool isMACsecAttached(const std::string &); + bool isMACsecIngressSAOk(const std::string &); uint16_t generateLacpKey(const std::string&); }; From 9a2f9f97437898911e4ba439c5d06ef2bd3042ec Mon Sep 17 00:00:00 2001 From: Judy Joseph Date: Tue, 21 Jun 2022 19:29:15 -0700 Subject: [PATCH 5/6] Update to remove the check for feature table. Just check for macsec attached to interface is enough. Can later plan to introduce the yang model validation for macsec leaf in PORT if needed. --- cfgmgr/teammgr.cpp | 48 +++++++++------------------------------------- cfgmgr/teammgr.h | 2 -- 2 files changed, 9 insertions(+), 41 deletions(-) diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index ebe28b2401..71a4659496 100644 --- a/cfgmgr/teammgr.cpp +++ b/cfgmgr/teammgr.cpp @@ -34,8 +34,7 @@ TeamMgr::TeamMgr(DBConnector *confDb, DBConnector *applDb, DBConnector *statDb, m_appLagTable(applDb, APP_LAG_TABLE_NAME), m_statePortTable(statDb, STATE_PORT_TABLE_NAME), m_stateLagTable(statDb, STATE_LAG_TABLE_NAME), - m_stateMACsecIngressSATable(statDb, STATE_MACSEC_INGRESS_SA_TABLE_NAME), - m_stateFeatureTable(statDb, "FEATURE") + m_stateMACsecIngressSATable(statDb, STATE_MACSEC_INGRESS_SA_TABLE_NAME) { SWSS_LOG_ENTER(); @@ -100,27 +99,6 @@ bool TeamMgr::isLagStateOk(const string &alias) return true; } -bool TeamMgr::isMACsecFeatureEnabled() -{ - SWSS_LOG_ENTER(); - - vector temp; - if (!m_stateFeatureTable.get("macsec", temp)) - { - return false; - } - - auto opt = swss::fvsGetValue(temp, "state", true); - - if (!opt || *opt != "enabled") - { - SWSS_LOG_INFO("MACsec feature isn't enabled"); - return false; - } - - return true; -} - bool TeamMgr::isMACsecAttached(const std::string &port) { SWSS_LOG_ENTER(); @@ -377,16 +355,11 @@ void TeamMgr::doLagMemberTask(Consumer &consumer) it++; continue; } - - if (isMACsecFeatureEnabled()) + if (isMACsecAttached(member) && !isMACsecIngressSAOk(member)) { - if (isMACsecAttached(member) && !isMACsecIngressSAOk(member)) - { - it++; - continue; - } + it++; + continue; } - if (addLagMember(lag, member) == task_need_retry) { it++; @@ -477,16 +450,13 @@ void TeamMgr::doPortUpdateTask(Consumer &consumer) string lag; if (findPortMaster(lag, alias)) { - if (isMACsecFeatureEnabled()) + if (isMACsecAttached(alias) && !isMACsecIngressSAOk(alias)) { - if (isMACsecAttached(alias) && !isMACsecIngressSAOk(alias)) - { - it++; - SWSS_LOG_INFO("MACsec is NOT ready on the port %s", alias.c_str()); - continue; - } - SWSS_LOG_NOTICE("MACsec is ready on the port %s", alias.c_str()); + it++; + SWSS_LOG_INFO("MACsec is NOT ready on the port %s", alias.c_str()); + continue; } + SWSS_LOG_NOTICE("MACsec is ready on the port %s", alias.c_str()); if (addLagMember(lag, alias) == task_need_retry) { diff --git a/cfgmgr/teammgr.h b/cfgmgr/teammgr.h index 5529ccf0a1..db87fdd1f4 100644 --- a/cfgmgr/teammgr.h +++ b/cfgmgr/teammgr.h @@ -28,7 +28,6 @@ class TeamMgr : public Orch Table m_statePortTable; Table m_stateLagTable; Table m_stateMACsecIngressSATable; - Table m_stateFeatureTable; ProducerStateTable m_appPortTable; ProducerStateTable m_appLagTable; @@ -57,7 +56,6 @@ class TeamMgr : public Orch bool checkPortIffUp(const std::string &); bool isPortStateOk(const std::string&); bool isLagStateOk(const std::string&); - bool isMACsecFeatureEnabled(); bool isMACsecAttached(const std::string &); bool isMACsecIngressSAOk(const std::string &); uint16_t generateLacpKey(const std::string&); From 182ba68d0df442d72bdfdf1ba9ef7380d0caf957 Mon Sep 17 00:00:00 2001 From: Judy Joseph Date: Thu, 23 Jun 2022 14:59:04 -0700 Subject: [PATCH 6/6] Remove unwanted log message --- cfgmgr/teammgr.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index 71a4659496..31f911741c 100644 --- a/cfgmgr/teammgr.cpp +++ b/cfgmgr/teammgr.cpp @@ -456,7 +456,6 @@ void TeamMgr::doPortUpdateTask(Consumer &consumer) SWSS_LOG_INFO("MACsec is NOT ready on the port %s", alias.c_str()); continue; } - SWSS_LOG_NOTICE("MACsec is ready on the port %s", alias.c_str()); if (addLagMember(lag, alias) == task_need_retry) {