From 43392f55aa76345230acccfed89d559df57bd52b Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Mon, 29 Jun 2026 03:48:46 +0000 Subject: [PATCH 1/6] fboss2 `config protocol bgp global` command Add `fboss2-dev config protocol bgp global ` on top of the BGP-aware ConfigSession (base PR). Edits the typed bgp::thrift::BgpConfig via ConfigSession::getBgpConfig()/saveBgpConfig() -- the whole-config, scope-agnostic typed API (no global/peer/peer-group special-casing in ConfigSession). - Collapse the 10 per-attribute global command classes into one dispatcher; reject cluster-id (no BgpConfig field) instead of writing dead config; bound switch-limit / max_golden_vips so out-of-range values aren't truncated. - Integration tests: ConfigBgpGlobalTest (each attr set+commit, verified in the promoted /etc/coop/bgpcpp/bgpcpp.conf) and ConfigBgpSessionTest (clear/diff/commit/rollback + a no-op-restart regression), sharing ConfigBgpTestBase. Test Plan: - bazel test //fboss/cli/fboss2/test/config:cmd_config_test - bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test - fboss2_integration_test on a DUT with bgp_pp active: ConfigBgpSessionTest 6/6 pass (clear, diff, commit-restarts-bgp_pp, rollback-restores-config, and the unchanged-config does-not-restart case); agent-session regression (ConfigInterfaceMtuTest) passes. --- cmake/CliFboss2.cmake | 20 - cmake/CliFboss2TestIntegrationTest.cmake | 2 + fboss/cli/fboss2/BUCK | 15 - fboss/cli/fboss2/CmdListConfig.cpp | 99 +---- .../config/protocol/bgp/BgpConfigSession.cpp | 14 - .../config/protocol/bgp/BgpConfigSession.h | 5 - .../bgp/global/CmdConfigProtocolBgpGlobal.cpp | 413 +++++++++++++++++- .../bgp/global/CmdConfigProtocolBgpGlobal.h | 32 +- .../CmdConfigProtocolBgpGlobalClusterId.cpp | 49 --- .../CmdConfigProtocolBgpGlobalClusterId.h | 42 -- .../CmdConfigProtocolBgpGlobalConfedAsn.cpp | 57 --- .../CmdConfigProtocolBgpGlobalConfedAsn.h | 42 -- .../CmdConfigProtocolBgpGlobalHoldTime.cpp | 60 --- .../CmdConfigProtocolBgpGlobalHoldTime.h | 41 -- .../CmdConfigProtocolBgpGlobalLocalAsn.cpp | 59 --- .../CmdConfigProtocolBgpGlobalLocalAsn.h | 41 -- .../CmdConfigProtocolBgpGlobalNetwork6Add.cpp | 84 ---- .../CmdConfigProtocolBgpGlobalNetwork6Add.h | 42 -- .../CmdConfigProtocolBgpGlobalRouterId.cpp | 52 --- .../CmdConfigProtocolBgpGlobalRouterId.h | 41 -- ...tocolBgpGlobalSwitchLimitMaxGoldenVips.cpp | 51 --- ...rotocolBgpGlobalSwitchLimitMaxGoldenVips.h | 44 -- ...lobalSwitchLimitOverloadProtectionMode.cpp | 51 --- ...pGlobalSwitchLimitOverloadProtectionMode.h | 46 -- ...rotocolBgpGlobalSwitchLimitPrefixLimit.cpp | 54 --- ...gProtocolBgpGlobalSwitchLimitPrefixLimit.h | 44 -- ...ocolBgpGlobalSwitchLimitTotalPathLimit.cpp | 51 --- ...otocolBgpGlobalSwitchLimitTotalPathLimit.h | 44 -- .../test/config/BgpConfigSessionTest.cpp | 8 - fboss/cli/fboss2/test/integration_test/BUCK | 2 + .../integration_test/ConfigBgpGlobalTest.cpp | 114 +++++ .../integration_test/ConfigBgpSessionTest.cpp | 218 +++++++++ .../test/integration_test/ConfigBgpTestBase.h | 212 +++++++++ fboss/cli/fboss2/tools/bgp_json_to_cli.py | 64 ++- .../tools/tests/test_bgp_json_to_cli.py | 22 +- 35 files changed, 1027 insertions(+), 1208 deletions(-) delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.h create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 2d4592882efa4..be8b1f772cc3e 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -950,26 +950,6 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/CmdConfigProtocolBgp.h fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.h - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.h fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index ea76d3acb48e8..8d1f56eaf8b92 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -14,6 +14,8 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigArpTest.cpp fboss/cli/fboss2/test/integration_test/ConfigSflowRateTest.cpp fboss/cli/fboss2/test/integration_test/ConfigSflowSampleDestTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp fboss/cli/fboss2/test/integration_test/ConfigConcurrentSessionsTest.cpp fboss/cli/fboss2/test/integration_test/ConfigHostnameTest.cpp fboss/cli/fboss2/test/integration_test/ConfigIcmpV4UnavailableSrcAddrTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 7007483a1c334..ecce92fa778a4 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1169,16 +1169,6 @@ cpp_library( "commands/config/protocol/bgp/BgpConfigSession.cpp", "commands/config/protocol/bgp/CmdConfigProtocolBgp.cpp", "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.cpp", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.cpp", "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp", "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.cpp", "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.cpp", @@ -1345,11 +1335,6 @@ cpp_library( "commands/config/protocol/bgp/BgpConfigSession.h", "commands/config/protocol/bgp/CmdConfigProtocolBgp.h", "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.h", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.h", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.h", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.h", - "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.h", "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h", "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.h", "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 7a49f6db56c1d..12bd0bfda4894 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -41,16 +41,6 @@ #include "fboss/cli/fboss2/commands/config/protocol/CmdConfigProtocol.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/CmdConfigProtocolBgp.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.h" @@ -416,91 +406,14 @@ const CommandTree& kConfigCommandTree() { { { "global", - "Configure BGP global settings", + "Configure BGP global settings: " + "(router-id, local-asn, hold-time, confed-asn, " + "count-confeds-in-as-path-len, " + "graceful-restart-time, rib-allocated-path-ids, " + "network6, switch-limit[-total-path|" + "-max-golden-vips|-overload-protection-mode])", commandHandler, argRegistrar, - { - { - "router-id", - "Set BGP router identifier", - commandHandler< - CmdConfigProtocolBgpGlobalRouterId>, - argRegistrar< - CmdConfigProtocolBgpGlobalRouterIdTraits>, - }, - { - "local-asn", - "Set local AS number", - commandHandler< - CmdConfigProtocolBgpGlobalLocalAsn>, - argRegistrar< - CmdConfigProtocolBgpGlobalLocalAsnTraits>, - }, - { - "hold-time", - "Set BGP hold time in seconds", - commandHandler< - CmdConfigProtocolBgpGlobalHoldTime>, - argRegistrar< - CmdConfigProtocolBgpGlobalHoldTimeTraits>, - }, - { - "confed-asn", - "Set BGP confederation AS number", - commandHandler< - CmdConfigProtocolBgpGlobalConfedAsn>, - argRegistrar< - CmdConfigProtocolBgpGlobalConfedAsnTraits>, - }, - { - "cluster-id", - "Set route reflector cluster ID", - commandHandler< - CmdConfigProtocolBgpGlobalClusterId>, - argRegistrar< - CmdConfigProtocolBgpGlobalClusterIdTraits>, - }, - { - "network6", - "Add IPv6 network to advertise", - commandHandler< - CmdConfigProtocolBgpGlobalNetwork6Add>, - argRegistrar< - CmdConfigProtocolBgpGlobalNetwork6AddTraits>, - }, - { - "switch-limit", - "Set switch limit prefix-limit", - commandHandler< - CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit>, - argRegistrar< - CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimitTraits>, - }, - { - "switch-limit-total-path", - "Set switch limit total-path-limit", - commandHandler< - CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit>, - argRegistrar< - CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimitTraits>, - }, - { - "switch-limit-max-golden-vips", - "Set switch limit max-golden-vips", - commandHandler< - CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips>, - argRegistrar< - CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVipsTraits>, - }, - { - "switch-limit-overload-protection-mode", - "Set switch limit overload-protection-mode", - commandHandler< - CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode>, - argRegistrar< - CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionModeTraits>, - }, - }, }, { "peer-group", diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.cpp index f5378e3609638..91e12b6195302 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.cpp @@ -208,20 +208,6 @@ std::optional BgpConfigSession::getConfedAsn() const { return std::nullopt; } -void BgpConfigSession::setClusterId(const std::string& clusterId) { - ensureConfigLoaded(); - bgpConfig_["cluster_id"] = clusterId; -} - -std::optional BgpConfigSession::getClusterId() const { - const_cast(this)->ensureConfigLoaded(); - if (bgpConfig_.count("cluster_id") && - !bgpConfig_["cluster_id"].asString().empty()) { - return bgpConfig_["cluster_id"].asString(); - } - return std::nullopt; -} - void BgpConfigSession::setListenAddress(const std::string& listenAddr) { ensureConfigLoaded(); bgpConfig_["listen_addr"] = listenAddr; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h index 30569f5a59bde..449d629027ed5 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h @@ -42,7 +42,6 @@ namespace facebook::fboss { * - local_as_4_byte: i64 - Local AS number (RFC 6793) * - hold_time: i32 - Hold time in seconds (default 30) * - local_confed_as_4_byte: i64 - Confederation AS number - * - cluster_id: string - Route reflector cluster ID * - peers: list - List of BGP peers * - peer_groups: list - List of peer groups * @@ -103,10 +102,6 @@ class BgpConfigSession { void setConfedAsn(uint64_t asn); std::optional getConfedAsn() const; - // cluster_id: string - Route reflector cluster ID - void setClusterId(const std::string& clusterId); - std::optional getClusterId() const; - // listen_addr: string - Listen address for BGP sessions void setListenAddress(const std::string& listenAddr); std::optional getListenAddress() const; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp index ff97042a344b6..a78fa33b34229 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp @@ -12,11 +12,420 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" + namespace facebook::fboss { +namespace { + +// CLI attribute names. Kept here (not as raw string literals at the dispatch +// sites) so the valid-attribute set and the handler table stay in sync. +constexpr std::string_view kRouterId = "router-id"; +constexpr std::string_view kLocalAsn = "local-asn"; +constexpr std::string_view kHoldTime = "hold-time"; +constexpr std::string_view kConfedAsn = "confed-asn"; +constexpr std::string_view kNetwork6 = "network6"; +constexpr std::string_view kSwitchLimit = "switch-limit"; +constexpr std::string_view kSwitchLimitTotalPath = "switch-limit-total-path"; +constexpr std::string_view kSwitchLimitMaxGoldenVips = + "switch-limit-max-golden-vips"; +constexpr std::string_view kSwitchLimitOverloadProtectionMode = + "switch-limit-overload-protection-mode"; +constexpr std::string_view kCountConfedsInAsPathLen = + "count-confeds-in-as-path-len"; +constexpr std::string_view kGracefulRestartTime = "graceful-restart-time"; +constexpr std::string_view kRibAllocatedPathIds = "rib-allocated-path-ids"; + +using Tokens = std::vector; +using BgpConfig = bgp::thrift::BgpConfig; + +// Outcome of an attribute handler: on failure the message is returned to the +// user and the session is NOT persisted, so a rejected value never lands on +// disk. +struct Result { + bool ok; + std::string message; +}; + +Result ok(std::string message) { + return Result{true, std::move(message)}; +} + +Result err(std::string message) { + return Result{false, std::move(message)}; +} + +// ---- value parsers (modular, shared across attributes) -------------------- + +std::optional parseBool(const std::string& value) { + if (value == "true" || value == "1" || value == "yes") { + return true; + } + if (value == "false" || value == "0" || value == "no") { + return false; + } + return std::nullopt; +} + +template +std::optional parseInt(const std::string& value) { + try { + return folly::to(value); + } catch (const std::exception&) { + return std::nullopt; + } +} + +// Parse a non-negative value that must fit in int32 (used for second-valued +// timers and min-routes). +std::optional parseNonNegInt32(const std::string& value) { + auto parsed = parseInt(value); + if (!parsed || *parsed < 0 || *parsed > std::numeric_limits::max()) { + return std::nullopt; + } + return static_cast(*parsed); +} + +// ---- per-attribute handlers ------------------------------------------------ +// Each handler mutates the typed bgp::thrift::BgpConfig directly. Field names +// map 1:1 to bgp_config.thrift, so the compiler validates them. + +Result applyRouterId(BgpConfig& cfg, const Tokens& values) { + if (values.size() != 1) { + return err("Error: router-id requires "); + } + cfg.router_id() = values[0]; + return ok(fmt::format("Successfully set BGP router-id to: {}", values[0])); +} + +Result applyLocalAsn(BgpConfig& cfg, const Tokens& values) { + if (values.size() != 1) { + return err("Error: local-asn requires "); + } + auto asn = parseInt(values[0]); + if (!asn) { + return err(fmt::format("Error: Invalid local-asn value '{}'", values[0])); + } + cfg.local_as_4_byte() = static_cast(*asn); + return ok(fmt::format("Successfully set BGP local-asn to: {}", *asn)); +} + +Result applyConfedAsn(BgpConfig& cfg, const Tokens& values) { + if (values.size() != 1) { + return err("Error: confed-asn requires "); + } + auto asn = parseInt(values[0]); + if (!asn) { + return err(fmt::format("Error: Invalid confed-asn value '{}'", values[0])); + } + cfg.local_confed_as_4_byte() = static_cast(*asn); + return ok( + fmt::format("Successfully set BGP confederation AS number to: {}", *asn)); +} + +Result applyHoldTime(BgpConfig& cfg, const Tokens& values) { + if (values.size() != 1) { + return err("Error: hold-time requires "); + } + auto seconds = parseNonNegInt32(values[0]); + if (!seconds) { + return err(fmt::format("Error: Invalid hold-time value '{}'", values[0])); + } + cfg.hold_time() = *seconds; + return ok( + fmt::format("Successfully set BGP hold-time to: {} seconds", *seconds)); +} + +Result applyGracefulRestartTime(BgpConfig& cfg, const Tokens& values) { + if (values.size() != 1) { + return err("Error: graceful-restart-time requires "); + } + auto seconds = parseNonNegInt32(values[0]); + if (!seconds) { + return err( + fmt::format( + "Error: graceful-restart-time must be a non-negative integer " + "that fits in int32, got '{}'", + values[0])); + } + cfg.graceful_restart_convergence_seconds() = *seconds; + return ok( + fmt::format( + "Successfully set BGP graceful-restart-time to: {} seconds", + *seconds)); +} + +Result applyCountConfedsInAsPathLen(BgpConfig& cfg, const Tokens& values) { + if (values.size() != 1) { + return err("Error: count-confeds-in-as-path-len requires "); + } + auto enable = parseBool(values[0]); + if (!enable) { + return err( + fmt::format( + "Error: Invalid value '{}'; expected true or false", values[0])); + } + cfg.count_confeds_in_as_path_len() = *enable; + return ok( + fmt::format( + "Successfully {} count-confeds-in-as-path-len", + *enable ? "enabled" : "disabled")); +} + +Result applyRibAllocatedPathIds(BgpConfig& cfg, const Tokens& values) { + if (values.size() != 1) { + return err("Error: rib-allocated-path-ids requires "); + } + auto enable = parseBool(values[0]); + if (!enable) { + return err( + fmt::format( + "Error: Invalid value '{}'; expected true or false", values[0])); + } + // enable_rib_allocated_path_id lives inside the nested bgp_setting_config. + cfg.bgp_setting_config().ensure().enable_rib_allocated_path_id() = *enable; + return ok( + fmt::format( + "Successfully {} rib-allocated-path-ids", + *enable ? "enabled" : "disabled")); +} + +// Shared implementation for the switch-limit attributes, which differ only in +// the field setter, the field name in the success message, and the upper bound +// (i64 fields use INT64_MAX; the i32 max_golden_vips field uses INT32_MAX so an +// out-of-range value is rejected rather than silently truncated). All limits +// must be non-negative. +Result applyInt64Limit( + BgpConfig& cfg, + const Tokens& values, + std::string_view attrName, + std::string_view fieldName, + int64_t maxValue, + const std::function& setter) { + if (values.size() != 1) { + return err(fmt::format("Error: {} requires ", attrName)); + } + auto limit = parseInt(values[0]); + if (!limit) { + return err( + fmt::format("Error: Invalid {} value '{}'", attrName, values[0])); + } + if (*limit < 0 || *limit > maxValue) { + return err( + fmt::format( + "Error: {} value '{}' is out of range [0, {}]", + attrName, + values[0], + maxValue)); + } + setter(cfg, *limit); + return ok( + fmt::format( + "Successfully set switch_limit_config {} to: {}", fieldName, *limit)); +} + +Result applyOverloadProtectionMode(BgpConfig& cfg, const Tokens& values) { + if (values.size() != 1) { + return err("Error: switch-limit-overload-protection-mode requires "); + } + auto mode = parseInt(values[0]); + if (!mode) { + return err( + fmt::format( + "Error: Invalid overload-protection-mode value '{}'", values[0])); + } + cfg.switch_limit_config().ensure().overload_protection_mode() = + static_cast(*mode); + return ok( + fmt::format( + "Successfully set switch_limit_config overload_protection_mode to: {}", + *mode)); +} + +Result applyNetwork6(BgpConfig& cfg, const Tokens& values) { + // Usage: network6 add [policy ] [install-to-fib ] + // [min-routes ] + if (values.size() < 2 || values[0] != "add") { + return err( + "Error: usage: network6 add [policy ] " + "[install-to-fib ] [min-routes ]"); + } + std::string prefix = values[1]; + std::string policyName; + bool installToFib = false; + int32_t minSupportingRoutes = 0; + + for (size_t i = 2; i < values.size(); i++) { + if (values[i] == "policy" && i + 1 < values.size()) { + policyName = values[++i]; + } else if (values[i] == "install-to-fib" && i + 1 < values.size()) { + auto parsed = parseBool(values[++i]); + if (!parsed) { + return err( + fmt::format( + "Error: Invalid install-to-fib value '{}'; expected true or false", + values[i])); + } + installToFib = *parsed; + } else if (values[i] == "min-routes" && i + 1 < values.size()) { + auto parsed = parseNonNegInt32(values[++i]); + if (!parsed) { + return err( + fmt::format("Error: Invalid min-routes value '{}'", values[i])); + } + minSupportingRoutes = *parsed; + } else { + return err( + fmt::format("Error: unexpected network6 token '{}'", values[i])); + } + } + + // Find-or-update the networks6 entry keyed by prefix. + auto& networks = *cfg.networks6(); + bgp::thrift::BgpNetwork* entry = nullptr; + for (auto& n : networks) { + if (*n.prefix() == prefix) { + entry = &n; + break; + } + } + if (entry == nullptr) { + networks.emplace_back(); + entry = &networks.back(); + } + entry->prefix() = prefix; + entry->policy_name() = policyName; + entry->install_to_fib() = installToFib; + entry->minimum_supporting_routes() = minSupportingRoutes; + + return ok( + fmt::format( + "Successfully added network6 prefix: {} (policy: {}, install_to_fib: {}, " + "min_routes: {})", + prefix, + policyName.empty() ? "(none)" : policyName, + installToFib ? "true" : "false", + minSupportingRoutes)); +} + +using AttrHandler = std::function; + +const std::map& attrHandlers() { + static const std::map kHandlers = { + {kRouterId, applyRouterId}, + {kLocalAsn, applyLocalAsn}, + {kConfedAsn, applyConfedAsn}, + {kHoldTime, applyHoldTime}, + {kGracefulRestartTime, applyGracefulRestartTime}, + {kCountConfedsInAsPathLen, applyCountConfedsInAsPathLen}, + {kRibAllocatedPathIds, applyRibAllocatedPathIds}, + {kNetwork6, applyNetwork6}, + {kSwitchLimit, + [](BgpConfig& cfg, const Tokens& v) { + return applyInt64Limit( + cfg, + v, + kSwitchLimit, + "prefix_limit", + std::numeric_limits::max(), + [](BgpConfig& c, int64_t l) { + c.switch_limit_config().ensure().prefix_limit() = l; + }); + }}, + {kSwitchLimitTotalPath, + [](BgpConfig& cfg, const Tokens& v) { + return applyInt64Limit( + cfg, + v, + kSwitchLimitTotalPath, + "total_path_limit", + std::numeric_limits::max(), + [](BgpConfig& c, int64_t l) { + c.switch_limit_config().ensure().total_path_limit() = l; + }); + }}, + {kSwitchLimitMaxGoldenVips, + [](BgpConfig& cfg, const Tokens& v) { + // max_golden_vips is an i32 field; bound it so an out-of-range value + // is rejected rather than silently truncated by the static_cast. + return applyInt64Limit( + cfg, + v, + kSwitchLimitMaxGoldenVips, + "max_golden_vips", + std::numeric_limits::max(), + [](BgpConfig& c, int64_t l) { + c.switch_limit_config().ensure().max_golden_vips() = + static_cast(l); + }); + }}, + {kSwitchLimitOverloadProtectionMode, applyOverloadProtectionMode}, + }; + return kHandlers; +} + +std::string validAttrList() { + std::string out; + for (const auto& [name, _] : attrHandlers()) { + if (!out.empty()) { + out += ", "; + } + out += std::string(name); + } + return out; +} + +} // namespace + +// Parse + validate at construction so queryClient stays a thin dispatch. +// Throwing std::invalid_argument is how the framework surfaces arg parse +// errors (same mechanism as InterfacesConfig / parseTokens). +BgpGlobalConfig::BgpGlobalConfig(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + fmt::format( + "Error: attribute is required. Valid attributes: {}", + validAttrList())); + } + if (attrHandlers().find(v[0]) == attrHandlers().end()) { + throw std::invalid_argument( + fmt::format( + "Error: unknown global attribute '{}'. Valid attributes: {}", + v[0], + validAttrList())); + } + attr_ = v[0]; + values_.assign(v.begin() + 1, v.end()); +} + CmdConfigProtocolBgpGlobalTraits::RetType -CmdConfigProtocolBgpGlobal::queryClient(const HostInfo& /* hostInfo */) { - return "BGP global configuration. Use subcommands: router-id, local-asn, hold-time, graceful-restart, best-path, link-bw, nh-tracking, network-originate"; +CmdConfigProtocolBgpGlobal::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + + // The attribute is guaranteed valid: BgpGlobalConfig's constructor rejects + // an empty/unknown attribute before we get here. + auto handler = attrHandlers().find(args.attr()); + Result result = handler->second(session.getBgpConfig(), args.values()); + if (result.ok) { + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } + return result.message; } void CmdConfigProtocolBgpGlobal::printOutput(const RetType& output) { diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h index 0a6faa7a09fb6..e8851b2bc4cea 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h @@ -12,12 +12,40 @@ #include "fboss/cli/fboss2/CmdHandler.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/CmdConfigProtocolBgp.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" namespace facebook::fboss { +// Parsed `config protocol bgp global [value ...]`, validated at +// construction. values() is greedy to keep network6's multi-token tail. +class BgpGlobalConfig : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpGlobalConfig(std::vector v); + const std::string& attr() const { + return attr_; + } + const std::vector& values() const { + return values_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::string attr_; + std::vector values_; +}; + +// Single handler for the whole `config protocol bgp global` family. The +// attribute name is the first positional token and the remaining tokens are +// its value(s); dispatch and per-attribute parsing live in the .cpp so adding +// a new global tunable is a one-entry change rather than a new command class. struct CmdConfigProtocolBgpGlobalTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgp; - using ObjectArgType = std::monostate; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("args", args, " [value ...]"); + } + using ObjectArgType = BgpGlobalConfig; using RetType = std::string; }; @@ -28,7 +56,7 @@ class CmdConfigProtocolBgpGlobal : public CmdHandler< using ObjectArgType = CmdConfigProtocolBgpGlobalTraits::ObjectArgType; using RetType = CmdConfigProtocolBgpGlobalTraits::RetType; - RetType queryClient(const HostInfo& hostInfo); + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& args); void printOutput(const RetType& output); }; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.cpp deleted file mode 100644 index ed1b541d0298f..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.cpp +++ /dev/null @@ -1,49 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalClusterIdTraits::RetType -CmdConfigProtocolBgpGlobalClusterId::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& clusterIdList) { - auto& session = BgpConfigSession::getInstance(); - - if (clusterIdList.data().empty()) { - return "Error: cluster-id is required"; - } - - std::string clusterId = clusterIdList.data()[0]; - session.setClusterId(clusterId); - session.saveConfig(); - - return fmt::format( - "Successfully set BGP cluster-id to: {}\nConfig saved to: {}", - clusterId, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalClusterId::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalClusterId, - CmdConfigProtocolBgpGlobalClusterIdTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.h deleted file mode 100644 index 8345a5b485888..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalClusterId.h +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalClusterIdTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("ipAddrs", args, "IPv4 or IPv6 addr(s)"); - } - using ObjectArgType = utils::IPList; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalClusterId - : public CmdHandler< - CmdConfigProtocolBgpGlobalClusterId, - CmdConfigProtocolBgpGlobalClusterIdTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpGlobalClusterIdTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpGlobalClusterIdTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& clusterId); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.cpp deleted file mode 100644 index bcc6512e89a38..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.cpp +++ /dev/null @@ -1,57 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalConfedAsnTraits::RetType -CmdConfigProtocolBgpGlobalConfedAsn::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& confedAsnMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (confedAsnMsg.data().empty()) { - return "Error: confed-asn value is required"; - } - - std::string asnStr = confedAsnMsg.data()[0]; - uint64_t confedAsn; - try { - confedAsn = folly::to(asnStr); - } catch (const std::exception& e) { - return fmt::format("Error: Invalid confed-asn '{}': {}", asnStr, e.what()); - } - - session.setConfedAsn(confedAsn); - session.saveConfig(); - - return fmt::format( - "Successfully set BGP confederation AS number to: {}\nConfig saved to: {}", - confedAsn, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalConfedAsn::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalConfedAsn, - CmdConfigProtocolBgpGlobalConfedAsnTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.h deleted file mode 100644 index 2020e1b4cb1f6..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalConfedAsn.h +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalConfedAsnTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalConfedAsn - : public CmdHandler< - CmdConfigProtocolBgpGlobalConfedAsn, - CmdConfigProtocolBgpGlobalConfedAsnTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpGlobalConfedAsnTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpGlobalConfedAsnTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& confedAsn); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.cpp deleted file mode 100644 index c526e68c97028..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.cpp +++ /dev/null @@ -1,60 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalHoldTimeTraits::RetType -CmdConfigProtocolBgpGlobalHoldTime::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& holdTimeMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (holdTimeMsg.data().empty()) { - return "Error: hold-time value is required"; - } - - // Extract string and parse to uint32_t - std::string holdTimeStr = holdTimeMsg.data()[0]; - uint32_t holdTime; - try { - holdTime = folly::to(holdTimeStr); - } catch (const std::exception& e) { - return fmt::format( - "Error: Invalid hold-time value '{}': {}", holdTimeStr, e.what()); - } - - // Set the hold-time in the BGP config session - session.setHoldTime(holdTime); - session.saveConfig(); - - return fmt::format( - "Successfully set BGP hold-time to: {} seconds\nConfig saved to: {}", - holdTime, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalHoldTime::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalHoldTime, - CmdConfigProtocolBgpGlobalHoldTimeTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.h deleted file mode 100644 index 1db1004d320b3..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalHoldTime.h +++ /dev/null @@ -1,41 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalHoldTimeTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalHoldTime - : public CmdHandler< - CmdConfigProtocolBgpGlobalHoldTime, - CmdConfigProtocolBgpGlobalHoldTimeTraits> { - public: - using ObjectArgType = CmdConfigProtocolBgpGlobalHoldTimeTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpGlobalHoldTimeTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& holdTime); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.cpp deleted file mode 100644 index 81a8571197a79..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.cpp +++ /dev/null @@ -1,59 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalLocalAsnTraits::RetType -CmdConfigProtocolBgpGlobalLocalAsn::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& localAsnMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (localAsnMsg.data().empty()) { - return "Error: local-asn value is required"; - } - - // Extract string and parse to uint32_t - std::string asnStr = localAsnMsg.data()[0]; - uint32_t localAsn; - try { - localAsn = folly::to(asnStr); - } catch (const std::exception& e) { - return fmt::format( - "Error: Invalid local-asn value '{}': {}", asnStr, e.what()); - } - - // Set the local-asn in the BGP config session - session.setLocalAsn(localAsn); - session.saveConfig(); - - return fmt::format( - "Successfully set BGP local-asn to: {}\nConfig saved to: {}", - localAsn, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalLocalAsn::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalLocalAsn, - CmdConfigProtocolBgpGlobalLocalAsnTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.h deleted file mode 100644 index d39cc07e490bf..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalLocalAsn.h +++ /dev/null @@ -1,41 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalLocalAsnTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalLocalAsn - : public CmdHandler< - CmdConfigProtocolBgpGlobalLocalAsn, - CmdConfigProtocolBgpGlobalLocalAsnTraits> { - public: - using ObjectArgType = CmdConfigProtocolBgpGlobalLocalAsnTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpGlobalLocalAsnTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& localAsn); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.cpp deleted file mode 100644 index 336f5f50f6f70..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.cpp +++ /dev/null @@ -1,84 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalNetwork6AddTraits::RetType -CmdConfigProtocolBgpGlobalNetwork6Add::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& args) { - auto& session = BgpConfigSession::getInstance(); - - // args.data() returns a vector of strings - // Command: "config protocol bgp global network6 add [policy ] - // [install-to-fib ] [min-routes ]" - // After "network6", args contains: ["add", "", ...] - // So we need to skip tokens[0] which is "add" - const auto& tokens = args.data(); - - if (tokens.size() < 2) { - return "Error: prefix is required. Usage: network6 add [policy " - "] [install-to-fib ] [min-routes ]"; - } - - // tokens[0] is "add", tokens[1] is the actual prefix - std::string prefixStr = tokens[1]; - std::string policyName; - bool installToFib = false; - int32_t minSupportingRoutes = 0; - - // Start parsing optional args from index 2 - for (size_t i = 2; i < tokens.size(); i++) { - if (tokens[i] == "policy" && i + 1 < tokens.size()) { - policyName = tokens[++i]; - } else if (tokens[i] == "install-to-fib" && i + 1 < tokens.size()) { - std::string val = tokens[++i]; - installToFib = (val == "true" || val == "1"); - } else if (tokens[i] == "min-routes" && i + 1 < tokens.size()) { - std::string minRoutesStr = tokens[++i]; - try { - minSupportingRoutes = folly::to(minRoutesStr); - } catch (const std::exception& e) { - return fmt::format( - "Error: Invalid min-routes value '{}': {}", minRoutesStr, e.what()); - } - } - } - - session.addNetwork6(prefixStr, policyName, installToFib, minSupportingRoutes); - session.saveConfig(); - - return fmt::format( - "Successfully added network6 prefix: {} (policy: {}, install_to_fib: {}, min_routes: {})\nConfig saved to: {}", - prefixStr, - policyName.empty() ? "(none)" : policyName, - installToFib ? "true" : "false", - minSupportingRoutes, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalNetwork6Add::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalNetwork6Add, - CmdConfigProtocolBgpGlobalNetwork6AddTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.h deleted file mode 100644 index 885e7b3e2b63a..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalNetwork6Add.h +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalNetwork6AddTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalNetwork6Add - : public CmdHandler< - CmdConfigProtocolBgpGlobalNetwork6Add, - CmdConfigProtocolBgpGlobalNetwork6AddTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpGlobalNetwork6AddTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpGlobalNetwork6AddTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& args); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.cpp deleted file mode 100644 index c7093026cd1e7..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.cpp +++ /dev/null @@ -1,52 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalRouterIdTraits::RetType -CmdConfigProtocolBgpGlobalRouterId::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& routerIdList) { - auto& session = BgpConfigSession::getInstance(); - - if (routerIdList.data().empty()) { - return "Error: router-id is required"; - } - - // Extract the first IP from the list as router-id - std::string routerId = routerIdList.data()[0]; - - // Set the router-id in the BGP config session - session.setRouterId(routerId); - session.saveConfig(); - - return fmt::format( - "Successfully set BGP router-id to: {}\nConfig saved to: {}", - routerId, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalRouterId::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalRouterId, - CmdConfigProtocolBgpGlobalRouterIdTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.h deleted file mode 100644 index dd2f2871144c5..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalRouterId.h +++ /dev/null @@ -1,41 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalRouterIdTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("ipAddrs", args, "IPv4 or IPv6 addr(s)"); - } - using ObjectArgType = utils::IPList; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalRouterId - : public CmdHandler< - CmdConfigProtocolBgpGlobalRouterId, - CmdConfigProtocolBgpGlobalRouterIdTraits> { - public: - using ObjectArgType = CmdConfigProtocolBgpGlobalRouterIdTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpGlobalRouterIdTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& routerId); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.cpp deleted file mode 100644 index 6ef8c4fd536f5..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.cpp +++ /dev/null @@ -1,51 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVipsTraits::RetType -CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& limitStr) { - auto& session = BgpConfigSession::getInstance(); - - if (limitStr.data().empty()) { - return "Error: max-golden-vips value is required"; - } - - int64_t limit = std::stoll(limitStr.data()[0]); - - session.setSwitchLimitMaxGoldenVips(limit); - session.saveConfig(); - - return fmt::format( - "Successfully set switch_limit_config max_golden_vips to: {}\nConfig saved to: {}", - limit, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips, - CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVipsTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.h deleted file mode 100644 index c28eab45fc3f9..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips.h +++ /dev/null @@ -1,44 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVipsTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips - : public CmdHandler< - CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVips, - CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVipsTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVipsTraits::ObjectArgType; - using RetType = - CmdConfigProtocolBgpGlobalSwitchLimitMaxGoldenVipsTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& limit); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.cpp deleted file mode 100644 index 78c634bf4bcd2..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.cpp +++ /dev/null @@ -1,51 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionModeTraits::RetType -CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& modeStr) { - auto& session = BgpConfigSession::getInstance(); - - if (modeStr.data().empty()) { - return "Error: overload-protection-mode value is required"; - } - - int32_t mode = std::stoi(modeStr.data()[0]); - - session.setSwitchLimitOverloadProtectionMode(mode); - session.saveConfig(); - - return fmt::format( - "Successfully set switch_limit_config overload_protection_mode to: {}\nConfig saved to: {}", - mode, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode, - CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionModeTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.h deleted file mode 100644 index 2a155cf1460a4..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode.h +++ /dev/null @@ -1,46 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionModeTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode - : public CmdHandler< - CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionMode, - CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionModeTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionModeTraits:: - ObjectArgType; - using RetType = - CmdConfigProtocolBgpGlobalSwitchLimitOverloadProtectionModeTraits:: - RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& mode); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.cpp deleted file mode 100644 index 0241e189f6f1c..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.cpp +++ /dev/null @@ -1,54 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimitTraits::RetType -CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& limitStr) { - auto& session = BgpConfigSession::getInstance(); - - // Command: "config protocol bgp global switch-limit " - // After "switch-limit", args contains just [""] - const auto& tokens = limitStr.data(); - if (tokens.empty()) { - return "Error: prefix-limit value is required. Usage: switch-limit "; - } - - int64_t limit = std::stoll(tokens[0]); - - session.setSwitchLimitPrefixLimit(limit); - session.saveConfig(); - - return fmt::format( - "Successfully set switch_limit_config prefix_limit to: {}\nConfig saved to: {}", - limit, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit, - CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimitTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.h deleted file mode 100644 index d6496f037c749..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit.h +++ /dev/null @@ -1,44 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimitTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit - : public CmdHandler< - CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimit, - CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimitTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimitTraits::ObjectArgType; - using RetType = - CmdConfigProtocolBgpGlobalSwitchLimitPrefixLimitTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& limit); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.cpp deleted file mode 100644 index b0d67f57ec616..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.cpp +++ /dev/null @@ -1,51 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimitTraits::RetType -CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit::queryClient( - const HostInfo& /* hostInfo */, - const ObjectArgType& limitStr) { - auto& session = BgpConfigSession::getInstance(); - - if (limitStr.data().empty()) { - return "Error: total-path-limit value is required"; - } - - int64_t limit = std::stoll(limitStr.data()[0]); - - session.setSwitchLimitTotalPathLimit(limit); - session.saveConfig(); - - return fmt::format( - "Successfully set switch_limit_config total_path_limit to: {}\nConfig saved to: {}", - limit, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit, - CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimitTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.h deleted file mode 100644 index 23e702fa743d7..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit.h +++ /dev/null @@ -1,44 +0,0 @@ -/* - * 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/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimitTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpGlobal; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit - : public CmdHandler< - CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimit, - CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimitTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimitTraits::ObjectArgType; - using RetType = - CmdConfigProtocolBgpGlobalSwitchLimitTotalPathLimitTraits::RetType; - - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& limit); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/BgpConfigSessionTest.cpp b/fboss/cli/fboss2/test/config/BgpConfigSessionTest.cpp index efaa968b47704..da9d2698aa0b8 100644 --- a/fboss/cli/fboss2/test/config/BgpConfigSessionTest.cpp +++ b/fboss/cli/fboss2/test/config/BgpConfigSessionTest.cpp @@ -75,14 +75,6 @@ TEST_F(BgpConfigSessionTest, setConfedAsn) { EXPECT_EQ(config["local_confed_as_4_byte"].asInt(), 65100); } -TEST_F(BgpConfigSessionTest, setClusterId) { - session_->setClusterId("1.2.3.4"); - EXPECT_EQ(session_->getClusterId(), "1.2.3.4"); - - auto& config = session_->getBgpConfig(); - EXPECT_EQ(config["cluster_id"].asString(), "1.2.3.4"); -} - TEST_F(BgpConfigSessionTest, setListenAddress) { session_->setListenAddress("0.0.0.0"); EXPECT_EQ(session_->getListenAddress(), "0.0.0.0"); diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 8b5771cb4072c..db70f955be817 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -20,6 +20,8 @@ cpp_binary( "ConfigAclTest.cpp", "ConfigAdminDistanceTest.cpp", "ConfigArpTest.cpp", + "ConfigBgpGlobalTest.cpp", + "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", "ConfigCoppTest.cpp", "ConfigDhcpSourceOverrideTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp new file mode 100644 index 0000000000000..b99b765b09e4f --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp @@ -0,0 +1,114 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp global `. + * + * Scope: the BGP *global* tunables only. Each positive test stages the change + * AND commits it, then asserts the value landed at the correct thrift field + * path in the promoted system config (/etc/coop/bgpcpp/bgpcpp.conf) that the + * bgp_pp daemon consumes. Session-lifecycle behavior (clear / diff / rollback / + * commit-restart mechanics) lives in ConfigBgpSessionTest. + * + * - count-confeds-in-as-path-len + * -> BgpConfig.count_confeds_in_as_path_len + * - graceful-restart-time + * -> BgpConfig.graceful_restart_convergence_seconds + * - rib-allocated-path-ids + * -> BgpConfig.bgp_setting_config.enable_rib_allocated_path_id + * + * Requirements: + * - The fboss2-dev binary under test (config subcommand tree). + * - HOME is set (the session file lives under $HOME/.fboss2). + * - bgp_pp is installed/active (commit restarts it). + */ + +#include +#include +#include + +#include "fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h" + +using namespace facebook::fboss; +using ::testing::HasSubstr; + +class ConfigBgpGlobalTest : public ConfigBgpTestBase {}; + +TEST_F(ConfigBgpGlobalTest, SetCountConfedsInAsPathLenTrue) { + auto config = setAndCommit("count-confeds-in-as-path-len", "true"); + ASSERT_TRUE(config.count("count_confeds_in_as_path_len")); + EXPECT_TRUE(config["count_confeds_in_as_path_len"].asBool()); +} + +TEST_F(ConfigBgpGlobalTest, SetCountConfedsInAsPathLenFalse) { + auto config = setAndCommit("count-confeds-in-as-path-len", "false"); + ASSERT_TRUE(config.count("count_confeds_in_as_path_len")); + EXPECT_FALSE(config["count_confeds_in_as_path_len"].asBool()); +} + +TEST_F(ConfigBgpGlobalTest, SetGracefulRestartTime) { + // Stage a value that differs from the current system config: committing an + // unchanged config is a no-op that yields no SHA, and the shipped + // bgpcpp.conf already sets 120. + int64_t target = 120; + try { + auto current = readSystemBgpConfig(); + if (current.count("graceful_restart_convergence_seconds") && + current["graceful_restart_convergence_seconds"].asInt() == target) { + target = 121; + } + } catch (const std::exception&) { + // No readable system config yet — any value is a change. + } + auto config = setAndCommit("graceful-restart-time", std::to_string(target)); + ASSERT_TRUE(config.count("graceful_restart_convergence_seconds")); + EXPECT_EQ(config["graceful_restart_convergence_seconds"].asInt(), target); + // Must not write the per-peer timer field name. + EXPECT_FALSE(config.count("graceful_restart_seconds")); +} + +TEST_F(ConfigBgpGlobalTest, SetRibAllocatedPathIdsTrue) { + auto config = setAndCommit("rib-allocated-path-ids", "true"); + ASSERT_TRUE(config.count("bgp_setting_config")); + ASSERT_TRUE( + config["bgp_setting_config"].count("enable_rib_allocated_path_id")); + EXPECT_TRUE( + config["bgp_setting_config"]["enable_rib_allocated_path_id"].asBool()); + // Must be nested, not at the top level. + EXPECT_FALSE(config.count("enable_rib_allocated_path_id")); +} + +TEST_F(ConfigBgpGlobalTest, SetRibAllocatedPathIdsFalse) { + auto config = setAndCommit("rib-allocated-path-ids", "false"); + ASSERT_TRUE(config.count("bgp_setting_config")); + ASSERT_TRUE( + config["bgp_setting_config"].count("enable_rib_allocated_path_id")); + EXPECT_FALSE( + config["bgp_setting_config"]["enable_rib_allocated_path_id"].asBool()); +} + +TEST_F(ConfigBgpGlobalTest, InvalidBoolValueRejected) { + clearBgpSession(); + auto result = runCli( + {"config", + "protocol", + "bgp", + "global", + "count-confeds-in-as-path-len", + "maybe"}); + // The handler reports the error on stdout; ensure it did not silently + // succeed and persist a bogus value. + EXPECT_THAT(result.stdout, HasSubstr("Invalid value")); + // The handler must bail out before saving, so no session file is written for + // the rejected input. + EXPECT_FALSE(std::filesystem::exists(bgpSessionPath())) + << "session file should not exist after rejected input"; +} + +TEST_F(ConfigBgpGlobalTest, NegativeGracefulRestartTimeRejected) { + clearBgpSession(); + auto result = runCli( + {"config", "protocol", "bgp", "global", "graceful-restart-time", "-1"}); + EXPECT_THAT(result.stdout, HasSubstr("non-negative")); + EXPECT_FALSE(std::filesystem::exists(bgpSessionPath())) + << "session file should not exist after rejected input"; +} diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp new file mode 100644 index 0000000000000..9dc600dd85ef8 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp @@ -0,0 +1,218 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for BGP config *session* management, mirroring the agent + * config session tests (e.g. ConfigSessionClearTest) but for the BGP domain. + * + * BGP global edits stage ~/.fboss2/bgp_config.json (no agent.conf), and + * `config session commit` promotes it to /etc/coop/bgpcpp/bgpcpp.conf and + * restarts bgp_pp. These tests exercise the session lifecycle around that: + * - clear (including a BGP-only session) + * - diff (a staged BGP change must be visible) + * - commit (bgp_pp is restarted) + * - rollback (the bgp_pp system config is restored) + * + * Per-attribute global config behavior lives in ConfigBgpGlobalTest. + * + * Requirements: + * - The fboss2-dev binary under test (config subcommand tree). + * - HOME is set (the session file lives under $HOME/.fboss2). + * - bgp_pp installed; commit/rollback tests skip if bgp_pp is not active. + */ + +#include +#include +#include + +#include "fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h" + +namespace fs = std::filesystem; + +using namespace facebook::fboss; +using ::testing::HasSubstr; +using ::testing::Not; + +class ConfigBgpSessionTest : public ConfigBgpTestBase {}; + +// Staging a BGP global change creates a session file; `config session clear` +// removes it (a BGP-only session has no agent.conf to key off of). +TEST_F(ConfigBgpSessionTest, CreateAndClearBgpSession) { + runBgpGlobal("graceful-restart-time", "55"); + ASSERT_TRUE(fs::exists(bgpSessionPath())) + << "expected a staged BGP session after a global change"; + + auto result = runCli({"config", "session", "clear"}); + ASSERT_EQ(result.exitCode, 0) << "stderr=" << result.stderr; + + EXPECT_FALSE(fs::exists(bgpSessionPath())) + << "config session clear must remove the staged BGP config"; +} + +TEST_F(ConfigBgpSessionTest, ClearNonExistentBgpSession) { + // Start fully clean: no agent session, no BGP session, no metadata. + discardSession(); + clearBgpSession(); + + auto result = runCli({"config", "session", "clear"}); + ASSERT_EQ(result.exitCode, 0) << "stderr=" << result.stderr; + EXPECT_THAT(result.stdout, HasSubstr("No config session exists")); +} + +// `config session diff` must surface a staged BGP global change. +TEST_F(ConfigBgpSessionTest, DiffShowsStagedBgpChange) { + runBgpGlobal("graceful-restart-time", "77"); + + auto result = runCli({"config", "session", "diff"}); + ASSERT_EQ(result.exitCode, 0) << "stderr=" << result.stderr; + EXPECT_THAT(result.stdout, Not(HasSubstr("No config session exists"))); + EXPECT_THAT(result.stdout, HasSubstr("graceful_restart_convergence_seconds")); +} + +// Committing a staged BGP change restarts the bgp_pp daemon. +TEST_F(ConfigBgpSessionTest, CommitRestartsBgpDaemon) { + if (bgpDaemonActiveState() != "active") { + GTEST_SKIP() << "bgp_pp is not active on this DUT; skipping commit/restart " + "verification"; + } + discardSession(); + const std::string pidBefore = bgpDaemonMainPid(); + + runBgpGlobal("graceful-restart-time", "123"); + ASSERT_FALSE(commitAndGetSha().empty()); + + EXPECT_TRUE(waitForBgpDaemonActive()) + << "bgp_pp did not return to active after commit; state=" + << bgpDaemonActiveState(); + + const std::string pidAfter = bgpDaemonMainPid(); + EXPECT_NE(pidAfter, "0") << "bgp_pp has no MainPID after commit"; + EXPECT_NE(pidAfter, pidBefore) << "bgp_pp MainPID unchanged (" << pidBefore + << "); expected a restart on commit"; +} + +// Rolling back a committed BGP change restores the bgp_pp system config and +// restarts bgp_pp (not just the agent config). +TEST_F(ConfigBgpSessionTest, RollbackRestoresBgpConfig) { + if (bgpDaemonActiveState() != "active") { + GTEST_SKIP() << "bgp_pp is not active on this DUT; skipping rollback " + "verification"; + } + discardSession(); + + auto convergenceSeconds = [&]() { + return readSystemBgpConfig()["graceful_restart_convergence_seconds"] + .asInt(); + }; + + // Commit A (111), then commit B (222). + runBgpGlobal("graceful-restart-time", "111"); + std::string shaA = commitAndGetSha(); + ASSERT_FALSE(shaA.empty()) << "could not parse commit A sha"; + ASSERT_TRUE(waitForBgpDaemonActive()); + + runBgpGlobal("graceful-restart-time", "222"); + ASSERT_FALSE(commitAndGetSha().empty()) << "could not parse commit B sha"; + ASSERT_TRUE(waitForBgpDaemonActive()); + + EXPECT_EQ(convergenceSeconds(), 222) + << "system bgp config should reflect the latest commit"; + + // Roll back to commit A -> system bgp config restored to 111, bgp_pp restart. + resetBgpDaemonLimit(); // avoid systemd start-limit on the rollback's restart + auto rb = runCli({"config", "rollback", shaA}); + EXPECT_EQ(rb.exitCode, 0) << "stderr=" << rb.stderr; + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgp_pp did not return active after rollback"; + + EXPECT_EQ(convergenceSeconds(), 111) + << "rollback must restore the bgp_pp system config"; +} + +// Committing a BGP config identical to what is already running must be a no-op: +// "Nothing to commit" and, crucially, NO bgp_pp restart (a restart is traffic +// affecting). saveBgpConfig() always records BGP_RESTART, so commit() has to +// compare the staged config against the running one. +TEST_F(ConfigBgpSessionTest, CommitUnchangedBgpConfigDoesNotRestart) { + if (bgpDaemonActiveState() != "active") { + GTEST_SKIP() << "bgp_pp is not active on this DUT; skipping no-op restart " + "verification"; + } + discardSession(); + + // Establish a known running value. + runBgpGlobal("graceful-restart-time", "88"); + ASSERT_FALSE(commitAndGetSha().empty()); + ASSERT_TRUE(waitForBgpDaemonActive()); + const std::string pidBefore = bgpDaemonMainPid(); + ASSERT_NE(pidBefore, "0"); + + // Stage the SAME value and commit again -> nothing to commit, no restart. + runBgpGlobal("graceful-restart-time", "88"); + auto result = runCli({"config", "session", "commit"}); + EXPECT_EQ(result.exitCode, 0) << "stderr=" << result.stderr; + EXPECT_THAT(result.stdout, HasSubstr("Nothing to commit")) + << "an unchanged BGP commit should report nothing to commit"; + + EXPECT_EQ(bgpDaemonMainPid(), pidBefore) + << "bgp_pp was restarted for an unchanged BGP config (pidBefore=" + << pidBefore << ")"; +} + +// An agent-only commit (no BGP staged) must still snapshot the running bgp_pp +// config, so a rollback to it restores BGP instead of wiping it. Pre-fix, +// bgpcpp.conf was untracked at such commits and rollback deleted it. +TEST_F(ConfigBgpSessionTest, AgentCommitSnapshotsBgpAndRollbackPreservesIt) { + if (bgpDaemonActiveState() != "active") { + GTEST_SKIP() << "bgp_pp is not active on this DUT; skipping cross-domain " + "rollback verification"; + } + discardSession(); + clearBgpSession(); + + // The running (default) BGP config that must survive a rollback unchanged. + ASSERT_TRUE(fs::exists(systemBgpConfigPath())) + << "expected a running bgp_pp config on an active-BGP DUT"; + const folly::dynamic defaultBgp = readSystemBgpConfig(); + + // Step 1: an agent-only commit (interface description). No BGP is staged. + Interface intf = getInterfaceInfo(getRandomInterfacePortName()); + auto setRes = + runCli({"config", "interface", intf.name, "description", "bgp-rb-test"}); + ASSERT_EQ(setRes.exitCode, 0) << "stderr=" << setRes.stderr; + ASSERT_FALSE(fs::exists(bgpSessionPath())) + << "an interface-description change must not stage a BGP session"; + resetBgpDaemonLimit(); + commitConfig(); + waitForAgentReady(); + const std::string agentSha = gitHead(); + ASSERT_FALSE(agentSha.empty()) + << "could not read git HEAD after agent commit"; + + // The fix: bgpcpp.conf must be tracked at the agent-only commit, so a later + // rollback to it has a faithful BGP snapshot to restore. + EXPECT_TRUE(bgpTrackedAtRevision(agentSha)) + << "agent-only commit " << agentSha + << " did not snapshot bgpcpp.conf; a rollback to it would wipe BGP"; + + // Step 2: stage + commit a BGP change so the running config diverges from the + // snapshot captured at agentSha. + runBgpGlobal("graceful-restart-time", "222"); + ASSERT_FALSE(commitAndGetSha().empty()) << "BGP commit produced no SHA"; + ASSERT_TRUE(waitForBgpDaemonActive()); + EXPECT_EQ( + readSystemBgpConfig()["graceful_restart_convergence_seconds"].asInt(), + 222); + + // Step 3: roll back to the agent-only commit. BGP must be RESTORED to the + // original default snapshot (not deleted), and bgp_pp must come back. + resetBgpDaemonLimit(); + auto rb = runCli({"config", "rollback", agentSha}); + EXPECT_EQ(rb.exitCode, 0) << "stderr=" << rb.stderr; + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgp_pp did not return active after rollback"; + + EXPECT_TRUE(fs::exists(systemBgpConfigPath())) + << "rollback to an agent-era commit wiped the running bgp_pp config"; + EXPECT_EQ(readSystemBgpConfig(), defaultBgp) + << "rollback did not restore the original (default) bgp_pp config"; +} diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h new file mode 100644 index 0000000000000..1010fc3227855 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h @@ -0,0 +1,212 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#pragma once + +/** + * Shared fixture + helpers for BGP config integration tests. + * + * BGP config is not pushed into the agent's running config like the + * SwitchConfig-backed commands. Instead the CLI stages a BGP++ + * thrift-compatible JSON file at ~/.fboss2/bgp_config.json; `config session + * commit` promotes it to /etc/coop/bgpcpp/bgpcpp.conf and restarts the bgp_pp + * daemon. These helpers let the global-attribute tests and the + * session-lifecycle tests share the staging, commit, daemon-state, and + * system-config plumbing. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" + +namespace facebook::fboss { + +class ConfigBgpTestBase : public Fboss2IntegrationTest { + protected: + void SetUp() override { + Fboss2IntegrationTest::SetUp(); + if (bgpDaemonActiveState() != "active") { + GTEST_SKIP() << "skipping because bgp is not active"; + } + } + + // Staged session file written by the CLI (~/.fboss2/bgp_config.json). + std::string bgpSessionPath() const { + // NOLINTNEXTLINE(concurrency-mt-unsafe): HOME is read-only in practice + const char* home = std::getenv("HOME"); + if (home == nullptr) { + throw std::runtime_error("HOME environment variable not set"); + } + return std::string(home) + "/.fboss2/bgp_config.json"; + } + + // System config consumed by the bgp_pp daemon (written on commit). + std::string systemBgpConfigPath() const { + return "/etc/coop/bgpcpp/bgpcpp.conf"; + } + + // Remove any pre-existing staged BGP session so each test starts clean. + void clearBgpSession() const { + std::remove(bgpSessionPath().c_str()); + } + + folly::dynamic readBgpSessionConfig() const { + std::string content; + if (!folly::readFile(bgpSessionPath().c_str(), content)) { + throw std::runtime_error( + "Failed to read BGP session config at " + bgpSessionPath()); + } + return folly::parseJson(content); + } + + folly::dynamic readSystemBgpConfig() const { + std::string content; + if (!folly::readFile(systemBgpConfigPath().c_str(), content)) { + throw std::runtime_error( + "Failed to read system BGP config at " + systemBgpConfigPath()); + } + return folly::parseJson(content); + } + + // Root of the git repo that versions both the agent config (cli/agent.conf) + // and the BGP config (bgpcpp/bgpcpp.conf). + std::string coopRepoDir() const { + return "/etc/coop"; + } + + // Current git HEAD sha of the /etc/coop repo (trimmed; "" on failure). + // runCmd uses folly::Subprocess, which does not search PATH, so git is + // invoked by full path. + std::string gitHead() const { + auto r = runCmd({"/usr/bin/git", "-C", coopRepoDir(), "rev-parse", "HEAD"}); + std::string s = r.stdout; + while (!s.empty() && + (s.back() == '\n' || s.back() == '\r' || s.back() == ' ')) { + s.pop_back(); + } + return s; + } + + // True if bgpcpp/bgpcpp.conf is tracked (present in the tree) at the given + // revision. `git cat-file -e :` exits 0 iff the blob exists. + bool bgpTrackedAtRevision(const std::string& sha) const { + auto r = runCmd( + {"/usr/bin/git", + "-C", + coopRepoDir(), + "cat-file", + "-e", + sha + ":bgpcpp/bgpcpp.conf"}); + return r.exitCode == 0; + } + + // Stage a `config protocol bgp global ` change (no commit). + // Returns the staged session JSON. + folly::dynamic runBgpGlobal(const std::string& attr, const std::string& value) + const { + clearBgpSession(); + XLOG(INFO) << "Running: config protocol bgp global " << attr << " " + << value; + auto result = runCli({"config", "protocol", "bgp", "global", attr, value}); + EXPECT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + // Handler error strings are printed to stdout with exit code 0, so guard + // against silent failures by rejecting an "Error" prefix. + EXPECT_THAT(result.stdout, ::testing::Not(::testing::HasSubstr("Error:"))); + return readBgpSessionConfig(); + } + + // Reset bgp_pp's systemd start-limit burst counter. Tests commit several + // times in quick succession, and each commit restarts bgp_pp; without this, + // systemd's default StartLimitBurst (5 starts / 10s) refuses a rapid restart + // with "start limit hit" — a test artifact, not a product issue. + void resetBgpDaemonLimit() const { + runCmd({"/usr/bin/systemctl", "reset-failed", "bgp_pp"}); + } + + // Run `config session commit` and return the short SHA parsed from + // "Config session committed successfully as ". Empty string on failure. + std::string commitAndGetSha() const { + resetBgpDaemonLimit(); + auto r = runCli({"config", "session", "commit"}); + EXPECT_EQ(r.exitCode, 0) << "stderr=" << r.stderr; + const std::string marker = "as "; + auto pos = r.stdout.find(marker); + if (pos == std::string::npos) { + return ""; + } + pos += marker.size(); + auto end = r.stdout.find_first_of(" .\n", pos); + return r.stdout.substr(pos, end - pos); + } + + // Stage a global change, commit it, wait for bgp_pp to come back, and return + // the promoted system config (the authoritative post-commit location). + // A no-op commit (staged value already in the system config) yields no SHA + // and is fine: the caller's assertions on the returned config still verify + // the value; commit/restart mechanics are covered by ConfigBgpSessionTest. + folly::dynamic setAndCommit(const std::string& attr, const std::string& value) + const { + discardSession(); + runBgpGlobal(attr, value); + commitAndGetSha(); + EXPECT_TRUE(waitForBgpDaemonActive()) + << "bgp_pp did not return active after commit; state=" + << bgpDaemonActiveState(); + return readSystemBgpConfig(); + } + + // Trailing-whitespace-trimmed stdout of a `systemctl` query for bgp_pp. + std::string systemctlValue(const std::string& property) const { + // Full path: folly::Subprocess (used by runCmd) does not search PATH. + auto r = runCmd( + {"/usr/bin/systemctl", "show", "bgp_pp", "-p", property, "--value"}); + std::string s = r.stdout; + while (!s.empty() && + (s.back() == '\n' || s.back() == '\r' || s.back() == ' ')) { + s.pop_back(); + } + return s; + } + + // "active" / "inactive" / "failed" / ... (trimmed). + std::string bgpDaemonActiveState() const { + auto r = runCmd({"/usr/bin/systemctl", "is-active", "bgp_pp"}); + std::string s = r.stdout; + while (!s.empty() && + (s.back() == '\n' || s.back() == '\r' || s.back() == ' ')) { + s.pop_back(); + } + return s; + } + + // bgp_pp main PID per systemd ("0" if not running). + std::string bgpDaemonMainPid() const { + return systemctlValue("MainPID"); + } + + // Poll until bgp_pp reports active or the timeout elapses. + bool waitForBgpDaemonActive( + std::chrono::seconds timeout = std::chrono::seconds(60)) const { + auto deadline = std::chrono::steady_clock::now() + timeout; + while (std::chrono::steady_clock::now() < deadline) { + if (bgpDaemonActiveState() == "active") { + return true; + } + std::this_thread::sleep_for(std::chrono::seconds(1)); + } + return bgpDaemonActiveState() == "active"; + } +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index ddf328c4fd3ca..cbc096e7a0bcd 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -20,7 +20,7 @@ import json import shlex import sys -from typing import Any, Dict, List +from typing import Any def escape_shell_arg(arg: str) -> str: @@ -28,7 +28,7 @@ def escape_shell_arg(arg: str) -> str: return shlex.quote(str(arg)) -def _generate_global_basic_commands(config: Dict[str, Any]) -> List[str]: +def _generate_global_basic_commands(config: dict[str, Any]) -> list[str]: """Generate basic global BGP commands.""" commands = [] if "router_id" in config: @@ -47,14 +47,10 @@ def _generate_global_basic_commands(config: Dict[str, Any]) -> List[str]: commands.append( f"config protocol bgp global confed-asn {escape_shell_arg(str(config['local_confed_as_4_byte']))}" ) - if "cluster_id" in config: - commands.append( - f"config protocol bgp global cluster-id {escape_shell_arg(config['cluster_id'])}" - ) return commands -def _generate_network6_commands(networks: List[Dict[str, Any]]) -> List[str]: +def _generate_network6_commands(networks: list[dict[str, Any]]) -> list[str]: """Generate network6 commands for global BGP config.""" commands = [] for network in networks: @@ -76,7 +72,7 @@ def _generate_network6_commands(networks: List[Dict[str, Any]]) -> List[str]: return commands -def _generate_switch_limit_commands(switch_limit: Dict[str, Any]) -> List[str]: +def _generate_switch_limit_commands(switch_limit: dict[str, Any]) -> list[str]: """Generate switch_limit_config commands.""" commands = [] if "prefix_limit" in switch_limit: @@ -98,7 +94,7 @@ def _generate_switch_limit_commands(switch_limit: Dict[str, Any]) -> List[str]: return commands -def generate_global_commands(config: Dict[str, Any]) -> List[str]: +def generate_global_commands(config: dict[str, Any]) -> list[str]: """Generate CLI commands for global BGP settings.""" commands = [] commands.extend(_generate_global_basic_commands(config)) @@ -110,8 +106,8 @@ def generate_global_commands(config: Dict[str, Any]) -> List[str]: def _generate_peer_group_basic_commands( - peer_group: Dict[str, Any], escaped_name: str -) -> List[str]: + peer_group: dict[str, Any], escaped_name: str +) -> list[str]: """Generate basic peer-group commands (remote-asn, description, policies).""" commands = [] if "remote_as_4_byte" in peer_group: @@ -138,8 +134,8 @@ def _generate_peer_group_basic_commands( def _generate_peer_group_bool_commands( - peer_group: Dict[str, Any], escaped_name: str -) -> List[str]: + peer_group: dict[str, Any], escaped_name: str +) -> list[str]: """Generate boolean flag commands for peer-group.""" commands = [] bool_fields = [ @@ -158,8 +154,8 @@ def _generate_peer_group_bool_commands( def _generate_peer_group_timer_commands( - timers: Dict[str, Any], escaped_name: str -) -> List[str]: + timers: dict[str, Any], escaped_name: str +) -> list[str]: """Generate timer commands for peer-group.""" commands = [] if timers.get("hold_time_seconds"): @@ -182,8 +178,8 @@ def _generate_peer_group_timer_commands( def _generate_peer_group_prefilter_commands( - pre_filter: Dict[str, Any], escaped_name: str -) -> List[str]: + pre_filter: dict[str, Any], escaped_name: str +) -> list[str]: """Generate pre_filter commands for peer-group.""" commands = [] if pre_filter.get("max_routes"): @@ -201,7 +197,7 @@ def _generate_peer_group_prefilter_commands( return commands -def generate_peer_group_commands(peer_group: Dict[str, Any]) -> List[str]: +def generate_peer_group_commands(peer_group: dict[str, Any]) -> list[str]: """Generate CLI commands for a peer group.""" name = peer_group.get("name", "") if not name: @@ -228,16 +224,16 @@ def format_bandwidth(bps: int) -> str: """Format bandwidth in bps to human-readable format (e.g., 10G).""" if bps >= 1_000_000_000_000 and bps % 1_000_000_000_000 == 0: return f"{bps // 1_000_000_000_000}T" - elif bps >= 1_000_000_000 and bps % 1_000_000_000 == 0: + if bps >= 1_000_000_000 and bps % 1_000_000_000 == 0: return f"{bps // 1_000_000_000}G" - elif bps >= 1_000_000 and bps % 1_000_000 == 0: + if bps >= 1_000_000 and bps % 1_000_000 == 0: return f"{bps // 1_000_000}M" - elif bps >= 1_000 and bps % 1_000 == 0: + if bps >= 1_000 and bps % 1_000 == 0: return f"{bps // 1_000}K" return str(bps) -def _generate_peer_basic_commands(peer: Dict[str, Any], peer_addr: str) -> List[str]: +def _generate_peer_basic_commands(peer: dict[str, Any], peer_addr: str) -> list[str]: """Generate basic peer commands (remote-asn, peer-group, description, etc.).""" commands = [] if "remote_as_4_byte" in peer: @@ -267,7 +263,7 @@ def _generate_peer_basic_commands(peer: Dict[str, Any], peer_addr: str) -> List[ return commands -def _generate_peer_bool_commands(peer: Dict[str, Any], peer_addr: str) -> List[str]: +def _generate_peer_bool_commands(peer: dict[str, Any], peer_addr: str) -> list[str]: """Generate boolean flag commands for peer.""" commands = [] bool_fields = [ @@ -286,7 +282,7 @@ def _generate_peer_bool_commands(peer: Dict[str, Any], peer_addr: str) -> List[s return commands -def _generate_peer_nexthop_commands(peer: Dict[str, Any], peer_addr: str) -> List[str]: +def _generate_peer_nexthop_commands(peer: dict[str, Any], peer_addr: str) -> list[str]: """Generate next-hop and link-bandwidth commands for peer.""" commands = [] if "next_hop4" in peer: @@ -306,7 +302,7 @@ def _generate_peer_nexthop_commands(peer: Dict[str, Any], peer_addr: str) -> Lis return commands -def _generate_peer_timer_commands(timers: Dict[str, Any], peer_addr: str) -> List[str]: +def _generate_peer_timer_commands(timers: dict[str, Any], peer_addr: str) -> list[str]: """Generate timer commands for peer.""" commands = [] if timers.get("hold_time_seconds"): @@ -329,8 +325,8 @@ def _generate_peer_timer_commands(timers: Dict[str, Any], peer_addr: str) -> Lis def _generate_peer_prefilter_commands( - pre_filter: Dict[str, Any], peer_addr: str -) -> List[str]: + pre_filter: dict[str, Any], peer_addr: str +) -> list[str]: """Generate pre_filter commands for peer.""" commands = [] if pre_filter.get("max_routes"): @@ -348,7 +344,7 @@ def _generate_peer_prefilter_commands( return commands -def _generate_peer_id_commands(peer: Dict[str, Any], peer_addr: str) -> List[str]: +def _generate_peer_id_commands(peer: dict[str, Any], peer_addr: str) -> list[str]: """Generate peer identification commands.""" commands = [] if "peer_id" in peer: @@ -362,7 +358,7 @@ def _generate_peer_id_commands(peer: Dict[str, Any], peer_addr: str) -> List[str return commands -def generate_peer_commands(peer: Dict[str, Any]) -> List[str]: +def generate_peer_commands(peer: dict[str, Any]) -> list[str]: """Generate CLI commands for a peer.""" peer_addr = peer.get("peer_addr", "") if not peer_addr: @@ -385,7 +381,7 @@ def generate_peer_commands(peer: Dict[str, Any]) -> List[str]: return commands -def generate_stub_commands(commands: List[str], binary: str = "fboss2") -> List[str]: +def generate_stub_commands(commands: list[str], binary: str = "fboss2") -> list[str]: """Wrap commands for stub mode execution.""" stub_commands = [ "#!/bin/bash", @@ -400,7 +396,7 @@ def generate_stub_commands(commands: List[str], binary: str = "fboss2") -> List[ return stub_commands -def generate_exec_commands(commands: List[str], binary: str = "fboss2") -> List[str]: +def generate_exec_commands(commands: list[str], binary: str = "fboss2") -> list[str]: """Generate executable commands with custom binary.""" exec_commands = [ "#!/bin/bash", @@ -425,8 +421,8 @@ def generate_exec_commands(commands: List[str], binary: str = "fboss2") -> List[ def json_to_cli( - config: Dict[str, Any], stub: bool = False, binary: str = "fboss2" -) -> List[str]: + config: dict[str, Any], stub: bool = False, binary: str = "fboss2" +) -> list[str]: """Convert BGP++ JSON config to CLI commands.""" commands = [] @@ -474,7 +470,7 @@ def main() -> int: args = parser.parse_args() try: - with open(args.input, "r") as f: + with open(args.input) as f: config = json.load(f) except FileNotFoundError: print(f"Error: Input file '{args.input}' not found", file=sys.stderr) diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index 0c5f7a604aad2..9d5377cb34730 100644 --- a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py @@ -134,14 +134,6 @@ def test_confed_asn(self) -> None: commands = generate_global_commands(config) self.assertEqual(commands, ["config protocol bgp global confed-asn 65001"]) - def test_cluster_id(self) -> None: - """Cluster ID should generate correct command.""" - config = {"cluster_id": "192.168.1.1"} - commands = generate_global_commands(config) - self.assertEqual( - commands, ["config protocol bgp global cluster-id 192.168.1.1"] - ) - def test_networks6_with_all_fields(self) -> None: """networks6 should generate command with all fields.""" config = { @@ -162,14 +154,7 @@ def test_networks6_with_all_fields(self) -> None: def test_networks6_with_install_to_fib_true(self) -> None: """networks6 with install_to_fib=true should preserve value.""" - config = { - "networks6": [ - { - "prefix": "2001:db8::/32", - "install_to_fib": True, - } - ] - } + config = {"networks6": [{"prefix": "2001:db8::/32", "install_to_fib": True}]} commands = generate_global_commands(config) self.assertIn("install-to-fib true", commands[0]) @@ -569,10 +554,7 @@ def test_full_rsw_config(self) -> None: "keep_alive_seconds": 10, }, }, - { - "name": "RSW-RTSW-V6", - "disable_ipv4_afi": True, - }, + {"name": "RSW-RTSW-V6", "disable_ipv4_afi": True}, ], "peers": [ { From 3caf3a0673b57ec829f5be1a168fa1f11961f063 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 16 Jul 2026 08:09:01 +0000 Subject: [PATCH 2/6] bgp global command fixes - Extract the value parsers (parseBool/parseInt/parseNonNegInt32, handler Result) into a shared BgpCliValueParsers.h so sibling BGP dispatchers can reuse them, and add a bounded parseAsn4Byte: local-asn/confed-asn accepted any uint64 and silently persisted out-of-range ASNs (>= 2^32 wrap the i64 field negative). - positionals_at_end() on the global command: CLI11's parent-chain subcommand fallthrough steals value tokens that match a sibling command name (e.g. a policy named "peer-group") and misparses the command. - Integration test base: probe the bgpd unit, and pass -c safe.directory=/etc/coop on the raw git invocations (gitHead / bgpTrackedAtRevision), mirroring the CLI's Git class -- /etc/coop is owned by another user (e.g. coop) on provisioned devices, which git otherwise rejects as dubious ownership and the helpers silently return empty results. --- cmake/CliFboss2.cmake | 1 + fboss/cli/fboss2/BUCK | 1 + .../config/protocol/bgp/BgpCliValueParsers.h | 96 +++++++++++++++++++ .../bgp/global/CmdConfigProtocolBgpGlobal.cpp | 85 ++++++---------- .../bgp/global/CmdConfigProtocolBgpGlobal.h | 8 ++ .../integration_test/ConfigBgpGlobalTest.cpp | 4 +- .../integration_test/ConfigBgpSessionTest.cpp | 54 +++++------ .../test/integration_test/ConfigBgpTestBase.h | 39 +++++--- .../tools/tests/test_bgp_json_to_cli.py | 9 +- 9 files changed, 191 insertions(+), 106 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index be8b1f772cc3e..3207e7936f0e2 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -944,6 +944,7 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/mac/aging_time/CmdConfigMacAgingTime.h fboss/cli/fboss2/commands/config/protocol/CmdConfigProtocol.cpp fboss/cli/fboss2/commands/config/protocol/CmdConfigProtocol.h + fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.cpp fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h fboss/cli/fboss2/commands/config/protocol/bgp/CmdConfigProtocolBgp.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index ecce92fa778a4..7fcfb325e6ca3 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1332,6 +1332,7 @@ cpp_library( "commands/config/mac/CmdConfigMac.h", "commands/config/mac/aging_time/CmdConfigMacAgingTime.h", "commands/config/protocol/CmdConfigProtocol.h", + "commands/config/protocol/bgp/BgpCliValueParsers.h", "commands/config/protocol/bgp/BgpConfigSession.h", "commands/config/protocol/bgp/CmdConfigProtocolBgp.h", "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h", diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h new file mode 100644 index 0000000000000..6e80deec368b4 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h @@ -0,0 +1,96 @@ +/* + * 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 +#include +#include +#include +#include + +/** + * Value parsing helpers shared by the BGP config dispatchers + * (`config protocol bgp global` / `config protocol bgp neighbor`). + * + * All parsers return std::nullopt on invalid input instead of throwing so a + * handler can reject the value with a user-facing message and leave the + * session unpersisted. + */ +namespace facebook::fboss::bgpcli { + +// Outcome of an attribute handler: on failure the message is returned to the +// user and the session is NOT persisted, so a rejected value never lands on +// disk. +struct Result { + bool ok; + std::string message; +}; + +inline Result ok(std::string message) { + return Result{true, std::move(message)}; +} + +inline Result err(std::string message) { + return Result{false, std::move(message)}; +} + +inline std::optional parseBool(const std::string& value) { + if (value == "true" || value == "1" || value == "yes") { + return true; + } + if (value == "false" || value == "0" || value == "no") { + return false; + } + return std::nullopt; +} + +template +std::optional parseInt(const std::string& value) { + try { + return folly::to(value); + } catch (const std::exception&) { + return std::nullopt; + } +} + +// Parse a non-negative value that must fit in int32 (used for second-valued +// timers and min-routes). +inline std::optional parseNonNegInt32(const std::string& value) { + auto parsed = parseInt(value); + if (!parsed || *parsed < 0 || *parsed > std::numeric_limits::max()) { + return std::nullopt; + } + return static_cast(*parsed); +} + +// Parse a non-negative int64 (used for route limits). +inline std::optional parseNonNegInt64(const std::string& value) { + auto parsed = parseInt(value); + if (!parsed || *parsed < 0) { + return std::nullopt; + } + return parsed; +} + +// Parse a 4-byte ASN (RFC 6793): an unsigned value in [0, 2^32-1]. The thrift +// fields are i64, so an unchecked uint64 parse would let an out-of-range ASN +// wrap or exceed the protocol range and be persisted. +inline std::optional parseAsn4Byte(const std::string& value) { + auto parsed = parseInt(value); + if (!parsed || *parsed > std::numeric_limits::max()) { + return std::nullopt; + } + return static_cast(*parsed); +} + +} // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp index a78fa33b34229..b824d1aaae0e7 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp @@ -13,17 +13,23 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include -#include #include +#include #include #include +#include #include #include -#include +#include +#include #include #include #include +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" #include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" +#include "fmt/format.h" namespace facebook::fboss { @@ -49,53 +55,14 @@ constexpr std::string_view kRibAllocatedPathIds = "rib-allocated-path-ids"; using Tokens = std::vector; using BgpConfig = bgp::thrift::BgpConfig; - -// Outcome of an attribute handler: on failure the message is returned to the -// user and the session is NOT persisted, so a rejected value never lands on -// disk. -struct Result { - bool ok; - std::string message; -}; - -Result ok(std::string message) { - return Result{true, std::move(message)}; -} - -Result err(std::string message) { - return Result{false, std::move(message)}; -} - -// ---- value parsers (modular, shared across attributes) -------------------- - -std::optional parseBool(const std::string& value) { - if (value == "true" || value == "1" || value == "yes") { - return true; - } - if (value == "false" || value == "0" || value == "no") { - return false; - } - return std::nullopt; -} - -template -std::optional parseInt(const std::string& value) { - try { - return folly::to(value); - } catch (const std::exception&) { - return std::nullopt; - } -} - -// Parse a non-negative value that must fit in int32 (used for second-valued -// timers and min-routes). -std::optional parseNonNegInt32(const std::string& value) { - auto parsed = parseInt(value); - if (!parsed || *parsed < 0 || *parsed > std::numeric_limits::max()) { - return std::nullopt; - } - return static_cast(*parsed); -} +// Value parsers and the handler Result type are shared with the neighbor +// dispatcher (see BgpCliValueParsers.h). +using bgpcli::err; +using bgpcli::ok; +using bgpcli::parseBool; +using bgpcli::parseInt; +using bgpcli::parseNonNegInt32; +using bgpcli::Result; // ---- per-attribute handlers ------------------------------------------------ // Each handler mutates the typed bgp::thrift::BgpConfig directly. Field names @@ -113,11 +80,15 @@ Result applyLocalAsn(BgpConfig& cfg, const Tokens& values) { if (values.size() != 1) { return err("Error: local-asn requires "); } - auto asn = parseInt(values[0]); + auto asn = bgpcli::parseAsn4Byte(values[0]); if (!asn) { - return err(fmt::format("Error: Invalid local-asn value '{}'", values[0])); + return err( + fmt::format( + "Error: Invalid local-asn value '{}'; expected an unsigned " + "4-byte ASN", + values[0])); } - cfg.local_as_4_byte() = static_cast(*asn); + cfg.local_as_4_byte() = *asn; return ok(fmt::format("Successfully set BGP local-asn to: {}", *asn)); } @@ -125,11 +96,15 @@ Result applyConfedAsn(BgpConfig& cfg, const Tokens& values) { if (values.size() != 1) { return err("Error: confed-asn requires "); } - auto asn = parseInt(values[0]); + auto asn = bgpcli::parseAsn4Byte(values[0]); if (!asn) { - return err(fmt::format("Error: Invalid confed-asn value '{}'", values[0])); + return err( + fmt::format( + "Error: Invalid confed-asn value '{}'; expected an unsigned " + "4-byte ASN", + values[0])); } - cfg.local_confed_as_4_byte() = static_cast(*asn); + cfg.local_confed_as_4_byte() = *asn; return ok( fmt::format("Successfully set BGP confederation AS number to: {}", *asn)); } diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h index e8851b2bc4cea..2c64bc0d4d537 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h @@ -10,9 +10,13 @@ #pragma once +#include +#include +#include "CLI/App.hpp" #include "fboss/cli/fboss2/CmdHandler.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/CmdConfigProtocolBgp.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" namespace facebook::fboss { @@ -43,6 +47,10 @@ class BgpGlobalConfig : public utils::BaseObjectArgType { struct CmdConfigProtocolBgpGlobalTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgp; static void addCliArg(CLI::App& cmd, std::vector& args) { + // Stop CLI11's parent-chain subcommand fallthrough from stealing value + // tokens that happen to match a sibling command name (e.g. a policy + // named "peer-group"); see CmdConfigProtocolBgpNeighborTraits. + cmd.positionals_at_end(); cmd.add_option("args", args, " [value ...]"); } using ObjectArgType = BgpGlobalConfig; diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp index b99b765b09e4f..d9449833e37cb 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp @@ -6,7 +6,7 @@ * Scope: the BGP *global* tunables only. Each positive test stages the change * AND commits it, then asserts the value landed at the correct thrift field * path in the promoted system config (/etc/coop/bgpcpp/bgpcpp.conf) that the - * bgp_pp daemon consumes. Session-lifecycle behavior (clear / diff / rollback / + * bgpd daemon consumes. Session-lifecycle behavior (clear / diff / rollback / * commit-restart mechanics) lives in ConfigBgpSessionTest. * * - count-confeds-in-as-path-len @@ -19,7 +19,7 @@ * Requirements: * - The fboss2-dev binary under test (config subcommand tree). * - HOME is set (the session file lives under $HOME/.fboss2). - * - bgp_pp is installed/active (commit restarts it). + * - bgpd is installed/active (commit restarts it). */ #include diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp index 9dc600dd85ef8..ead4bf28cecd2 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp @@ -6,18 +6,18 @@ * * BGP global edits stage ~/.fboss2/bgp_config.json (no agent.conf), and * `config session commit` promotes it to /etc/coop/bgpcpp/bgpcpp.conf and - * restarts bgp_pp. These tests exercise the session lifecycle around that: + * restarts bgpd. These tests exercise the session lifecycle around that: * - clear (including a BGP-only session) * - diff (a staged BGP change must be visible) - * - commit (bgp_pp is restarted) - * - rollback (the bgp_pp system config is restored) + * - commit (bgpd is restarted) + * - rollback (the bgpd system config is restored) * * Per-attribute global config behavior lives in ConfigBgpGlobalTest. * * Requirements: * - The fboss2-dev binary under test (config subcommand tree). * - HOME is set (the session file lives under $HOME/.fboss2). - * - bgp_pp installed; commit/rollback tests skip if bgp_pp is not active. + * - bgpd installed; commit/rollback tests skip if bgpd is not active. */ #include @@ -68,10 +68,10 @@ TEST_F(ConfigBgpSessionTest, DiffShowsStagedBgpChange) { EXPECT_THAT(result.stdout, HasSubstr("graceful_restart_convergence_seconds")); } -// Committing a staged BGP change restarts the bgp_pp daemon. +// Committing a staged BGP change restarts the bgpd daemon. TEST_F(ConfigBgpSessionTest, CommitRestartsBgpDaemon) { if (bgpDaemonActiveState() != "active") { - GTEST_SKIP() << "bgp_pp is not active on this DUT; skipping commit/restart " + GTEST_SKIP() << "bgpd is not active on this DUT; skipping commit/restart " "verification"; } discardSession(); @@ -81,20 +81,20 @@ TEST_F(ConfigBgpSessionTest, CommitRestartsBgpDaemon) { ASSERT_FALSE(commitAndGetSha().empty()); EXPECT_TRUE(waitForBgpDaemonActive()) - << "bgp_pp did not return to active after commit; state=" + << "bgpd did not return to active after commit; state=" << bgpDaemonActiveState(); const std::string pidAfter = bgpDaemonMainPid(); - EXPECT_NE(pidAfter, "0") << "bgp_pp has no MainPID after commit"; - EXPECT_NE(pidAfter, pidBefore) << "bgp_pp MainPID unchanged (" << pidBefore + EXPECT_NE(pidAfter, "0") << "bgpd has no MainPID after commit"; + EXPECT_NE(pidAfter, pidBefore) << "bgpd MainPID unchanged (" << pidBefore << "); expected a restart on commit"; } -// Rolling back a committed BGP change restores the bgp_pp system config and -// restarts bgp_pp (not just the agent config). +// Rolling back a committed BGP change restores the bgpd system config and +// restarts bgpd (not just the agent config). TEST_F(ConfigBgpSessionTest, RollbackRestoresBgpConfig) { if (bgpDaemonActiveState() != "active") { - GTEST_SKIP() << "bgp_pp is not active on this DUT; skipping rollback " + GTEST_SKIP() << "bgpd is not active on this DUT; skipping rollback " "verification"; } discardSession(); @@ -117,24 +117,24 @@ TEST_F(ConfigBgpSessionTest, RollbackRestoresBgpConfig) { EXPECT_EQ(convergenceSeconds(), 222) << "system bgp config should reflect the latest commit"; - // Roll back to commit A -> system bgp config restored to 111, bgp_pp restart. + // Roll back to commit A -> system bgp config restored to 111, bgpd restart. resetBgpDaemonLimit(); // avoid systemd start-limit on the rollback's restart auto rb = runCli({"config", "rollback", shaA}); EXPECT_EQ(rb.exitCode, 0) << "stderr=" << rb.stderr; ASSERT_TRUE(waitForBgpDaemonActive()) - << "bgp_pp did not return active after rollback"; + << "bgpd did not return active after rollback"; EXPECT_EQ(convergenceSeconds(), 111) - << "rollback must restore the bgp_pp system config"; + << "rollback must restore the bgpd system config"; } // Committing a BGP config identical to what is already running must be a no-op: -// "Nothing to commit" and, crucially, NO bgp_pp restart (a restart is traffic -// affecting). saveBgpConfig() always records BGP_RESTART, so commit() has to -// compare the staged config against the running one. +// "Nothing to commit" and, crucially, NO bgpd restart (a restart is traffic +// affecting). saveBgpConfig() always records SERVICE_RESTART, so commit() has +// to compare the staged config against the running one. TEST_F(ConfigBgpSessionTest, CommitUnchangedBgpConfigDoesNotRestart) { if (bgpDaemonActiveState() != "active") { - GTEST_SKIP() << "bgp_pp is not active on this DUT; skipping no-op restart " + GTEST_SKIP() << "bgpd is not active on this DUT; skipping no-op restart " "verification"; } discardSession(); @@ -154,16 +154,16 @@ TEST_F(ConfigBgpSessionTest, CommitUnchangedBgpConfigDoesNotRestart) { << "an unchanged BGP commit should report nothing to commit"; EXPECT_EQ(bgpDaemonMainPid(), pidBefore) - << "bgp_pp was restarted for an unchanged BGP config (pidBefore=" + << "bgpd was restarted for an unchanged BGP config (pidBefore=" << pidBefore << ")"; } -// An agent-only commit (no BGP staged) must still snapshot the running bgp_pp +// An agent-only commit (no BGP staged) must still snapshot the running bgpd // config, so a rollback to it restores BGP instead of wiping it. Pre-fix, // bgpcpp.conf was untracked at such commits and rollback deleted it. TEST_F(ConfigBgpSessionTest, AgentCommitSnapshotsBgpAndRollbackPreservesIt) { if (bgpDaemonActiveState() != "active") { - GTEST_SKIP() << "bgp_pp is not active on this DUT; skipping cross-domain " + GTEST_SKIP() << "bgpd is not active on this DUT; skipping cross-domain " "rollback verification"; } discardSession(); @@ -171,7 +171,7 @@ TEST_F(ConfigBgpSessionTest, AgentCommitSnapshotsBgpAndRollbackPreservesIt) { // The running (default) BGP config that must survive a rollback unchanged. ASSERT_TRUE(fs::exists(systemBgpConfigPath())) - << "expected a running bgp_pp config on an active-BGP DUT"; + << "expected a running bgpd config on an active-BGP DUT"; const folly::dynamic defaultBgp = readSystemBgpConfig(); // Step 1: an agent-only commit (interface description). No BGP is staged. @@ -204,15 +204,15 @@ TEST_F(ConfigBgpSessionTest, AgentCommitSnapshotsBgpAndRollbackPreservesIt) { 222); // Step 3: roll back to the agent-only commit. BGP must be RESTORED to the - // original default snapshot (not deleted), and bgp_pp must come back. + // original default snapshot (not deleted), and bgpd must come back. resetBgpDaemonLimit(); auto rb = runCli({"config", "rollback", agentSha}); EXPECT_EQ(rb.exitCode, 0) << "stderr=" << rb.stderr; ASSERT_TRUE(waitForBgpDaemonActive()) - << "bgp_pp did not return active after rollback"; + << "bgpd did not return active after rollback"; EXPECT_TRUE(fs::exists(systemBgpConfigPath())) - << "rollback to an agent-era commit wiped the running bgp_pp config"; + << "rollback to an agent-era commit wiped the running bgpd config"; EXPECT_EQ(readSystemBgpConfig(), defaultBgp) - << "rollback did not restore the original (default) bgp_pp config"; + << "rollback did not restore the original (default) bgpd config"; } diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h index 1010fc3227855..00caadaeac3e7 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h @@ -8,7 +8,7 @@ * BGP config is not pushed into the agent's running config like the * SwitchConfig-backed commands. Instead the CLI stages a BGP++ * thrift-compatible JSON file at ~/.fboss2/bgp_config.json; `config session - * commit` promotes it to /etc/coop/bgpcpp/bgpcpp.conf and restarts the bgp_pp + * commit` promotes it to /etc/coop/bgpcpp/bgpcpp.conf and restarts the bgpd * daemon. These helpers let the global-attribute tests and the * session-lifecycle tests share the staging, commit, daemon-state, and * system-config plumbing. @@ -50,7 +50,7 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { return std::string(home) + "/.fboss2/bgp_config.json"; } - // System config consumed by the bgp_pp daemon (written on commit). + // System config consumed by the bgpd daemon (written on commit). std::string systemBgpConfigPath() const { return "/etc/coop/bgpcpp/bgpcpp.conf"; } @@ -86,9 +86,18 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { // Current git HEAD sha of the /etc/coop repo (trimmed; "" on failure). // runCmd uses folly::Subprocess, which does not search PATH, so git is - // invoked by full path. + // invoked by full path. -c safe.directory mirrors the CLI's Git class: + // /etc/coop is typically owned by another user (e.g. coop), which git + // otherwise rejects as dubious ownership. std::string gitHead() const { - auto r = runCmd({"/usr/bin/git", "-C", coopRepoDir(), "rev-parse", "HEAD"}); + auto r = runCmd( + {"/usr/bin/git", + "-c", + "safe.directory=" + coopRepoDir(), + "-C", + coopRepoDir(), + "rev-parse", + "HEAD"}); std::string s = r.stdout; while (!s.empty() && (s.back() == '\n' || s.back() == '\r' || s.back() == ' ')) { @@ -102,6 +111,8 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { bool bgpTrackedAtRevision(const std::string& sha) const { auto r = runCmd( {"/usr/bin/git", + "-c", + "safe.directory=" + coopRepoDir(), "-C", coopRepoDir(), "cat-file", @@ -126,12 +137,12 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { return readBgpSessionConfig(); } - // Reset bgp_pp's systemd start-limit burst counter. Tests commit several - // times in quick succession, and each commit restarts bgp_pp; without this, + // Reset bgpd's systemd start-limit burst counter. Tests commit several + // times in quick succession, and each commit restarts bgpd; without this, // systemd's default StartLimitBurst (5 starts / 10s) refuses a rapid restart // with "start limit hit" — a test artifact, not a product issue. void resetBgpDaemonLimit() const { - runCmd({"/usr/bin/systemctl", "reset-failed", "bgp_pp"}); + runCmd({"/usr/bin/systemctl", "reset-failed", "bgpd"}); } // Run `config session commit` and return the short SHA parsed from @@ -150,7 +161,7 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { return r.stdout.substr(pos, end - pos); } - // Stage a global change, commit it, wait for bgp_pp to come back, and return + // Stage a global change, commit it, wait for bgpd to come back, and return // the promoted system config (the authoritative post-commit location). // A no-op commit (staged value already in the system config) yields no SHA // and is fine: the caller's assertions on the returned config still verify @@ -161,16 +172,16 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { runBgpGlobal(attr, value); commitAndGetSha(); EXPECT_TRUE(waitForBgpDaemonActive()) - << "bgp_pp did not return active after commit; state=" + << "bgpd did not return active after commit; state=" << bgpDaemonActiveState(); return readSystemBgpConfig(); } - // Trailing-whitespace-trimmed stdout of a `systemctl` query for bgp_pp. + // Trailing-whitespace-trimmed stdout of a `systemctl` query for bgpd. std::string systemctlValue(const std::string& property) const { // Full path: folly::Subprocess (used by runCmd) does not search PATH. auto r = runCmd( - {"/usr/bin/systemctl", "show", "bgp_pp", "-p", property, "--value"}); + {"/usr/bin/systemctl", "show", "bgpd", "-p", property, "--value"}); std::string s = r.stdout; while (!s.empty() && (s.back() == '\n' || s.back() == '\r' || s.back() == ' ')) { @@ -181,7 +192,7 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { // "active" / "inactive" / "failed" / ... (trimmed). std::string bgpDaemonActiveState() const { - auto r = runCmd({"/usr/bin/systemctl", "is-active", "bgp_pp"}); + auto r = runCmd({"/usr/bin/systemctl", "is-active", "bgpd"}); std::string s = r.stdout; while (!s.empty() && (s.back() == '\n' || s.back() == '\r' || s.back() == ' ')) { @@ -190,12 +201,12 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { return s; } - // bgp_pp main PID per systemd ("0" if not running). + // bgpd main PID per systemd ("0" if not running). std::string bgpDaemonMainPid() const { return systemctlValue("MainPID"); } - // Poll until bgp_pp reports active or the timeout elapses. + // Poll until bgpd reports active or the timeout elapses. bool waitForBgpDaemonActive( std::chrono::seconds timeout = std::chrono::seconds(60)) const { auto deadline = std::chrono::steady_clock::now() + timeout; diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index 9d5377cb34730..265e9db61af00 100644 --- a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py @@ -620,14 +620,7 @@ def test_injection_in_router_id_neutralized(self) -> None: def test_injection_in_peer_addr_neutralized(self) -> None: """Command injection in peer_addr should be single-quoted.""" - config = { - "peers": [ - { - "peer_addr": "$(malicious)", - "remote_as_4_byte": 65000, - } - ] - } + config = {"peers": [{"peer_addr": "$(malicious)", "remote_as_4_byte": 65000}]} commands = json_to_cli(config, stub=False, binary="fboss2") joined = "\n".join(commands) self.assertIn("peer '$(malicious)'", joined) From 5172b4f38177aabd907ccbce47e2c4c6ab7378a7 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 17 Jul 2026 06:14:05 +0000 Subject: [PATCH 3/6] Verify committed global config via bgpd getRunningConfig RPC The commit-path global-attribute tests asserted on the promoted /etc/coop/bgpcpp/bgpcpp.conf, which only proves what the CLI wrote to disk. Route ConfigBgpTestBase::setAndCommit() through a new readRunningBgpConfigViaRpc() -- TBgpService::getRunningConfig against the local bgpd -- so every positive ConfigBgpGlobalTest asserts its attribute in the daemon's own view of its config, proving bgpd parsed and adopted the promoted file after the commit-triggered restart. The helper retries briefly on connection errors: systemd reports bgpd active as soon as the process starts, but its thrift server binds the port a few seconds later, so an RPC issued right after a restart races that window. Test Plan: - bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test - fboss2_integration_test on a DUT with bgpd active: ConfigBgpGlobalTest 7/7 pass (each attribute set+commit verified in bgpd's getRunningConfig view, plus the invalid-bool and negative-graceful-restart-time reject paths). --- fboss/cli/fboss2/test/integration_test/BUCK | 1 + .../integration_test/ConfigBgpGlobalTest.cpp | 9 ++-- .../test/integration_test/ConfigBgpTestBase.h | 51 +++++++++++++++++-- 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index db70f955be817..c65789e666b77 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -13,6 +13,7 @@ oncall("fboss_oss") # # Unlike the Python-based CLI tests, these C++ tests directly invoke the CLI # library code rather than spawning a subprocess. +# bazelify: extra_dep = //neteng/fboss/bgp/if:bgp_thrift cpp_binary( name = "fboss2_integration_test", srcs = [ diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp index d9449833e37cb..a4afe16a8f35c 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp @@ -5,9 +5,12 @@ * * Scope: the BGP *global* tunables only. Each positive test stages the change * AND commits it, then asserts the value landed at the correct thrift field - * path in the promoted system config (/etc/coop/bgpcpp/bgpcpp.conf) that the - * bgpd daemon consumes. Session-lifecycle behavior (clear / diff / rollback / - * commit-restart mechanics) lives in ConfigBgpSessionTest. + * path in bgpd's own running config — fetched over its getRunningConfig RPC + * (see ConfigBgpTestBase::setAndCommit) — which proves the daemon parsed and + * adopted the promoted /etc/coop/bgpcpp/bgpcpp.conf after the commit-triggered + * restart, not merely that the CLI wrote the file. Session-lifecycle behavior + * (clear / diff / rollback / commit-restart mechanics) lives in + * ConfigBgpSessionTest. * * - count-confeds-in-as-path-len * -> BgpConfig.count_confeds_in_as_path_len diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h index 00caadaeac3e7..2103373448c8a 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h @@ -28,6 +28,8 @@ #include #include "fboss/cli/fboss2/test/integration_test/Fboss2IntegrationTest.h" +#include "fboss/cli/fboss2/utils/CmdClientUtilsCommon.h" +#include "neteng/fboss/bgp/if/gen-cpp2/TBgpService.h" namespace facebook::fboss { @@ -78,6 +80,36 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { return folly::parseJson(content); } + // The running config as reported by the bgpd daemon itself, over its + // TBgpService::getRunningConfig thrift RPC. Unlike readSystemBgpConfig() + // (the promoted file on disk, which only proves what the CLI wrote), this + // proves the daemon parsed and adopted the config after the post-commit + // restart. Mirrors Fboss2IntegrationTest::getRunningConfig() for the agent. + // + // Retries on connection errors: systemd reports bgpd "active" as soon as + // the process starts, but the thrift server only binds its port a few + // seconds later — an RPC issued right after a commit-triggered restart + // would otherwise race that window and get ECONNREFUSED. + folly::dynamic readRunningBgpConfigViaRpc( + std::chrono::seconds timeout = std::chrono::seconds(30)) const { + auto deadline = std::chrono::steady_clock::now() + timeout; + while (true) { + try { + HostInfo hostInfo("localhost"); + auto client = utils::createClient>(hostInfo); + std::string configStr; + client->sync_getRunningConfig(configStr); + return folly::parseJson(configStr); + } catch (const std::exception&) { + if (std::chrono::steady_clock::now() >= deadline) { + throw; + } + std::this_thread::sleep_for(std::chrono::seconds(1)); + } + } + } + // Root of the git repo that versions both the agent config (cli/agent.conf) // and the BGP config (bgpcpp/bgpcpp.conf). std::string coopRepoDir() const { @@ -162,10 +194,19 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { } // Stage a global change, commit it, wait for bgpd to come back, and return - // the promoted system config (the authoritative post-commit location). - // A no-op commit (staged value already in the system config) yields no SHA - // and is fine: the caller's assertions on the returned config still verify - // the value; commit/restart mechanics are covered by ConfigBgpSessionTest. + // bgpd's own running config as reported over its getRunningConfig RPC. + // + // The returned config is the daemon's adopted view, which is strictly + // stronger than the promoted file on disk (readSystemBgpConfig): a value + // present here proves the commit promoted /etc/coop/bgpcpp/bgpcpp.conf AND + // that bgpd parsed and adopted it after the commit-triggered restart, not + // merely that the CLI wrote the file. The running config shares the same + // BgpConfig JSON schema as the promoted file, so callers assert the same + // field paths against it. + // + // A no-op commit (staged value already current) yields no SHA and is fine: + // the caller's assertions on the returned config still verify the value; + // commit/restart mechanics are covered by ConfigBgpSessionTest. folly::dynamic setAndCommit(const std::string& attr, const std::string& value) const { discardSession(); @@ -174,7 +215,7 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { EXPECT_TRUE(waitForBgpDaemonActive()) << "bgpd did not return active after commit; state=" << bgpDaemonActiveState(); - return readSystemBgpConfig(); + return readRunningBgpConfigViaRpc(); } // Trailing-whitespace-trimmed stdout of a `systemctl` query for bgpd. From 0b99f31e138a054baad5f2387eb90f0daed58398 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 11 Aug 2026 05:50:31 +0000 Subject: [PATCH 4/6] fboss2 BGP test base reads the daemon's config path Companion to "fboss2 config session seeds BGP config from the daemon's config path": ConfigBgpTestBase::systemBgpConfigPath() reads the stable /etc/coop/bgpcpp.conf daemon path instead of the promoted file, so the fixture no longer requires a prior commit to have happened and BGP tests can run first-thing on a freshly imaged box. Co-Authored-By: Claude Fable 5 --- .../cli/fboss2/test/integration_test/ConfigBgpTestBase.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h index 2103373448c8a..48d722a4ff6ab 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h @@ -52,9 +52,14 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { return std::string(home) + "/.fboss2/bgp_config.json"; } - // System config consumed by the bgpd daemon (written on commit). + // System config consumed by the bgpd daemon: the stable path bgpd is started + // with (--config). After the first commit this is a symlink to the promoted + // bgpcpp/bgpcpp.conf; before it, it is the plain file the bgp++ RPM installs. + // Reading it (rather than the promoted path directly) means the fixture does + // not require a commit to have happened, so these tests can run first on a + // freshly imaged box. std::string systemBgpConfigPath() const { - return "/etc/coop/bgpcpp/bgpcpp.conf"; + return "/etc/coop/bgpcpp.conf"; } // Remove any pre-existing staged BGP session so each test starts clean. From 4568df7e300a18938ac3c49533a1ce77e25c2915 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 11:29:55 +0000 Subject: [PATCH 5/6] Validate router-id and overload-protection-mode in config protocol bgp global `router-id` stored whatever token it was given. bgpd builds an IPAddress from it at config load (an unparseable string fails the load) and later calls asV4() on it, so a v6 address passes construction and throws afterwards. Parse with folly::IPAddress::tryFromString, require IPv4 and store the normalized form. `switch-limit-overload-protection-mode` cast any int32 straight into the enum; bgpd switches on the value without a default and logs anything but 2 as DROP_EXCESS_PREFIXES, so -5 or 999 silently behaved as mode 1. Accept the enum name or its integer, reject everything else listing the valid names, and validate before ensure() so a rejected value does not materialize switch_limit_config. The success message keeps printing the integer so the j2c round-trip is unchanged. Adds CmdConfigBgpGlobalTest.cpp (the global command had no unit fixture) and rejection/positive-commit cases to ConfigBgpGlobalTest.cpp. Co-Authored-By: Claude Fable 5.1 --- cmake/CliFboss2TestConfig.cmake | 1 + .../bgp/global/CmdConfigProtocolBgpGlobal.cpp | 52 +++++- fboss/cli/fboss2/test/config/BUCK | 1 + .../test/config/CmdConfigBgpGlobalTest.cpp | 173 ++++++++++++++++++ .../integration_test/ConfigBgpGlobalTest.cpp | 43 +++++ 5 files changed, 262 insertions(+), 8 deletions(-) create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpGlobalTest.cpp diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index 1814d5a9f86da..fadd35a1c0c50 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -10,6 +10,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigAclRuleTest.cpp fboss/cli/fboss2/test/config/CmdConfigAclTest.cpp fboss/cli/fboss2/test/config/CmdConfigArpTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpGlobalTest.cpp fboss/cli/fboss2/test/config/CmdConfigIcmpV4UnavailableSrcAddrTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp index b824d1aaae0e7..a1660ae69ea6c 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp @@ -13,6 +13,7 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include +#include #include #include #include @@ -30,6 +31,7 @@ #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fmt/format.h" +#include "thrift/lib/cpp/Thrift.h" namespace facebook::fboss { @@ -68,12 +70,24 @@ using bgpcli::Result; // Each handler mutates the typed bgp::thrift::BgpConfig directly. Field names // map 1:1 to bgp_config.thrift, so the compiler validates them. +// bgpd constructs an IPAddress from router_id at config load (a bad string +// fails the load) and later calls asV4() on it, so only an IPv4 address is +// safe to persist. Stored normalized. Result applyRouterId(BgpConfig& cfg, const Tokens& values) { if (values.size() != 1) { return err("Error: router-id requires "); } - cfg.router_id() = values[0]; - return ok(fmt::format("Successfully set BGP router-id to: {}", values[0])); + auto addr = folly::IPAddress::tryFromString(values[0]); + if (!addr.hasValue()) { + return err(fmt::format("Error: Invalid router-id address '{}'", values[0])); + } + if (!addr->isV4()) { + return err( + fmt::format( + "Error: router-id requires an IPv4 address, got '{}'", values[0])); + } + cfg.router_id() = addr->str(); + return ok(fmt::format("Successfully set BGP router-id to: {}", addr->str())); } Result applyLocalAsn(BgpConfig& cfg, const Tokens& values) { @@ -210,22 +224,44 @@ Result applyInt64Limit( "Successfully set switch_limit_config {} to: {}", fieldName, *limit)); } +// Accepts the enum value name or its integer; anything outside the enum is +// rejected (bgpd would otherwise treat an unknown value as +// DROP_EXCESS_PREFIXES without complaint). Validated before ensure() so a +// rejected value leaves switch_limit_config untouched. The success message +// prints the integer so the j2c round-trip is stable. Result applyOverloadProtectionMode(BgpConfig& cfg, const Tokens& values) { + using OverloadProtectionMode = bgp::thrift::OverloadProtectionMode; if (values.size() != 1) { return err("Error: switch-limit-overload-protection-mode requires "); } - auto mode = parseInt(values[0]); - if (!mode) { + OverloadProtectionMode mode; + bool valid = false; + if (auto parsed = parseInt(values[0])) { + mode = static_cast(*parsed); + valid = apache::thrift::TEnumTraits::findName( + mode) != nullptr; + } else { + valid = apache::thrift::TEnumTraits::findValue( + values[0], &mode); + } + if (!valid) { + std::string names; + for (auto n : apache::thrift::TEnumTraits::names) { + names += names.empty() ? "" : ", "; + names += std::string(n); + } return err( fmt::format( - "Error: Invalid overload-protection-mode value '{}'", values[0])); + "Error: switch-limit-overload-protection-mode value '{}' is not a " + "valid mode; expected one of: {}", + values[0], + names)); } - cfg.switch_limit_config().ensure().overload_protection_mode() = - static_cast(*mode); + cfg.switch_limit_config().ensure().overload_protection_mode() = mode; return ok( fmt::format( "Successfully set switch_limit_config overload_protection_mode to: {}", - *mode)); + static_cast(mode))); } Result applyNetwork6(BgpConfig& cfg, const Tokens& values) { diff --git a/fboss/cli/fboss2/test/config/BUCK b/fboss/cli/fboss2/test/config/BUCK index c34962717b58e..bfcd2410df9dd 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -12,6 +12,7 @@ cpp_unittest( "CmdConfigAdminDistanceTest.cpp", "CmdConfigAppliedInfoTest.cpp", "CmdConfigArpTest.cpp", + "CmdConfigBgpGlobalTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDhcpTest.cpp", "CmdConfigHistoryTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpGlobalTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpGlobalTest.cpp new file mode 100644 index 0000000000000..15f543608052b --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpGlobalTest.cpp @@ -0,0 +1,173 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include + +#include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +using namespace ::testing; + +namespace facebook::fboss { + +// The global dispatcher only touches the BGP side of ConfigSession, which +// seeds from thrift schema defaults when neither a staged session nor a +// system bgpcpp.conf exists — so no seed agent config is needed (mirrors +// CmdConfigBgpNeighborTest). +class CmdConfigBgpGlobalTestFixture : public CmdConfigTestBase { + public: + CmdConfigBgpGlobalTestFixture() + : CmdConfigTestBase("bgp_global_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string run(const std::vector& tokens) { + CmdConfigProtocolBgpGlobal cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpGlobalConfig(tokens)); + } + + bgp::thrift::BgpConfig& config() { + return ConfigSession::getInstance().getBgpConfig(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpGlobalConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpGlobalTestFixture, argValidation) { + auto parsed = BgpGlobalConfig({"router-id", "10.0.0.1"}); + EXPECT_EQ(parsed.attr(), "router-id"); + EXPECT_EQ(parsed.values(), std::vector({"10.0.0.1"})); + + // Invalid: empty, unknown attribute. + EXPECT_THROW(BgpGlobalConfig({}), std::invalid_argument); + EXPECT_THROW(BgpGlobalConfig({"no-such-attr", "1"}), std::invalid_argument); +} + +// ============================================================================== +// router-id +// ============================================================================== + +TEST_F(CmdConfigBgpGlobalTestFixture, routerIdAccepted) { + auto result = run({"router-id", "10.0.0.1"}); + EXPECT_THAT(result, HasSubstr("Successfully set BGP router-id to: 10.0.0.1")); + EXPECT_EQ(*config().router_id(), "10.0.0.1"); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpGlobalTestFixture, routerIdInvalidRejected) { + const auto before = *config().router_id(); + auto result = run({"router-id", "not-an-ip"}); + EXPECT_THAT(result, HasSubstr("Invalid router-id address")); + EXPECT_EQ(*config().router_id(), before); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpGlobalTestFixture, routerIdV6Rejected) { + const auto before = *config().router_id(); + auto result = run({"router-id", "2001:db8::1"}); + EXPECT_THAT(result, HasSubstr("requires an IPv4 address")); + EXPECT_EQ(*config().router_id(), before); + EXPECT_FALSE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpGlobalTestFixture, routerIdArityRejected) { + EXPECT_THAT(run({"router-id"}), HasSubstr("requires ")); + EXPECT_THAT( + run({"router-id", "1.1.1.1", "2.2.2.2"}), + HasSubstr("requires ")); + EXPECT_FALSE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpGlobalTestFixture, rejectedRouterIdKeepsStagedValue) { + run({"router-id", "10.0.0.1"}); + auto result = run({"router-id", "not-an-ip"}); + EXPECT_THAT(result, HasSubstr("Invalid router-id address")); + EXPECT_EQ(*config().router_id(), "10.0.0.1"); +} + +// ============================================================================== +// switch-limit-overload-protection-mode +// ============================================================================== + +TEST_F(CmdConfigBgpGlobalTestFixture, overloadProtectionModeIntegerAccepted) { + auto result = run({"switch-limit-overload-protection-mode", "1"}); + EXPECT_THAT(result, HasSubstr("overload_protection_mode to: 1")); + ASSERT_TRUE(config().switch_limit_config().has_value()); + EXPECT_EQ( + *config().switch_limit_config()->overload_protection_mode(), + bgp::thrift::OverloadProtectionMode::DROP_EXCESS_PREFIXES); + + result = run({"switch-limit-overload-protection-mode", "2"}); + EXPECT_THAT(result, HasSubstr("overload_protection_mode to: 2")); + EXPECT_EQ( + *config().switch_limit_config()->overload_protection_mode(), + bgp::thrift::OverloadProtectionMode::APPLY_GOLDEN_PREFIX_POLICY); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpGlobalTestFixture, overloadProtectionModeNameAccepted) { + auto result = + run({"switch-limit-overload-protection-mode", "DROP_EXCESS_PREFIXES"}); + // The success message prints the integer so the j2c round-trip is stable. + EXPECT_THAT(result, HasSubstr("overload_protection_mode to: 1")); + ASSERT_TRUE(config().switch_limit_config().has_value()); + EXPECT_EQ( + *config().switch_limit_config()->overload_protection_mode(), + bgp::thrift::OverloadProtectionMode::DROP_EXCESS_PREFIXES); + + result = run( + {"switch-limit-overload-protection-mode", "APPLY_GOLDEN_PREFIX_POLICY"}); + EXPECT_THAT(result, HasSubstr("overload_protection_mode to: 2")); + EXPECT_EQ( + *config().switch_limit_config()->overload_protection_mode(), + bgp::thrift::OverloadProtectionMode::APPLY_GOLDEN_PREFIX_POLICY); +} + +TEST_F(CmdConfigBgpGlobalTestFixture, overloadProtectionModeInvalidRejected) { + for (const auto& bad : {"0", "-5", "999", "fast"}) { + auto result = run({"switch-limit-overload-protection-mode", bad}); + EXPECT_THAT(result, HasSubstr("is not a valid mode")) << bad; + // The rejection lists every valid name. + EXPECT_THAT(result, HasSubstr("DROP_EXCESS_PREFIXES")) << bad; + EXPECT_THAT(result, HasSubstr("APPLY_GOLDEN_PREFIX_POLICY")) << bad; + } + // Validation happens before ensure(): a rejected value must not create the + // nested switch_limit_config. + EXPECT_FALSE(config().switch_limit_config().has_value()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F( + CmdConfigBgpGlobalTestFixture, + rejectedOverloadProtectionModeKeepsStagedValue) { + run({"switch-limit-overload-protection-mode", "2"}); + auto result = run({"switch-limit-overload-protection-mode", "999"}); + EXPECT_THAT(result, HasSubstr("is not a valid mode")); + ASSERT_TRUE(config().switch_limit_config().has_value()); + EXPECT_EQ( + *config().switch_limit_config()->overload_protection_mode(), + bgp::thrift::OverloadProtectionMode::APPLY_GOLDEN_PREFIX_POLICY); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp index a4afe16a8f35c..9c3a68d779672 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp @@ -18,6 +18,8 @@ * -> BgpConfig.graceful_restart_convergence_seconds * - rib-allocated-path-ids * -> BgpConfig.bgp_setting_config.enable_rib_allocated_path_id + * - switch-limit-overload-protection-mode + * -> BgpConfig.switch_limit_config.overload_protection_mode * * Requirements: * - The fboss2-dev binary under test (config subcommand tree). @@ -107,6 +109,47 @@ TEST_F(ConfigBgpGlobalTest, InvalidBoolValueRejected) { << "session file should not exist after rejected input"; } +TEST_F(ConfigBgpGlobalTest, RouterIdInvalidRejected) { + clearBgpSession(); + auto result = + runCli({"config", "protocol", "bgp", "global", "router-id", "not-an-ip"}); + EXPECT_THAT(result.stdout, HasSubstr("Invalid router-id address")); + EXPECT_FALSE(std::filesystem::exists(bgpSessionPath())) + << "session file should not exist after rejected input"; +} + +TEST_F(ConfigBgpGlobalTest, RouterIdV6Rejected) { + clearBgpSession(); + auto result = runCli( + {"config", "protocol", "bgp", "global", "router-id", "2001:db8::1"}); + EXPECT_THAT(result.stdout, HasSubstr("requires an IPv4 address")); + EXPECT_FALSE(std::filesystem::exists(bgpSessionPath())) + << "session file should not exist after rejected input"; +} + +TEST_F(ConfigBgpGlobalTest, OverloadProtectionModeOutOfRangeRejected) { + clearBgpSession(); + auto result = runCli( + {"config", + "protocol", + "bgp", + "global", + "switch-limit-overload-protection-mode", + "999"}); + EXPECT_THAT(result.stdout, HasSubstr("is not a valid mode")); + EXPECT_THAT(result.stdout, HasSubstr("APPLY_GOLDEN_PREFIX_POLICY")); + EXPECT_FALSE(std::filesystem::exists(bgpSessionPath())) + << "session file should not exist after rejected input"; +} + +TEST_F(ConfigBgpGlobalTest, SetOverloadProtectionModeAndCommit) { + auto config = setAndCommit("switch-limit-overload-protection-mode", "2"); + ASSERT_TRUE(config.count("switch_limit_config")); + ASSERT_TRUE(config["switch_limit_config"].count("overload_protection_mode")); + EXPECT_EQ( + config["switch_limit_config"]["overload_protection_mode"].asInt(), 2); +} + TEST_F(ConfigBgpGlobalTest, NegativeGracefulRestartTimeRejected) { clearBgpSession(); auto result = runCli( From 87cc114eed63870ea72b5e8802c79e4fd60680bd Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 16 Sep 2026 07:56:38 +0000 Subject: [PATCH 6/6] Drop the unused Thrift.h include from the BGP global command clang-tidy's include-cleaner rejects it (TEnumTraits comes in through the generated types header), which fails the PR build. Also default-initialize the parsed mode. Co-Authored-By: Claude Fable 5.1 --- .../config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp index a1660ae69ea6c..1f515b840d820 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp @@ -31,7 +31,6 @@ #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fmt/format.h" -#include "thrift/lib/cpp/Thrift.h" namespace facebook::fboss { @@ -234,7 +233,7 @@ Result applyOverloadProtectionMode(BgpConfig& cfg, const Tokens& values) { if (values.size() != 1) { return err("Error: switch-limit-overload-protection-mode requires "); } - OverloadProtectionMode mode; + OverloadProtectionMode mode{}; bool valid = false; if (auto parsed = parseInt(values[0])) { mode = static_cast(*parsed);