From 24fb817d0a4153631ab981bba6472158d9f376e1 Mon Sep 17 00:00:00 2001 From: karthik-nexthop Date: Fri, 10 Jul 2026 12:16:17 +0530 Subject: [PATCH 1/4] Disable LAG member forwarding on MACsec session down Signed-off-by: karthik-nexthop --- orchagent/macsecorch.cpp | 53 +++++++++ orchagent/macsecorch.h | 3 + orchagent/port.h | 15 +++ orchagent/portsorch.cpp | 82 ++++++++++++++ orchagent/portsorch.h | 1 + tests/mock_tests/portsorch_ut.cpp | 177 ++++++++++++++++++++++++++++++ 6 files changed, 331 insertions(+) diff --git a/orchagent/macsecorch.cpp b/orchagent/macsecorch.cpp index 51febf62ea9..d288dabe27a 100644 --- a/orchagent/macsecorch.cpp +++ b/orchagent/macsecorch.cpp @@ -2379,6 +2379,21 @@ task_process_status MACsecOrch::createMACsecSA( SWSS_LOG_NOTICE("MACsec SA %s is created.", port_sci_an.c_str()); recover.clear(); + + // If the port is a LAG member, clear the MACsec-down suppression only once + // the MACsec data plane is up in both directions. MKA installs the egress + // SA before the ingress SA, so gating on both prevents lifting suppression + // while inbound traffic would still fail ICV validation. teamsyncd drives + // the actual SAI re-enable once LACP selects the member. + if (bothDirectionsUp(*ctx.get_macsec_port())) + { + Port port; + if (m_port_orch->getPort(port_name, port)) + { + m_port_orch->setLagMemberState(port, true); + } + } + return task_success; } @@ -2425,6 +2440,20 @@ task_process_status MACsecOrch::deleteMACsecSA( SWSS_LOG_WARN("Cannot change the ACL entry action from MACsec flow to packet action"); result = task_failed; } + + // Disable the LAG member only once the MACsec data plane is fully down in + // both directions. Deleting the last SA on one SC while the other + // direction still has active SAs (asymmetric teardown / rekey) must not + // drop the member prematurely -- symmetric with createMACsecSA gating on + // bothDirectionsUp(). + if (bothDirectionsDown(*ctx.get_macsec_port())) + { + Port port; + if (m_port_orch->getPort(port_name, port)) + { + m_port_orch->setLagMemberState(port, false); + } + } } @@ -2441,6 +2470,30 @@ task_process_status MACsecOrch::deleteMACsecSA( return result; } +bool MACsecOrch::bothDirectionsUp(const MACsecPort &macsec_port) const +{ + return hasActiveSaInDirection(macsec_port.m_egress_scs) && + hasActiveSaInDirection(macsec_port.m_ingress_scs); +} + +bool MACsecOrch::bothDirectionsDown(const MACsecPort &macsec_port) const +{ + return !hasActiveSaInDirection(macsec_port.m_egress_scs) && + !hasActiveSaInDirection(macsec_port.m_ingress_scs); +} + +bool MACsecOrch::hasActiveSaInDirection(const std::map &scs) const +{ + for (const auto &sc : scs) + { + if (!sc.second.m_sa_ids.empty()) + { + return true; + } + } + return false; +} + bool MACsecOrch::createMACsecSA( sai_object_id_t &sa_id, sai_object_id_t switch_id, diff --git a/orchagent/macsecorch.h b/orchagent/macsecorch.h index 6673f701018..711586301fe 100644 --- a/orchagent/macsecorch.h +++ b/orchagent/macsecorch.h @@ -281,6 +281,9 @@ class MACsecOrch : public Orch sai_object_id_t entry_id, sai_object_id_t flow_id, bool active); + bool bothDirectionsUp(const MACsecPort &macsec_port) const; + bool bothDirectionsDown(const MACsecPort &macsec_port) const; + bool hasActiveSaInDirection(const std::map &scs) const; bool deleteMACsecACLEntry(sai_object_id_t entry_id); bool getAclPriority( sai_object_id_t switch_id, diff --git a/orchagent/port.h b/orchagent/port.h index 914587b4b4a..7ef878e925d 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -213,6 +213,21 @@ class Port sai_object_id_t m_hif_id = 0; sai_object_id_t m_lag_id = 0; sai_object_id_t m_lag_member_id = 0; +<<<<<<< HEAD +======= + /* PHY port admin state is overriden by parent LAG admin-down */ + bool m_lag_forced_admin_down = false; + /* MACsec data-plane state for a LAG member. Set false when the last + * MACsec SA on the port is torn down (session timeout) so a teamsyncd + * refresh of APP_LAG_MEMBER_TABLE does not silently re-enable the member + * while MACsec is down. + * + * Known limitation: this intent is in-memory only and defaults true. If + * orchagent/swss restarts (e.g. warm reboot) while MACsec is down, the + * member comes back enabled before its SAs are re-established. It is not + * reconciled from STATE_DB MACsec SA presence on init. */ + bool m_macsec_sa_active = true; +>>>>>>> 2a3f104c (NOS-10638: Disable LAG member forwarding on MACsec session down (#717)) sai_object_id_t m_tunnel_id = 0; sai_object_id_t m_nexthop_group_id = 0; sai_object_id_t m_ingress_acl_table_group_id = 0; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index e684119d941..6751ad23fac 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -6519,6 +6519,16 @@ void PortsOrch::doLagMemberTask(Consumer &consumer) /* Sync an enabled member */ if (status == "enabled") { + /* If the MACsec data plane on this member is down, suppress the + * teamsyncd-driven re-enable. MACsec controls collection and + * distribution directly until its SAs are re-established. */ + if (!port.m_macsec_sa_active) + { + SWSS_LOG_NOTICE("Skip enabling LAG member %s: MACsec SA inactive", + port.m_alias.c_str()); + it = consumer.m_toSync.erase(it); + continue; + } /* enable collection first, distribution-only mode * is not supported on Mellanox platform */ @@ -11560,6 +11570,78 @@ void PortsOrch::setMACsecEnabledState(sai_object_id_t port_id, bool enabled) { setPortMtu(p, p.m_mtu); } + + /* + * When MACsec is enabled on a port, the MACsec hardware will drop traffic + * until the SAs are established. Thus, the MACsec data plane is considered + * down (false). When MACsec is disabled on the port, the port returns to + * normal cleartext forwarding, so the MACsec data plane constraint is lifted (true). + */ + setLagMemberState(p, !enabled); +} + +void PortsOrch::setLagMemberState(Port &port, bool enabled) +{ + SWSS_LOG_ENTER(); + + /* Nothing to do if the intent is unchanged. Both MACsec SCs going empty on + * a session timeout would otherwise drive a redundant disable (and a + * duplicate SAI write + log notice) per direction. */ + if (port.m_macsec_sa_active == enabled) + { + return; + } + + /* Persist the MACsec data-plane intent so that a later teamsyncd refresh + * of APP_LAG_MEMBER_TABLE (handled in doLagMemberTask) does not silently + * re-enable the member while MACsec is down. Always update this, including + * for ports that are not yet (or no longer) LAG members. */ + port.m_macsec_sa_active = enabled; + auto it = m_portList.find(port.m_alias); + if (it != m_portList.end()) + { + it->second.m_macsec_sa_active = enabled; + } + + const bool is_lag_member = (port.m_lag_member_id != SAI_NULL_OBJECT_ID); + + if (!enabled && is_lag_member) + { + /* Flap the host interface oper status to force teamd to instantly drop + * the LAG member (bypassing the 90s LACP timeout) without permanently + * holding carrier down (which would block wpa_supplicant EAPOL). Only + * flap LAG members -- doing so on a standalone port would drop routing + * adjacencies. */ + if (port.m_oper_status == SAI_PORT_OPER_STATUS_UP) + { + SWSS_LOG_NOTICE("Flapping host interface %s to force teamd LACP reset due to MACsec down", + port.m_alias.c_str()); + setHostIntfsOperStatus(port, false); + setHostIntfsOperStatus(port, true); + } + + /* Disable collection/distribution directly via SAI rather than writing + * APP_LAG_MEMBER_TABLE, to avoid a write race with teamsyncd. */ + bool distribution_ok = setDistributionOnLagMember(port, false); + bool collection_ok = setCollectionOnLagMember(port, false); + + if (!collection_ok || !distribution_ok) + { + SWSS_LOG_ERROR("Failed to disable collection/distribution on LAG member %s", + port.m_alias.c_str()); + return; + } + + SWSS_LOG_NOTICE("MACsec disabled LAG member %s", port.m_alias.c_str()); + } + else if (enabled && is_lag_member) + { + /* MACsec data plane is up again. Clear the suppression flag and let + * teamsyncd's status=enabled refresh drive SAI re-enable once LACP + * completes, avoiding hashing to a member before teamd selects it. */ + SWSS_LOG_NOTICE("MACsec SA active on %s; awaiting teamsyncd to re-enable LAG member", + port.m_alias.c_str()); + } } bool PortsOrch::isMACsecPort(sai_object_id_t port_id) const diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index e0d392547c3..c0829492183 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -295,6 +295,7 @@ class PortsOrch : public Orch, public Subject bool decrFdbCount(const string& alias, int count); void setMACsecEnabledState(sai_object_id_t port_id, bool enabled); + void setLagMemberState(Port &port, bool enabled); bool isMACsecPort(sai_object_id_t port_id) const; vector getPortVoQIds(Port& port); bool isFrontPanelPort(Port& port); diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index cb39122f882..5f1d87b7b03 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -4888,6 +4888,183 @@ namespace portsorch_test ASSERT_FALSE(lagMemberCreateCalled); } + /* + * Verify the MACsec / LAG-member data-plane interaction: + * - setLagMemberState(false) disables collection + distribution on the + * member via SAI and records the MACsec-down intent. + * - A subsequent teamsyncd "enabled" refresh on APP_LAG_MEMBER_TABLE does + * NOT re-enable the member while MACsec is down. + * - setLagMemberState(true) clears the suppression flag and teamsyncd drives + * the SAI re-enable (no direct SAI enable on recovery). + */ + TEST_F(PortsOrchTest, MacsecDownDisablesLagMemberAndSuppressesTeamdReEnable) + { + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + Table lagTable = Table(m_app_db.get(), APP_LAG_TABLE_NAME); + Table lagMemberTable = Table(m_app_db.get(), APP_LAG_MEMBER_TABLE_NAME); + + auto ports = ut_helper::getInitialSaiPorts(); + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + portTable.set("PortInitDone", { { } }); + + lagTable.set("PortChannel999", { {"admin_status", "up"}, {"mtu", "9100"} }); + + const std::string memberAlias = ports.begin()->first; + const std::string memberKey = + std::string("PortChannel999") + lagMemberTable.getTableNameSeparator() + memberAlias; + lagMemberTable.set(memberKey, { {"status", "enabled"} }); + + gPortsOrch->addExistingData(&portTable); + gPortsOrch->addExistingData(&lagTable); + gPortsOrch->addExistingData(&lagMemberTable); + static_cast(gPortsOrch)->doTask(); + + // The member should now exist and have a LAG member id. + Port member; + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, member)); + ASSERT_NE(member.m_lag_member_id, SAI_NULL_OBJECT_ID); + ASSERT_TRUE(member.m_macsec_sa_active); + + // Spy on set_lag_member_attribute to capture EGRESS/INGRESS disable values. + auto orig_lag_api = sai_lag_api; + sai_lag_api = new sai_lag_api_t(); + memcpy(sai_lag_api, orig_lag_api, sizeof(*sai_lag_api)); + + bool egressDisable = false, ingressDisable = false; + int setAttrCalls = 0; + auto lagSpy = SpyOn(&sai_lag_api->set_lag_member_attribute); + lagSpy->callFake([&](sai_object_id_t oid, const sai_attribute_t *attr) -> sai_status_t + { + setAttrCalls++; + if (attr->id == SAI_LAG_MEMBER_ATTR_EGRESS_DISABLE) + egressDisable = attr->value.booldata; + else if (attr->id == SAI_LAG_MEMBER_ATTR_INGRESS_DISABLE) + ingressDisable = attr->value.booldata; + return orig_lag_api->set_lag_member_attribute(oid, attr); + } + ); + + // --- MACsec session down: disable the member directly --- + gPortsOrch->setLagMemberState(member, false); + ASSERT_TRUE(egressDisable); + ASSERT_TRUE(ingressDisable); + + Port afterDown; + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, afterDown)); + ASSERT_FALSE(afterDown.m_macsec_sa_active); + + // --- redundant disable (both SCs empty) must be a no-op --- + setAttrCalls = 0; + gPortsOrch->setLagMemberState(afterDown, false); + ASSERT_EQ(setAttrCalls, 0); + + // --- teamsyncd refresh: a status=enabled write must be suppressed --- + setAttrCalls = 0; + lagMemberTable.set(memberKey, { {"status", "enabled"} }); + gPortsOrch->addExistingData(&lagMemberTable); + static_cast(gPortsOrch)->doTask(); + + // No SAI re-enable should have happened, and the task should be drained. + ASSERT_EQ(setAttrCalls, 0); + { + vector ts; + auto exec = gPortsOrch->getExecutor(APP_LAG_MEMBER_TABLE_NAME); + auto consumer = static_cast(exec); + consumer->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + } + + // --- MACsec session restored: clear suppression, teamsyncd re-enables --- + setAttrCalls = 0; + gPortsOrch->setLagMemberState(afterDown, true); + ASSERT_EQ(setAttrCalls, 0) << "Recovery must not SAI-enable directly"; + + Port afterFlagSet; + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, afterFlagSet)); + ASSERT_TRUE(afterFlagSet.m_macsec_sa_active); + + egressDisable = ingressDisable = true; + lagMemberTable.set(memberKey, { {"status", "enabled"} }); + gPortsOrch->addExistingData(&lagMemberTable); + static_cast(gPortsOrch)->doTask(); + + ASSERT_FALSE(egressDisable); + ASSERT_FALSE(ingressDisable); + + sai_lag_api = orig_lag_api; + } + + /* + * setLagMemberState(false) on a non-LAG port must only persist + * m_macsec_sa_active and must not flap the host interface or touch SAI LAG + * member attributes. + */ + TEST_F(PortsOrchTest, MacsecDownDoesNotFlapNonLagPort) + { + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + + auto ports = ut_helper::getInitialSaiPorts(); + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + portTable.set("PortInitDone", { { } }); + + gPortsOrch->addExistingData(&portTable); + static_cast(gPortsOrch)->doTask(); + + const std::string portAlias = ports.begin()->first; + Port port; + ASSERT_TRUE(gPortsOrch->getPort(portAlias, port)); + ASSERT_EQ(port.m_lag_member_id, SAI_NULL_OBJECT_ID); + ASSERT_TRUE(port.m_macsec_sa_active); + + auto orig_hostif_api = sai_hostif_api; + sai_hostif_api = new sai_hostif_api_t(); + memcpy(sai_hostif_api, orig_hostif_api, sizeof(*sai_hostif_api)); + + auto orig_lag_api = sai_lag_api; + sai_lag_api = new sai_lag_api_t(); + memcpy(sai_lag_api, orig_lag_api, sizeof(*sai_lag_api)); + + int hostifOperStatusCalls = 0; + int lagMemberAttrCalls = 0; + auto hostifSpy = SpyOn(&sai_hostif_api->set_hostif_attribute); + hostifSpy->callFake([&](sai_object_id_t oid, const sai_attribute_t *attr) -> sai_status_t + { + if (attr->id == SAI_HOSTIF_ATTR_OPER_STATUS) + { + hostifOperStatusCalls++; + } + return orig_hostif_api->set_hostif_attribute(oid, attr); + } + ); + + auto lagSpy = SpyOn(&sai_lag_api->set_lag_member_attribute); + lagSpy->callFake([&](sai_object_id_t oid, const sai_attribute_t *attr) -> sai_status_t + { + lagMemberAttrCalls++; + return orig_lag_api->set_lag_member_attribute(oid, attr); + } + ); + + gPortsOrch->setLagMemberState(port, false); + + Port afterDown; + ASSERT_TRUE(gPortsOrch->getPort(portAlias, afterDown)); + ASSERT_FALSE(afterDown.m_macsec_sa_active); + ASSERT_EQ(hostifOperStatusCalls, 0); + ASSERT_EQ(lagMemberAttrCalls, 0); + + sai_hostif_api = orig_hostif_api; + sai_lag_api = orig_lag_api; + } + /* * The scope of this test is a negative test which verify that: * if port operational status is up but operational speed is 0, the port speed should not be From e799b7dc573aea12f5f0bd1c7d8dbdf6650805f7 Mon Sep 17 00:00:00 2001 From: Karthik Siruvalam Date: Fri, 10 Jul 2026 10:59:21 +0000 Subject: [PATCH 2/4] Resolve cherry-pick conflict in orchagent/port.h Keep m_lag_forced_admin_down and m_macsec_sa_active Port members added for MACsec LAG member forwarding control. Signed-off-by: Karthik Siruvalam --- orchagent/port.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/orchagent/port.h b/orchagent/port.h index 7ef878e925d..0d288c0f462 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -213,8 +213,6 @@ class Port sai_object_id_t m_hif_id = 0; sai_object_id_t m_lag_id = 0; sai_object_id_t m_lag_member_id = 0; -<<<<<<< HEAD -======= /* PHY port admin state is overriden by parent LAG admin-down */ bool m_lag_forced_admin_down = false; /* MACsec data-plane state for a LAG member. Set false when the last @@ -227,7 +225,6 @@ class Port * member comes back enabled before its SAs are re-established. It is not * reconciled from STATE_DB MACsec SA presence on init. */ bool m_macsec_sa_active = true; ->>>>>>> 2a3f104c (NOS-10638: Disable LAG member forwarding on MACsec session down (#717)) sai_object_id_t m_tunnel_id = 0; sai_object_id_t m_nexthop_group_id = 0; sai_object_id_t m_ingress_acl_table_group_id = 0; From 3e304e9b515d5058a1d7886618503abacf69e468 Mon Sep 17 00:00:00 2001 From: Karthik Siruvalam Date: Mon, 27 Jul 2026 06:46:02 +0000 Subject: [PATCH 3/4] [macsec] Add continuous flap race UT; drop unused Port member Add MacsecContinuousFlapNoAppDbSaiRace to cover APP_DB/SAI races under repeated MACsec session flaps. Remove unused m_lag_forced_admin_down left over from a cherry-pick conflict (not present on upstream master). Signed-off-by: Karthik Siruvalam --- orchagent/port.h | 2 - tests/mock_tests/portsorch_ut.cpp | 130 ++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 2 deletions(-) diff --git a/orchagent/port.h b/orchagent/port.h index 0d288c0f462..483f1010f75 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -213,8 +213,6 @@ class Port sai_object_id_t m_hif_id = 0; sai_object_id_t m_lag_id = 0; sai_object_id_t m_lag_member_id = 0; - /* PHY port admin state is overriden by parent LAG admin-down */ - bool m_lag_forced_admin_down = false; /* MACsec data-plane state for a LAG member. Set false when the last * MACsec SA on the port is torn down (session timeout) so a teamsyncd * refresh of APP_LAG_MEMBER_TABLE does not silently re-enable the member diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 5f1d87b7b03..3a84c1e0891 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -5065,6 +5065,136 @@ namespace portsorch_test sai_lag_api = orig_lag_api; } + /* + * Continuous MACsec session flaps must not race with teamsyncd APP_DB + * refreshes: while MACsec is down, status=enabled must stay suppressed at + * SAI; after each recovery, teamsyncd may re-enable collection/distribution. + * Rapid down/up cycles exercise the in-memory m_macsec_sa_active guard + * against stale APP_LAG_MEMBER_TABLE enables. + */ + TEST_F(PortsOrchTest, MacsecContinuousFlapNoAppDbSaiRace) + { + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + Table lagTable = Table(m_app_db.get(), APP_LAG_TABLE_NAME); + Table lagMemberTable = Table(m_app_db.get(), APP_LAG_MEMBER_TABLE_NAME); + + auto ports = ut_helper::getInitialSaiPorts(); + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + portTable.set("PortInitDone", { { } }); + + lagTable.set("PortChannel999", { {"admin_status", "up"}, {"mtu", "9100"} }); + + const std::string memberAlias = ports.begin()->first; + const std::string memberKey = + std::string("PortChannel999") + lagMemberTable.getTableNameSeparator() + memberAlias; + lagMemberTable.set(memberKey, { {"status", "enabled"} }); + + gPortsOrch->addExistingData(&portTable); + gPortsOrch->addExistingData(&lagTable); + gPortsOrch->addExistingData(&lagMemberTable); + static_cast(gPortsOrch)->doTask(); + + Port member; + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, member)); + ASSERT_NE(member.m_lag_member_id, SAI_NULL_OBJECT_ID); + ASSERT_TRUE(member.m_macsec_sa_active); + + auto orig_lag_api = sai_lag_api; + sai_lag_api = new sai_lag_api_t(); + memcpy(sai_lag_api, orig_lag_api, sizeof(*sai_lag_api)); + + bool egressDisable = false, ingressDisable = false; + int setAttrCalls = 0; + auto lagSpy = SpyOn(&sai_lag_api->set_lag_member_attribute); + lagSpy->callFake([&](sai_object_id_t oid, const sai_attribute_t *attr) -> sai_status_t + { + setAttrCalls++; + if (attr->id == SAI_LAG_MEMBER_ATTR_EGRESS_DISABLE) + egressDisable = attr->value.booldata; + else if (attr->id == SAI_LAG_MEMBER_ATTR_INGRESS_DISABLE) + ingressDisable = attr->value.booldata; + return orig_lag_api->set_lag_member_attribute(oid, attr); + } + ); + + constexpr int kFlapCycles = 25; + for (int i = 0; i < kFlapCycles; i++) + { + Port cur; + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, cur)) << "cycle " << i; + + // MACsec session down: SAI-disable member and suppress teamsyncd enable. + egressDisable = ingressDisable = false; + setAttrCalls = 0; + gPortsOrch->setLagMemberState(cur, false); + ASSERT_TRUE(egressDisable) << "cycle " << i; + ASSERT_TRUE(ingressDisable) << "cycle " << i; + + Port afterDown; + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, afterDown)) << "cycle " << i; + ASSERT_FALSE(afterDown.m_macsec_sa_active) << "cycle " << i; + + setAttrCalls = 0; + lagMemberTable.set(memberKey, { {"status", "enabled"} }); + gPortsOrch->addExistingData(&lagMemberTable); + static_cast(gPortsOrch)->doTask(); + ASSERT_EQ(setAttrCalls, 0) << "teamsyncd enable leaked while MACsec down, cycle " << i; + { + vector ts; + auto exec = gPortsOrch->getExecutor(APP_LAG_MEMBER_TABLE_NAME); + auto consumer = static_cast(exec); + consumer->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()) << "cycle " << i; + } + + // MACsec session up: clear flag only; teamsyncd drives SAI re-enable. + setAttrCalls = 0; + gPortsOrch->setLagMemberState(afterDown, true); + ASSERT_EQ(setAttrCalls, 0) << "recovery must not SAI-enable directly, cycle " << i; + + Port afterUp; + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, afterUp)) << "cycle " << i; + ASSERT_TRUE(afterUp.m_macsec_sa_active) << "cycle " << i; + + egressDisable = ingressDisable = true; + lagMemberTable.set(memberKey, { {"status", "enabled"} }); + gPortsOrch->addExistingData(&lagMemberTable); + static_cast(gPortsOrch)->doTask(); + ASSERT_FALSE(egressDisable) << "cycle " << i; + ASSERT_FALSE(ingressDisable) << "cycle " << i; + } + + // Rapid down/up without an intervening teamsyncd enable: final intent wins. + Port rapid; + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); + for (int i = 0; i < 10; i++) + { + gPortsOrch->setLagMemberState(rapid, false); + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); + ASSERT_FALSE(rapid.m_macsec_sa_active) << "rapid down " << i; + + gPortsOrch->setLagMemberState(rapid, true); + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); + ASSERT_TRUE(rapid.m_macsec_sa_active) << "rapid up " << i; + } + + // Stale enable after a final down must still be suppressed. + gPortsOrch->setLagMemberState(rapid, false); + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); + ASSERT_FALSE(rapid.m_macsec_sa_active); + setAttrCalls = 0; + lagMemberTable.set(memberKey, { {"status", "enabled"} }); + gPortsOrch->addExistingData(&lagMemberTable); + static_cast(gPortsOrch)->doTask(); + ASSERT_EQ(setAttrCalls, 0); + + sai_lag_api = orig_lag_api; + } + /* * The scope of this test is a negative test which verify that: * if port operational status is up but operational speed is 0, the port speed should not be From 5fd809320ef381814bc259a7ad0b4db1d986b325 Mon Sep 17 00:00:00 2001 From: Karthik Siruvalam Date: Thu, 6 Aug 2026 08:46:38 +0000 Subject: [PATCH 4/4] [macsec] Address review: rename LAG MACsec helper and clarify comments Rename setLagMemberState to setLagMemberMacsecSaActive, early-return for non-LAG members after persisting m_macsec_sa_active, and document the teamsyncd/doLagMemberTask re-enable path plus any-orchagent-restart limitation for the in-memory flag. Signed-off-by: Karthik Siruvalam --- orchagent/macsecorch.cpp | 4 ++-- orchagent/port.h | 10 +++++---- orchagent/portsorch.cpp | 35 ++++++++++++++++++++----------- orchagent/portsorch.h | 2 +- tests/mock_tests/portsorch_ut.cpp | 24 ++++++++++----------- 5 files changed, 44 insertions(+), 31 deletions(-) diff --git a/orchagent/macsecorch.cpp b/orchagent/macsecorch.cpp index d288dabe27a..638fdf38ac9 100644 --- a/orchagent/macsecorch.cpp +++ b/orchagent/macsecorch.cpp @@ -2390,7 +2390,7 @@ task_process_status MACsecOrch::createMACsecSA( Port port; if (m_port_orch->getPort(port_name, port)) { - m_port_orch->setLagMemberState(port, true); + m_port_orch->setLagMemberMacsecSaActive(port, true); } } @@ -2451,7 +2451,7 @@ task_process_status MACsecOrch::deleteMACsecSA( Port port; if (m_port_orch->getPort(port_name, port)) { - m_port_orch->setLagMemberState(port, false); + m_port_orch->setLagMemberMacsecSaActive(port, false); } } } diff --git a/orchagent/port.h b/orchagent/port.h index 483f1010f75..fa49d93a301 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -218,10 +218,12 @@ class Port * refresh of APP_LAG_MEMBER_TABLE does not silently re-enable the member * while MACsec is down. * - * Known limitation: this intent is in-memory only and defaults true. If - * orchagent/swss restarts (e.g. warm reboot) while MACsec is down, the - * member comes back enabled before its SAs are re-established. It is not - * reconciled from STATE_DB MACsec SA presence on init. */ + * Known limitation: this intent is in-memory only and defaults true. Any + * orchagent/swss restart loses the prior value, so until macsecorch + * rebuilds MACsec state (createMACsecPort -> setMACsecEnabledState(true) + * -> setLagMemberMacsecSaActive(false)), a teamsyncd status=enabled + * refresh is not suppressed. It is not reconciled from STATE_DB MACsec SA + * presence on init. */ bool m_macsec_sa_active = true; sai_object_id_t m_tunnel_id = 0; sai_object_id_t m_nexthop_group_id = 0; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 6751ad23fac..49b6ad5355b 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -11577,10 +11577,10 @@ void PortsOrch::setMACsecEnabledState(sai_object_id_t port_id, bool enabled) * down (false). When MACsec is disabled on the port, the port returns to * normal cleartext forwarding, so the MACsec data plane constraint is lifted (true). */ - setLagMemberState(p, !enabled); + setLagMemberMacsecSaActive(p, !enabled); } -void PortsOrch::setLagMemberState(Port &port, bool enabled) +void PortsOrch::setLagMemberMacsecSaActive(Port &port, bool enabled) { SWSS_LOG_ENTER(); @@ -11595,7 +11595,9 @@ void PortsOrch::setLagMemberState(Port &port, bool enabled) /* Persist the MACsec data-plane intent so that a later teamsyncd refresh * of APP_LAG_MEMBER_TABLE (handled in doLagMemberTask) does not silently * re-enable the member while MACsec is down. Always update this, including - * for ports that are not yet (or no longer) LAG members. */ + * for ports that are not yet (or no longer) LAG members. setMACsecEnabledState + * is shared for all MACsec ports; only hostif/SAI side effects below are + * LAG-member-specific. */ port.m_macsec_sa_active = enabled; auto it = m_portList.find(port.m_alias); if (it != m_portList.end()) @@ -11603,15 +11605,19 @@ void PortsOrch::setLagMemberState(Port &port, bool enabled) it->second.m_macsec_sa_active = enabled; } - const bool is_lag_member = (port.m_lag_member_id != SAI_NULL_OBJECT_ID); + /* Non-LAG ports: intent is recorded above; skip hostif flap and SAI LAG + * member attribute writes (flapping a standalone hostif would risk dropping + * routing adjacencies). */ + if (port.m_lag_member_id == SAI_NULL_OBJECT_ID) + { + return; + } - if (!enabled && is_lag_member) + if (!enabled) { /* Flap the host interface oper status to force teamd to instantly drop * the LAG member (bypassing the 90s LACP timeout) without permanently - * holding carrier down (which would block wpa_supplicant EAPOL). Only - * flap LAG members -- doing so on a standalone port would drop routing - * adjacencies. */ + * holding carrier down (which would block wpa_supplicant EAPOL). */ if (port.m_oper_status == SAI_PORT_OPER_STATUS_UP) { SWSS_LOG_NOTICE("Flapping host interface %s to force teamd LACP reset due to MACsec down", @@ -11634,11 +11640,16 @@ void PortsOrch::setLagMemberState(Port &port, bool enabled) SWSS_LOG_NOTICE("MACsec disabled LAG member %s", port.m_alias.c_str()); } - else if (enabled && is_lag_member) + else { - /* MACsec data plane is up again. Clear the suppression flag and let - * teamsyncd's status=enabled refresh drive SAI re-enable once LACP - * completes, avoiding hashing to a member before teamd selects it. */ + /* MACsec data plane is up again. This path only clears m_macsec_sa_active + * above; it does not call setCollectionOnLagMember / + * setDistributionOnLagMember. Re-enable is driven by teamsyncd + * (TeamPortSync::onChange writes APP_LAG_MEMBER_TABLE status=enabled + * when teamd selects the member). doLagMemberTask then calls + * setCollectionOnLagMember(true) and setDistributionOnLagMember(true) + * once LACP has completed, avoiding hashing to a member before teamd + * selects it. */ SWSS_LOG_NOTICE("MACsec SA active on %s; awaiting teamsyncd to re-enable LAG member", port.m_alias.c_str()); } diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index c0829492183..ca2563ceb4e 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -295,7 +295,7 @@ class PortsOrch : public Orch, public Subject bool decrFdbCount(const string& alias, int count); void setMACsecEnabledState(sai_object_id_t port_id, bool enabled); - void setLagMemberState(Port &port, bool enabled); + void setLagMemberMacsecSaActive(Port &port, bool enabled); bool isMACsecPort(sai_object_id_t port_id) const; vector getPortVoQIds(Port& port); bool isFrontPanelPort(Port& port); diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 3a84c1e0891..afe0e9f4ca1 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -4890,11 +4890,11 @@ namespace portsorch_test /* * Verify the MACsec / LAG-member data-plane interaction: - * - setLagMemberState(false) disables collection + distribution on the + * - setLagMemberMacsecSaActive(false) disables collection + distribution on the * member via SAI and records the MACsec-down intent. * - A subsequent teamsyncd "enabled" refresh on APP_LAG_MEMBER_TABLE does * NOT re-enable the member while MACsec is down. - * - setLagMemberState(true) clears the suppression flag and teamsyncd drives + * - setLagMemberMacsecSaActive(true) clears the suppression flag and teamsyncd drives * the SAI re-enable (no direct SAI enable on recovery). */ TEST_F(PortsOrchTest, MacsecDownDisablesLagMemberAndSuppressesTeamdReEnable) @@ -4949,7 +4949,7 @@ namespace portsorch_test ); // --- MACsec session down: disable the member directly --- - gPortsOrch->setLagMemberState(member, false); + gPortsOrch->setLagMemberMacsecSaActive(member, false); ASSERT_TRUE(egressDisable); ASSERT_TRUE(ingressDisable); @@ -4959,7 +4959,7 @@ namespace portsorch_test // --- redundant disable (both SCs empty) must be a no-op --- setAttrCalls = 0; - gPortsOrch->setLagMemberState(afterDown, false); + gPortsOrch->setLagMemberMacsecSaActive(afterDown, false); ASSERT_EQ(setAttrCalls, 0); // --- teamsyncd refresh: a status=enabled write must be suppressed --- @@ -4980,7 +4980,7 @@ namespace portsorch_test // --- MACsec session restored: clear suppression, teamsyncd re-enables --- setAttrCalls = 0; - gPortsOrch->setLagMemberState(afterDown, true); + gPortsOrch->setLagMemberMacsecSaActive(afterDown, true); ASSERT_EQ(setAttrCalls, 0) << "Recovery must not SAI-enable directly"; Port afterFlagSet; @@ -4999,7 +4999,7 @@ namespace portsorch_test } /* - * setLagMemberState(false) on a non-LAG port must only persist + * setLagMemberMacsecSaActive(false) on a non-LAG port must only persist * m_macsec_sa_active and must not flap the host interface or touch SAI LAG * member attributes. */ @@ -5053,7 +5053,7 @@ namespace portsorch_test } ); - gPortsOrch->setLagMemberState(port, false); + gPortsOrch->setLagMemberMacsecSaActive(port, false); Port afterDown; ASSERT_TRUE(gPortsOrch->getPort(portAlias, afterDown)); @@ -5130,7 +5130,7 @@ namespace portsorch_test // MACsec session down: SAI-disable member and suppress teamsyncd enable. egressDisable = ingressDisable = false; setAttrCalls = 0; - gPortsOrch->setLagMemberState(cur, false); + gPortsOrch->setLagMemberMacsecSaActive(cur, false); ASSERT_TRUE(egressDisable) << "cycle " << i; ASSERT_TRUE(ingressDisable) << "cycle " << i; @@ -5153,7 +5153,7 @@ namespace portsorch_test // MACsec session up: clear flag only; teamsyncd drives SAI re-enable. setAttrCalls = 0; - gPortsOrch->setLagMemberState(afterDown, true); + gPortsOrch->setLagMemberMacsecSaActive(afterDown, true); ASSERT_EQ(setAttrCalls, 0) << "recovery must not SAI-enable directly, cycle " << i; Port afterUp; @@ -5173,17 +5173,17 @@ namespace portsorch_test ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); for (int i = 0; i < 10; i++) { - gPortsOrch->setLagMemberState(rapid, false); + gPortsOrch->setLagMemberMacsecSaActive(rapid, false); ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); ASSERT_FALSE(rapid.m_macsec_sa_active) << "rapid down " << i; - gPortsOrch->setLagMemberState(rapid, true); + gPortsOrch->setLagMemberMacsecSaActive(rapid, true); ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); ASSERT_TRUE(rapid.m_macsec_sa_active) << "rapid up " << i; } // Stale enable after a final down must still be suppressed. - gPortsOrch->setLagMemberState(rapid, false); + gPortsOrch->setLagMemberMacsecSaActive(rapid, false); ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); ASSERT_FALSE(rapid.m_macsec_sa_active); setAttrCalls = 0;