From 2de89993327dffd5ecddcdcf01892a94ab31d194 Mon Sep 17 00:00:00 2001 From: Benoit Sigoure Date: Mon, 5 Jan 2026 21:22:01 +0000 Subject: [PATCH] Add CLI command: config interface switchport access vlan # Summary Implements a new fboss2-dev CLI command to configure the access VLAN for an interface (switchport). This allows operators to change which VLAN a port belongs to. The command syntax is: ``` config interface switchport access vlan ``` Where: - `` is the interface name (e.g., eth1/1/1) - `` is the VLAN ID (1-4094) Note: This command doesn't yet automatically create the VLAN if it doesn't exist. The VLAN must already be configured in the system. Since VLAN membership changes cannot be applied dynamically via reloadConfig(), this change requires an agent warmboot to take effect. # Test Plan ## New unit tests ``` Note: Google Test filter = *SwitchportAccessVlan* [==========] Running 13 tests from 1 test suite. [----------] Global test environment set-up. [----------] 13 tests from CmdConfigInterfaceSwitchportAccessVlanTestFixture [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdValidMin [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdValidMin (3 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdValidMax [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdValidMax (1 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdValidMid [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdValidMid (1 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdZeroInvalid [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdZeroInvalid (7 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdTooHighInvalid [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdTooHighInvalid (1 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdNegativeInvalid [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdNegativeInvalid (1 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdNonNumericInvalid [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdNonNumericInvalid (1 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdEmptyInvalid [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdEmptyInvalid (1 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdMultipleValuesInvalid [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdMultipleValuesInvalid (1 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdOutOfRangeErrorMessage [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdOutOfRangeErrorMessage (1 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdNonNumericErrorMessage [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.vlanIdNonNumericErrorMessage (0 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.queryClientSetsIngressVlanMultiplePorts [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.queryClientSetsIngressVlanMultiplePorts (21 ms) [ RUN ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.queryClientThrowsOnEmptyInterfaceList [ OK ] CmdConfigInterfaceSwitchportAccessVlanTestFixture.queryClientThrowsOnEmptyInterfaceList (5 ms) [----------] 13 tests from CmdConfigInterfaceSwitchportAccessVlanTestFixture (49 ms total) [----------] Global test environment tear-down [==========] 13 tests from 1 test suite ran. (49 ms total) [ PASSED ] 13 tests. ``` ## New end-to-end tests on minipack3 ``` ============================================================ CLI E2E Test: config interface switchport access vlan ============================================================ [Step 1] Finding an interface to test... [CLI] Running: show interface [CLI] Completed in 0.08s: show interface Using interface: eth1/1/1 (VLAN: 2005) [Step 2] Getting current access VLAN... [CLI] Running: show interface eth1/1/1 [CLI] Completed in 0.07s: show interface eth1/1/1 Current access VLAN: 2005 [Step 3] Setting access VLAN to 2006... [CLI] Running: config interface eth1/1/1 switchport access vlan 2006 [CLI] Completed in 0.06s: config interface eth1/1/1 switchport access vlan 2006 [CLI] Running: config session commit [CLI] Completed in 8.49s: config session commit [CLI] Agent is ready (waited 7.6s) [CLI] Applied info: {"localhost": {"lastAppliedInMs": 1771036580360}} Access VLAN set to 2006 [Step 4] Verifying access VLAN via 'show interface'... [CLI] Running: show interface eth1/1/1 [CLI] Completed in 0.07s: show interface eth1/1/1 Verified: Access VLAN is 2006 [Step 5] Restoring original access VLAN (2005)... [CLI] Running: config interface eth1/1/1 switchport access vlan 2005 [CLI] Completed in 0.06s: config interface eth1/1/1 switchport access vlan 2005 [CLI] Running: config session commit [CLI] Completed in 9.92s: config session commit [CLI] Agent is ready (waited 7.7s) [CLI] Applied info: {"localhost": {"lastAppliedInMs": 1771036598267}} Restored access VLAN to 2005 [CLI] Running: show interface eth1/1/1 [CLI] Completed in 0.08s: show interface eth1/1/1 ============================================================ TEST PASSED ============================================================ ``` ## Sample usage ``` [admin@fboss101 ~]$ /opt/fboss/bin/fboss2-dev config interface eth1/1/1 switchport access vlan 2007 Successfully set access VLAN for interface(s) eth1/1/1 to 2007 [admin@fboss101 ~]$ /opt/fboss/bin/fboss2-dev config session diff --- current live config +++ session config @@ -2178,7 +2178,7 @@ "2": "eth1/5/1" }, "expectedNeighborReachability": [], - "ingressVlan": 2005, + "ingressVlan": 2007, "logicalID": 9, "lookupClasses": [], "loopbackMode": 0, @@ -7061,7 +7061,7 @@ "emitTags": false, "logicalPort": 9, "spanningTreeState": 2, - "vlanID": 2005 + "vlanID": 2007 } ], "vlans": [ ``` --- cmake/CliFboss2.cmake | 4 + cmake/CliFboss2Test.cmake | 1 + fboss/cli/fboss2/BUCK | 4 + fboss/cli/fboss2/CmdHandlerImplConfig.cpp | 12 + fboss/cli/fboss2/CmdListConfig.cpp | 22 ++ fboss/cli/fboss2/CmdSubcommands.cpp | 3 + .../switchport/CmdConfigInterfaceSwitchport.h | 42 +++ .../CmdConfigInterfaceSwitchportAccess.h | 43 +++ ...CmdConfigInterfaceSwitchportAccessVlan.cpp | 71 +++++ .../CmdConfigInterfaceSwitchportAccessVlan.h | 82 ++++++ fboss/cli/fboss2/session/ConfigSession.cpp | 8 +- fboss/cli/fboss2/test/BUCK | 1 + ...onfigInterfaceSwitchportAccessVlanTest.cpp | 266 ++++++++++++++++++ fboss/cli/fboss2/utils/CmdUtilsCommon.h | 1 + fboss/oss/cli_tests/cli_test_lib.py | 55 +++- fboss/oss/cli_tests/test_config_vlan.py | 102 +++++++ 16 files changed, 710 insertions(+), 7 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h create mode 100644 fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h create mode 100644 fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp create mode 100644 fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h create mode 100644 fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp create mode 100644 fboss/oss/cli_tests/test_config_vlan.py diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index d4e37bf5d7a26..8f88d9cdc20a4 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -632,6 +632,10 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.cpp fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.cpp + fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h + fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h + fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h + fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h diff --git a/cmake/CliFboss2Test.cmake b/cmake/CliFboss2Test.cmake index f3f68a8f20401..fbd094efa1048 100644 --- a/cmake/CliFboss2Test.cmake +++ b/cmake/CliFboss2Test.cmake @@ -38,6 +38,7 @@ add_executable(fboss2_cmd_test fboss/cli/fboss2/test/CmdConfigHistoryTest.cpp fboss/cli/fboss2/test/CmdConfigInterfaceDescriptionTest.cpp fboss/cli/fboss2/test/CmdConfigInterfaceMtuTest.cpp + fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp fboss/cli/fboss2/test/CmdConfigQosBufferPoolTest.cpp fboss/cli/fboss2/test/CmdConfigReloadTest.cpp fboss/cli/fboss2/test/CmdConfigSessionDiffTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 8f0d4dde5a06b..690034ade6cd7 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -853,6 +853,7 @@ cpp_library( "commands/config/history/CmdConfigHistory.cpp", "commands/config/interface/CmdConfigInterfaceDescription.cpp", "commands/config/interface/CmdConfigInterfaceMtu.cpp", + "commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp", "commands/config/qos/buffer_pool/CmdConfigQosBufferPool.cpp", "commands/config/rollback/CmdConfigRollback.cpp", "commands/config/session/CmdConfigSessionCommit.cpp", @@ -867,6 +868,9 @@ cpp_library( "commands/config/interface/CmdConfigInterface.h", "commands/config/interface/CmdConfigInterfaceDescription.h", "commands/config/interface/CmdConfigInterfaceMtu.h", + "commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h", + "commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h", + "commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h", "commands/config/qos/CmdConfigQos.h", "commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h", "commands/config/rollback/CmdConfigRollback.h", diff --git a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp index 290831c6c86be..515f4dbb239e4 100644 --- a/fboss/cli/fboss2/CmdHandlerImplConfig.cpp +++ b/fboss/cli/fboss2/CmdHandlerImplConfig.cpp @@ -19,6 +19,9 @@ #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" #include "fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h" #include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" @@ -36,6 +39,15 @@ template void CmdHandler< CmdConfigInterfaceDescriptionTraits>::run(); template void CmdHandler::run(); +template void CmdHandler< + CmdConfigInterfaceSwitchport, + CmdConfigInterfaceSwitchportTraits>::run(); +template void CmdHandler< + CmdConfigInterfaceSwitchportAccess, + CmdConfigInterfaceSwitchportAccessTraits>::run(); +template void CmdHandler< + CmdConfigInterfaceSwitchportAccessVlan, + CmdConfigInterfaceSwitchportAccessVlanTraits>::run(); template void CmdHandler::run(); template void CmdHandler::run(); template void diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 78e4ad709d3ee..697111d4daa13 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -17,6 +17,9 @@ #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceDescription.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterfaceMtu.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" #include "fboss/cli/fboss2/commands/config/qos/CmdConfigQos.h" #include "fboss/cli/fboss2/commands/config/qos/buffer_pool/CmdConfigQosBufferPool.h" #include "fboss/cli/fboss2/commands/config/rollback/CmdConfigRollback.h" @@ -56,6 +59,25 @@ const CommandTree& kConfigCommandTree() { "Set interface MTU", commandHandler, argTypeHandler, + }, + { + "switchport", + "Configure switchport settings", + commandHandler, + argTypeHandler, + {{ + "access", + "Configure access mode settings", + commandHandler, + argTypeHandler, + {{ + "vlan", + "Set access VLAN (ingressVlan) for the interface", + commandHandler, + argTypeHandler< + CmdConfigInterfaceSwitchportAccessVlanTraits>, + }}, + }}, }}, }, diff --git a/fboss/cli/fboss2/CmdSubcommands.cpp b/fboss/cli/fboss2/CmdSubcommands.cpp index 24221d3d6b753..f457b94d73513 100644 --- a/fboss/cli/fboss2/CmdSubcommands.cpp +++ b/fboss/cli/fboss2/CmdSubcommands.cpp @@ -239,6 +239,9 @@ CLI::App* CmdSubcommands::addCommand( " [ ...] where is one " "of: shared-bytes, headroom-bytes, reserved-bytes"); break; + case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_VLAN_ID: + subCmd->add_option("vlan_id", args, "VLAN ID (1-4094)"); + break; case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_UNINITIALIZE: case utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE: break; diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h b/fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h new file mode 100644 index 0000000000000..c32a5eb3e0f32 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" +#include "fboss/cli/fboss2/utils/InterfaceList.h" + +namespace facebook::fboss { + +struct CmdConfigInterfaceSwitchportTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigInterface; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdConfigInterfaceSwitchport : public CmdHandler< + CmdConfigInterfaceSwitchport, + CmdConfigInterfaceSwitchportTraits> { + public: + RetType queryClient( + const HostInfo& /* hostInfo */, + const utils::InterfaceList& /* interfaces */) { + throw std::runtime_error( + "Incomplete command, please use one of the subcommands"); + } + + void printOutput(const RetType& /* model */) {} +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h b/fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h new file mode 100644 index 0000000000000..0eb3ce70b3556 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/CmdConfigInterfaceSwitchport.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" +#include "fboss/cli/fboss2/utils/InterfaceList.h" + +namespace facebook::fboss { + +struct CmdConfigInterfaceSwitchportAccessTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigInterfaceSwitchport; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdConfigInterfaceSwitchportAccess + : public CmdHandler< + CmdConfigInterfaceSwitchportAccess, + CmdConfigInterfaceSwitchportAccessTraits> { + public: + RetType queryClient( + const HostInfo& /* hostInfo */, + const utils::InterfaceList& /* interfaces */) { + throw std::runtime_error( + "Incomplete command, please use one of the subcommands"); + } + + void printOutput(const RetType& /* model */) {} +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp new file mode 100644 index 0000000000000..ed32c7c9efbe4 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp @@ -0,0 +1,71 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" + +#include + +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +CmdConfigInterfaceSwitchportAccessVlanTraits::RetType +CmdConfigInterfaceSwitchportAccessVlan::queryClient( + const HostInfo& hostInfo, + const utils::InterfaceList& interfaces, + const CmdConfigInterfaceSwitchportAccessVlanTraits::ObjectArgType& + vlanIdValue) { + if (interfaces.empty()) { + throw std::invalid_argument("No interface name provided"); + } + + // Extract the VLAN ID (validation already done in VlanIdValue constructor) + int32_t vlanId = vlanIdValue.getVlanId(); + + // Collect the logical port IDs we need to update + std::unordered_set portIds; + + // Update ingressVlan for all resolved ports + for (const utils::Intf& intf : interfaces) { + cfg::Port* port = intf.getPort(); + if (port) { + port->ingressVlan() = vlanId; + portIds.insert(*port->logicalID()); + } + } + + // Also update the vlanPorts entries for these ports + auto& config = ConfigSession::getInstance().getAgentConfig(); + auto& vlanPorts = *config.sw()->vlanPorts(); + for (auto& vlanPort : vlanPorts) { + if (portIds.count(*vlanPort.logicalPort())) { + vlanPort.vlanID() = vlanId; + } + } + + // Save the updated config + // VLAN changes require agent warmboot as reloadConfig() doesn't apply them + ConfigSession::getInstance().saveConfig( + cli::ServiceType::AGENT, cli::ConfigActionLevel::AGENT_WARMBOOT); + + std::string interfaceList = folly::join(", ", interfaces.getNames()); + std::string message = "Successfully set access VLAN for interface(s) " + + interfaceList + " to " + std::to_string(vlanId); + + return message; +} + +void CmdConfigInterfaceSwitchportAccessVlan::printOutput( + const CmdConfigInterfaceSwitchportAccessVlanTraits::RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h new file mode 100644 index 0000000000000..e2af310574b50 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h @@ -0,0 +1,82 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/CmdConfigInterfaceSwitchportAccess.h" +#include "fboss/cli/fboss2/utils/CmdUtils.h" +#include "fboss/cli/fboss2/utils/InterfaceList.h" + +namespace facebook::fboss { + +// Custom type for VLAN ID argument with validation +class VlanIdValue : public utils::BaseObjectArgType { + public: + /* implicit */ VlanIdValue(std::vector v) { + if (v.empty()) { + throw std::invalid_argument("VLAN ID is required"); + } + if (v.size() != 1) { + throw std::invalid_argument( + "Expected single VLAN ID, got: " + folly::join(", ", v)); + } + + try { + int32_t vlanId = folly::to(v[0]); + // VLAN IDs are typically 1-4094 (0 and 4095 are reserved) + if (vlanId < 1 || vlanId > 4094) { + throw std::invalid_argument( + "VLAN ID must be between 1 and 4094 inclusive, got: " + + std::to_string(vlanId)); + } + data_.push_back(vlanId); + } catch (const folly::ConversionError&) { + throw std::invalid_argument("Invalid VLAN ID: " + v[0]); + } + } + + int32_t getVlanId() const { + return data_[0]; + } + + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_VLAN_ID; +}; + +struct CmdConfigInterfaceSwitchportAccessVlanTraits + : public WriteCommandTraits { + using ParentCmd = CmdConfigInterfaceSwitchportAccess; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_VLAN_ID; + using ObjectArgType = VlanIdValue; + using RetType = std::string; +}; + +class CmdConfigInterfaceSwitchportAccessVlan + : public CmdHandler< + CmdConfigInterfaceSwitchportAccessVlan, + CmdConfigInterfaceSwitchportAccessVlanTraits> { + public: + using ObjectArgType = + CmdConfigInterfaceSwitchportAccessVlanTraits::ObjectArgType; + using RetType = CmdConfigInterfaceSwitchportAccessVlanTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const utils::InterfaceList& interfaces, + const ObjectArgType& vlanId); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/session/ConfigSession.cpp b/fboss/cli/fboss2/session/ConfigSession.cpp index e3eeb347aeb18..c3eceaace2734 100644 --- a/fboss/cli/fboss2/session/ConfigSession.cpp +++ b/fboss/cli/fboss2/session/ConfigSession.cpp @@ -460,7 +460,8 @@ void ConfigSession::restartService( if (level == cli::ConfigActionLevel::AGENT_COLDBOOT) { // Step 1: Stop the service try { - folly::Subprocess stopProc({"/usr/bin/systemctl", "stop", serviceName}); + folly::Subprocess stopProc( + {"/usr/bin/sudo", "/usr/bin/systemctl", "stop", serviceName}); stopProc.waitChecked(); } catch (const std::exception& ex) { throw std::runtime_error( @@ -488,7 +489,8 @@ void ConfigSession::restartService( // Step 3: Start the service try { - folly::Subprocess startProc({"/usr/bin/systemctl", "start", serviceName}); + folly::Subprocess startProc( + {"/usr/bin/sudo", "/usr/bin/systemctl", "start", serviceName}); startProc.waitChecked(); } catch (const std::exception& ex) { throw std::runtime_error( @@ -498,7 +500,7 @@ void ConfigSession::restartService( // For warmboot, just do a simple restart try { folly::Subprocess restartProc( - {"/usr/bin/systemctl", "restart", serviceName}); + {"/usr/bin/sudo", "/usr/bin/systemctl", "restart", serviceName}); restartProc.waitChecked(); } catch (const std::exception& ex) { throw std::runtime_error( diff --git a/fboss/cli/fboss2/test/BUCK b/fboss/cli/fboss2/test/BUCK index e00e3226cc278..a2c7cf4184584 100644 --- a/fboss/cli/fboss2/test/BUCK +++ b/fboss/cli/fboss2/test/BUCK @@ -67,6 +67,7 @@ cpp_unittest( "CmdConfigHistoryTest.cpp", "CmdConfigInterfaceDescriptionTest.cpp", "CmdConfigInterfaceMtuTest.cpp", + "CmdConfigInterfaceSwitchportAccessVlanTest.cpp", "CmdConfigQosBufferPoolTest.cpp", "CmdConfigReloadTest.cpp", "CmdConfigSessionDiffTest.cpp", diff --git a/fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp b/fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp new file mode 100644 index 0000000000000..1b740ca660f6c --- /dev/null +++ b/fboss/cli/fboss2/test/CmdConfigInterfaceSwitchportAccessVlanTest.cpp @@ -0,0 +1,266 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include +#include +#include +#include +#include +#include + +#include "fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.h" +#include "fboss/cli/fboss2/test/CmdHandlerTestBase.h" +#include "fboss/cli/fboss2/test/TestableConfigSession.h" +#include "fboss/cli/fboss2/utils/PortMap.h" + +namespace fs = std::filesystem; + +using namespace ::testing; + +namespace facebook::fboss { + +class CmdConfigInterfaceSwitchportAccessVlanTestFixture + : public CmdHandlerTestBase { + public: + void SetUp() override { + CmdHandlerTestBase::SetUp(); + + // Create unique test directories + auto tempBase = fs::temp_directory_path(); + auto uniquePath = boost::filesystem::unique_path( + "fboss_switchport_test_%%%%-%%%%-%%%%-%%%%"); + testHomeDir_ = tempBase / (uniquePath.string() + "_home"); + testEtcDir_ = tempBase / (uniquePath.string() + "_etc"); + + std::error_code ec; + if (fs::exists(testHomeDir_)) { + fs::remove_all(testHomeDir_, ec); + } + if (fs::exists(testEtcDir_)) { + fs::remove_all(testEtcDir_, ec); + } + + // Create test directories + fs::create_directories(testHomeDir_); + fs::create_directories(testEtcDir_ / "coop"); + fs::create_directories(testEtcDir_ / "coop" / "cli"); + + // Set environment variables + setenv("HOME", testHomeDir_.c_str(), 1); + setenv("USER", "testuser", 1); + + // Create a test system config file with ports and vlanPorts + fs::path initialRevision = testEtcDir_ / "coop" / "cli" / "agent-r1.conf"; + createTestConfig(initialRevision, R"({ + "sw": { + "ports": [ + { + "logicalID": 1, + "name": "eth1/1/1", + "state": 2, + "speed": 100000, + "ingressVlan": 1 + }, + { + "logicalID": 2, + "name": "eth1/2/1", + "state": 2, + "speed": 100000, + "ingressVlan": 1 + } + ], + "vlanPorts": [ + { + "vlanID": 1, + "logicalPort": 1, + "spanningTreeState": 2, + "emitTags": false + }, + { + "vlanID": 1, + "logicalPort": 2, + "spanningTreeState": 2, + "emitTags": false + } + ] + } +})"); + + // Create symlink + systemConfigPath_ = testEtcDir_ / "coop" / "agent.conf"; + fs::create_symlink(initialRevision, systemConfigPath_); + + // Create session config path + sessionConfigPath_ = testHomeDir_ / ".fboss2" / "agent.conf"; + cliConfigDir_ = testEtcDir_ / "coop" / "cli"; + } + + void TearDown() override { + std::error_code ec; + if (fs::exists(testHomeDir_)) { + fs::remove_all(testHomeDir_, ec); + } + if (fs::exists(testEtcDir_)) { + fs::remove_all(testEtcDir_, ec); + } + CmdHandlerTestBase::TearDown(); + } + + protected: + void createTestConfig(const fs::path& path, const std::string& content) { + std::ofstream file(path); + file << content; + file.close(); + } + + fs::path testHomeDir_; + fs::path testEtcDir_; + fs::path systemConfigPath_; + fs::path sessionConfigPath_; + fs::path cliConfigDir_; +}; + +// Tests for VlanIdValue validation + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdValidMin) { + VlanIdValue vlanId({"1"}); + EXPECT_EQ(vlanId.getVlanId(), 1); +} + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdValidMax) { + VlanIdValue vlanId({"4094"}); + EXPECT_EQ(vlanId.getVlanId(), 4094); +} + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdValidMid) { + VlanIdValue vlanId({"100"}); + EXPECT_EQ(vlanId.getVlanId(), 100); +} + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdZeroInvalid) { + EXPECT_THROW(VlanIdValue({"0"}), std::invalid_argument); +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdTooHighInvalid) { + EXPECT_THROW(VlanIdValue({"4095"}), std::invalid_argument); +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdNegativeInvalid) { + EXPECT_THROW(VlanIdValue({"-1"}), std::invalid_argument); +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdNonNumericInvalid) { + EXPECT_THROW(VlanIdValue({"abc"}), std::invalid_argument); +} + +TEST_F(CmdConfigInterfaceSwitchportAccessVlanTestFixture, vlanIdEmptyInvalid) { + EXPECT_THROW(VlanIdValue({}), std::invalid_argument); +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdMultipleValuesInvalid) { + EXPECT_THROW(VlanIdValue({"100", "200"}), std::invalid_argument); +} + +// Test error message format +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdOutOfRangeErrorMessage) { + try { + VlanIdValue({"9999"}); + FAIL() << "Expected std::invalid_argument"; + } catch (const std::invalid_argument& e) { + std::string errorMsg = e.what(); + EXPECT_THAT(errorMsg, HasSubstr("VLAN ID must be between 1 and 4094")); + EXPECT_THAT(errorMsg, HasSubstr("9999")); + } +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + vlanIdNonNumericErrorMessage) { + try { + VlanIdValue({"notanumber"}); + FAIL() << "Expected std::invalid_argument"; + } catch (const std::invalid_argument& e) { + std::string errorMsg = e.what(); + EXPECT_THAT(errorMsg, HasSubstr("Invalid VLAN ID")); + EXPECT_THAT(errorMsg, HasSubstr("notanumber")); + } +} + +// Tests for queryClient + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + queryClientSetsIngressVlanMultiplePorts) { + TestableConfigSession session( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string()); + + auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); + VlanIdValue vlanId({"2001"}); + + // Create InterfaceList from port names + utils::InterfaceList interfaces({"eth1/1/1", "eth1/2/1"}); + + auto result = cmd.queryClient(localhost(), interfaces, vlanId); + + EXPECT_THAT(result, HasSubstr("Successfully set access VLAN")); + EXPECT_THAT(result, HasSubstr("eth1/1/1")); + EXPECT_THAT(result, HasSubstr("eth1/2/1")); + EXPECT_THAT(result, HasSubstr("2001")); + + // Verify the ingressVlan was updated for both ports + auto& config = session.getAgentConfig(); + auto& switchConfig = *config.sw(); + auto& ports = *switchConfig.ports(); + for (const auto& port : ports) { + if (*port.name() == "eth1/1/1" || *port.name() == "eth1/2/1") { + EXPECT_EQ(*port.ingressVlan(), 2001); + } + } + + // Verify the vlanPorts entries were also updated + auto& vlanPorts = *switchConfig.vlanPorts(); + for (const auto& vlanPort : vlanPorts) { + if (*vlanPort.logicalPort() == 1 || *vlanPort.logicalPort() == 2) { + EXPECT_EQ(*vlanPort.vlanID(), 2001); + } + } +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + queryClientThrowsOnEmptyInterfaceList) { + TestableConfigSession session( + sessionConfigPath_.string(), + systemConfigPath_.string(), + cliConfigDir_.string()); + + auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); + VlanIdValue vlanId({"100"}); + + // Empty InterfaceList is valid to construct but queryClient should throw + utils::InterfaceList emptyInterfaces({}); + EXPECT_THROW( + cmd.queryClient(localhost(), emptyInterfaces, vlanId), + std::invalid_argument); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/utils/CmdUtilsCommon.h b/fboss/cli/fboss2/utils/CmdUtilsCommon.h index 53adaa39a9132..384deef7a7cdd 100644 --- a/fboss/cli/fboss2/utils/CmdUtilsCommon.h +++ b/fboss/cli/fboss2/utils/CmdUtilsCommon.h @@ -67,6 +67,7 @@ enum class ObjectArgTypeId : uint8_t { OBJECT_ARG_TYPE_ID_INTERFACE_LIST, OBJECT_ARG_TYPE_ID_REVISION_LIST, OBJECT_ARG_TYPE_ID_BUFFER_POOL_NAME, + OBJECT_ARG_TYPE_VLAN_ID, }; template diff --git a/fboss/oss/cli_tests/cli_test_lib.py b/fboss/oss/cli_tests/cli_test_lib.py index be38ebcf1ec2b..670a0731ba951 100644 --- a/fboss/oss/cli_tests/cli_test_lib.py +++ b/fboss/oss/cli_tests/cli_test_lib.py @@ -132,19 +132,26 @@ def run_cmd(cmd: list[str], check: bool = True) -> subprocess.CompletedProcess: return result -def run_cli(args: list[str], check: bool = True) -> dict[str, Any]: +def run_cli(args: list[str], check: bool = True, quiet: bool = False) -> dict[str, Any]: """Run the fboss2-dev CLI with the given arguments. The --fmt json flag is automatically prepended to all commands. Returns the parsed JSON output as a dict. + + Args: + args: CLI arguments to pass after 'fboss2-dev --fmt json' + check: If True, raise RuntimeError on non-zero return code + quiet: If True, suppress logging of command execution """ cli = get_fboss_cli() cmd = [cli, "--fmt", "json"] + args - print(f"[CLI] Running: {' '.join(args)}") + if not quiet: + print(f"[CLI] Running: {' '.join(args)}") start_time = time.time() result = subprocess.run(cmd, capture_output=True, text=True) elapsed = time.time() - start_time - print(f"[CLI] Completed in {elapsed:.2f}s: {' '.join(args)}") + if not quiet: + print(f"[CLI] Completed in {elapsed:.2f}s: {' '.join(args)}") if check and result.returncode != 0: print(f"Command failed with return code {result.returncode}") print(f"stdout: {result.stdout}") @@ -236,6 +243,46 @@ def is_valid_eth_interface(intf: Interface) -> bool: return matches[0] +def wait_for_agent_ready( + timeout_seconds: float = 60.0, + poll_interval_ms: int = 500, +) -> None: + """ + Wait for the FBOSS agent to be ready by polling 'config applied-info'. + + Args: + timeout_seconds: Maximum time to wait in seconds (default: 60) + poll_interval_ms: How often to poll in milliseconds (default: 500) + + Raises: + RuntimeError: If the agent is not ready within the timeout + """ + start_time = time.time() + poll_interval_s = poll_interval_ms / 1000.0 + + while True: + elapsed = time.time() - start_time + if elapsed >= timeout_seconds: + raise RuntimeError(f"Agent not ready after {timeout_seconds}s") + + try: + # Try to run 'config applied-info' - a simple command to check agent is ready + data = run_cli(["config", "applied-info"], check=False, quiet=True) + if data: + print(f"[CLI] Agent is ready (waited {elapsed:.1f}s)") + # Log the applied-info output for debugging + print(f"[CLI] Applied info: {json.dumps(data)}") + return + except Exception: + pass + + # Agent not ready yet, wait and retry + time.sleep(poll_interval_s) + + def commit_config() -> None: - """Commit the current configuration session.""" + """Commit the current configuration session and wait for agent to be ready.""" run_cli(["config", "session", "commit"]) + # After commit, the agent may restart (warmboot/coldboot). + # Wait for it to be ready before returning. + wait_for_agent_ready() diff --git a/fboss/oss/cli_tests/test_config_vlan.py b/fboss/oss/cli_tests/test_config_vlan.py new file mode 100644 index 0000000000000..5de7cdb62e07b --- /dev/null +++ b/fboss/oss/cli_tests/test_config_vlan.py @@ -0,0 +1,102 @@ +#!/usr/bin/env python3 +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the BSD-style license found in the +# LICENSE file in the root directory of this source tree. + +""" +End-to-end test for the 'fboss2-dev config interface switchport access vlan ' command. + +This test: +1. Picks an interface from the running system +2. Gets the current access VLAN +3. Sets a new access VLAN +4. Verifies the VLAN was set correctly via 'fboss2-dev show interface' +5. Restores the original VLAN + +Requirements: +- FBOSS agent must be running with a valid configuration +- The test must be run as root (or with appropriate permissions) +""" + +import sys + +from cli_test_lib import ( + commit_config, + find_first_eth_interface, + get_interface_info, + run_cli, +) + + +def get_interface_vlan(interface_name: str) -> int: + """Get the current access VLAN for an interface.""" + info = get_interface_info(interface_name) + if info.vlan is None: + raise RuntimeError(f"Could not find VLAN for interface {interface_name}") + return info.vlan + + +def set_interface_vlan(interface_name: str, vlan_id: int) -> None: + """Set the access VLAN for an interface and commit the change.""" + run_cli( + [ + "config", + "interface", + interface_name, + "switchport", + "access", + "vlan", + str(vlan_id), + ] + ) + commit_config() + + +def main() -> int: + print("=" * 60) + print("CLI E2E Test: config interface switchport access vlan ") + print("=" * 60) + + # Step 1: Get an interface to test with + print("\n[Step 1] Finding an interface to test...") + interface = find_first_eth_interface() + print(f" Using interface: {interface.name} (VLAN: {interface.vlan})") + + # Step 2: Get the current VLAN + print("\n[Step 2] Getting current access VLAN...") + original_vlan = get_interface_vlan(interface.name) + print(f" Current access VLAN: {original_vlan}") + + # Step 3: Set a new VLAN (toggle between current and current+1) + new_vlan = original_vlan + 1 + print(f"\n[Step 3] Setting access VLAN to {new_vlan}...") + set_interface_vlan(interface.name, new_vlan) + print(f" Access VLAN set to {new_vlan}") + + # Step 4: Verify VLAN via 'show interface' + print("\n[Step 4] Verifying access VLAN via 'show interface'...") + actual_vlan = get_interface_vlan(interface.name) + if actual_vlan != new_vlan: + print(f" ERROR: Expected VLAN {new_vlan}, got {actual_vlan}") + return 1 + print(f" Verified: Access VLAN is {actual_vlan}") + + # Step 5: Restore original VLAN + print(f"\n[Step 5] Restoring original access VLAN ({original_vlan})...") + set_interface_vlan(interface.name, original_vlan) + print(f" Restored access VLAN to {original_vlan}") + + # Verify restoration + restored_vlan = get_interface_vlan(interface.name) + if restored_vlan != original_vlan: + print(f" WARNING: Restoration may have failed. Current: {restored_vlan}") + + print("\n" + "=" * 60) + print("TEST PASSED") + print("=" * 60) + return 0 + + +if __name__ == "__main__": + sys.exit(main())