From 44c1a911f8f050c813dd5c3d8e8981b89866a9b3 Mon Sep 17 00:00:00 2001 From: "mabel-bot[bot]" <266911995+mabel-bot[bot]@users.noreply.github.com> Date: Mon, 11 May 2026 11:06:31 +0200 Subject: [PATCH 1/2] fboss2-dev switchport access vlan: validate VLAN exists before commit Signed-off-by: mabel-bot[bot] <266911995+mabel-bot[bot]@users.noreply.github.com> --- ...CmdConfigInterfaceSwitchportAccessVlan.cpp | 31 ++++++- ...onfigInterfaceSwitchportAccessVlanTest.cpp | 86 +++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) 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 index 2ab6a2bf0714e..48eb95c7bf25b 100644 --- a/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp @@ -31,6 +31,36 @@ CmdConfigInterfaceSwitchportAccessVlan::queryClient( // Extract the VLAN ID (validation already done in VlanIdValue constructor) int32_t vlanId = vlanIdValue.getVlanId(); + // Validate that the VLAN exists and has an interface before modifying config + auto& config = ConfigSession::getInstance().getAgentConfig(); + bool vlanExists = false; + for (const auto& vlan : *config.sw()->vlans()) { + if (*vlan.id() == vlanId) { + vlanExists = true; + break; + } + } + if (!vlanExists) { + throw std::invalid_argument( + "VLAN " + std::to_string(vlanId) + + " does not exist. Create the VLAN first before assigning ports to it."); + } + bool isDefaultVlan = *config.sw()->defaultVlan() == vlanId; + if (!isDefaultVlan) { + bool vlanHasInterface = false; + for (const auto& intf : *config.sw()->interfaces()) { + if (*intf.vlanID() == vlanId) { + vlanHasInterface = true; + break; + } + } + if (!vlanHasInterface) { + throw std::invalid_argument( + "VLAN " + std::to_string(vlanId) + + " has no interface. Add an interface to the VLAN before assigning ports to it."); + } + } + // Collect the logical port IDs we need to update std::unordered_set portIds; @@ -44,7 +74,6 @@ CmdConfigInterfaceSwitchportAccessVlan::queryClient( } // 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())) { diff --git a/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp index ae336dbe32392..309634df7e025 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp @@ -52,6 +52,17 @@ class CmdConfigInterfaceSwitchportAccessVlanTestFixture "spanningTreeState": 2, "emitTags": false } + ], + "defaultVlan": 4000, + "vlans": [ + {"id": 1, "name": "vlan1", "routable": true, "intfID": 1}, + {"id": 2001, "name": "vlan2001", "routable": true, "intfID": 2001}, + {"id": 3000, "name": "vlan3000", "routable": true, "intfID": 0}, + {"id": 4000, "name": "vlan4000", "routable": false, "intfID": 0} + ], + "interfaces": [ + {"intfID": 1, "vlanID": 1, "routerID": 0, "type": 1, "mtu": 9412}, + {"intfID": 2001, "vlanID": 2001, "routerID": 0, "type": 1, "mtu": 9412} ] } })") {} @@ -191,4 +202,79 @@ TEST_F( std::invalid_argument); } +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + queryClientThrowsWhenVlanDoesNotExist) { + auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); + utils::InterfaceList interfaces({"eth1/1/1"}); + VlanIdValue vlanId({"4094"}); + + try { + cmd.queryClient(localhost(), interfaces, vlanId); + FAIL() << "Expected std::invalid_argument"; + } catch (const std::invalid_argument& e) { + std::string msg = e.what(); + EXPECT_THAT(msg, HasSubstr("VLAN 4094")); + EXPECT_THAT(msg, HasSubstr("does not exist")); + } + + auto& config = ConfigSession::getInstance().getAgentConfig(); + for (const auto& port : *config.sw()->ports()) { + if (*port.name() == "eth1/1/1") { + EXPECT_EQ(*port.ingressVlan(), 1); + } + } +} + +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + queryClientThrowsWhenVlanHasNoInterface) { + auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); + utils::InterfaceList interfaces({"eth1/1/1"}); + VlanIdValue vlanId({"3000"}); + + try { + cmd.queryClient(localhost(), interfaces, vlanId); + FAIL() << "Expected std::invalid_argument"; + } catch (const std::invalid_argument& e) { + std::string msg = e.what(); + EXPECT_THAT(msg, HasSubstr("VLAN 3000")); + EXPECT_THAT(msg, HasSubstr("no interface")); + } + + auto& config = ConfigSession::getInstance().getAgentConfig(); + for (const auto& port : *config.sw()->ports()) { + if (*port.name() == "eth1/1/1") { + EXPECT_EQ(*port.ingressVlan(), 1); + } + } + for (const auto& vlanPort : *config.sw()->vlanPorts()) { + if (*vlanPort.logicalPort() == 1) { + EXPECT_EQ(*vlanPort.vlanID(), 1); + } + } +} + +// The default VLAN (sw.defaultVlan) is a valid target even when it has no +// L3 interface — FBOSS allows ports to be assigned to it without one. +TEST_F( + CmdConfigInterfaceSwitchportAccessVlanTestFixture, + queryClientSucceedsForDefaultVlanWithNoInterface) { + auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); + utils::InterfaceList interfaces({"eth1/1/1"}); + VlanIdValue vlanId( + {"4000"}); // defaultVlan in fixture, no sw.interfaces entry + + auto result = cmd.queryClient(localhost(), interfaces, vlanId); + EXPECT_THAT(result, HasSubstr("Successfully set access VLAN")); + EXPECT_THAT(result, HasSubstr("4000")); + + auto& config = ConfigSession::getInstance().getAgentConfig(); + for (const auto& port : *config.sw()->ports()) { + if (*port.name() == "eth1/1/1") { + EXPECT_EQ(*port.ingressVlan(), 4000); + } + } +} + } // namespace facebook::fboss From 041142e39581dd5515fdcc115dbfdfeb5988dee1 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 13 May 2026 05:30:06 +0000 Subject: [PATCH 2/2] remove interface check --- ...CmdConfigInterfaceSwitchportAccessVlan.cpp | 17 +------ ...onfigInterfaceSwitchportAccessVlanTest.cpp | 51 ------------------- 2 files changed, 1 insertion(+), 67 deletions(-) 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 index 48eb95c7bf25b..cf035e32d69e9 100644 --- a/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp +++ b/fboss/cli/fboss2/commands/config/interface/switchport/access/vlan/CmdConfigInterfaceSwitchportAccessVlan.cpp @@ -31,7 +31,7 @@ CmdConfigInterfaceSwitchportAccessVlan::queryClient( // Extract the VLAN ID (validation already done in VlanIdValue constructor) int32_t vlanId = vlanIdValue.getVlanId(); - // Validate that the VLAN exists and has an interface before modifying config + // Validate that the VLAN exists before modifying config auto& config = ConfigSession::getInstance().getAgentConfig(); bool vlanExists = false; for (const auto& vlan : *config.sw()->vlans()) { @@ -45,21 +45,6 @@ CmdConfigInterfaceSwitchportAccessVlan::queryClient( "VLAN " + std::to_string(vlanId) + " does not exist. Create the VLAN first before assigning ports to it."); } - bool isDefaultVlan = *config.sw()->defaultVlan() == vlanId; - if (!isDefaultVlan) { - bool vlanHasInterface = false; - for (const auto& intf : *config.sw()->interfaces()) { - if (*intf.vlanID() == vlanId) { - vlanHasInterface = true; - break; - } - } - if (!vlanHasInterface) { - throw std::invalid_argument( - "VLAN " + std::to_string(vlanId) + - " has no interface. Add an interface to the VLAN before assigning ports to it."); - } - } // Collect the logical port IDs we need to update std::unordered_set portIds; diff --git a/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp index 309634df7e025..915134089b11e 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigInterfaceSwitchportAccessVlanTest.cpp @@ -226,55 +226,4 @@ TEST_F( } } -TEST_F( - CmdConfigInterfaceSwitchportAccessVlanTestFixture, - queryClientThrowsWhenVlanHasNoInterface) { - auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); - utils::InterfaceList interfaces({"eth1/1/1"}); - VlanIdValue vlanId({"3000"}); - - try { - cmd.queryClient(localhost(), interfaces, vlanId); - FAIL() << "Expected std::invalid_argument"; - } catch (const std::invalid_argument& e) { - std::string msg = e.what(); - EXPECT_THAT(msg, HasSubstr("VLAN 3000")); - EXPECT_THAT(msg, HasSubstr("no interface")); - } - - auto& config = ConfigSession::getInstance().getAgentConfig(); - for (const auto& port : *config.sw()->ports()) { - if (*port.name() == "eth1/1/1") { - EXPECT_EQ(*port.ingressVlan(), 1); - } - } - for (const auto& vlanPort : *config.sw()->vlanPorts()) { - if (*vlanPort.logicalPort() == 1) { - EXPECT_EQ(*vlanPort.vlanID(), 1); - } - } -} - -// The default VLAN (sw.defaultVlan) is a valid target even when it has no -// L3 interface — FBOSS allows ports to be assigned to it without one. -TEST_F( - CmdConfigInterfaceSwitchportAccessVlanTestFixture, - queryClientSucceedsForDefaultVlanWithNoInterface) { - auto cmd = CmdConfigInterfaceSwitchportAccessVlan(); - utils::InterfaceList interfaces({"eth1/1/1"}); - VlanIdValue vlanId( - {"4000"}); // defaultVlan in fixture, no sw.interfaces entry - - auto result = cmd.queryClient(localhost(), interfaces, vlanId); - EXPECT_THAT(result, HasSubstr("Successfully set access VLAN")); - EXPECT_THAT(result, HasSubstr("4000")); - - auto& config = ConfigSession::getInstance().getAgentConfig(); - for (const auto& port : *config.sw()->ports()) { - if (*port.name() == "eth1/1/1") { - EXPECT_EQ(*port.ingressVlan(), 4000); - } - } -} - } // namespace facebook::fboss