[Nexthop][fboss2-dev] Add fboss2-dev config protocol bgp global command

cmake/CliFboss2.cmake

@@ -867,32 +867,13 @@ 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
   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

cmake/CliFboss2TestIntegrationTest.cmake

@@ -11,6 +11,8 @@ add_executable(fboss2_integration_test
   fboss/cli/fboss2/test/integration_test/ConfigAdminDistanceTest.cpp
   fboss/cli/fboss2/test/integration_test/ConfigAclRuleTest.cpp
   fboss/cli/fboss2/test/integration_test/ConfigArpTest.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

fboss/cli/fboss2/BUCK

@@ -1098,16 +1098,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",
@@ -1249,14 +1239,10 @@ 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",
-        "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",

fboss/cli/fboss2/CmdListConfig.cpp

@@ -38,16 +38,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"
@@ -379,91 +369,14 @@ const CommandTree& kConfigCommandTree() {
                   {
                       {
                           "global",
-                          "Configure BGP global settings",
+                          "Configure BGP global settings: <attribute> <value> "
+                          "(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<CmdConfigProtocolBgpGlobal>,
                           argRegistrar<CmdConfigProtocolBgpGlobalTraits>,
-                          {
-                              {
-                                  "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",

fboss/cli/fboss2/cli_metadata.thrift

@@ -17,9 +17,11 @@ namespace cpp2 facebook.fboss.cli
 // changes.
 enum ConfigActionLevel {
   HITLESS = 0, // Can be applied with reloadConfig() - default
-  AGENT_WARMBOOT = 1, // Requires agent warmboot restart
+  // Requires a service restart that preserves state where possible. For the
+  // agent this is a warmboot (forwarding state retained); for bgpd (BGP++),
+  // which has no hitless reload, it is a plain service restart.
+  AGENT_WARMBOOT = 1,
   AGENT_COLDBOOT = 2, // Requires agent coldboot restart (clears ASIC state)
-  BGP_RESTART = 3, // Requires a restart of the bgpd (BGP++) service
 }
 
 // Identifier for different services that can be configured

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 <folly/Conv.h>
+#include <cstdint>
+#include <exception>
+#include <limits>
+#include <optional>
+#include <string>
+#include <utility>
+
+/**
+ * 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<bool> 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 <typename T>
+std::optional<T> parseInt(const std::string& value) {
+  try {
+    return folly::to<T>(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<int32_t> parseNonNegInt32(const std::string& value) {
+  auto parsed = parseInt<int64_t>(value);
+  if (!parsed || *parsed < 0 || *parsed > std::numeric_limits<int32_t>::max()) {
+    return std::nullopt;
+  }
+  return static_cast<int32_t>(*parsed);
+}
+
+// Parse a non-negative int64 (used for route limits).
+inline std::optional<int64_t> parseNonNegInt64(const std::string& value) {
+  auto parsed = parseInt<int64_t>(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<int64_t> parseAsn4Byte(const std::string& value) {
+  auto parsed = parseInt<uint64_t>(value);
+  if (!parsed || *parsed > std::numeric_limits<uint32_t>::max()) {
+    return std::nullopt;
+  }
+  return static_cast<int64_t>(*parsed);
+}
+
+} // namespace facebook::fboss::bgpcli

fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.cpp

@@ -208,20 +208,6 @@ std::optional<uint64_t> BgpConfigSession::getConfedAsn() const {
   return std::nullopt;
 }
 
-void BgpConfigSession::setClusterId(const std::string& clusterId) {
-  ensureConfigLoaded();
-  bgpConfig_["cluster_id"] = clusterId;
-}
-
-std::optional<std::string> BgpConfigSession::getClusterId() const {
-  const_cast<BgpConfigSession*>(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;

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<BgpPeer> - List of BGP peers
  *   - peer_groups: list<PeerGroup> - List of peer groups
  *
@@ -103,10 +102,6 @@ class BgpConfigSession {
   void setConfedAsn(uint64_t asn);
   std::optional<uint64_t> getConfedAsn() const;
 
-  // cluster_id: string - Route reflector cluster ID
-  void setClusterId(const std::string& clusterId);
-  std::optional<std::string> getClusterId() const;
-
   // listen_addr: string - Listen address for BGP sessions
   void setListenAddress(const std::string& listenAddr);
   std::optional<std::string> getListenAddress() const;