diff --git a/orchagent/macsecorch.cpp b/orchagent/macsecorch.cpp index 51febf62ea..638fdf38ac 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->setLagMemberMacsecSaActive(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->setLagMemberMacsecSaActive(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 6673f70101..711586301f 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 914587b4b4..fa49d93a30 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -213,6 +213,18 @@ 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; + /* 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. 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; sai_object_id_t m_ingress_acl_table_group_id = 0; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index e684119d94..49b6ad5355 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,89 @@ 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). + */ + setLagMemberMacsecSaActive(p, !enabled); +} + +void PortsOrch::setLagMemberMacsecSaActive(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. 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()) + { + it->second.m_macsec_sa_active = enabled; + } + + /* 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) + { + /* 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). */ + 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 + { + /* 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()); + } } bool PortsOrch::isMACsecPort(sai_object_id_t port_id) const diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index e0d392547c..ca2563ceb4 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 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 cb39122f88..afe0e9f4ca 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -4888,6 +4888,313 @@ namespace portsorch_test ASSERT_FALSE(lagMemberCreateCalled); } + /* + * Verify the MACsec / LAG-member data-plane interaction: + * - 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. + * - setLagMemberMacsecSaActive(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->setLagMemberMacsecSaActive(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->setLagMemberMacsecSaActive(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->setLagMemberMacsecSaActive(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; + } + + /* + * 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. + */ + 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->setLagMemberMacsecSaActive(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; + } + + /* + * 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->setLagMemberMacsecSaActive(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->setLagMemberMacsecSaActive(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->setLagMemberMacsecSaActive(rapid, false); + ASSERT_TRUE(gPortsOrch->getPort(memberAlias, rapid)); + ASSERT_FALSE(rapid.m_macsec_sa_active) << "rapid down " << i; + + 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->setLagMemberMacsecSaActive(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