diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 54f2ad09f3b46..b1401bb363b86 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -762,6 +762,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.cpp fboss/cli/fboss2/commands/config/CmdConfigReload.h fboss/cli/fboss2/commands/config/CmdConfigReload.cpp + fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.cpp + fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.h fboss/cli/fboss2/commands/config/arp/CmdConfigArp.cpp fboss/cli/fboss2/commands/config/arp/CmdConfigArp.h fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.cpp diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index 09ff046abc3a0..b44b17da480bf 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -4,6 +4,7 @@ add_executable(fboss2_cmd_config_test fboss/util/oss/TestMain.cpp fboss/cli/fboss2/oss/config/CmdListImpl.cpp + fboss/cli/fboss2/test/config/CmdConfigAdminDistanceTest.cpp fboss/cli/fboss2/test/config/CmdConfigAppliedInfoTest.cpp fboss/cli/fboss2/test/config/CmdConfigArpTest.cpp fboss/cli/fboss2/test/config/CmdConfigHistoryTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index 10a258f26c742..bd418a7fc7c55 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -8,6 +8,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/oss/config/CmdListImpl.cpp fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigAdminDistanceTest.cpp fboss/cli/fboss2/test/integration_test/ConfigArpTest.cpp fboss/cli/fboss2/test/integration_test/ConfigConcurrentSessionsTest.cpp fboss/cli/fboss2/test/integration_test/ConfigInterfaceDescriptionTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index e1c8f3ab6f34c..47df243a83416 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1031,6 +1031,7 @@ cpp_library( "CmdListConfig.cpp", "commands/config/CmdConfigAppliedInfo.cpp", "commands/config/CmdConfigReload.cpp", + "commands/config/admin_distance/CmdConfigAdminDistance.cpp", "commands/config/arp/CmdConfigArp.cpp", "commands/config/history/CmdConfigHistory.cpp", "commands/config/interface/CmdConfigInterface.cpp", @@ -1140,6 +1141,7 @@ cpp_library( headers = [ "commands/config/CmdConfigAppliedInfo.h", "commands/config/CmdConfigReload.h", + "commands/config/admin_distance/CmdConfigAdminDistance.h", "commands/config/arp/CmdConfigArp.h", "commands/config/history/CmdConfigHistory.h", "commands/config/interface/CmdConfigInterface.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index b3432811bc3da..0f598aa7387bc 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -10,9 +10,9 @@ #include #include "fboss/cli/fboss2/CmdList.h" - #include "fboss/cli/fboss2/commands/config/CmdConfigAppliedInfo.h" #include "fboss/cli/fboss2/commands/config/CmdConfigReload.h" +#include "fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.h" #include "fboss/cli/fboss2/commands/config/arp/CmdConfigArp.h" #include "fboss/cli/fboss2/commands/config/history/CmdConfigHistory.h" #include "fboss/cli/fboss2/commands/config/interface/CmdConfigInterface.h" @@ -116,6 +116,12 @@ namespace facebook::fboss { const CommandTree& kConfigCommandTree() { static CommandTree root = { + {"config", + "admin-distance", + "Set administrative distance for a routing client", + commandHandler, + argRegistrar}, + {"config", "applied-info", "Show config applied information", diff --git a/fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.cpp b/fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.cpp new file mode 100644 index 0000000000000..115c783f28115 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.cpp @@ -0,0 +1,104 @@ +/* + * 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/admin_distance/CmdConfigAdminDistance.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" + +namespace facebook::fboss { + +namespace { +constexpr int32_t kMaxAdminDistance = 255; +} // namespace + +AdminDistanceArg::AdminDistanceArg(std::vector v) { + if (v.size() != 2) { + throw std::invalid_argument( + fmt::format( + "Expected exactly two arguments: , got {}", + v.size())); + } + + try { + clientId_ = folly::to(v[0]); + } catch (const folly::ConversionError&) { + throw std::invalid_argument( + fmt::format("Invalid client-id '{}': must be an integer", v[0])); + } + if (clientId_ < 0) { + throw std::invalid_argument( + fmt::format("client-id must be a non-negative integer, got {}", v[0])); + } + + try { + distance_ = folly::to(v[1]); + } catch (const folly::ConversionError&) { + throw std::invalid_argument( + fmt::format("Invalid distance '{}': must be an integer", v[1])); + } + if (distance_ < 0 || distance_ > kMaxAdminDistance) { + throw std::invalid_argument( + fmt::format( + "distance must be in the range [0, {}], got {}", + kMaxAdminDistance, + v[1])); + } + + data_ = std::move(v); +} + +CmdConfigAdminDistanceTraits::RetType CmdConfigAdminDistance::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& arg) { + auto& session = ConfigSession::getInstance(); + auto& config = session.getAgentConfig(); + auto& swConfig = *config.sw(); + + int32_t clientId = arg.getClientId(); + int32_t distance = arg.getDistance(); + + // Read-modify-write a single map entry, preserving all other clientId + // mappings already present in the config. + auto& adminDistanceMap = *swConfig.clientIdToAdminDistance(); + auto it = adminDistanceMap.find(clientId); + if (it != adminDistanceMap.end()) { + if (it->second == distance) { + return fmt::format( + "Admin distance for client-id {} is already set to {}", + clientId, + distance); + } + it->second = distance; + } else { + adminDistanceMap.emplace(clientId, distance); + } + + session.saveConfig(cli::ServiceType::AGENT, cli::ConfigActionLevel::HITLESS); + + return fmt::format( + "Successfully set admin distance for client-id {} to {}", + clientId, + distance); +} + +void CmdConfigAdminDistance::printOutput(const RetType& output) { + std::cout << output << "\n"; +} + +// Explicit template instantiation +template void +CmdHandler::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.h b/fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.h new file mode 100644 index 0000000000000..b76600313ccf8 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.h @@ -0,0 +1,62 @@ +/* + * 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" + +namespace facebook::fboss { + +// Parses the two positional arguments of +// config admin-distance +// where is a routing ClientID (e.g. 0=BGP, 1=static, 786=local-BGP) +// and is the administrative distance to associate with it. +class AdminDistanceArg : public utils::BaseObjectArgType { + public: + /* implicit */ AdminDistanceArg( // NOLINT(google-explicit-constructor) + std::vector v); + + int32_t getClientId() const { + return clientId_; + } + + int32_t getDistance() const { + return distance_; + } + + private: + int32_t clientId_{0}; + int32_t distance_{0}; +}; + +struct CmdConfigAdminDistanceTraits : public WriteCommandTraits { + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option( + "client_distance", + args, + " - admin distance for a routing client " + "(client-id: 0=BGP, 1=static, 786=local-BGP; distance: 0-255)"); + } + using ObjectArgType = AdminDistanceArg; + using RetType = std::string; +}; + +class CmdConfigAdminDistance + : public CmdHandler { + public: + using ObjectArgType = CmdConfigAdminDistanceTraits::ObjectArgType; + using RetType = CmdConfigAdminDistanceTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& arg); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/BUCK b/fboss/cli/fboss2/test/config/BUCK index 5736f2c2e0def..111aa95e5406b 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -6,6 +6,7 @@ cpp_unittest( name = "cmd_config_test", srcs = [ "BgpConfigSessionTest.cpp", + "CmdConfigAdminDistanceTest.cpp", "CmdConfigAppliedInfoTest.cpp", "CmdConfigArpTest.cpp", "CmdConfigHistoryTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigAdminDistanceTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigAdminDistanceTest.cpp new file mode 100644 index 0000000000000..1391bc25d36e6 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigAdminDistanceTest.cpp @@ -0,0 +1,142 @@ +/* + * 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 "fboss/cli/fboss2/commands/config/admin_distance/CmdConfigAdminDistance.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" + +using namespace ::testing; + +namespace facebook::fboss { + +// Seed mirrors the production clientIdToAdminDistance map shipped on Meta +// devices: every routing client is mapped (BGP=20, static=1, interface/ +// linklocal/remote=0, local-BGP/OpenR=10, internal=255). The CLI mutates a +// single entry, so the test asserts the other entries survive untouched. +class CmdConfigAdminDistanceTestFixture : public CmdConfigTestBase { + public: + CmdConfigAdminDistanceTestFixture() + : CmdConfigTestBase( + "fboss_admin_distance_test_%%%%-%%%%-%%%%-%%%%", + R"({ + "sw": { + "clientIdToAdminDistance": { + "0": 20, + "1": 1, + "2": 0, + "3": 0, + "4": 0, + "700": 255, + "786": 10 + } + } +})") {} + + protected: + const std::string setPrefix_ = "config admin-distance"; +}; + +// ============================================================================== +// AdminDistanceArg Validation Tests +// ============================================================================== + +TEST_F(CmdConfigAdminDistanceTestFixture, argValidation) { + // Valid values + EXPECT_EQ(AdminDistanceArg({"0", "20"}).getClientId(), 0); + EXPECT_EQ(AdminDistanceArg({"0", "20"}).getDistance(), 20); + EXPECT_EQ(AdminDistanceArg({"786", "10"}).getClientId(), 786); + EXPECT_EQ(AdminDistanceArg({"1", "0"}).getDistance(), 0); + EXPECT_EQ(AdminDistanceArg({"700", "255"}).getDistance(), 255); + + // Invalid: empty + EXPECT_THROW(AdminDistanceArg({}), std::invalid_argument); + + // Invalid: wrong arity + EXPECT_THROW(AdminDistanceArg({"0"}), std::invalid_argument); + EXPECT_THROW(AdminDistanceArg({"0", "20", "30"}), std::invalid_argument); + + // Invalid: non-integer + EXPECT_THROW(AdminDistanceArg({"bgp", "20"}), std::invalid_argument); + EXPECT_THROW(AdminDistanceArg({"0", "twenty"}), std::invalid_argument); + EXPECT_THROW(AdminDistanceArg({"0", "20.5"}), std::invalid_argument); + + // Invalid: negative client-id + EXPECT_THROW(AdminDistanceArg({"-1", "20"}), std::invalid_argument); + + // Invalid: distance out of [0, 255] + EXPECT_THROW(AdminDistanceArg({"0", "-1"}), std::invalid_argument); + EXPECT_THROW(AdminDistanceArg({"0", "256"}), std::invalid_argument); +} + +// ============================================================================== +// Set Command Tests +// ============================================================================== + +TEST_F(CmdConfigAdminDistanceTestFixture, updateExistingClient) { + setupTestableConfigSession(setPrefix_, "0 30"); + CmdConfigAdminDistance cmd; + HostInfo hostInfo("testhost"); + AdminDistanceArg arg({"0", "30"}); + + auto result = cmd.queryClient(hostInfo, arg); + EXPECT_THAT(result, HasSubstr("30")); + + auto& config = ConfigSession::getInstance().getAgentConfig(); + const auto& map = *config.sw()->clientIdToAdminDistance(); + EXPECT_EQ(map.at(0), 30); + + // Other entries must survive untouched. + EXPECT_EQ(map.at(1), 1); + EXPECT_EQ(map.at(2), 0); + EXPECT_EQ(map.at(3), 0); + EXPECT_EQ(map.at(4), 0); + EXPECT_EQ(map.at(700), 255); + EXPECT_EQ(map.at(786), 10); + EXPECT_EQ(map.size(), 7); +} + +TEST_F(CmdConfigAdminDistanceTestFixture, addNewClient) { + setupTestableConfigSession(setPrefix_, "800 2"); + CmdConfigAdminDistance cmd; + HostInfo hostInfo("testhost"); + AdminDistanceArg arg({"800", "2"}); + + auto result = cmd.queryClient(hostInfo, arg); + EXPECT_THAT(result, HasSubstr("800")); + + auto& config = ConfigSession::getInstance().getAgentConfig(); + const auto& map = *config.sw()->clientIdToAdminDistance(); + EXPECT_EQ(map.at(800), 2); + // All original entries preserved; new key added. + EXPECT_EQ(map.at(0), 20); + EXPECT_EQ(map.at(1), 1); + EXPECT_EQ(map.at(2), 0); + EXPECT_EQ(map.at(3), 0); + EXPECT_EQ(map.at(4), 0); + EXPECT_EQ(map.at(700), 255); + EXPECT_EQ(map.at(786), 10); + EXPECT_EQ(map.size(), 8); +} + +TEST_F(CmdConfigAdminDistanceTestFixture, alreadySet) { + // Seed has client-id 786 -> 10; setting to 10 is a no-op. + setupTestableConfigSession(setPrefix_, "786 10"); + CmdConfigAdminDistance cmd; + HostInfo hostInfo("testhost"); + AdminDistanceArg arg({"786", "10"}); + + auto result = cmd.queryClient(hostInfo, arg); + EXPECT_THAT(result, HasSubstr("already")); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 018f180af7827..740c4f6071c11 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -16,6 +16,7 @@ oncall("fboss_oss") cpp_binary( name = "fboss2_integration_test", srcs = [ + "ConfigAdminDistanceTest.cpp", "ConfigArpTest.cpp", "ConfigConcurrentSessionsTest.cpp", "ConfigInterfaceDescriptionTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigAdminDistanceTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigAdminDistanceTest.cpp new file mode 100644 index 0000000000000..c89ccdc7a7409 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigAdminDistanceTest.cpp @@ -0,0 +1,92 @@ +/* + * 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. + * + */ + +/** + * End-to-end test for: + * fboss2-dev config admin-distance + * + * Reads the current clientIdToAdminDistance map, picks an existing client, + * changes its distance, verifies the new value round-trips through the agent's + * running config (and that the other entries survive), then restores the + * original. The change is HITLESS so no agent restart is needed between steps. + */ + +#include +#include +#include +#include +#include +#include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" + +using namespace facebook::fboss; + +class ConfigAdminDistanceTest : public Fboss2IntegrationTest { + protected: + std::map getAdminDistances() const { + std::map result; + auto config = getRunningConfig(); + const auto& sw = config["sw"]; + if (!sw.count("clientIdToAdminDistance")) { + return result; + } + for (const auto& [clientId, distance] : + sw["clientIdToAdminDistance"].items()) { + result[folly::to(clientId.asString())] = distance.asInt(); + } + return result; + } + + void setAdminDistance(int clientId, int distance) { + auto result = runCli( + {"config", + "admin-distance", + std::to_string(clientId), + std::to_string(distance)}); + ASSERT_EQ(result.exitCode, 0) + << "admin-distance CLI failed: " << result.stderr; + commitConfig(); + } +}; + +TEST_F(ConfigAdminDistanceTest, SetAndRestoreAdminDistance) { + XLOG(INFO) << "[Step 1] Reading current admin distances..."; + auto original = getAdminDistances(); + ASSERT_FALSE(original.empty()) + << "Running config has no clientIdToAdminDistance entries to test with"; + + // Derive the target client from the live config rather than hardcoding. + int clientId = original.begin()->first; + int originalDistance = original.begin()->second; + XLOG(INFO) << " client-id " << clientId << " -> " << originalDistance; + + // Pick a distinct, valid distance in [0, 255]. + int newDistance = (originalDistance == 42) ? 43 : 42; + + XLOG(INFO) << "[Step 2] Setting client-id " << clientId << " to " + << newDistance << "..."; + setAdminDistance(clientId, newDistance); + auto updated = getAdminDistances(); + EXPECT_EQ(updated[clientId], newDistance); + + // The other entries must survive untouched. + for (const auto& [id, distance] : original) { + if (id != clientId) { + EXPECT_EQ(updated[id], distance) + << "client-id " << id << " changed unexpectedly"; + } + } + + XLOG(INFO) << "[Step 3] Restoring client-id " << clientId << " to " + << originalDistance << "..."; + setAdminDistance(clientId, originalDistance); + EXPECT_EQ(getAdminDistances()[clientId], originalDistance); + + XLOG(INFO) << "TEST PASSED"; +}