diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 994e61995250a..1d6c97823a12e 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -867,6 +867,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.cpp fboss/cli/fboss2/commands/config/vlan/CmdConfigVlan.cpp fboss/cli/fboss2/commands/config/vlan/CmdConfigVlan.h + fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.cpp + fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.h fboss/cli/fboss2/commands/config/vlan/port/CmdConfigVlanPort.cpp fboss/cli/fboss2/commands/config/vlan/port/CmdConfigVlanPort.h fboss/cli/fboss2/commands/config/vlan/port/tagging_mode/CmdConfigVlanPortTaggingMode.h diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index f96c1b51ee1f7..d7f8dc029a0e8 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -16,6 +16,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigSessionDiffTest.cpp fboss/cli/fboss2/test/config/CmdConfigSessionTest.cpp fboss/cli/fboss2/test/config/CmdConfigTestBase.cpp + fboss/cli/fboss2/test/config/CmdConfigVlanDefaultTest.cpp fboss/cli/fboss2/test/config/CmdConfigVlanPortTaggingModeTest.cpp fboss/cli/fboss2/test/config/CmdConfigVlanStaticMacTest.cpp fboss/cli/fboss2/test/config/ConfigSessionSystemdTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index fcabb298c603b..dca0da77a576b 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -18,6 +18,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigPortQueueConfigTest.cpp fboss/cli/fboss2/test/integration_test/ConfigQosPolicyMapTest.cpp fboss/cli/fboss2/test/integration_test/ConfigSessionClearTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp fboss/cli/fboss2/test/integration_test/ConfigVlanPortTaggingModeTest.cpp fboss/cli/fboss2/test/integration_test/ConfigVlanStaticMacTest.cpp fboss/cli/fboss2/test/integration_test/ConfigVlanSwitchportAccessTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 55ee76758b2f9..720fef16d60e1 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1105,6 +1105,7 @@ cpp_library( "commands/config/session/CmdConfigSessionDiff.cpp", "commands/config/session/CmdConfigSessionRebase.cpp", "commands/config/vlan/CmdConfigVlan.cpp", + "commands/config/vlan/CmdConfigVlanDefault.cpp", "commands/config/vlan/port/CmdConfigVlanPort.cpp", "commands/config/vlan/port/tagging_mode/CmdConfigVlanPortTaggingMode.cpp", "commands/config/vlan/static_mac/CmdConfigVlanStaticMac.cpp", @@ -1187,6 +1188,7 @@ cpp_library( "commands/config/session/CmdConfigSessionDiff.h", "commands/config/session/CmdConfigSessionRebase.h", "commands/config/vlan/CmdConfigVlan.h", + "commands/config/vlan/CmdConfigVlanDefault.h", "commands/config/vlan/port/CmdConfigVlanPort.h", "commands/config/vlan/port/tagging_mode/CmdConfigVlanPortTaggingMode.h", "commands/config/vlan/static_mac/CmdConfigVlanStaticMac.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 1ca2688d35c84..4de8bfbb89d34 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -95,6 +95,7 @@ #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionDiff.h" #include "fboss/cli/fboss2/commands/config/session/CmdConfigSessionRebase.h" #include "fboss/cli/fboss2/commands/config/vlan/CmdConfigVlan.h" +#include "fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.h" #include "fboss/cli/fboss2/commands/config/vlan/port/CmdConfigVlanPort.h" #include "fboss/cli/fboss2/commands/config/vlan/port/tagging_mode/CmdConfigVlanPortTaggingMode.h" #include "fboss/cli/fboss2/commands/config/vlan/static_mac/CmdConfigVlanStaticMac.h" @@ -774,6 +775,23 @@ const CommandTree& kConfigCommandTree() { }}, }, + // Registered separately (merged into the existing "config vlan" by + // CmdSubcommands::addCommandBranch) so "default" lands at depth 0 and + // owns data_[0] for its own VLAN ID arg, independent of the + // "config vlan " context (whose arg also lives at data_[0] but is + // not populated when the "default" subcommand is matched). + { + "config", + "vlan", + "Configure VLAN settings", + {{ + "default", + "Set global default VLAN ID for untagged traffic", + commandHandler, + argRegistrar, + }}, + }, + {"delete", "config", "Delete config objects", diff --git a/fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.cpp b/fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.cpp new file mode 100644 index 0000000000000..af2b075f60e6c --- /dev/null +++ b/fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.cpp @@ -0,0 +1,116 @@ +/* + * 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/vlan/CmdConfigVlanDefault.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include "fboss/agent/gen-cpp2/switch_config_types.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +CmdConfigVlanDefaultTraits::RetType CmdConfigVlanDefault::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& vlanIdArg) { + auto& session = ConfigSession::getInstance(); + auto& config = session.getAgentConfig(); + auto& swConfig = *config.sw(); + int32_t vlanId = vlanIdArg.getVlanId(); + int32_t currentDefault = *swConfig.defaultVlan(); + + if (currentDefault == vlanId) { + return fmt::format("Default VLAN is already set to {}", vlanId); + } + + // Small helper for ID-based membership checks on different config lists. + auto hasElemWithId = [](const auto& list, int id, auto accessor) { + return std::any_of(list.begin(), list.end(), [&](const auto& elem) { + return accessor(elem) == id; + }); + }; + + // Validate the current default VLAN before changing it. + // + // 1) If the old default VLAN is used as ingressVlan for any port and also + // has an Interface, we allow the change. + // 2) If the old default VLAN is used as ingressVlan for any port but has + // no Interface, we refuse to proceed and ask the user to fix the config + // first (this is the state that causes fboss_sw_agent to crash). + bool oldVlanHasInterface = hasElemWithId( + *swConfig.interfaces(), currentDefault, [](const auto& intf) { + return *intf.vlanID(); + }); + + bool oldVlanUsedAsIngress = + hasElemWithId(*swConfig.ports(), currentDefault, [](const auto& port) { + return *port.ingressVlan(); + }); + + if (oldVlanUsedAsIngress && !oldVlanHasInterface) { + return fmt::format( + "Refusing to change default VLAN from {} to {} because VLAN {} is " + "used as ingressVlan for one or more ports but has no interface. " + "Move those ports to a different VLAN or attach an interface before " + "changing the default.", + currentDefault, + vlanId, + currentDefault); + } + + // New default VLAN handling: + // 3) If the new VLAN already exists in the VLAN table, do not touch it; + // just update sw.defaultVlan. + // 4) If the new VLAN does not exist, create a non-routable placeholder + // entry for it (no interface). + auto& vlans = *swConfig.vlans(); + auto findById = [](auto& vlanList, int id) { + return std::find_if( + vlanList.begin(), vlanList.end(), [id](const auto& vlan) { + return *vlan.id() == id; + }); + }; + + auto targetVlanIt = findById(vlans, vlanId); + if (targetVlanIt == vlans.end()) { + cfg::Vlan newVlan; + newVlan.id() = vlanId; + newVlan.name() = fmt::format("default_{}", vlanId); + newVlan.routable() = false; + vlans.push_back(newVlan); + } + + // Remove the older VLAN id from the VLAN object, if it does not have any + // interface associated with it. + auto oldVlanIt = findById(vlans, currentDefault); + if (!oldVlanHasInterface && oldVlanIt != vlans.end()) { + vlans.erase(oldVlanIt); + } + + swConfig.defaultVlan() = vlanId; + + session.saveConfig(); + + return fmt::format( + "Successfully set default VLAN to {} (was {})", vlanId, currentDefault); +} + +void CmdConfigVlanDefault::printOutput(const RetType& logMsg) { + std::cout << logMsg << std::endl; +} + +// Explicit template instantiation +template void +CmdHandler::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.h b/fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.h new file mode 100644 index 0000000000000..6508af4642198 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.h @@ -0,0 +1,37 @@ +/* + * 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/utils/CmdUtils.h" + +namespace facebook::fboss { + +struct CmdConfigVlanDefaultTraits : public WriteCommandTraits { + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("vlan_id", args, "VLAN ID (1-4094)"); + } + using ObjectArgType = utils::VlanIdValue; + using RetType = std::string; +}; + +class CmdConfigVlanDefault + : public CmdHandler { + public: + using ObjectArgType = CmdConfigVlanDefaultTraits::ObjectArgType; + using RetType = CmdConfigVlanDefaultTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& vlanId); + + void printOutput(const RetType& logMsg); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/BUCK b/fboss/cli/fboss2/test/config/BUCK index 2cdcea444ea14..44cbe48316b88 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -18,6 +18,7 @@ cpp_unittest( "CmdConfigSessionDiffTest.cpp", "CmdConfigSessionTest.cpp", "CmdConfigTestBase.cpp", + "CmdConfigVlanDefaultTest.cpp", "CmdConfigVlanPortTaggingModeTest.cpp", "CmdConfigVlanStaticMacTest.cpp", "ConfigSessionSystemdTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigVlanDefaultTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigVlanDefaultTest.cpp new file mode 100644 index 0000000000000..469761535fd86 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigVlanDefaultTest.cpp @@ -0,0 +1,242 @@ +/* + * 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 "fboss/cli/fboss2/commands/config/vlan/CmdConfigVlanDefault.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" + +using namespace ::testing; + +namespace facebook::fboss { + +using VlanId = CmdConfigVlanDefaultTraits::ObjectArgType; + +// Fixture with a config that has a VLAN whose id matches the current default. +class CmdConfigVlanDefaultTestFixture : public CmdConfigTestBase { + public: + CmdConfigVlanDefaultTestFixture() + : CmdConfigTestBase( + "fboss2_config_vlan_default_test_%%%%-%%%%-%%%%-%%%%", + R"({ + "sw": { + "vlans": [{"id": 100, "name": "default", "routable": true}], + "defaultVlan": 100 + } +})") {} + + protected: + const std::string cmdPrefix_ = "config vlan"; +}; + +// Fixture where no VLAN in the list matches the current defaultVlan id. +class CmdConfigVlanDefaultNoMatchFixture : public CmdConfigTestBase { + public: + CmdConfigVlanDefaultNoMatchFixture() + : CmdConfigTestBase( + "fboss2_config_vlan_default_nomatch_test_%%%%-%%%%-%%%%-%%%%", + R"({ + "sw": { + "vlans": [{"id": 200, "name": "other-vlan"}], + "defaultVlan": 100 + } +})") {} + + protected: + const std::string cmdPrefix_ = "config vlan"; +}; + +// Fixture where the target VLAN already exists but doesn't match defaultVlan. +class CmdConfigVlanDefaultTargetExistsFixture : public CmdConfigTestBase { + public: + CmdConfigVlanDefaultTargetExistsFixture() + : CmdConfigTestBase( + "fboss2_config_vlan_default_targetexists_test_%%%%-%%%%-%%%%-%%%%", + R"({ + "sw": { + "vlans": [ + {"id": 200, "name": "other-vlan"}, + {"id": 300, "name": "target-vlan"} + ], + "defaultVlan": 100 + } +})") {} + + protected: + const std::string cmdPrefix_ = "config vlan"; +}; + +// Fixture with no defaultVlan field set. +class CmdConfigVlanDefaultNoFieldFixture : public CmdConfigTestBase { + public: + CmdConfigVlanDefaultNoFieldFixture() + : CmdConfigTestBase( + "fboss2_config_vlan_default_nofield_test_%%%%-%%%%-%%%%-%%%%", + R"({ + "sw": { + "vlans": [] + } +})") {} + + protected: + const std::string cmdPrefix_ = "config vlan"; +}; + +// ============================================================================ +// Tests for CmdConfigVlanDefault::queryClient +// ============================================================================ + +TEST_F(CmdConfigVlanDefaultTestFixture, setDefaultVlanAlreadySet) { + setupTestableConfigSession(cmdPrefix_, "default 100"); + auto result = + CmdConfigVlanDefault().queryClient(localhost(), VlanId({"100"})); + EXPECT_THAT(result, HasSubstr("already set to 100")); +} + +TEST_F(CmdConfigVlanDefaultTestFixture, setDefaultVlanSuccess) { + setupTestableConfigSession(cmdPrefix_, "default 300"); + auto& swConfig = *ConfigSession::getInstance().getAgentConfig().sw(); + + auto result = + CmdConfigVlanDefault().queryClient(localhost(), VlanId({"300"})); + + EXPECT_THAT(result, HasSubstr("Successfully set default VLAN to 300")); + EXPECT_THAT(result, HasSubstr("was 100")); + EXPECT_EQ(*swConfig.defaultVlan(), 300); + + // Since VLAN 100 has no interface, it should be removed when we change + // the default VLAN. A new VLAN 300 is created. + int count100 = 0; + int count300 = 0; + for (const auto& v : *swConfig.vlans()) { + if (*v.id() == 100) { + ++count100; + } else if (*v.id() == 300) { + ++count300; + } + } + EXPECT_EQ(count100, 0); // Old VLAN removed (no interface) + EXPECT_EQ(count300, 1); // New VLAN created +} + +TEST_F(CmdConfigVlanDefaultTestFixture, setDefaultVlanUpdatesOnlyOneEntry) { + setupTestableConfigSession(cmdPrefix_, "default 300"); + auto& swConfig = *ConfigSession::getInstance().getAgentConfig().sw(); + + CmdConfigVlanDefault().queryClient(localhost(), VlanId({"300"})); + + // Ensure we did not create duplicate entries. Old VLAN 100 is removed + // since it has no interface. + int count100 = 0; + int count300 = 0; + for (const auto& v : *swConfig.vlans()) { + if (*v.id() == 100) { + ++count100; + } + if (*v.id() == 300) { + ++count300; + } + } + EXPECT_EQ(count100, 0); // Old VLAN removed (no interface) + EXPECT_EQ(count300, 1); // New VLAN created +} + +TEST_F(CmdConfigVlanDefaultTestFixture, changeDefaultVlanMultipleTimes) { + setupTestableConfigSession(cmdPrefix_, "default 200"); + auto& swConfig = *ConfigSession::getInstance().getAgentConfig().sw(); + + auto r1 = CmdConfigVlanDefault().queryClient(localhost(), VlanId({"200"})); + EXPECT_THAT(r1, HasSubstr("Successfully set default VLAN to 200")); + EXPECT_THAT(r1, HasSubstr("was 100")); + EXPECT_EQ(*swConfig.defaultVlan(), 200); + + auto r2 = CmdConfigVlanDefault().queryClient(localhost(), VlanId({"400"})); + EXPECT_THAT(r2, HasSubstr("Successfully set default VLAN to 400")); + EXPECT_THAT(r2, HasSubstr("was 200")); + EXPECT_EQ(*swConfig.defaultVlan(), 400); + + // After multiple changes, only the final VLAN 400 should remain. + // Each previous default VLAN is removed if it has no interface. + // 100 removed when changing to 200, 200 removed when changing to 400. + int count100 = 0; + int count200 = 0; + int count400 = 0; + for (const auto& v : *swConfig.vlans()) { + if (*v.id() == 100) { + ++count100; + } else if (*v.id() == 200) { + ++count200; + } else if (*v.id() == 400) { + ++count400; + } + } + EXPECT_EQ(count100, 0); // Old VLAN removed (no interface) + EXPECT_EQ(count200, 0); // Old VLAN removed (no interface) + EXPECT_EQ(count400, 1); // Current default VLAN +} + +// When no VLAN in the list matches currentDefault but the target VLAN id +// doesn't exist either, the command must create a new VLAN entry. +TEST_F(CmdConfigVlanDefaultNoMatchFixture, createsNewVlanWhenNeitherExists) { + setupTestableConfigSession(cmdPrefix_, "default 300"); + auto& swConfig = *ConfigSession::getInstance().getAgentConfig().sw(); + + auto result = + CmdConfigVlanDefault().queryClient(localhost(), VlanId({"300"})); + + EXPECT_THAT(result, HasSubstr("Successfully set default VLAN to 300")); + EXPECT_EQ(*swConfig.defaultVlan(), 300); + + auto vitr = std::find_if( + swConfig.vlans()->cbegin(), swConfig.vlans()->cend(), [](const auto& v) { + return *v.id() == 300; + }); + ASSERT_NE(vitr, swConfig.vlans()->cend()); +} + +// When no VLAN in the list matches currentDefault but the target VLAN already +// exists, that existing entry must be reused (not duplicated). +TEST_F(CmdConfigVlanDefaultTargetExistsFixture, reusesExistingTargetVlan) { + setupTestableConfigSession(cmdPrefix_, "default 300"); + auto& swConfig = *ConfigSession::getInstance().getAgentConfig().sw(); + + auto result = + CmdConfigVlanDefault().queryClient(localhost(), VlanId({"300"})); + + EXPECT_THAT(result, HasSubstr("Successfully set default VLAN to 300")); + EXPECT_EQ(*swConfig.defaultVlan(), 300); + + // Exactly one VLAN with id=300. + int count = 0; + for (const auto& v : *swConfig.vlans()) { + if (v.id().has_value() && *v.id() == 300) { + ++count; + } + } + EXPECT_EQ(count, 1); +} + +// When defaultVlan is not present in the config, the command must fall back to +// the system constant and still complete successfully. +TEST_F(CmdConfigVlanDefaultNoFieldFixture, succeedsWithNoDefaultVlanField) { + setupTestableConfigSession(cmdPrefix_, "default 300"); + auto& swConfig = *ConfigSession::getInstance().getAgentConfig().sw(); + + auto result = + CmdConfigVlanDefault().queryClient(localhost(), VlanId({"300"})); + + EXPECT_THAT(result, HasSubstr("Successfully set default VLAN to 300")); + EXPECT_EQ(*swConfig.defaultVlan(), 300); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 8d20b4185b44d..dd8a99b12cd7e 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -26,6 +26,7 @@ cpp_binary( "ConfigPortQueueConfigTest.cpp", "ConfigQosPolicyMapTest.cpp", "ConfigSessionClearTest.cpp", + "ConfigVlanDefaultTest.cpp", "ConfigVlanPortTaggingModeTest.cpp", "ConfigVlanStaticMacTest.cpp", "ConfigVlanSwitchportAccessTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp new file mode 100644 index 0000000000000..31163b1dad53a --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp @@ -0,0 +1,365 @@ +/** + * End-to-end tests for 'fboss2-dev config vlan default '. + * + * Covers the happy-path workflow (move ports, set default, commit, verify via + * thrift), the no-op case when the target equals the current default, a safety + * guard that refuses the command when the precondition is unsafe, and the case + * where ports have already been moved off the current default VLAN. + */ + +#include +#include +#include +#include +#include +#include +#include "fboss/agent/if/gen-cpp2/FbossCtrlAsyncClient.h" +#include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" +#include "fboss/cli/fboss2/utils/CmdClientUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +using namespace facebook::fboss; + +class ConfigVlanDefaultTest : public Fboss2IntegrationTest { + protected: + folly::dynamic getRunningConfig() const { + HostInfo hostInfo("localhost"); + auto client = + utils::createClient>(hostInfo); + std::string configStr; + client->sync_getRunningConfig(configStr); + return folly::parseJson(configStr); + } +}; + +/** + * Moves all eth ports to a target VLAN, sets it as the new default, commits + * once, and verifies via thrift. Restores original VLAN membership and + * defaultVlan at the end. + */ +TEST_F(ConfigVlanDefaultTest, SetDefaultVlanTo300) { + waitForAgentReady(); + + // Pick a target that differs from the current default so the commit is + // never a no-op. + auto initialConfig = getRunningConfig(); + int32_t currentDefault = + initialConfig["sw"].getDefault("defaultVlan", 1).asInt(); + const std::string targetVlan = (currentDefault == 300) ? "301" : "300"; + XLOG(INFO) << "[Test] currentDefault=" << currentDefault + << " targetVlan=" << targetVlan; + + // Step 0: Create the target VLAN if it doesn't already exist. + // VlanManager auto-creates the VLAN entry and a barebone cfg::Interface so + // the agent accepts the commit. The dummy MAC is removed in restore below. + const std::string kDummyMac = "02:00:00:00:01:2c"; + bool targetVlanCreated = false; + { + bool exists = false; + for (const auto& vlan : initialConfig["sw"]["vlans"]) { + if (vlan.getDefault("id", -1).asInt() == std::stoi(targetVlan)) { + exists = true; + break; + } + } + if (!exists) { + auto firstEthPort = findFirstEthInterface().name; + ASSERT_FALSE(firstEthPort.empty()) + << "No eth port found for VLAN creation"; + auto createResult = runCli( + {"config", + "vlan", + targetVlan, + "static-mac", + "add", + kDummyMac, + firstEthPort}); + ASSERT_EQ(createResult.exitCode, 0) + << "Failed to create VLAN " << targetVlan << ": " + << createResult.stderr; + XLOG(INFO) << "[Step 0] " << createResult.stdout; + targetVlanCreated = true; + } + } + + // Step 1: Move all eth ports whose ingressVlan matches the current default + // VLAN + for (const auto& port : initialConfig["sw"]["ports"]) { + auto name = port.getDefault("name", "").asString(); + if (name.rfind("eth", 0) != 0) { + continue; + } + if (port.getDefault("ingressVlan", currentDefault).asInt() != + currentDefault) { + continue; + } + auto result = runCli( + {"config", + "interface", + name, + "switchport", + "access", + "vlan", + targetVlan}); + if (result.exitCode != 0) { + XLOG(WARN) << "Skipping " << name << ": " << result.stderr; + continue; + } + } + // Step 2: Set default VLAN + auto setResult = runCli({"config", "vlan", "default", targetVlan}); + ASSERT_EQ(setResult.exitCode, 0) << setResult.stderr; + EXPECT_THAT( + setResult.stdout, + ::testing::HasSubstr("Successfully set default VLAN to " + targetVlan)); + XLOG(INFO) << "[Step 2] " << setResult.stdout; + + // Step 3: Single commit — one restart, then wait only for the thrift server + // to load the config (not full ASIC init, which takes much longer). + commitConfig(); + waitForAgentReady(); + // Step 4: Verify via running config. + // sync_getRunningConfig reflects the committed config as soon as the agent + // loads it, independently of ASIC programming progress. + auto config = getRunningConfig(); + EXPECT_EQ(config["sw"]["defaultVlan"].asInt(), std::stoi(targetVlan)); + XLOG(INFO) << "[Step 4] Verified defaultVlan=" + << config["sw"]["defaultVlan"].asInt(); + + // Restore: move every eth port that originally had ingressVlan == + // currentDefault back to its original VLAN, reset defaultVlan, and commit so + // the next test starts from the original state. + XLOG(INFO) << "[Restore] Reverting ports and defaultVlan to " + << currentDefault; + if (targetVlanCreated) { + runCli({"config", "vlan", targetVlan, "static-mac", "delete", kDummyMac}); + } + for (const auto& port : initialConfig["sw"]["ports"]) { + auto name = port.getDefault("name", "").asString(); + if (name.rfind("eth", 0) != 0) { + continue; + } + auto origVlan = port.getDefault("ingressVlan", currentDefault).asInt(); + if (origVlan != currentDefault) { + continue; + } + auto r = runCli( + {"config", + "interface", + name, + "switchport", + "access", + "vlan", + std::to_string(origVlan)}); + if (r.exitCode != 0) { + XLOG(WARN) << "[Restore] Failed to move " << name << " back to VLAN " + << origVlan << ": " << r.stderr; + } + } + + auto restoreDefault = + runCli({"config", "vlan", "default", std::to_string(currentDefault)}); + ASSERT_EQ(restoreDefault.exitCode, 0) + << "[Restore] Failed to reset default VLAN: " << restoreDefault.stderr; + + commitConfig(); + waitForAgentReady(); + discardSession(); +} + +/** + * Idempotency test: setting the default VLAN to its current value should be a + * no-op and return a clear message, without changing the running config. + */ +TEST_F(ConfigVlanDefaultTest, NoOpWhenDefaultVlanUnchanged) { + waitForAgentReady(); + + auto initialConfig = getRunningConfig(); + int32_t currentDefault = + initialConfig["sw"].getDefault("defaultVlan", 1).asInt(); + + auto result = + runCli({"config", "vlan", "default", std::to_string(currentDefault)}); + ASSERT_EQ(result.exitCode, 0) << result.stderr; + EXPECT_THAT( + result.stdout, + ::testing::HasSubstr( + "Default VLAN is already set to " + std::to_string(currentDefault))); + + // No commit needed (command is a no-op), but ensure we don't leave a stale + // session lying around for subsequent tests. + discardSession(); +} + +/** + * Validation guard: if the current default VLAN has at least one port whose + * ingressVlan equals it but has no matching interface, the command must refuse + * with a descriptive message and leave the config unchanged. + * + * This is the path guarded by `oldVlanUsedAsIngress && !oldVlanHasInterface` + * in CmdConfigVlanDefault::queryClient. + * + * To keep the test simple, we do not try to synthesize this "dangerous" state. + * Instead, we only run the negative test when the running config already has + * a default VLAN that is used as ingressVlan by at least one port and has no + * interface. If those conditions are not met, the test is skipped. + */ +TEST_F(ConfigVlanDefaultTest, RefuseWhenPortOnDefaultVlanWithNoInterface) { + waitForAgentReady(); + + auto initialConfig = getRunningConfig(); + int32_t currentDefault = + initialConfig["sw"].getDefault("defaultVlan", 1).asInt(); + + // Check if any port uses the current default VLAN as its ingressVlan. + bool portOnDefault = false; + for (const auto& port : initialConfig["sw"]["ports"]) { + if (port.getDefault("ingressVlan", currentDefault).asInt() == + currentDefault) { + portOnDefault = true; + break; + } + } + if (!portOnDefault) { + GTEST_SKIP() << "No port uses the default VLAN as ingressVlan — cannot " + "exercise guard"; + } + + // Check that there is *no* interface mapped to the current default VLAN. + bool interfaceOnDefault = false; + if (initialConfig["sw"].count("interfaces")) { + for (const auto& intf : initialConfig["sw"]["interfaces"]) { + if (intf.getDefault("vlanID", -1).asInt() == currentDefault) { + interfaceOnDefault = true; + break; + } + } + } + if (interfaceOnDefault) { + GTEST_SKIP() << "Default VLAN has an interface — dangerous state not " + "present"; + } + + // At this point the running config matches the guard's precondition: + // - At least one port has ingressVlan == currentDefault + // - No interface has vlanID == currentDefault + // Any attempt to change the default VLAN must be refused. + const std::string nextVlan = (currentDefault == 300) ? "301" : "300"; + auto result = runCli({"config", "vlan", "default", nextVlan}); + EXPECT_THAT( + result.stdout, ::testing::HasSubstr("Refusing to change default VLAN")); + XLOG(INFO) << "[NegativeGuard] stdout='" << result.stdout << "'"; + + // Nothing was committed — discard is sufficient to restore state. + discardSession(); +} + +/** + * Verifies that changing the default VLAN succeeds when at least one port has + * an ingressVlan that differs from the current defaultVlan. + * + * Uses an existing VLAN (one with an interface already mapped) so the VLAN + * table entry is guaranteed to exist when we set it as the new default. + */ +TEST_F(ConfigVlanDefaultTest, ChangeDefaultVlanWithPortInNonDefaultVlan) { + waitForAgentReady(); + + auto initialConfig = getRunningConfig(); + int32_t currentDefault = + initialConfig["sw"].getDefault("defaultVlan", 1).asInt(); + + // Find an existing non-default VLAN to use as the port reassignment target. + int32_t sideVlan = -1; + for (const auto& vlan : initialConfig["sw"]["vlans"]) { + int32_t vid = vlan.getDefault("id", -1).asInt(); + if (vid > 0 && vid != currentDefault) { + sideVlan = vid; + break; + } + } + if (sideVlan == -1) { + GTEST_SKIP() << "No VLAN other than the default found in the VLAN table"; + } + XLOG(INFO) << "currentDefault=" << currentDefault << " sideVlan=" << sideVlan; + + // Reassign ports off the current default VLAN to exercise the case where + // ingressVlan differs from defaultVlan. + int portsMoved = 0; + for (const auto& port : initialConfig["sw"]["ports"]) { + auto name = port.getDefault("name", "").asString(); + if (name.rfind("eth", 0) != 0) { + continue; + } + if (port.getDefault("ingressVlan", currentDefault).asInt() != + currentDefault) { + continue; + } + auto r = runCli( + {"config", + "interface", + name, + "switchport", + "access", + "vlan", + std::to_string(sideVlan)}); + if (r.exitCode == 0) { + ++portsMoved; + } + } + if (portsMoved == 0) { + GTEST_SKIP() << "No eth ports on the default VLAN to move"; + } + XLOG(INFO) << "[Step 1] Moved " << portsMoved << " ports from VLAN " + << currentDefault << " to " << sideVlan; + + // Set a new default VLAN — the command must succeed even when no ports + // remain on the old default VLAN. + const int32_t newDefault = 4093; + auto result = + runCli({"config", "vlan", "default", std::to_string(newDefault)}); + ASSERT_EQ(result.exitCode, 0) + << "failed to set default VLAN: " << result.stderr; + XLOG(INFO) << "[Step 2] " << result.stdout; + + // Step 3: Commit and verify. + commitConfig(); + waitForAgentReady(); + auto postCommitConfig = getRunningConfig(); + EXPECT_EQ(postCommitConfig["sw"]["defaultVlan"].asInt(), newDefault); + + // Restore: move every eth port currently on sideVlan back to currentDefault + // and reset defaultVlan, then commit so the next test starts from the + // original state. + XLOG(INFO) << "[Restore] Moving ports from VLAN " << sideVlan << " back to " + << currentDefault; + for (const auto& port : postCommitConfig["sw"]["ports"]) { + auto name = port.getDefault("name", "").asString(); + if (name.rfind("eth", 0) != 0) { + continue; + } + if (port.getDefault("ingressVlan", sideVlan).asInt() != sideVlan) { + continue; + } + auto r = runCli( + {"config", + "interface", + name, + "switchport", + "access", + "vlan", + std::to_string(currentDefault)}); + if (r.exitCode != 0) { + XLOG(WARN) << "[Restore] Failed to move " << name << " back to VLAN " + << currentDefault << ": " << r.stderr; + } + } + + auto restoreDefault = + runCli({"config", "vlan", "default", std::to_string(currentDefault)}); + ASSERT_EQ(restoreDefault.exitCode, 0) + << "[Restore] Failed to reset default VLAN: " << restoreDefault.stderr; + + commitConfig(); + waitForAgentReady(); + discardSession(); +}