From 0dfb8e7a3f32a409f9132b64b7a82e015e8c3d79 Mon Sep 17 00:00:00 2001 From: Sonic Build Admin Date: Mon, 12 Jan 2026 19:55:20 +0000 Subject: [PATCH] [gearsyncd,macsec]: Deterministic MACsec backend selection for gearbox ports **What I did** Introduce a platform capability flag in the gearbox config to determine, per PHY, whether MACsec is supported (applies to all ports mapped to that PHY). MACsec orchestration will: - Use PHY switch by default on gearbox ports - Use NPU/global switch only when the platform marks the PHY as not supporting MACsec Have added three DVS testcases: test_macsec_phy_switch_default: This tests the scenario when the macsec_supported field is absent in the gearbox_config.json test_macsec_phy_switch_explicit: This tests the scenario when the macsec_supported field is set as true in the gearbox_config.json test_macsec_npu_switch: This tests the scenario when the macsec_supported field is set as false in the gearbox_config.json **Why I did it** On gearbox ports, creating MACsec on the PHY switch fails (SAI_STATUS_NOT_IMPLEMENTED) if gearbox PHY does not have the MACsec engine. **How I verified it** Manually verified on DUT by adding macsec_supported=false in gearbox_config.json and configuring the macsec on the PHY port. Also ran the dvs testcase and made sure it is passing `sudo pytest -v tests/test_macsec_gearbox.py` **Details if related** HLD: https://github.com/sonic-net/SONiC/pull/2072 gearbox_config.json changes are posted here: https://github.com/sonic-net/sonic-buildimage/pull/24169/files#diff-737ea59a7eba8ea0ed71a15a052868815f7faad351fd353736ad196932bed57a Co-authored by @shreyansh-nexthop --- gearsyncd/gearboxparser.cpp | 6 ++ lib/gearboxutils.cpp | 7 ++ lib/gearboxutils.h | 1 + orchagent/macsecorch.cpp | 42 +++++++- tests/gearbox.py | 105 ++++++++++++++++++++ tests/macsec.py | 185 +++++++++++++++++++++++++++++++++++ tests/test_macsec_gearbox.py | 133 +++++++++++++++++++++++++ 7 files changed, 474 insertions(+), 5 deletions(-) create mode 100644 tests/gearbox.py create mode 100644 tests/macsec.py create mode 100644 tests/test_macsec_gearbox.py diff --git a/gearsyncd/gearboxparser.cpp b/gearsyncd/gearboxparser.cpp index 879624fd252..1760a449a87 100644 --- a/gearsyncd/gearboxparser.cpp +++ b/gearsyncd/gearboxparser.cpp @@ -158,6 +158,12 @@ bool GearboxParser::parse() attr = std::make_pair("macsec_ipg", std::to_string(val.get())); attrs.push_back(attr); } + if (phy.find("macsec_supported") != phy.end()) + { + val = phy["macsec_supported"]; + attr = std::make_pair("macsec_supported", val.get() ? "true" : "false"); + attrs.push_back(attr); + } if (phy.find("hwinfo") == phy.end()) { SWSS_LOG_ERROR("missing 'hwinfo' field in 'phys' item %d in gearbox configuration", iter); diff --git a/lib/gearboxutils.cpp b/lib/gearboxutils.cpp index bc35ed34568..c987b28bc8d 100644 --- a/lib/gearboxutils.cpp +++ b/lib/gearboxutils.cpp @@ -137,6 +137,9 @@ std::map GearboxUtils::loadPhyMap(Table *gearboxTable) { gearbox_phy_t phy = {}; + // default capability if absent + phy.macsec_supported = true; + gearboxTable->get(k, ovalues); for (auto &val : ovalues) { @@ -193,6 +196,10 @@ std::map GearboxUtils::loadPhyMap(Table *gearboxTable) { phy.macsec_ipg = std::stoi(val.second); } + else if (val.first == "macsec_supported") + { + phy.macsec_supported = (val.second == "true"); + } } gearboxPhyMap[phy.phy_id] = phy; } diff --git a/lib/gearboxutils.h b/lib/gearboxutils.h index a239aa3a10f..070e30c8a1d 100644 --- a/lib/gearboxutils.h +++ b/lib/gearboxutils.h @@ -64,6 +64,7 @@ typedef struct uint32_t bus_id; uint32_t context_id; uint32_t macsec_ipg; + bool macsec_supported; } gearbox_phy_t; typedef struct diff --git a/orchagent/macsecorch.cpp b/orchagent/macsecorch.cpp index dbce7eb1a8a..51febf62ea9 100644 --- a/orchagent/macsecorch.cpp +++ b/orchagent/macsecorch.cpp @@ -355,12 +355,25 @@ class MACsecOrchContext { return nullptr; } - if (port->m_line_side_id != SAI_NULL_OBJECT_ID) + + const auto *phy = get_gearbox_phy(); + bool force_npu = true; + if (phy) + { + force_npu = !phy->macsec_supported; + } + + if (!force_npu && port->m_line_side_id != SAI_NULL_OBJECT_ID) { m_port_id = std::make_unique(port->m_line_side_id); } else { + if (force_npu && port->m_line_side_id != SAI_NULL_OBJECT_ID) + { + SWSS_LOG_NOTICE("MACsec: %s -> backend=NPU (phy marked unsupported), using NPU port", + m_port_name ? m_port_name->c_str() : ""); + } m_port_id = std::make_unique(port->m_port_id); } } @@ -381,12 +394,27 @@ class MACsecOrchContext { switchId = port->m_switch_id; } + if (switchId == SAI_NULL_OBJECT_ID) { SWSS_LOG_ERROR("Switch ID cannot be found"); return nullptr; } m_switch_id = std::make_unique(switchId); + + const auto *phy = port ? m_orch->m_port_orch->getGearboxPhy(*port) : nullptr; + bool force_npu = true; + if (phy) + { + force_npu = !phy->macsec_supported; + } + + if (force_npu && (*m_switch_id != gSwitchId)) + { + SWSS_LOG_NOTICE("MACsec: %s -> backend=NPU (phy marked unsupported), override switch to global", + m_port_name ? m_port_name->c_str() : ""); + *m_switch_id = gSwitchId; + } } return m_switch_id.get(); } @@ -2507,28 +2535,32 @@ bool MACsecOrch::deleteMACsecSA(sai_object_id_t sa_id) FlexCounterManager& MACsecOrch::MACsecSaStatManager(MACsecOrchContext &ctx) { - if (ctx.get_gearbox_phy() != nullptr) + const auto *phy = ctx.get_gearbox_phy(); + if (phy && phy->macsec_supported) return m_gb_macsec_sa_stat_manager; return m_macsec_sa_stat_manager; } FlexCounterManager& MACsecOrch::MACsecSaAttrStatManager(MACsecOrchContext &ctx) { - if (ctx.get_gearbox_phy() != nullptr) + const auto *phy = ctx.get_gearbox_phy(); + if (phy && phy->macsec_supported) return m_gb_macsec_sa_attr_manager; return m_macsec_sa_attr_manager; } FlexCounterManager& MACsecOrch::MACsecFlowStatManager(MACsecOrchContext &ctx) { - if (ctx.get_gearbox_phy() != nullptr) + const auto *phy = ctx.get_gearbox_phy(); + if (phy && phy->macsec_supported) return m_gb_macsec_flow_stat_manager; return m_macsec_flow_stat_manager; } Table& MACsecOrch::MACsecCountersMap(MACsecOrchContext &ctx) { - if (ctx.get_gearbox_phy() != nullptr) + const auto *phy = ctx.get_gearbox_phy(); + if (phy && phy->macsec_supported) return m_gb_macsec_counters_map; return m_macsec_counters_map; } diff --git a/tests/gearbox.py b/tests/gearbox.py new file mode 100644 index 00000000000..1f98dc1b3a4 --- /dev/null +++ b/tests/gearbox.py @@ -0,0 +1,105 @@ +""" +Generic helper functions for gearbox testing. + +This module provides reusable utility functions for gearbox-related tests, +including port management and configuration setup. +""" + +import json + + +class TestGearboxHelper: + """Helper class for gearbox-related test operations.""" + + @staticmethod + def get_first_gearbox_port(gearbox): + """ + Get the first port from Gearbox object (reads from _GEARBOX_TABLE in APPL_DB). + + Args: + gearbox: Gearbox fixture + + Returns: + tuple: (port_name, phy_id) - First available gearbox port and its PHY ID + """ + assert len(gearbox.interfaces) > 0, "No interfaces found in gearbox" + + # Get first interface + first_idx = next(iter(gearbox.interfaces)) + first_intf = gearbox.interfaces[first_idx] + + port_name = first_intf.get("name") + phy_id = first_intf.get("phy_id") + + assert port_name, "First interface has no 'name' field" + assert phy_id is not None, "First interface has no 'phy_id' field" + + return port_name, phy_id + + @staticmethod + def configure_gearbox_macsec_support(dvs, gearbox, phy_id=None, macsec_supported=None): + """ + Configure MACsec support on a gearbox PHY by modifying gearbox_config.json and restarting DVS. + + This is necessary because: + 1. gearsyncd reads gearbox_config.json only at startup + 2. PortsOrch caches _GEARBOX_TABLE only at startup (initGearbox) + 3. MACsecOrch reads from PortsOrch's cache, not from _GEARBOX_TABLE + 4. Full DVS restart is the only reliable way to reload the configuration + because partial service restarts cause inconsistent port state + + Args: + dvs: Docker Virtual Switch instance + gearbox: Gearbox fixture + phy_id: PHY ID (string, e.g., "1"). If None, uses the first PHY from Gearbox object. + macsec_supported: None (remove field), True, or False + """ + # If phy_id not provided, use the first PHY from Gearbox object + if phy_id is None: + assert len(gearbox.phys) > 0, "No PHYs found in gearbox" + phy_id = next(iter(gearbox.phys)) + print(f"No phy_id provided, using first PHY: {phy_id}") + + # Resolve symlink to get actual config path + config_path = "/usr/share/sonic/hwsku/gearbox_config.json" + rc, actual_path = dvs.runcmd(f"readlink -f {config_path}") + if rc == 0 and actual_path.strip(): + config_path = actual_path.strip() + + # Read current config + rc, config_json = dvs.runcmd(f"cat {config_path}") + assert rc == 0, f"Failed to read gearbox_config.json from {config_path}" + config = json.loads(config_json) + + phy_id = int(phy_id) + + # Find and modify the PHY configuration + phy_found = False + for phy in config.get("phys", []): + if phy.get("phy_id") == phy_id: + phy_found = True + if macsec_supported is None: + # Remove the field if it exists + if "macsec_supported" in phy: + del phy["macsec_supported"] + else: + # Set the field + phy["macsec_supported"] = macsec_supported + break + + assert phy_found, f"PHY {phy_id} not found in gearbox_config.json" + + # Write modified config back using heredoc + config_str = json.dumps(config, indent=2) + heredoc = "__GEARBOX_JSON__" + rc, _ = dvs.runcmd( + "bash -lc 'cat > {path} <<\"{tag}\"\n{payload}\n{tag}\n'".format( + path=config_path, + tag=heredoc, + payload=config_str, + ) + ) + assert rc == 0, f"Failed to write modified config to {config_path}" + + # Restart DVS to reload configuration + dvs.restart() diff --git a/tests/macsec.py b/tests/macsec.py new file mode 100644 index 00000000000..7501f9fb8f5 --- /dev/null +++ b/tests/macsec.py @@ -0,0 +1,185 @@ +""" +Generic helper functions for MACsec testing. + +This module provides reusable utility functions for MACsec-related tests, +including port configuration, secure channel/association management, and verification. +""" + +from swsscommon import swsscommon +from dvslib.dvs_database import DVSDatabase +from test_macsec import WPASupplicantMock + + +class TestMacsecHelper: + """Helper class for MACsec-related test operations.""" + + @staticmethod + def enable_macsec_on_port(dvs, port_name, with_secure_channels=True): + """ + Enable MACsec on a port with optional secure channels and associations. + + Args: + dvs: Docker Virtual Switch instance + port_name: Port name to enable MACsec on + with_secure_channels: If True, create Secure Channels and Associations with static keys + + Returns: + WPASupplicantMock: The WPA supplicant mock instance + """ + wpa = WPASupplicantMock(dvs) + wpa.init_macsec_port(port_name) + + # Configure MACsec port with protection and encryption enabled + wpa.config_macsec_port(port_name, { + "enable": True, + "enable_protect": True, + "enable_encrypt": True, + "send_sci": True, + }) + + # If requested, create Secure Channels and Associations with static keys + if with_secure_channels: + local_mac_address = "00-15-5D-78-FF-C1" + peer_mac_address = "00-15-5D-78-FF-C2" + macsec_port_identifier = 1 + an = 0 # Association Number + sak = "0" * 32 # SAK: 128-bit key (32 hex chars) + auth_key = "0" * 32 # Auth key: 128-bit key (32 hex chars) + packet_number = 1 + ssci = 1 # Short SCI + salt = "0" * 24 # Salt for XPN cipher suites + + # Create Transmit Secure Channel (local) - MUST come first! + wpa.create_transmit_sc( + port_name, + local_mac_address, + macsec_port_identifier) + + # Create Receive Secure Channel (from peer) + wpa.create_receive_sc( + port_name, + peer_mac_address, + macsec_port_identifier) + + # Create Receive Secure Association with static keys + wpa.create_receive_sa( + port_name, + peer_mac_address, + macsec_port_identifier, + an, + sak, + auth_key, + packet_number, + ssci, + salt) + + # Create Transmit Secure Association with static keys + wpa.create_transmit_sa( + port_name, + local_mac_address, + macsec_port_identifier, + an, + sak, + auth_key, + packet_number, + ssci, + salt) + + # Enable Receive SA + wpa.set_enable_receive_sa( + port_name, + peer_mac_address, + macsec_port_identifier, + an, + True) + + # Enable MACsec control + wpa.set_macsec_control(port_name, True) + + # Enable Transmit SA + wpa.set_enable_transmit_sa( + port_name, + local_mac_address, + macsec_port_identifier, + an, + True) + + return wpa + + @staticmethod + def cleanup_macsec(dvs, port_name): + """ + Cleanup MACsec configuration on a port to prevent test pollution. + + Args: + dvs: Docker Virtual Switch instance + port_name: Port name to cleanup + """ + try: + wpa = WPASupplicantMock(dvs) + app_db = dvs.get_app_db() + + # Disable MACsec control first + wpa.set_macsec_control(port_name, False) + + # Delete all SAs for this port (must delete before SCs) + for table in ["MACSEC_EGRESS_SA_TABLE", "MACSEC_INGRESS_SA_TABLE"]: + for key in app_db.get_keys(table): + if key.startswith(f"{port_name}:"): + app_db.delete_entry(table, key) + + # Delete all SCs for this port + for table in ["MACSEC_EGRESS_SC_TABLE", "MACSEC_INGRESS_SC_TABLE"]: + for key in app_db.get_keys(table): + if key.startswith(f"{port_name}:"): + app_db.delete_entry(table, key) + + # Finally delete the MACsec port entry + wpa.deinit_macsec_port(port_name) + + except Exception as e: + print(f"Cleanup encountered error: {e}") + + @staticmethod + def verify_macsec_in_gb_asic_db(dvs, should_exist=True): + """ + Verify MACsec objects exist (or don't exist) in GB_ASIC_DB + + Args: + dvs: Docker Virtual Switch instance + should_exist: True if objects should exist, False otherwise + + Returns: + bool: True if verification passes + """ + + gb_asic_db = DVSDatabase(swsscommon.GB_ASIC_DB, dvs.redis_sock) + + macsec_keys = gb_asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_MACSEC") + + if should_exist: + return len(macsec_keys) > 0 # Should have at least one object + else: + return len(macsec_keys) == 0 # Should have no objects + + @staticmethod + def verify_macsec_in_asic_db(dvs, should_exist=True): + """ + Verify MACsec objects exist (or don't exist) in ASIC_DB (NPU) + + Args: + dvs: Docker Virtual Switch instance + should_exist: True if objects should exist, False otherwise + + Returns: + bool: True if verification passes + """ + asic_db = dvs.get_asic_db() + + macsec_keys = asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_MACSEC") + + if should_exist: + return len(macsec_keys) > 0 + else: + return len(macsec_keys) == 0 + diff --git a/tests/test_macsec_gearbox.py b/tests/test_macsec_gearbox.py new file mode 100644 index 00000000000..2d0c4ec71bf --- /dev/null +++ b/tests/test_macsec_gearbox.py @@ -0,0 +1,133 @@ +import pytest + +from test_gearbox import Gearbox +from gearbox import TestGearboxHelper +from macsec import TestMacsecHelper + +DVS_ENV = ["HWSKU=brcm_gearbox_vs"] + +@pytest.fixture(scope="module") +def gearbox(dvs): + return Gearbox(dvs) + +@pytest.fixture(scope="module") +def gearbox_config(dvs): + """ + Backup and restore gearbox_config.json once for all tests in the module. + + Backup happens before the first test, restore happens after the last test. + """ + # Resolve symlink to get actual config path + config_path = "/usr/share/sonic/hwsku/gearbox_config.json" + rc, actual_path = dvs.runcmd(f"readlink -f {config_path}") + if rc == 0 and actual_path.strip(): + config_path = actual_path.strip() + + # Backup original config (once at start) + dvs.runcmd(f"cp {config_path} {config_path}.bak") + + yield config_path + + # Restore original config (once at end) + dvs.runcmd(f"mv {config_path}.bak {config_path}") + +class TestMacsecGearbox(object): + + def test_macsec_phy_switch_default(self, dvs, gearbox, gearbox_config): + """ + When macsec_supported field is ABSENT (not specified), the system should: + 1. Default to using PHY switch for MACsec + 2. Create MACsec objects in GB_ASIC_DB (not ASIC_DB) + 3. This preserves backward compatibility with existing platforms + + Args: + dvs: Docker Virtual Switch instance (pytest fixture) + gearbox: Gearbox fixture + gearbox_config: Gearbox config fixture (auto backup/restore) + """ + # Derive port and phy_id from Gearbox object + port_name, phy_id = TestGearboxHelper.get_first_gearbox_port(gearbox) + + try: + TestGearboxHelper.configure_gearbox_macsec_support(dvs, gearbox, phy_id=phy_id, macsec_supported=None) + TestMacsecHelper.enable_macsec_on_port(dvs, port_name=port_name, with_secure_channels=True) + + assert TestMacsecHelper.verify_macsec_in_gb_asic_db(dvs, should_exist=True), ( + "FAILED: MACsec objects should exist in GB_ASIC_DB " + "when macsec_supported is absent" + ) + + assert TestMacsecHelper.verify_macsec_in_asic_db(dvs, should_exist=False), ( + "FAILED: MACsec objects should NOT exist in ASIC_DB " + "when using PHY backend" + ) + + finally: + TestMacsecHelper.cleanup_macsec(dvs, port_name) + + def test_macsec_phy_switch_explicit(self, dvs, gearbox, gearbox_config): + """ + When macsec_supported field is explicitly set to TRUE, the system should: + 1. Use PHY switch for MACsec (same as default) + 2. Create MACsec objects in GB_ASIC_DB (not ASIC_DB) + 3. This is the explicit way to declare PHY MACsec support + + Args: + dvs: Docker Virtual Switch instance (pytest fixture) + gearbox: Gearbox fixture + gearbox_config: Gearbox config fixture (auto backup/restore) + """ + # Derive port and phy_id from Gearbox object + port_name, phy_id = TestGearboxHelper.get_first_gearbox_port(gearbox) + + try: + TestGearboxHelper.configure_gearbox_macsec_support(dvs, gearbox, phy_id=phy_id, macsec_supported=True) + TestMacsecHelper.enable_macsec_on_port(dvs, port_name=port_name, with_secure_channels=True) + + assert TestMacsecHelper.verify_macsec_in_gb_asic_db(dvs, should_exist=True), ( + "FAILED: MACsec objects should exist in GB_ASIC_DB " + "when macsec_supported=true" + ) + + assert TestMacsecHelper.verify_macsec_in_asic_db(dvs, should_exist=False), ( + "FAILED: MACsec objects should NOT exist in ASIC_DB " + "when using PHY backend" + ) + + finally: + TestMacsecHelper.cleanup_macsec(dvs, port_name) + + def test_macsec_npu_switch(self, dvs, gearbox, gearbox_config): + """ + Test MACsec NPU backend selection when macsec_supported=false. + + 1. When a gearbox PHY has macsec_supported=false in _GEARBOX_TABLE, + 2. MACsec objects should be created in ASIC_DB (NPU backend), not in + GB_ASIC_DB (PHY backend). + + Args: + dvs: Docker Virtual Switch instance (pytest fixture) + gearbox: Gearbox fixture + gearbox_config: Gearbox config fixture (auto backup/restore) + """ + # Derive port and phy_id from Gearbox object + port_name, phy_id = TestGearboxHelper.get_first_gearbox_port(gearbox) + + try: + # Setup gearbox with macsec_supported=false + TestGearboxHelper.configure_gearbox_macsec_support(dvs, gearbox, phy_id=phy_id, macsec_supported=False) + TestMacsecHelper.enable_macsec_on_port(dvs, port_name=port_name, with_secure_channels=True) + + assert TestMacsecHelper.verify_macsec_in_asic_db(dvs, should_exist=True), ( + "FAILED: MACsec objects should exist in ASIC_DB " + "when macsec_supported=false" + ) + + assert TestMacsecHelper.verify_macsec_in_gb_asic_db(dvs, should_exist=False), ( + "FAILED: MACsec objects should NOT exist in GB_ASIC_DB " + "when macsec_supported=false" + ) + + finally: + TestMacsecHelper.cleanup_macsec(dvs, port_name) +