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.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: hillol-nexthop

cmake/CliFboss2.cmake

@@ -844,6 +844,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

fboss/cli/fboss2/BUCK

@@ -1197,6 +1197,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",

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/global/CmdConfigProtocolBgpGlobal.cpp

@@ -13,17 +13,23 @@
 #include "fboss/cli/fboss2/CmdHandler.cpp"
 
 #include <fmt/core.h>
-#include <folly/Conv.h>
 #include <neteng/fboss/bgp/public_tld/configerator/structs/neteng/fboss/bgp/gen-cpp2/bgp_config_types.h>
+#include <cstddef>
 #include <cstdint>
 #include <functional>
+#include <iostream>
 #include <limits>
 #include <map>
-#include <optional>
+#include <ostream>
+#include <stdexcept>
 #include <string>
 #include <string_view>
 #include <vector>
+#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<std::string>;
 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<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).
-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);
-}
+// 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,23 +80,31 @@ Result applyLocalAsn(BgpConfig& cfg, const Tokens& values) {
   if (values.size() != 1) {
     return err("Error: local-asn requires <asn>");
   }
-  auto asn = parseInt<uint64_t>(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<int64_t>(*asn);
+  cfg.local_as_4_byte() = *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 <asn>");
   }
-  auto asn = parseInt<uint64_t>(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<int64_t>(*asn);
+  cfg.local_confed_as_4_byte() = *asn;
   return ok(
       fmt::format("Successfully set BGP confederation AS number to: {}", *asn));
 }

fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h

@@ -10,9 +10,13 @@
 
 #pragma once
 
+#include <string>
+#include <vector>
+#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<std::string> {
 struct CmdConfigProtocolBgpGlobalTraits : public WriteCommandTraits {
   using ParentCmd = CmdConfigProtocolBgp;
   static void addCliArg(CLI::App& cmd, std::vector<std::string>& 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, "<attribute> <value> [value ...]");
   }
   using ObjectArgType = BgpGlobalConfig;

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 <true|false>
@@ -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 <gtest/gtest.h>