From 742e465de295d084056aae3481bae9e9a2ed4c72 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 7 Aug 2026 18:27:03 +0000 Subject: [PATCH 1/9] [Nexthop] fboss2 config/delete protocol bgp peer-group commands Collapses the 18 per-attribute `config protocol bgp peer-group` command classes (built on the deprecated `folly::dynamic` BgpConfigSession) into a single typed dispatcher, mirroring CmdConfigProtocolBgpNeighbor. Also adds `delete protocol bgp peer-group `. The group name is the first positional token; the next one or two tokens name the attribute, matched longest-prefix-first so `timers hold-time` wins over any `timers` prefix; the rest are its value(s). Handlers mutate the typed bgp::thrift::PeerGroup through ConfigSession::getBgpConfig() / saveBgpConfig(), so adding a tunable is a one-entry change in the dispatch table rather than a new command class. 36 command files are deleted. - Covers the 29 dispatch keys that map to a PeerGroup thrift field: remote-asn / local-asn (4-byte-bounded), description, peer-tag, ingress-policy / egress-policy, rr-client, confed-peer, redistribute-peer, enhanced-route-refresh, connect-mode, add-path send|receive, afi disable-ipv4-afi|disable-ipv6-afi|ipv4-over-ipv6-nh, graceful-restart restart-time|stateful-ha, max-route pre-filter|post-filter (plus the warning-threshold / warning-only knobs), timers hold-time|keepalive|out-delay|withdraw-unprog-delay, and next-hop-self. - Rejected rather than persisted as dead config (precedent: cluster-id in the global command, connect-mode BOTH in the neighbor command): connect-mode BOTH, since thrift only models is_passive. add-path send|receive merges into the AddPath enum bitmask, and clearing the last direction unsets the field. - Attributes with no per-peer-group thrift field (afi ipv4-labeled-unicast, afi ipv6-labeled-unicast, peer-port) are absent from the dispatch table, so they are refused at parse time as unknown attributes; a rejected value never lands on disk. - positionals_at_end() stops parent-chain subcommand fallthrough from reclassifying an attribute token that matches a sibling command name once the group name has been consumed (same fix as the neighbor command). - unit tests (16): CmdConfigBgpPeerGroupTest (12) covering arg validation, longest-prefix match, the add-path merge matrix, connect-mode, the bool/string/route-limit attributes, value validation and unknown-attribute rejection, plus CmdDeleteBgpPeerGroupTest (4). - integration tests (3): ConfigBgpPeerGroupTest asserts the committed peer_groups through bgpd's getRunningConfig RPC, proving the daemon parsed and adopted the promoted config rather than only checking the file the CLI wrote. --- cmake/CliFboss2.cmake | 38 +- cmake/CliFboss2TestConfig.cmake | 2 + cmake/CliFboss2TestIntegrationTest.cmake | 1 + fboss/cli/fboss2/BUCK | 34 +- fboss/cli/fboss2/CmdListConfig.cpp | 198 +------ .../CmdConfigProtocolBgpPeerGroup.cpp | 482 +++++++++++++++++- .../CmdConfigProtocolBgpPeerGroup.h | 44 +- ...mdConfigProtocolBgpPeerGroupConfedPeer.cpp | 61 --- .../CmdConfigProtocolBgpPeerGroupConfedPeer.h | 46 -- ...dConfigProtocolBgpPeerGroupDescription.cpp | 57 --- ...CmdConfigProtocolBgpPeerGroupDescription.h | 46 -- ...nfigProtocolBgpPeerGroupDisableIpv4Afi.cpp | 62 --- ...ConfigProtocolBgpPeerGroupDisableIpv4Afi.h | 46 -- ...ConfigProtocolBgpPeerGroupEgressPolicy.cpp | 61 --- ...mdConfigProtocolBgpPeerGroupEgressPolicy.h | 46 -- ...onfigProtocolBgpPeerGroupIngressPolicy.cpp | 61 --- ...dConfigProtocolBgpPeerGroupIngressPolicy.h | 46 -- ...CmdConfigProtocolBgpPeerGroupMaxRoutes.cpp | 71 --- .../CmdConfigProtocolBgpPeerGroupMaxRoutes.h | 46 -- ...dConfigProtocolBgpPeerGroupNextHopSelf.cpp | 63 --- ...CmdConfigProtocolBgpPeerGroupNextHopSelf.h | 46 -- .../CmdConfigProtocolBgpPeerGroupPeerTag.cpp | 58 --- .../CmdConfigProtocolBgpPeerGroupPeerTag.h | 45 -- ...CmdConfigProtocolBgpPeerGroupRemoteAsn.cpp | 64 --- .../CmdConfigProtocolBgpPeerGroupRemoteAsn.h | 46 -- .../CmdConfigProtocolBgpPeerGroupRrClient.cpp | 62 --- .../CmdConfigProtocolBgpPeerGroupRrClient.h | 45 -- .../CmdConfigProtocolBgpPeerGroupTimers.cpp | 44 -- .../CmdConfigProtocolBgpPeerGroupTimers.h | 41 -- ...nfigProtocolBgpPeerGroupTimersHoldTime.cpp | 65 --- ...ConfigProtocolBgpPeerGroupTimersHoldTime.h | 46 -- ...figProtocolBgpPeerGroupTimersKeepalive.cpp | 71 --- ...onfigProtocolBgpPeerGroupTimersKeepalive.h | 46 -- ...nfigProtocolBgpPeerGroupTimersOutDelay.cpp | 71 --- ...ConfigProtocolBgpPeerGroupTimersOutDelay.h | 46 -- ...lBgpPeerGroupTimersWithdrawUnprogDelay.cpp | 70 --- ...colBgpPeerGroupTimersWithdrawUnprogDelay.h | 48 -- ...mdConfigProtocolBgpPeerGroupV4OverV6Nh.cpp | 61 --- .../CmdConfigProtocolBgpPeerGroupV4OverV6Nh.h | 46 -- ...ConfigProtocolBgpPeerGroupWarningLimit.cpp | 70 --- ...mdConfigProtocolBgpPeerGroupWarningLimit.h | 46 -- ...dConfigProtocolBgpPeerGroupWarningOnly.cpp | 72 --- ...CmdConfigProtocolBgpPeerGroupWarningOnly.h | 46 -- .../CmdDeleteProtocolBgpPeerGroup.cpp | 81 +++ .../CmdDeleteProtocolBgpPeerGroup.h | 61 +++ fboss/cli/fboss2/test/config/BUCK | 2 + .../test/config/CmdConfigBgpPeerGroupTest.cpp | 247 +++++++++ .../test/config/CmdDeleteBgpPeerGroupTest.cpp | 106 ++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + .../ConfigBgpPeerGroupTest.cpp | 131 +++++ 50 files changed, 1168 insertions(+), 2227 deletions(-) delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.h delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.cpp delete mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.h create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp create mode 100644 fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 8eb1d1772a09f..23c59a1341f23 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -968,42 +968,6 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/neighbor/CmdConfigProtocolBgpNeighbor.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 - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.h - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.cpp - fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.h fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.cpp fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp @@ -1138,6 +1102,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/delete/protocol/bgp/CmdDeleteProtocolBgp.h fboss/cli/fboss2/commands/delete/protocol/bgp/neighbor/CmdDeleteProtocolBgpNeighbor.cpp fboss/cli/fboss2/commands/delete/protocol/bgp/neighbor/CmdDeleteProtocolBgpNeighbor.h + fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp + fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h fboss/cli/fboss2/commands/delete/protocol/CmdDeleteProtocol.h fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index 9e48bc4410406..34829a21d3f3e 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -13,6 +13,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigBgpGlobalTest.cpp fboss/cli/fboss2/test/config/CmdConfigIcmpV4UnavailableSrcAddrTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpNeighborTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp @@ -48,6 +49,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdDeleteAclTableTest.cpp fboss/cli/fboss2/test/config/CmdDeleteArpTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpNeighborTest.cpp + fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp fboss/cli/fboss2/test/config/CmdDeleteCoppQueueTest.cpp fboss/cli/fboss2/test/config/CmdDeleteCoppReasonTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index 4fc177faea46a..7c5b4fc6560d5 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -17,6 +17,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigSflowSampleDestTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpGlobalTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpNeighborTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.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 diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 2b5a7e13d8c20..677a736a47a23 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1177,24 +1177,6 @@ cpp_library( "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.cpp", "commands/config/protocol/bgp/neighbor/CmdConfigProtocolBgpNeighbor.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", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.cpp", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1261,6 +1243,7 @@ cpp_library( "commands/delete/protocol/CmdDeleteProtocol.cpp", "commands/delete/protocol/bgp/CmdDeleteProtocolBgp.cpp", "commands/delete/protocol/bgp/neighbor/CmdDeleteProtocolBgpNeighbor.cpp", + "commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp", "commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp", "commands/delete/qos/CmdDeleteQos.cpp", @@ -1334,20 +1317,6 @@ cpp_library( "commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h", "commands/config/protocol/bgp/neighbor/CmdConfigProtocolBgpNeighbor.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", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.h", - "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", @@ -1415,6 +1384,7 @@ cpp_library( "commands/delete/protocol/CmdDeleteProtocol.h", "commands/delete/protocol/bgp/CmdDeleteProtocolBgp.h", "commands/delete/protocol/bgp/neighbor/CmdDeleteProtocolBgpNeighbor.h", + "commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h", "commands/delete/protocol/static/CmdDeleteProtocolStatic.h", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h", "commands/delete/qos/CmdDeleteQos.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 6f69cfbb59512..72e57947a5d4d 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -45,24 +45,6 @@ #include "fboss/cli/fboss2/commands/config/protocol/bgp/global/CmdConfigProtocolBgpGlobal.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/neighbor/CmdConfigProtocolBgpNeighbor.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" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.h" #include "fboss/cli/fboss2/commands/config/protocol/static/CmdConfigProtocolStatic.h" #include "fboss/cli/fboss2/commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h" #include "fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h" @@ -126,6 +108,7 @@ #include "fboss/cli/fboss2/commands/delete/protocol/CmdDeleteProtocol.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/CmdDeleteProtocolBgp.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/neighbor/CmdDeleteProtocolBgpNeighbor.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h" #include "fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h" #include "fboss/cli/fboss2/commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h" #include "fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h" @@ -424,157 +407,22 @@ const CommandTree& kConfigCommandTree() { }, { "peer-group", - "Configure BGP peer group", + "Configure BGP peer-group: " + "[ ...] (remote-asn, local-asn, " + "description, peer-tag, ingress-policy, " + "egress-policy, rr-client, confed-peer, " + "redistribute-peer, enhanced-route-refresh, " + "connect-mode, next-hop-self, add-path send|receive, " + "afi disable-ipv4-afi|disable-ipv6-afi|" + "ipv4-over-ipv6-nh, " + "graceful-restart restart-time|stateful-ha, " + "max-route pre-filter|post-filter|" + "pre-warning-threshold|post-warning-threshold|" + "pre-warning-only|post-warning-only, " + "timers hold-time|keepalive|out-delay|" + "withdraw-unprog-delay)", commandHandler, argRegistrar, - { - { - "description", - "Set peer group description", - commandHandler< - CmdConfigProtocolBgpPeerGroupDescription>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupDescriptionTraits>, - }, - { - "remote-asn", - "Set remote AS number", - commandHandler< - CmdConfigProtocolBgpPeerGroupRemoteAsn>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupRemoteAsnTraits>, - }, - { - "next-hop-self", - "Enable/disable next-hop-self", - commandHandler< - CmdConfigProtocolBgpPeerGroupNextHopSelf>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupNextHopSelfTraits>, - }, - { - "confed-peer", - "Enable/disable confederation peer", - commandHandler< - CmdConfigProtocolBgpPeerGroupConfedPeer>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupConfedPeerTraits>, - }, - { - "disable-ipv4-afi", - "Enable/disable IPv4 AFI", - commandHandler< - CmdConfigProtocolBgpPeerGroupDisableIpv4Afi>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupDisableIpv4AfiTraits>, - }, - { - "v4-over-v6-nh", - "Enable/disable v4-over-v6 next-hop", - commandHandler< - CmdConfigProtocolBgpPeerGroupV4OverV6Nh>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupV4OverV6NhTraits>, - }, - { - "rr-client", - "Enable/disable route reflector client", - commandHandler< - CmdConfigProtocolBgpPeerGroupRrClient>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupRrClientTraits>, - }, - { - "ingress-policy", - "Set ingress policy", - commandHandler< - CmdConfigProtocolBgpPeerGroupIngressPolicy>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupIngressPolicyTraits>, - }, - { - "egress-policy", - "Set egress policy", - commandHandler< - CmdConfigProtocolBgpPeerGroupEgressPolicy>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupEgressPolicyTraits>, - }, - { - "max-routes", - "Set maximum routes", - commandHandler< - CmdConfigProtocolBgpPeerGroupMaxRoutes>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupMaxRoutesTraits>, - }, - { - "peer-tag", - "Set peer tag", - commandHandler< - CmdConfigProtocolBgpPeerGroupPeerTag>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupPeerTagTraits>, - }, - { - "warning-only", - "Enable/disable warning-only mode", - commandHandler< - CmdConfigProtocolBgpPeerGroupWarningOnly>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupWarningOnlyTraits>, - }, - { - "warning-limit", - "Set warning limit percentage", - commandHandler< - CmdConfigProtocolBgpPeerGroupWarningLimit>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupWarningLimitTraits>, - }, - { - "timers", - "Configure BGP timers", - commandHandler< - CmdConfigProtocolBgpPeerGroupTimers>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupTimersTraits>, - { - { - "hold-time", - "Set hold time", - commandHandler< - CmdConfigProtocolBgpPeerGroupTimersHoldTime>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupTimersHoldTimeTraits>, - }, - { - "keepalive", - "Set keepalive interval", - commandHandler< - CmdConfigProtocolBgpPeerGroupTimersKeepalive>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupTimersKeepaliveTraits>, - }, - { - "out-delay", - "Set output delay", - commandHandler< - CmdConfigProtocolBgpPeerGroupTimersOutDelay>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupTimersOutDelayTraits>, - }, - { - "withdraw-unprog-delay", - "Set withdraw unprogrammed delay", - commandHandler< - CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay>, - argRegistrar< - CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelayTraits>, - }, - }, - }, - }, }, { "neighbor", @@ -915,11 +763,17 @@ const CommandTree& kConfigCommandTree() { commandHandler, argTypeHandler, {{ - "neighbor", - "Delete a BGP neighbor: ", - commandHandler, - argRegistrar, - }}, + "neighbor", + "Delete a BGP neighbor: ", + commandHandler, + argRegistrar, + }, + { + "peer-group", + "Delete a BGP peer-group: ", + commandHandler, + argRegistrar, + }}, }, { "static", diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp index 9fdb301293a89..a9eede5cdd694 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp @@ -13,30 +13,480 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "configerator/structs/neteng/fboss/bgp/if/gen-cpp2/bgp_attr_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.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 { +namespace { + +// CLI attribute names, exactly as documented (two-token attributes keep their +// space). 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 kRemoteAsn = "remote-asn"; +constexpr std::string_view kLocalAsn = "local-asn"; +constexpr std::string_view kDescription = "description"; +constexpr std::string_view kPeerTag = "peer-tag"; +constexpr std::string_view kIngressPolicy = "ingress-policy"; +constexpr std::string_view kEgressPolicy = "egress-policy"; +constexpr std::string_view kRrClient = "rr-client"; +constexpr std::string_view kRedistributePeer = "redistribute-peer"; +constexpr std::string_view kEnhancedRouteRefresh = "enhanced-route-refresh"; +constexpr std::string_view kConnectMode = "connect-mode"; +constexpr std::string_view kAddPathSend = "add-path send"; +constexpr std::string_view kAddPathReceive = "add-path receive"; +constexpr std::string_view kAfiDisableIpv4Afi = "afi disable-ipv4-afi"; +constexpr std::string_view kAfiDisableIpv6Afi = "afi disable-ipv6-afi"; +constexpr std::string_view kAfiIpv4OverIpv6Nh = "afi ipv4-over-ipv6-nh"; +constexpr std::string_view kGracefulRestartTime = + "graceful-restart restart-time"; +constexpr std::string_view kGracefulRestartStatefulHa = + "graceful-restart stateful-ha"; +constexpr std::string_view kMaxRoutePreFilter = "max-route pre-filter"; +constexpr std::string_view kMaxRoutePostFilter = "max-route post-filter"; +constexpr std::string_view kMaxRoutePostWarningThreshold = + "max-route post-warning-threshold"; +constexpr std::string_view kTimersHoldTime = "timers hold-time"; +constexpr std::string_view kTimersKeepalive = "timers keepalive"; +constexpr std::string_view kTimersOutDelay = "timers out-delay"; +// Parity attributes: not in the documented peer-group grammar, but supported +// by the per-attribute peer-group commands this dispatcher replaces (and by +// real PeerGroup thrift fields), so dropping them would regress existing +// usage. +constexpr std::string_view kConfedPeer = "confed-peer"; +constexpr std::string_view kNextHopSelf = "next-hop-self"; +constexpr std::string_view kTimersWithdrawUnprogDelay = + "timers withdraw-unprog-delay"; +constexpr std::string_view kMaxRoutePreWarningThreshold = + "max-route pre-warning-threshold"; +constexpr std::string_view kMaxRoutePreWarningOnly = + "max-route pre-warning-only"; +constexpr std::string_view kMaxRoutePostWarningOnly = + "max-route post-warning-only"; + +// connect-mode values +constexpr std::string_view kConnectModePassive = "PASSIVE"; +constexpr std::string_view kConnectModeActive = "ACTIVE"; +constexpr std::string_view kConnectModeBoth = "BOTH"; + +using PeerGroup = bgp::thrift::PeerGroup; +using bgpcli::asnAttr; +using bgpcli::AttrHandler; +using bgpcli::boolAttr; +using bgpcli::err; +using bgpcli::joinedStringAttr; +using bgpcli::ok; +using bgpcli::parseBool; +using bgpcli::Result; +using bgpcli::routeCountAttr; +using bgpcli::secondsAttr; +using bgpcli::stringAttr; +using bgpcli::Tokens; +using facebook::neteng::fboss::bgp_attr::AddPath; + +// ---- per-attribute setters ------------------------------------------------- +// Pure thrift assignment: no validation, no messages. Parsing, bounds and the +// user-facing text all live in the shared factories in BgpCliAttrHandlers.h, +// so a setter is just the field this attribute writes. Field names map 1:1 to +// bgp_config.thrift, so the compiler validates them. + +void setRemoteAsn(PeerGroup& g, int64_t v) { + g.remote_as_4_byte() = v; +} + +void setLocalAsn(PeerGroup& g, int64_t v) { + g.local_as_4_byte() = v; +} + +void setDescription(PeerGroup& g, const std::string& v) { + g.description() = v; +} + +void setPeerTag(PeerGroup& g, const std::string& v) { + g.peer_tag() = v; +} + +void setIngressPolicy(PeerGroup& g, const std::string& v) { + g.ingress_policy_name() = v; +} + +void setEgressPolicy(PeerGroup& g, const std::string& v) { + g.egress_policy_name() = v; +} + +void setRrClient(PeerGroup& g, bool v) { + g.is_rr_client() = v; +} + +void setConfedPeer(PeerGroup& g, bool v) { + g.is_confed_peer() = v; +} + +void setRedistributePeer(PeerGroup& g, bool v) { + g.is_redistribute_peer() = v; +} + +void setEnhancedRouteRefresh(PeerGroup& g, bool v) { + g.enhanced_route_refresh() = v; +} + +void setDisableIpv4Afi(PeerGroup& g, bool v) { + g.disable_ipv4_afi() = v; +} + +void setDisableIpv6Afi(PeerGroup& g, bool v) { + g.disable_ipv6_afi() = v; +} + +void setIpv4OverIpv6Nh(PeerGroup& g, bool v) { + g.v4_over_v6_nexthop() = v; +} + +void setGracefulRestartTime(PeerGroup& g, int32_t v) { + g.bgp_peer_timers().ensure().graceful_restart_seconds() = v; +} + +void setGracefulRestartStatefulHa(PeerGroup& g, bool v) { + g.enable_stateful_ha() = v; +} + +void setMaxRoutePreFilter(PeerGroup& g, int64_t v) { + g.pre_filter().ensure().max_routes() = v; +} + +void setMaxRoutePostFilter(PeerGroup& g, int64_t v) { + g.post_filter().ensure().max_routes() = v; +} + +// RouteLimit.warning_limit is an absolute route count in the thrift schema, +// not a percentage. +void setMaxRoutePreWarningThreshold(PeerGroup& g, int64_t v) { + g.pre_filter().ensure().warning_limit() = v; +} + +void setMaxRoutePostWarningThreshold(PeerGroup& g, int64_t v) { + g.post_filter().ensure().warning_limit() = v; +} + +void setMaxRoutePreWarningOnly(PeerGroup& g, bool v) { + g.pre_filter().ensure().warning_only() = v; +} + +void setMaxRoutePostWarningOnly(PeerGroup& g, bool v) { + g.post_filter().ensure().warning_only() = v; +} + +void setTimersHoldTime(PeerGroup& g, int32_t v) { + g.bgp_peer_timers().ensure().hold_time_seconds() = v; +} + +void setTimersKeepalive(PeerGroup& g, int32_t v) { + g.bgp_peer_timers().ensure().keep_alive_seconds() = v; +} + +void setTimersOutDelay(PeerGroup& g, int32_t v) { + g.bgp_peer_timers().ensure().out_delay_seconds() = v; +} + +void setTimersWithdrawUnprogDelay(PeerGroup& g, int32_t v) { + g.bgp_peer_timers().ensure().withdraw_unprog_delay_seconds() = v; +} + +void setNextHopSelf(PeerGroup& g, bool v) { + g.next_hop_self() = v; +} + +// ---- hand-written handlers ------------------------------------------------- +// Only for value shapes no factory covers, because the shape is unique to this +// attribute rather than reusable. Both mirror the neighbor dispatcher's +// versions, which write the identically-named BgpPeer fields. + +// connect-mode: the thrift model only has is_passive (listen vs actively +// connect), so this is not an enum mapping — BOTH has no representation and is +// rejected with an explanation rather than silently mapped. +Result connectMode(PeerGroup& group, const Tokens& values) { + if (values.size() != 1) { + return err( + fmt::format( + "Error: {} requires <{}|{}>", + kConnectMode, + kConnectModePassive, + kConnectModeActive)); + } + if (values[0] == kConnectModePassive) { + group.is_passive() = true; + } else if (values[0] == kConnectModeActive) { + group.is_passive() = false; + } else if (values[0] == kConnectModeBoth) { + return err( + fmt::format( + "Error: {} {} has no representation in bgp_config.thrift " + "(PeerGroup only models is_passive); use {} or {}", + kConnectMode, + kConnectModeBoth, + kConnectModePassive, + kConnectModeActive)); + } else { + return err( + fmt::format( + "Error: Invalid {} value '{}'; expected {} or {}", + kConnectMode, + values[0], + kConnectModePassive, + kConnectModeActive)); + } + return ok(fmt::format("Successfully set {} to: {}", kConnectMode, values[0])); +} + +// add-path send/receive: a boolean at the CLI, but the two attributes share +// one thrift field — add_path is a single enum whose values form a bitmask by +// design (RECEIVE=1, SEND=2, BOTH=3). Each takes the boolAttr text but merges +// its direction into the current value instead of overwriting it; clearing the +// last direction unsets the field. +Result applyAddPath(PeerGroup& group, bool send, const Tokens& values) { + const std::string_view name = send ? kAddPathSend : kAddPathReceive; + if (values.size() != 1) { + return err(fmt::format("Error: {} requires ", name)); + } + auto enable = parseBool(values[0]); + if (!enable) { + return err( + fmt::format( + "Error: Invalid {} value '{}'; expected true or false", + name, + values[0])); + } + int bits = group.add_path() ? static_cast(*group.add_path()) : 0; + const int bit = send ? static_cast(AddPath::SEND) + : static_cast(AddPath::RECEIVE); + bits = *enable ? (bits | bit) : (bits & ~bit); + if (bits == 0) { + group.add_path().reset(); + } else { + group.add_path() = static_cast(bits); + } + return ok( + fmt::format( + "Successfully {} {}", *enable ? "enabled" : "disabled", name)); +} + +Result addPathSend(PeerGroup& group, const Tokens& values) { + return applyAddPath(group, /* send */ true, values); +} + +Result addPathReceive(PeerGroup& group, const Tokens& values) { + return applyAddPath(group, /* send */ false, values); +} + +// ---- the dispatch table ---------------------------------------------------- +// One line per attribute: dispatch key, value shape, setter. There are no +// handler bodies here by design — if an attribute appears to need one, its +// value shape is missing a factory and the fix is to add the factory. + +const std::map, std::less<>>& +attrHandlers() { + static const std::map, std::less<>> + kHandlers = { + {std::string(kRemoteAsn), + asnAttr(kRemoteAsn, setRemoteAsn)}, + {std::string(kLocalAsn), asnAttr(kLocalAsn, setLocalAsn)}, + {std::string(kDescription), + joinedStringAttr(kDescription, setDescription)}, + {std::string(kPeerTag), + stringAttr(kPeerTag, "string", setPeerTag)}, + {std::string(kIngressPolicy), + stringAttr( + kIngressPolicy, "policy-name", setIngressPolicy)}, + {std::string(kEgressPolicy), + stringAttr( + kEgressPolicy, "policy-name", setEgressPolicy)}, + {std::string(kRrClient), boolAttr(kRrClient, setRrClient)}, + {std::string(kConfedPeer), + boolAttr(kConfedPeer, setConfedPeer)}, + {std::string(kRedistributePeer), + boolAttr(kRedistributePeer, setRedistributePeer)}, + {std::string(kEnhancedRouteRefresh), + boolAttr(kEnhancedRouteRefresh, setEnhancedRouteRefresh)}, + {std::string(kConnectMode), connectMode}, + {std::string(kAddPathSend), addPathSend}, + {std::string(kAddPathReceive), addPathReceive}, + {std::string(kAfiDisableIpv4Afi), + boolAttr(kAfiDisableIpv4Afi, setDisableIpv4Afi)}, + {std::string(kAfiDisableIpv6Afi), + boolAttr(kAfiDisableIpv6Afi, setDisableIpv6Afi)}, + {std::string(kAfiIpv4OverIpv6Nh), + boolAttr(kAfiIpv4OverIpv6Nh, setIpv4OverIpv6Nh)}, + {std::string(kGracefulRestartTime), + secondsAttr( + kGracefulRestartTime, setGracefulRestartTime)}, + {std::string(kGracefulRestartStatefulHa), + boolAttr( + kGracefulRestartStatefulHa, setGracefulRestartStatefulHa)}, + {std::string(kMaxRoutePreFilter), + routeCountAttr(kMaxRoutePreFilter, setMaxRoutePreFilter)}, + {std::string(kMaxRoutePostFilter), + routeCountAttr( + kMaxRoutePostFilter, setMaxRoutePostFilter)}, + {std::string(kMaxRoutePreWarningThreshold), + routeCountAttr( + kMaxRoutePreWarningThreshold, setMaxRoutePreWarningThreshold)}, + {std::string(kMaxRoutePostWarningThreshold), + routeCountAttr( + kMaxRoutePostWarningThreshold, setMaxRoutePostWarningThreshold)}, + {std::string(kMaxRoutePreWarningOnly), + boolAttr( + kMaxRoutePreWarningOnly, setMaxRoutePreWarningOnly)}, + {std::string(kMaxRoutePostWarningOnly), + boolAttr( + kMaxRoutePostWarningOnly, setMaxRoutePostWarningOnly)}, + {std::string(kTimersHoldTime), + secondsAttr(kTimersHoldTime, setTimersHoldTime)}, + {std::string(kTimersKeepalive), + secondsAttr(kTimersKeepalive, setTimersKeepalive)}, + {std::string(kTimersOutDelay), + secondsAttr(kTimersOutDelay, setTimersOutDelay)}, + {std::string(kTimersWithdrawUnprogDelay), + secondsAttr( + kTimersWithdrawUnprogDelay, setTimersWithdrawUnprogDelay)}, + {std::string(kNextHopSelf), + boolAttr(kNextHopSelf, setNextHopSelf)}, + }; + return kHandlers; +} + +std::string validAttrList() { + std::string out; + for (const auto& [name, _] : attrHandlers()) { + if (!out.empty()) { + out += ", "; + } + out += name; + } + return out; +} + +// Find the peer-group keyed by name, creating it if absent. Setting an +// attribute on a not-yet-created group implicitly creates it, so command +// ordering stays forgiving; a bare `peer-group ` creates one explicitly. +// Unlike a BgpPeer, PeerGroup's only non-optional field is `name`, so no +// address seeding is needed. +bgp::thrift::PeerGroup& findOrCreatePeerGroup( + bgp::thrift::BgpConfig& cfg, + const std::string& groupName) { + auto& groups = cfg.peer_groups().ensure(); + for (auto& group : groups) { + if (*group.name() == groupName) { + return group; + } + } + groups.emplace_back(); + auto& group = groups.back(); + group.name() = groupName; + return group; +} + +// Whether a peer-group with the given name already exists. +bool peerGroupExists( + const bgp::thrift::BgpConfig& cfg, + const std::string& groupName) { + if (!cfg.peer_groups().has_value()) { + return false; + } + for (const auto& group : *cfg.peer_groups()) { + if (*group.name() == groupName) { + return true; + } + } + return false; +} + +} // 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 BgpNeighborConfig). +BgpPeerGroupConfig::BgpPeerGroupConfig(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: peer-group is required, followed by an optional " + " "); + } + if (v[0].empty()) { + throw std::invalid_argument("Error: peer-group name must not be empty"); + } + groupName_ = v[0]; + + if (v.size() == 1) { + return; // bare `peer-group `: create the group + } + + // Attributes may span two tokens (e.g. `timers hold-time`); match the + // longest prefix of the remaining tokens against the dispatch table. + const auto& handlers = attrHandlers(); + if (v.size() >= 3) { + std::string twoToken = v[1] + " " + v[2]; + if (handlers.find(twoToken) != handlers.end()) { + attr_ = std::move(twoToken); + values_.assign(v.begin() + 3, v.end()); + return; + } + } + if (handlers.find(v[1]) != handlers.end()) { + attr_ = v[1]; + values_.assign(v.begin() + 2, v.end()); + return; + } + throw std::invalid_argument( + fmt::format( + "Error: unknown peer-group attribute '{}'. Valid attributes: {}", + v[1], + validAttrList())); +} + CmdConfigProtocolBgpPeerGroupTraits::RetType CmdConfigProtocolBgpPeerGroup::queryClient( const HostInfo& /* hostInfo */, - const ObjectArgType& groupNameMsg) { - auto& session = BgpConfigSession::getInstance(); + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool created = !peerGroupExists(cfg, args.groupName()); + auto& group = findOrCreatePeerGroup(cfg, args.groupName()); - if (groupNameMsg.data().empty()) { - return "Error: peer-group name is required"; + Result result = args.attr().empty() + ? ok(fmt::format( + "Successfully created BGP peer-group {}", args.groupName())) + // The attribute is guaranteed valid: BgpPeerGroupConfig's constructor + // rejects an unknown attribute before we get here. + : attrHandlers().find(args.attr())->second(group, args.values()); + if (result.ok) { + if (!args.attr().empty()) { + result.message += fmt::format(" for peer-group {}", args.groupName()); + } + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } else if (created) { + // A rejected value must not leave a half-created group in the in-memory + // config (visible to later lookups in the same process, e.g. tests). + cfg.peer_groups()->pop_back(); } - - std::string groupName = groupNameMsg.data()[0]; - session.createPeerGroup(groupName); - session.saveConfig(); - - return fmt::format( - "BGP peer-group {} created.\n" - "Use subcommands: remote-asn, description, hold-time, keepalive\n" - "Config saved to: {}", - groupName, - session.getSessionConfigPath()); + return result.message; } void CmdConfigProtocolBgpPeerGroup::printOutput(const RetType& output) { diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h index 1c6d74e0f375a..1eca3e696eff3 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h @@ -10,18 +10,54 @@ #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/CmdUtils.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" namespace facebook::fboss { +// Parsed `peer-group [ ...]`, validated at +// construction. Attributes may span two tokens (e.g. `timers hold-time`), +// matched longest-prefix-first. Bare `peer-group ` creates the group. +// Mirrors BgpNeighborConfig. +class BgpPeerGroupConfig : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpPeerGroupConfig(std::vector v); + const std::string& groupName() const { + return groupName_; + } + 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 groupName_; + std::string attr_; // matched dispatch key ("" = bare create) + std::vector values_; +}; + +// Single handler for the whole `config protocol bgp peer-group` family; +// dispatch and per-attribute parsing live in the .cpp so a new tunable is a +// one-entry change. Mirrors CmdConfigProtocolBgpNeighbor. struct CmdConfigProtocolBgpPeerGroupTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgp; static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); + // Stops CLI11 from classifying attribute tokens as subcommands once the + // group name is consumed. See CmdConfigProtocolBgpNeighborTraits. + cmd.positionals_at_end(); + cmd.add_option("args", args, " [ ...]"); } - using ObjectArgType = utils::Message; + using ObjectArgType = BgpPeerGroupConfig; using RetType = std::string; }; @@ -32,7 +68,7 @@ class CmdConfigProtocolBgpPeerGroup : public CmdHandler< using ObjectArgType = CmdConfigProtocolBgpPeerGroupTraits::ObjectArgType; using RetType = CmdConfigProtocolBgpPeerGroupTraits::RetType; - RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& groupName); + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& args); void printOutput(const RetType& output); }; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.cpp deleted file mode 100644 index 13b8c1bd31e76..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.cpp +++ /dev/null @@ -1,61 +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/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupConfedPeerTraits::RetType -CmdConfigProtocolBgpPeerGroupConfedPeer::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& enableMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (enableMsg.data().empty()) { - return fmt::format( - "Error: enable value required for peer-group '{}'", peerGroupName); - } - std::string enableStr = enableMsg.data()[0]; - bool enable = (enableStr == "true" || enableStr == "1" || enableStr == "yes"); - - session.createPeerGroup(peerGroupName); - session.setPeerGroupConfedPeer(peerGroupName, enable); - session.saveConfig(); - - return fmt::format( - "Successfully {} confederation-peer for peer-group '{}'\n" - "Config saved to: {}", - enable ? "enabled" : "disabled", - peerGroupName, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupConfedPeer::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupConfedPeer, - CmdConfigProtocolBgpPeerGroupConfedPeerTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.h deleted file mode 100644 index 32fb0f19c28a6..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupConfedPeer.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupConfedPeerTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupConfedPeer - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupConfedPeer, - CmdConfigProtocolBgpPeerGroupConfedPeerTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupConfedPeerTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupConfedPeerTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& enable); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.cpp deleted file mode 100644 index f316e3477be66..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.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/peer-group/CmdConfigProtocolBgpPeerGroupDescription.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupDescriptionTraits::RetType -CmdConfigProtocolBgpPeerGroupDescription::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& groupNameMsg, - const ObjectArgType& descriptionMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (groupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - if (descriptionMsg.data().empty()) { - return "Error: description is required"; - } - - std::string groupName = groupNameMsg.data()[0]; - std::string description = descriptionMsg.data()[0]; - - session.setPeerGroupDescription(groupName, description); - session.saveConfig(); - - return fmt::format( - "Successfully set peer-group {} description to: {}\nConfig saved to: {}", - groupName, - description, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupDescription::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupDescription, - CmdConfigProtocolBgpPeerGroupDescriptionTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.h deleted file mode 100644 index 37ee936eeaaa1..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDescription.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupDescriptionTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupDescription - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupDescription, - CmdConfigProtocolBgpPeerGroupDescriptionTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupDescriptionTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupDescriptionTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& groupName, - const ObjectArgType& description); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.cpp deleted file mode 100644 index 0ab361e4cbeb7..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.cpp +++ /dev/null @@ -1,62 +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/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupDisableIpv4AfiTraits::RetType -CmdConfigProtocolBgpPeerGroupDisableIpv4Afi::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& disableMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (disableMsg.data().empty()) { - return fmt::format( - "Error: disable value required for peer-group '{}'", peerGroupName); - } - std::string disableStr = disableMsg.data()[0]; - bool disable = - (disableStr == "true" || disableStr == "1" || disableStr == "yes"); - - session.createPeerGroup(peerGroupName); - session.setPeerGroupDisableIpv4Afi(peerGroupName, disable); - session.saveConfig(); - - return fmt::format( - "Successfully {} IPv4 AFI for peer-group '{}'\n" - "Config saved to: {}", - disable ? "disabled" : "enabled", - peerGroupName, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupDisableIpv4Afi::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupDisableIpv4Afi, - CmdConfigProtocolBgpPeerGroupDisableIpv4AfiTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.h deleted file mode 100644 index cd06d0f7568d8..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupDisableIpv4Afi.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupDisableIpv4AfiTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupDisableIpv4Afi - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupDisableIpv4Afi, - CmdConfigProtocolBgpPeerGroupDisableIpv4AfiTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupDisableIpv4AfiTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupDisableIpv4AfiTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& disable); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.cpp deleted file mode 100644 index de09a95dedf21..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.cpp +++ /dev/null @@ -1,61 +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/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupEgressPolicyTraits::RetType -CmdConfigProtocolBgpPeerGroupEgressPolicy::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& policyMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (policyMsg.data().empty()) { - return fmt::format( - "Error: egress policy name is required for peer-group '{}'", - peerGroupName); - } - std::string policyName = policyMsg.data()[0]; - - session.createPeerGroup(peerGroupName); - session.setPeerGroupEgressPolicy(peerGroupName, policyName); - session.saveConfig(); - - return fmt::format( - "Successfully set egress policy for peer-group '{}' to: '{}'\n" - "Config saved to: {}", - peerGroupName, - policyName, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupEgressPolicy::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupEgressPolicy, - CmdConfigProtocolBgpPeerGroupEgressPolicyTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.h deleted file mode 100644 index e55ae153a80d7..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupEgressPolicy.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupEgressPolicyTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupEgressPolicy - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupEgressPolicy, - CmdConfigProtocolBgpPeerGroupEgressPolicyTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupEgressPolicyTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupEgressPolicyTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& policyName); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.cpp deleted file mode 100644 index 3e7b7fe50d2b9..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.cpp +++ /dev/null @@ -1,61 +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/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupIngressPolicyTraits::RetType -CmdConfigProtocolBgpPeerGroupIngressPolicy::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& policyMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (policyMsg.data().empty()) { - return fmt::format( - "Error: ingress policy name is required for peer-group '{}'", - peerGroupName); - } - std::string policyName = policyMsg.data()[0]; - - session.createPeerGroup(peerGroupName); - session.setPeerGroupIngressPolicy(peerGroupName, policyName); - session.saveConfig(); - - return fmt::format( - "Successfully set ingress policy for peer-group '{}' to: '{}'\n" - "Config saved to: {}", - peerGroupName, - policyName, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupIngressPolicy::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupIngressPolicy, - CmdConfigProtocolBgpPeerGroupIngressPolicyTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.h deleted file mode 100644 index f3345936405ae..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupIngressPolicy.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupIngressPolicyTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupIngressPolicy - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupIngressPolicy, - CmdConfigProtocolBgpPeerGroupIngressPolicyTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupIngressPolicyTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupIngressPolicyTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& policyName); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.cpp deleted file mode 100644 index a3af4c904f8b4..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.cpp +++ /dev/null @@ -1,71 +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/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupMaxRoutesTraits::RetType -CmdConfigProtocolBgpPeerGroupMaxRoutes::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& maxRoutesMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (maxRoutesMsg.data().empty()) { - return fmt::format( - "Error: max-routes value is required for peer-group '{}'", - peerGroupName); - } - - int64_t maxRoutes; - try { - maxRoutes = folly::to(maxRoutesMsg.data()[0]); - } catch (const std::exception& e) { - return fmt::format( - "Error: invalid max-routes value '{}': {}", - maxRoutesMsg.data()[0], - e.what()); - } - - session.createPeerGroup(peerGroupName); - session.setPeerGroupPreFilterMaxRoutes(peerGroupName, maxRoutes); - session.saveConfig(); - - return fmt::format( - "Successfully set max-routes for peer-group '{}' to: {}\n" - "Config saved to: {}", - peerGroupName, - maxRoutes, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupMaxRoutes::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupMaxRoutes, - CmdConfigProtocolBgpPeerGroupMaxRoutesTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.h deleted file mode 100644 index 655b445f342cb..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupMaxRoutes.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupMaxRoutesTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupMaxRoutes - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupMaxRoutes, - CmdConfigProtocolBgpPeerGroupMaxRoutesTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupMaxRoutesTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupMaxRoutesTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& maxRoutes); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.cpp deleted file mode 100644 index 6800cdddac8c9..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.cpp +++ /dev/null @@ -1,63 +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/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupNextHopSelfTraits::RetType -CmdConfigProtocolBgpPeerGroupNextHopSelf::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& enableMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (enableMsg.data().empty()) { - return fmt::format( - "Error: enable value (true/false) is required for peer-group '{}'", - peerGroupName); - } - - std::string enableStr = enableMsg.data()[0]; - bool enable = (enableStr == "true" || enableStr == "1" || enableStr == "yes"); - - session.createPeerGroup(peerGroupName); - session.setPeerGroupNextHopSelf(peerGroupName, enable); - session.saveConfig(); - - return fmt::format( - "Successfully {} next-hop-self for peer-group '{}'\n" - "Config saved to: {}", - enable ? "enabled" : "disabled", - peerGroupName, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupNextHopSelf::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupNextHopSelf, - CmdConfigProtocolBgpPeerGroupNextHopSelfTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.h deleted file mode 100644 index 83e39041a99b9..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupNextHopSelf.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupNextHopSelfTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupNextHopSelf - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupNextHopSelf, - CmdConfigProtocolBgpPeerGroupNextHopSelfTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupNextHopSelfTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupNextHopSelfTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& enable); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.cpp deleted file mode 100644 index 1fbb6a228e603..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.cpp +++ /dev/null @@ -1,58 +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/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupPeerTagTraits::RetType -CmdConfigProtocolBgpPeerGroupPeerTag::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& groupNameArg, - const ObjectArgType& peerTagMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (groupNameArg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string groupName = groupNameArg.data()[0]; - - if (peerTagMsg.data().empty()) { - return fmt::format( - "Error: peer-tag value is required for peer-group '{}'", groupName); - } - - session.createPeerGroup(groupName); - session.setPeerGroupPeerTag(groupName, peerTagMsg.data()[0]); - session.saveConfig(); - - return fmt::format( - "Successfully set peer_tag for peer-group '{}' to: '{}'\n" - "Config saved to: {}", - groupName, - peerTagMsg.data()[0], - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupPeerTag::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupPeerTag, - CmdConfigProtocolBgpPeerGroupPeerTagTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.h deleted file mode 100644 index 4715f131fe8b8..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupPeerTag.h +++ /dev/null @@ -1,45 +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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupPeerTagTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupPeerTag - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupPeerTag, - CmdConfigProtocolBgpPeerGroupPeerTagTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupPeerTagTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupPeerTagTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& groupName, - const ObjectArgType& peerTag); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.cpp deleted file mode 100644 index ea967b32e0e2d..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.cpp +++ /dev/null @@ -1,64 +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/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupRemoteAsnTraits::RetType -CmdConfigProtocolBgpPeerGroupRemoteAsn::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& groupNameMsg, - const ObjectArgType& remoteAsnMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (groupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - if (remoteAsnMsg.data().empty()) { - return "Error: remote-asn value is required"; - } - - std::string groupName = groupNameMsg.data()[0]; - std::string asnStr = remoteAsnMsg.data()[0]; - uint64_t remoteAsn; - try { - remoteAsn = folly::to(asnStr); - } catch (const std::exception& e) { - return fmt::format("Error: Invalid remote-asn '{}': {}", asnStr, e.what()); - } - - session.setPeerGroupRemoteAsn(groupName, remoteAsn); - session.saveConfig(); - - return fmt::format( - "Successfully set peer-group {} remote-asn to: {}\nConfig saved to: {}", - groupName, - remoteAsn, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupRemoteAsn::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupRemoteAsn, - CmdConfigProtocolBgpPeerGroupRemoteAsnTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.h deleted file mode 100644 index 7f7c79898d233..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRemoteAsn.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupRemoteAsnTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupRemoteAsn - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupRemoteAsn, - CmdConfigProtocolBgpPeerGroupRemoteAsnTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupRemoteAsnTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupRemoteAsnTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& groupName, - const ObjectArgType& remoteAsn); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.cpp deleted file mode 100644 index e6cff47df37bb..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.cpp +++ /dev/null @@ -1,62 +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/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupRrClientTraits::RetType -CmdConfigProtocolBgpPeerGroupRrClient::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& enableMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (enableMsg.data().empty()) { - return fmt::format( - "Error: enable value (true/false) is required for peer-group '{}'", - peerGroupName); - } - - std::string enableStr = enableMsg.data()[0]; - bool enable = (enableStr == "true" || enableStr == "1" || enableStr == "yes"); - - session.createPeerGroup(peerGroupName); - session.setPeerGroupRrClient(peerGroupName, enable); - session.saveConfig(); - - return fmt::format( - "Successfully {} route-reflector-client for peer-group '{}'\n" - "Config saved to: {}", - enable ? "enabled" : "disabled", - peerGroupName, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupRrClient::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupRrClient, - CmdConfigProtocolBgpPeerGroupRrClientTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.h deleted file mode 100644 index c98cac2f2f97c..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupRrClient.h +++ /dev/null @@ -1,45 +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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupRrClientTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupRrClient - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupRrClient, - CmdConfigProtocolBgpPeerGroupRrClientTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupRrClientTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupRrClientTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& enable); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.cpp deleted file mode 100644 index f1ebe72da4592..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.cpp +++ /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. - * - */ - -#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupTimersTraits::RetType -CmdConfigProtocolBgpPeerGroupTimers::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg) { - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - - std::string groupName = peerGroupNameMsg.data()[0]; - - return fmt::format( - "Configure timers for peer-group {}.\n" - "Use subcommands: hold-time, keepalive, out-delay", - groupName); -} - -void CmdConfigProtocolBgpPeerGroupTimers::printOutput(const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupTimers, - CmdConfigProtocolBgpPeerGroupTimersTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h deleted file mode 100644 index 2b457c60909f3..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimers.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupTimersTraits : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - using ObjectArgType = std::monostate; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupTimers - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupTimers, - CmdConfigProtocolBgpPeerGroupTimersTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupTimersTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupTimersTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.cpp deleted file mode 100644 index 0e5b83da36dd2..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.cpp +++ /dev/null @@ -1,65 +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/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupTimersHoldTimeTraits::RetType -CmdConfigProtocolBgpPeerGroupTimersHoldTime::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& holdTimeMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - if (holdTimeMsg.data().empty()) { - return "Error: hold-time value is required"; - } - - std::string groupName = peerGroupNameMsg.data()[0]; - std::string holdTimeStr = holdTimeMsg.data()[0]; - int32_t holdTime; - try { - holdTime = folly::to(holdTimeStr); - } catch (const std::exception& e) { - return fmt::format( - "Error: Invalid hold-time '{}': {}", holdTimeStr, e.what()); - } - - session.setPeerGroupHoldTime(groupName, holdTime); - session.saveConfig(); - - return fmt::format( - "Successfully set peer-group {} hold-time to: {} seconds\nConfig saved to: {}", - groupName, - holdTime, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupTimersHoldTime::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupTimersHoldTime, - CmdConfigProtocolBgpPeerGroupTimersHoldTimeTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.h deleted file mode 100644 index 21351570d9400..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersHoldTime.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/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupTimersHoldTimeTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroupTimers; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupTimersHoldTime - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupTimersHoldTime, - CmdConfigProtocolBgpPeerGroupTimersHoldTimeTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupTimersHoldTimeTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupTimersHoldTimeTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& holdTime); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.cpp deleted file mode 100644 index 7038f9a0e2c50..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.cpp +++ /dev/null @@ -1,71 +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/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupTimersKeepaliveTraits::RetType -CmdConfigProtocolBgpPeerGroupTimersKeepalive::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& keepaliveMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (keepaliveMsg.data().empty()) { - return fmt::format( - "Error: keepalive interval is required for peer-group '{}'", - peerGroupName); - } - - int32_t keepalive; - try { - keepalive = folly::to(keepaliveMsg.data()[0]); - } catch (const std::exception& e) { - return fmt::format( - "Error: invalid keepalive value '{}': {}", - keepaliveMsg.data()[0], - e.what()); - } - - session.createPeerGroup(peerGroupName); - session.setPeerGroupKeepalive(peerGroupName, keepalive); - session.saveConfig(); - - return fmt::format( - "Successfully set keepalive interval for peer-group '{}' to: {} seconds\n" - "Config saved to: {}", - peerGroupName, - keepalive, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupTimersKeepalive::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupTimersKeepalive, - CmdConfigProtocolBgpPeerGroupTimersKeepaliveTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.h deleted file mode 100644 index c2088385b391d..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersKeepalive.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/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupTimersKeepaliveTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroupTimers; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupTimersKeepalive - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupTimersKeepalive, - CmdConfigProtocolBgpPeerGroupTimersKeepaliveTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupTimersKeepaliveTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupTimersKeepaliveTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& keepalive); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.cpp deleted file mode 100644 index 21e4dc4c47eeb..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.cpp +++ /dev/null @@ -1,71 +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/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupTimersOutDelayTraits::RetType -CmdConfigProtocolBgpPeerGroupTimersOutDelay::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& outDelayMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (outDelayMsg.data().empty()) { - return fmt::format( - "Error: out-delay value is required for peer-group '{}'", - peerGroupName); - } - - int32_t outDelay; - try { - outDelay = folly::to(outDelayMsg.data()[0]); - } catch (const std::exception& e) { - return fmt::format( - "Error: invalid out-delay value '{}': {}", - outDelayMsg.data()[0], - e.what()); - } - - session.createPeerGroup(peerGroupName); - session.setPeerGroupOutDelay(peerGroupName, outDelay); - session.saveConfig(); - - return fmt::format( - "Successfully set out-delay for peer-group '{}' to: {} seconds\n" - "Config saved to: {}", - peerGroupName, - outDelay, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupTimersOutDelay::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupTimersOutDelay, - CmdConfigProtocolBgpPeerGroupTimersOutDelayTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.h deleted file mode 100644 index 16542ecc3b225..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersOutDelay.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/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupTimersOutDelayTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroupTimers; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupTimersOutDelay - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupTimersOutDelay, - CmdConfigProtocolBgpPeerGroupTimersOutDelayTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupTimersOutDelayTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupTimersOutDelayTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& outDelay); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.cpp deleted file mode 100644 index 92ccb97ff4a69..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.cpp +++ /dev/null @@ -1,70 +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/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelayTraits::RetType -CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& groupNameArg, - const ObjectArgType& delayMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (groupNameArg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string groupName = groupNameArg.data()[0]; - - if (delayMsg.data().empty()) { - return fmt::format( - "Error: withdraw-unprog-delay value (seconds) is required for peer-group '{}'", - groupName); - } - - int32_t delay; - try { - delay = std::stoi(delayMsg.data()[0]); - } catch (const std::exception& e) { - return fmt::format( - "Error: invalid withdraw-unprog-delay value '{}': {}", - delayMsg.data()[0], - e.what()); - } - - session.createPeerGroup(groupName); - session.setPeerGroupWithdrawUnprogDelay(groupName, delay); - session.saveConfig(); - - return fmt::format( - "Successfully set withdraw-unprog-delay for peer-group '{}' to: {} seconds\n" - "Config saved to: {}", - groupName, - delay, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay, - CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelayTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.h deleted file mode 100644 index 83355fc1b1e96..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay.h +++ /dev/null @@ -1,48 +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/peer-group/CmdConfigProtocolBgpPeerGroupTimers.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelayTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroupTimers; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelay, - CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelayTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelayTraits:: - ObjectArgType; - using RetType = - CmdConfigProtocolBgpPeerGroupTimersWithdrawUnprogDelayTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& withdrawUnprogDelay); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.cpp deleted file mode 100644 index ae03372ca1686..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.cpp +++ /dev/null @@ -1,61 +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/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupV4OverV6NhTraits::RetType -CmdConfigProtocolBgpPeerGroupV4OverV6Nh::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& peerGroupNameMsg, - const ObjectArgType& enableMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (peerGroupNameMsg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string peerGroupName = peerGroupNameMsg.data()[0]; - - if (enableMsg.data().empty()) { - return fmt::format( - "Error: enable value required for peer-group '{}'", peerGroupName); - } - std::string enableStr = enableMsg.data()[0]; - bool enable = (enableStr == "true" || enableStr == "1" || enableStr == "yes"); - - session.createPeerGroup(peerGroupName); - session.setPeerGroupIpv4OverIpv6Nh(peerGroupName, enable); - session.saveConfig(); - - return fmt::format( - "Successfully {} v4-over-v6-nexthop for peer-group '{}'\n" - "Config saved to: {}", - enable ? "enabled" : "disabled", - peerGroupName, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupV4OverV6Nh::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupV4OverV6Nh, - CmdConfigProtocolBgpPeerGroupV4OverV6NhTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.h deleted file mode 100644 index 89e5f72751f7b..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupV4OverV6Nh.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupV4OverV6NhTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupV4OverV6Nh - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupV4OverV6Nh, - CmdConfigProtocolBgpPeerGroupV4OverV6NhTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupV4OverV6NhTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupV4OverV6NhTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& peerGroupName, - const ObjectArgType& enable); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.cpp deleted file mode 100644 index ec21c05d82865..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.cpp +++ /dev/null @@ -1,70 +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/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupWarningLimitTraits::RetType -CmdConfigProtocolBgpPeerGroupWarningLimit::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& groupNameArg, - const ObjectArgType& warningLimitMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (groupNameArg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string groupName = groupNameArg.data()[0]; - - if (warningLimitMsg.data().empty()) { - return fmt::format( - "Error: warning-limit value is required for peer-group '{}'", - groupName); - } - - int64_t warningLimit; - try { - warningLimit = std::stoll(warningLimitMsg.data()[0]); - } catch (const std::exception& e) { - return fmt::format( - "Error: invalid warning-limit value '{}': {}", - warningLimitMsg.data()[0], - e.what()); - } - - session.createPeerGroup(groupName); - session.setPeerGroupPreFilterWarningLimit(groupName, warningLimit); - session.saveConfig(); - - return fmt::format( - "Successfully set pre_filter warning_limit for peer-group '{}' to: {}\n" - "Config saved to: {}", - groupName, - warningLimit, - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupWarningLimit::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupWarningLimit, - CmdConfigProtocolBgpPeerGroupWarningLimitTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.h deleted file mode 100644 index c72b72da59750..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningLimit.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupWarningLimitTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupWarningLimit - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupWarningLimit, - CmdConfigProtocolBgpPeerGroupWarningLimitTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupWarningLimitTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupWarningLimitTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& groupName, - const ObjectArgType& warningLimit); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.cpp deleted file mode 100644 index ca39a63b8ecd4..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.cpp +++ /dev/null @@ -1,72 +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/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.h" - -#include "fboss/cli/fboss2/CmdHandler.cpp" - -#include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpConfigSession.h" - -namespace facebook::fboss { - -CmdConfigProtocolBgpPeerGroupWarningOnlyTraits::RetType -CmdConfigProtocolBgpPeerGroupWarningOnly::queryClient( - const HostInfo& /* hostInfo */, - const utils::Message& groupNameArg, - const ObjectArgType& warningOnlyMsg) { - auto& session = BgpConfigSession::getInstance(); - - if (groupNameArg.data().empty()) { - return "Error: peer-group name is required"; - } - std::string groupName = groupNameArg.data()[0]; - - if (warningOnlyMsg.data().empty()) { - return fmt::format( - "Error: warning-only value (true|false) is required for peer-group '{}'", - groupName); - } - - std::string value = warningOnlyMsg.data()[0]; - bool warningOnly; - if (value == "true" || value == "1") { - warningOnly = true; - } else if (value == "false" || value == "0") { - warningOnly = false; - } else { - return fmt::format( - "Error: invalid warning-only value '{}', expected 'true' or 'false'", - value); - } - - session.createPeerGroup(groupName); - session.setPeerGroupPreFilterWarningOnly(groupName, warningOnly); - session.saveConfig(); - - return fmt::format( - "Successfully set pre_filter warning_only for peer-group '{}' to: {}\n" - "Config saved to: {}", - groupName, - warningOnly ? "true" : "false", - session.getSessionConfigPath()); -} - -void CmdConfigProtocolBgpPeerGroupWarningOnly::printOutput( - const RetType& output) { - std::cout << output << std::endl; -} - -// Explicit template instantiation -template void CmdHandler< - CmdConfigProtocolBgpPeerGroupWarningOnly, - CmdConfigProtocolBgpPeerGroupWarningOnlyTraits>::run(); - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.h b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.h deleted file mode 100644 index 9288c4dd9ae45..0000000000000 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroupWarningOnly.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/peer-group/CmdConfigProtocolBgpPeerGroup.h" -#include "fboss/cli/fboss2/utils/CmdUtils.h" - -namespace facebook::fboss { - -struct CmdConfigProtocolBgpPeerGroupWarningOnlyTraits - : public WriteCommandTraits { - using ParentCmd = CmdConfigProtocolBgpPeerGroup; - static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("msg", args, "Message"); - } - using ObjectArgType = utils::Message; - using RetType = std::string; -}; - -class CmdConfigProtocolBgpPeerGroupWarningOnly - : public CmdHandler< - CmdConfigProtocolBgpPeerGroupWarningOnly, - CmdConfigProtocolBgpPeerGroupWarningOnlyTraits> { - public: - using ObjectArgType = - CmdConfigProtocolBgpPeerGroupWarningOnlyTraits::ObjectArgType; - using RetType = CmdConfigProtocolBgpPeerGroupWarningOnlyTraits::RetType; - - RetType queryClient( - const HostInfo& hostInfo, - const utils::Message& groupName, - const ObjectArgType& warningOnly); - - void printOutput(const RetType& output); -}; - -} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp new file mode 100644 index 0000000000000..7fbe569d326e1 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp @@ -0,0 +1,81 @@ +/* + * 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/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#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 { + +// 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 BgpPeerGroupConfig). +BgpPeerGroupRef::BgpPeerGroupRef(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.size() != 1) { + throw std::invalid_argument( + "Error: delete protocol bgp peer-group requires exactly one "); + } + if (v[0].empty()) { + throw std::invalid_argument("Error: peer-group name must not be empty"); + } + groupName_ = std::move(v[0]); +} + +CmdDeleteProtocolBgpPeerGroupTraits::RetType +CmdDeleteProtocolBgpPeerGroup::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + if (!cfg.peer_groups().has_value()) { + // Nothing is persisted for an unknown group, so a typo'd delete can't + // stage an unrelated session change. + return fmt::format("Error: BGP peer-group {} not found", args.groupName()); + } + auto& groups = *cfg.peer_groups(); + auto it = std::find_if(groups.begin(), groups.end(), [&](const auto& group) { + return *group.name() == args.groupName(); + }); + if (it == groups.end()) { + return fmt::format("Error: BGP peer-group {} not found", args.groupName()); + } + groups.erase(it); + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP peer-group {}\nConfig saved to: {}", + args.groupName(), + session.getBgpSessionConfigPath()); +} + +void CmdDeleteProtocolBgpPeerGroup::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdDeleteProtocolBgpPeerGroup, + CmdDeleteProtocolBgpPeerGroupTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h new file mode 100644 index 0000000000000..c9793226e9a24 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h @@ -0,0 +1,61 @@ +/* + * 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 "CLI/App.hpp" +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/CmdDeleteProtocolBgp.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `delete protocol bgp peer-group `, validated at construction. +// The group name is the group's identity (an arbitrary string), matched +// exactly the same way the config command stores it. Mirrors BgpNeighborRef. +class BgpPeerGroupRef : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpPeerGroupRef(std::vector v); + const std::string& groupName() const { + return groupName_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::string groupName_; +}; + +struct CmdDeleteProtocolBgpPeerGroupTraits : public WriteCommandTraits { + using ParentCmd = CmdDeleteProtocolBgp; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("args", args, ""); + } + using ObjectArgType = BgpPeerGroupRef; + using RetType = std::string; +}; + +class CmdDeleteProtocolBgpPeerGroup : public CmdHandler< + CmdDeleteProtocolBgpPeerGroup, + CmdDeleteProtocolBgpPeerGroupTraits> { + public: + using ObjectArgType = CmdDeleteProtocolBgpPeerGroupTraits::ObjectArgType; + using RetType = CmdDeleteProtocolBgpPeerGroupTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& args); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/BUCK b/fboss/cli/fboss2/test/config/BUCK index d1ba98e802f39..2a062557d1e12 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -14,6 +14,7 @@ cpp_unittest( "CmdConfigArpTest.cpp", "CmdConfigBgpGlobalTest.cpp", "CmdConfigBgpNeighborTest.cpp", + "CmdConfigBgpPeerGroupTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", "CmdConfigDhcpTest.cpp", @@ -66,6 +67,7 @@ cpp_unittest( "CmdDeleteTunnelIpInIpTest.cpp", "CmdDeleteVlanTest.cpp", "ConfigSessionSystemdTest.cpp", + "CmdDeleteBgpPeerGroupTest.cpp", "ProfileValidationTest.cpp", ], headers = [ diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp new file mode 100644 index 0000000000000..1585e6e683dcb --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp @@ -0,0 +1,247 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include + +#include "configerator/structs/neteng/fboss/bgp/if/gen-cpp2/bgp_attr_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.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; +using facebook::neteng::fboss::bgp_attr::AddPath; + +namespace facebook::fboss { + +// The peer-group 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 CmdConfigBgpPeerGroupTestFixture : public CmdConfigTestBase { + public: + CmdConfigBgpPeerGroupTestFixture() + : CmdConfigTestBase("bgp_peer_group_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string run(const std::vector& tokens) { + CmdConfigProtocolBgpPeerGroup cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPeerGroupConfig(tokens)); + } + + const std::vector& groups() { + return ConfigSession::getInstance().getBgpConfig().peer_groups().ensure(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpPeerGroupConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpPeerGroupTestFixture, argValidation) { + // Bare create. + EXPECT_EQ(BgpPeerGroupConfig({"SPINE"}).groupName(), "SPINE"); + EXPECT_TRUE(BgpPeerGroupConfig({"SPINE"}).attr().empty()); + + // Single-token attribute. + auto singleTok = BgpPeerGroupConfig({"SPINE", "remote-asn", "65000"}); + EXPECT_EQ(singleTok.attr(), "remote-asn"); + EXPECT_EQ(singleTok.values(), std::vector({"65000"})); + + // Two-token attribute wins longest-prefix match. + auto twoTok = BgpPeerGroupConfig({"SPINE", "timers", "hold-time", "30"}); + EXPECT_EQ(twoTok.attr(), "timers hold-time"); + EXPECT_EQ(twoTok.values(), std::vector({"30"})); + + // Invalid: empty, empty name, unknown attribute, incomplete group. + EXPECT_THROW(BgpPeerGroupConfig({}), std::invalid_argument); + EXPECT_THROW(BgpPeerGroupConfig({""}), std::invalid_argument); + EXPECT_THROW( + BgpPeerGroupConfig({"SPINE", "no-such-attr", "1"}), + std::invalid_argument); + EXPECT_THROW(BgpPeerGroupConfig({"SPINE", "timers"}), std::invalid_argument); +} + +// ============================================================================== +// Attribute handlers +// ============================================================================== + +TEST_F(CmdConfigBgpPeerGroupTestFixture, bareCreate) { + auto result = run({"SPINE"}); + EXPECT_THAT(result, HasSubstr("Successfully created BGP peer-group SPINE")); + ASSERT_EQ(groups().size(), 1); + EXPECT_EQ(*groups()[0].name(), "SPINE"); + // PeerGroup's only non-optional field is `name`; nothing else is seeded. + EXPECT_FALSE(groups()[0].local_addr().has_value()); + EXPECT_TRUE(sessionFileExists()); + + run({"LEAF"}); + ASSERT_EQ(groups().size(), 2); + EXPECT_EQ(*groups()[1].name(), "LEAF"); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, setRemoteAsn) { + auto result = run({"SPINE", "remote-asn", "65000"}); + EXPECT_THAT(result, HasSubstr("65000")); + ASSERT_EQ(groups().size(), 1); + EXPECT_EQ(groups()[0].remote_as_4_byte().value_or(0), 65000); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, timersAccumulate) { + run({"SPINE", "timers", "hold-time", "90"}); + run({"SPINE", "timers", "keepalive", "30"}); + run({"SPINE", "graceful-restart", "restart-time", "120"}); + ASSERT_EQ(groups().size(), 1); + const auto& timers = *groups()[0].bgp_peer_timers(); + EXPECT_EQ(*timers.hold_time_seconds(), 90); + EXPECT_EQ(*timers.keep_alive_seconds(), 30); + EXPECT_EQ(timers.graceful_restart_seconds().value_or(0), 120); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, addPathMerge) { + run({"SPINE", "add-path", "receive", "true"}); + EXPECT_EQ(groups()[0].add_path().value_or(AddPath(0)), AddPath::RECEIVE); + + run({"SPINE", "add-path", "send", "true"}); + EXPECT_EQ(groups()[0].add_path().value_or(AddPath(0)), AddPath::BOTH); + + run({"SPINE", "add-path", "receive", "false"}); + EXPECT_EQ(groups()[0].add_path().value_or(AddPath(0)), AddPath::SEND); + + run({"SPINE", "add-path", "send", "false"}); + EXPECT_FALSE(groups()[0].add_path().has_value()); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, connectMode) { + run({"SPINE", "connect-mode", "PASSIVE"}); + EXPECT_TRUE(groups()[0].is_passive().value_or(false)); + + run({"SPINE", "connect-mode", "ACTIVE"}); + EXPECT_FALSE(groups()[0].is_passive().value_or(true)); + + // BOTH has no thrift representation and must be rejected, leaving the + // previous value intact and nothing new persisted. + auto result = run({"SPINE", "connect-mode", "BOTH"}); + EXPECT_THAT(result, HasSubstr("Error")); + EXPECT_FALSE(groups()[0].is_passive().value_or(true)); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, boolAndStringAttributes) { + run({"SPINE", "rr-client", "true"}); + run({"SPINE", "confed-peer", "false"}); + run({"SPINE", "redistribute-peer", "true"}); + run({"SPINE", "enhanced-route-refresh", "true"}); + run({"SPINE", "afi", "disable-ipv4-afi", "true"}); + run({"SPINE", "afi", "disable-ipv6-afi", "false"}); + run({"SPINE", "afi", "ipv4-over-ipv6-nh", "true"}); + run({"SPINE", "graceful-restart", "stateful-ha", "true"}); + run({"SPINE", "peer-tag", "FSW"}); + run({"SPINE", "ingress-policy", "IN_POLICY"}); + run({"SPINE", "egress-policy", "OUT_POLICY"}); + run({"SPINE", "local-asn", "64512"}); + run({"SPINE", "next-hop-self", "true"}); + run({"SPINE", "description", "spine", "group"}); + + ASSERT_EQ(groups().size(), 1); + const auto& group = groups()[0]; + EXPECT_TRUE(group.is_rr_client().value_or(false)); + EXPECT_FALSE(group.is_confed_peer().value_or(true)); + EXPECT_TRUE(group.is_redistribute_peer().value_or(false)); + EXPECT_TRUE(group.enhanced_route_refresh().value_or(false)); + EXPECT_TRUE(group.disable_ipv4_afi().value_or(false)); + EXPECT_FALSE(group.disable_ipv6_afi().value_or(true)); + EXPECT_TRUE(group.v4_over_v6_nexthop().value_or(false)); + EXPECT_TRUE(group.enable_stateful_ha().value_or(false)); + EXPECT_EQ(group.peer_tag().value_or(""), "FSW"); + EXPECT_EQ(group.ingress_policy_name().value_or(""), "IN_POLICY"); + EXPECT_EQ(group.egress_policy_name().value_or(""), "OUT_POLICY"); + EXPECT_EQ(group.local_as_4_byte().value_or(0), 64512); + EXPECT_TRUE(group.next_hop_self().value_or(false)); + // Multi-token description is re-joined. + EXPECT_EQ(group.description().value_or(""), "spine group"); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, routeLimits) { + run({"SPINE", "max-route", "pre-filter", "45000"}); + run({"SPINE", "max-route", "post-filter", "50000"}); + run({"SPINE", "max-route", "post-warning-threshold", "48000"}); + // Parity attributes carried over from the deleted per-attribute commands. + run({"SPINE", "max-route", "pre-warning-threshold", "40000"}); + run({"SPINE", "max-route", "pre-warning-only", "true"}); + run({"SPINE", "max-route", "post-warning-only", "false"}); + const auto& group = groups()[0]; + EXPECT_EQ(*group.pre_filter()->max_routes(), 45000); + EXPECT_EQ(*group.post_filter()->max_routes(), 50000); + EXPECT_EQ(*group.post_filter()->warning_limit(), 48000); + EXPECT_EQ(*group.pre_filter()->warning_limit(), 40000); + EXPECT_TRUE(*group.pre_filter()->warning_only()); + EXPECT_FALSE(*group.post_filter()->warning_only()); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, valueValidation) { + // ASN must fit in 4 bytes (RFC 6793). + EXPECT_THAT(run({"SPINE", "remote-asn", "4294967296"}), HasSubstr("Invalid")); + EXPECT_THAT( + run({"SPINE", "local-asn", "18446744073709551615"}), + HasSubstr("Invalid")); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, namedGroupsAreDistinct) { + run({"SPINE", "remote-asn", "65000"}); + run({"LEAF", "remote-asn", "65001"}); + ASSERT_EQ(groups().size(), 2); + // Re-referencing an existing group by name updates it, not appends. + run({"SPINE", "local-asn", "64512"}); + EXPECT_EQ(groups().size(), 2); +} + +// ============================================================================== +// Reject paths — the error is surfaced and nothing is persisted +// ============================================================================== + +TEST_F(CmdConfigBgpPeerGroupTestFixture, invalidValuesRejected) { + EXPECT_THAT(run({"SPINE", "rr-client", "maybe"}), HasSubstr("Invalid")); + EXPECT_THAT( + run({"SPINE", "timers", "hold-time", "-1"}), HasSubstr("non-negative")); + EXPECT_THAT( + run({"SPINE", "remote-asn", "not-a-number"}), HasSubstr("Invalid")); + EXPECT_THAT( + run({"SPINE", "max-route", "pre-filter", "-5"}), + HasSubstr("non-negative")); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, unsupportedAttributesRejected) { + // These attributes have no per-peer-group thrift field, so they are not in + // the dispatch table and are refused at parse time as unknown attributes. + EXPECT_THROW(run({"SPINE", "peer-port", "1179"}), std::invalid_argument); + EXPECT_THROW( + run({"SPINE", "afi", "ipv4-labeled-unicast", "true"}), + std::invalid_argument); + EXPECT_THROW( + run({"SPINE", "afi", "ipv6-labeled-unicast", "true"}), + std::invalid_argument); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp new file mode 100644 index 0000000000000..826e20c805127 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp @@ -0,0 +1,106 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include + +#include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.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 { + +// Deleting 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 +// CmdConfigBgpPeerGroupTest). +class CmdDeleteBgpPeerGroupTestFixture : public CmdConfigTestBase { + public: + CmdDeleteBgpPeerGroupTestFixture() + : CmdConfigTestBase("bgp_peer_group_delete_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string configure(const std::vector& tokens) { + CmdConfigProtocolBgpPeerGroup cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPeerGroupConfig(tokens)); + } + + std::string del(const std::vector& tokens) { + CmdDeleteProtocolBgpPeerGroup cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPeerGroupRef(tokens)); + } + + const std::vector& groups() { + return ConfigSession::getInstance().getBgpConfig().peer_groups().ensure(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpPeerGroupRef (arg) validation +// ============================================================================== + +TEST_F(CmdDeleteBgpPeerGroupTestFixture, argValidation) { + EXPECT_EQ(BgpPeerGroupRef({"SPINE"}).groupName(), "SPINE"); + + // Invalid: empty, empty name, extra tokens. + EXPECT_THROW(BgpPeerGroupRef({}), std::invalid_argument); + EXPECT_THROW(BgpPeerGroupRef({""}), std::invalid_argument); + EXPECT_THROW(BgpPeerGroupRef({"SPINE", "LEAF"}), std::invalid_argument); +} + +// ============================================================================== +// queryClient +// ============================================================================== + +TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteExistingGroup) { + configure({"SPINE", "remote-asn", "65000"}); + configure({"LEAF", "remote-asn", "65001"}); + ASSERT_EQ(groups().size(), 2); + + auto result = del({"SPINE"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted BGP peer-group SPINE")); + // The other group survives untouched. + ASSERT_EQ(groups().size(), 1); + EXPECT_EQ(*groups()[0].name(), "LEAF"); + EXPECT_EQ(groups()[0].remote_as_4_byte().value_or(0), 65001); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteUnknownGroupRejected) { + auto result = del({"NO-SUCH-GROUP"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP peer-group NO-SUCH-GROUP not found")); + // Nothing was persisted for the failed delete. + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected delete"; +} + +TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteUnknownLeavesOthersIntact) { + configure({"SPINE", "remote-asn", "65000"}); + auto result = del({"LEAF"}); + EXPECT_THAT(result, HasSubstr("not found")); + ASSERT_EQ(groups().size(), 1); + EXPECT_EQ(*groups()[0].name(), "SPINE"); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index ac3bd2d5b5576..b8ebe6a138482 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -24,6 +24,7 @@ cpp_binary( "ConfigArpTest.cpp", "ConfigBgpGlobalTest.cpp", "ConfigBgpNeighborTest.cpp", + "ConfigBgpPeerGroupTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", "ConfigCoppTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp new file mode 100644 index 0000000000000..544a437efea22 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp @@ -0,0 +1,131 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp peer-group + * [ ...]`. + * + * Scope: the per-peer-group attributes. Every test stages the change AND + * commits it, then asserts the value landed at the correct thrift field path + * inside the matching .peer_groups[] entry of bgpd's running config (via + * getRunningConfig RPC). Stage-only behavior (attribute parsing, validation, + * rejection) is covered by the unit tests; an integration test that never + * commits exercises no daemon. + * + * Requirements: + * - The fboss2-dev binary under test (config subcommand tree). + * - HOME is set (the session file lives under $HOME/.fboss2). + * - bgpd is installed/active (commit restarts it). + */ + +#include +#include +#include + +#include "fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h" +#include "folly/json/dynamic.h" +#include "gmock/gmock.h" + +using namespace facebook::fboss; +using ::testing::HasSubstr; +using ::testing::Not; + +namespace { +// Test-only peer-group names, unlikely to collide with a real group in the +// device's running BGP config. +const std::string kGroup = "FBOSS2-TEST-GROUP"; +} // namespace + +class ConfigBgpPeerGroupTest : public ConfigBgpTestBase { + protected: + // Stage `config protocol bgp peer-group ` WITHOUT clearing the + // staged session, so attributes can accumulate across invocations. Returns + // the staged session JSON. + folly::dynamic stagePeerGroup(const std::vector& tokens) { + std::vector args = {"config", "protocol", "bgp", "peer-group"}; + args.insert(args.end(), tokens.begin(), tokens.end()); + auto result = runCli(args); + EXPECT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + EXPECT_THAT(result.stdout, Not(HasSubstr("Error:"))); + return readBgpSessionConfig(); + } + + // The .peer_groups[] entry named `name`, or nullptr. + static const folly::dynamic* findGroup( + const folly::dynamic& config, + const std::string& name) { + if (config.count("peer_groups") == 0) { + return nullptr; + } + for (const auto& group : config["peer_groups"]) { + if (group.count("name") && group["name"].asString() == name) { + return &group; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPeerGroupTest, SetRemoteAsnAndCommit) { + discardSession(); + clearBgpSession(); + stagePeerGroup({kGroup, "remote-asn", "65000"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + // Two layers: the promoted file proves what the commit wrote; the daemon's + // getRunningConfig RPC proves bgpd parsed and adopted it after the restart. + auto config = readSystemBgpConfig(); + const auto* group = findGroup(config, kGroup); + ASSERT_NE(group, nullptr) << "committed config has no peer-group " << kGroup; + ASSERT_TRUE(group->count("remote_as_4_byte")); + EXPECT_EQ((*group)["remote_as_4_byte"].asInt(), 65000); + + auto running = readRunningBgpConfigViaRpc(); + const auto* runningGroup = findGroup(running, kGroup); + ASSERT_NE(runningGroup, nullptr) + << "bgpd's running config has no peer-group " << kGroup; + EXPECT_EQ((*runningGroup)["remote_as_4_byte"].asInt(), 65000); +} + +TEST_F(ConfigBgpPeerGroupTest, SetTimersHoldTimeAndCommit) { + discardSession(); + clearBgpSession(); + stagePeerGroup({kGroup, "timers", "hold-time", "90"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + // Verify nested timer fields through the daemon's own view of its config. + auto running = readRunningBgpConfigViaRpc(); + const auto* group = findGroup(running, kGroup); + ASSERT_NE(group, nullptr) + << "bgpd's running config has no peer-group " << kGroup; + ASSERT_TRUE(group->count("bgp_peer_timers")); + EXPECT_EQ((*group)["bgp_peer_timers"]["hold_time_seconds"].asInt(), 90); +} + +TEST_F(ConfigBgpPeerGroupTest, DeleteGroupAndCommit) { + // Land a peer-group in the system config, then delete it through a second + // commit and verify it is gone from bgpd's running config. + discardSession(); + clearBgpSession(); + stagePeerGroup({kGroup, "remote-asn", "65000"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + ASSERT_NE(findGroup(readRunningBgpConfigViaRpc(), kGroup), nullptr) + << "setup commit did not land the peer-group in bgpd's running config"; + + clearBgpSession(); + auto result = runCli({"delete", "protocol", "bgp", "peer-group", kGroup}); + EXPECT_THAT(result.stdout, HasSubstr("Successfully deleted")); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after delete commit; state=" + << bgpDaemonActiveState(); + EXPECT_EQ(findGroup(readRunningBgpConfigViaRpc(), kGroup), nullptr) + << "deleted peer-group still present in bgpd's running config"; +} From 2e4dbfa0f62f020345909d94d8749d3ad4309715 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 20 Aug 2026 16:41:30 +0000 Subject: [PATCH 2/9] [Nexthop] Guard peer-group delete against neighbor references; close UT gaps delete protocol bgp peer-group now refuses to delete a group that a neighbor's peer_group_name still references, naming the referencing neighbors, instead of committing a dangling reference bgpd has no defined behavior for. New/extended unit tests close the coverage gaps found in review: - timers out-delay and withdraw-unprog-delay (previously untested; withdraw-unprog-delay is a parity attribute tested for neighbor) - valueValidation now asserts the half-created group is rolled back (state assertion, not just the error string) - wrongValueCountRejected: zero/too-many value token counts - overwriteSameAttributeLastWins: re-setting an attribute updates in place - namedGroupsAreDistinct now asserts field preservation across mutations and sibling isolation - delete: referenced-group rejected (all referencing neighbors named), unreferenced delete unaffected by other references, delete-then- recreate starts clean Co-Authored-By: Claude Fable 5 --- .../CmdDeleteProtocolBgpPeerGroup.cpp | 18 +++++++ .../test/config/CmdConfigBgpPeerGroupTest.cpp | 41 +++++++++++++- .../test/config/CmdDeleteBgpPeerGroupTest.cpp | 54 +++++++++++++++++++ 3 files changed, 111 insertions(+), 2 deletions(-) diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp index 7fbe569d326e1..b34cd75ece87c 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp @@ -13,6 +13,7 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include +#include #include #include #include @@ -61,6 +62,23 @@ CmdDeleteProtocolBgpPeerGroup::queryClient( if (it == groups.end()) { return fmt::format("Error: BGP peer-group {} not found", args.groupName()); } + // A neighbor's peer_group_name resolves against this group by name at + // daemon load; erasing the group while a neighbor still points at it would + // commit a dangling reference. Refuse and name the neighbors instead. + std::vector referencingPeers; + for (const auto& peer : *cfg.peers()) { + if (peer.peer_group_name().has_value() && + *peer.peer_group_name() == args.groupName()) { + referencingPeers.push_back(*peer.peer_addr()); + } + } + if (!referencingPeers.empty()) { + return fmt::format( + "Error: BGP peer-group {} is still referenced by neighbor(s): {}; " + "delete or re-point those neighbors first", + args.groupName(), + folly::join(", ", referencingPeers)); + } groups.erase(it); session.saveBgpConfig(); return fmt::format( diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp index 1585e6e683dcb..b894f859b1a90 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp @@ -106,11 +106,15 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, setRemoteAsn) { TEST_F(CmdConfigBgpPeerGroupTestFixture, timersAccumulate) { run({"SPINE", "timers", "hold-time", "90"}); run({"SPINE", "timers", "keepalive", "30"}); + run({"SPINE", "timers", "out-delay", "5"}); + run({"SPINE", "timers", "withdraw-unprog-delay", "10"}); run({"SPINE", "graceful-restart", "restart-time", "120"}); ASSERT_EQ(groups().size(), 1); const auto& timers = *groups()[0].bgp_peer_timers(); EXPECT_EQ(*timers.hold_time_seconds(), 90); EXPECT_EQ(*timers.keep_alive_seconds(), 30); + EXPECT_EQ(*timers.out_delay_seconds(), 5); + EXPECT_EQ(*timers.withdraw_unprog_delay_seconds(), 10); EXPECT_EQ(timers.graceful_restart_seconds().value_or(0), 120); } @@ -200,17 +204,50 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, valueValidation) { EXPECT_THAT( run({"SPINE", "local-asn", "18446744073709551615"}), HasSubstr("Invalid")); + // The rejected value implicitly created SPINE; the rollback must remove it, + // not just skip persisting it. + EXPECT_TRUE(groups().empty()) + << "rejected first attribute must not leave a half-created group"; EXPECT_FALSE(sessionFileExists()) << "session file should not exist after rejected input"; } +TEST_F(CmdConfigBgpPeerGroupTestFixture, wrongValueCountRejected) { + // Known attribute, but no value: parses fine, handler rejects at runtime. + EXPECT_THAT(run({"SPINE", "remote-asn"}), HasSubstr("requires")); + EXPECT_THAT(run({"SPINE", "timers", "hold-time"}), HasSubstr("requires")); + EXPECT_THAT(run({"SPINE", "description"}), HasSubstr("requires")); + // Too many values for single-value attributes. + EXPECT_THAT( + run({"SPINE", "remote-asn", "65000", "65001"}), HasSubstr("requires")); + EXPECT_THAT( + run({"SPINE", "rr-client", "true", "false"}), HasSubstr("requires")); + EXPECT_TRUE(groups().empty()) + << "rejected first attribute must not leave a half-created group"; + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, overwriteSameAttributeLastWins) { + run({"SPINE", "remote-asn", "65000"}); + run({"SPINE", "remote-asn", "65001"}); + ASSERT_EQ(groups().size(), 1); + EXPECT_EQ(groups()[0].remote_as_4_byte().value_or(0), 65001); +} + TEST_F(CmdConfigBgpPeerGroupTestFixture, namedGroupsAreDistinct) { run({"SPINE", "remote-asn", "65000"}); run({"LEAF", "remote-asn", "65001"}); ASSERT_EQ(groups().size(), 2); - // Re-referencing an existing group by name updates it, not appends. + // Re-referencing an existing group by name updates it, not appends — + // and mutating one field preserves the group's other fields and leaves + // the sibling group untouched. run({"SPINE", "local-asn", "64512"}); - EXPECT_EQ(groups().size(), 2); + ASSERT_EQ(groups().size(), 2); + EXPECT_EQ(groups()[0].remote_as_4_byte().value_or(0), 65000); + EXPECT_EQ(groups()[0].local_as_4_byte().value_or(0), 64512); + EXPECT_EQ(groups()[1].remote_as_4_byte().value_or(0), 65001); + EXPECT_FALSE(groups()[1].local_as_4_byte().has_value()); } // ============================================================================== diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp index 826e20c805127..5ef75558b0e4e 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp @@ -49,6 +49,20 @@ class CmdDeleteBgpPeerGroupTestFixture : public CmdConfigTestBase { return ConfigSession::getInstance().getBgpConfig().peer_groups().ensure(); } + // Reference a peer-group from a neighbor the way a committed config would, + // without pulling the neighbor command (and its whole dispatch table) into + // this test binary. + void addPeerReferencing( + const std::string& peerAddr, + const std::string& groupName) { + bgp::thrift::BgpPeer peer; + peer.local_addr() = "10.0.0.1"; + peer.peer_addr() = peerAddr; + peer.peer_group_name() = groupName; + ConfigSession::getInstance().getBgpConfig().peers()->push_back( + std::move(peer)); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -103,4 +117,44 @@ TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteUnknownLeavesOthersIntact) { EXPECT_EQ(*groups()[0].name(), "SPINE"); } +TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteReferencedGroupRejected) { + configure({"SPINE", "remote-asn", "65000"}); + addPeerReferencing("10.0.0.2", "SPINE"); + addPeerReferencing("10.0.0.3", "SPINE"); + + auto result = del({"SPINE"}); + EXPECT_THAT(result, HasSubstr("still referenced")); + // The refusal names every referencing neighbor so the user can act on it. + EXPECT_THAT(result, HasSubstr("10.0.0.2")); + EXPECT_THAT(result, HasSubstr("10.0.0.3")); + ASSERT_EQ(groups().size(), 1); + EXPECT_EQ(*groups()[0].name(), "SPINE"); + EXPECT_EQ(groups()[0].remote_as_4_byte().value_or(0), 65000); +} + +TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteUnreferencedGroupSucceeds) { + configure({"SPINE", "remote-asn", "65000"}); + configure({"LEAF", "remote-asn", "65001"}); + // A neighbor referencing SPINE must not block deleting LEAF. + addPeerReferencing("10.0.0.2", "SPINE"); + + auto result = del({"LEAF"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted BGP peer-group LEAF")); + ASSERT_EQ(groups().size(), 1); + EXPECT_EQ(*groups()[0].name(), "SPINE"); +} + +TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteThenRecreateStartsClean) { + configure({"SPINE", "remote-asn", "65000"}); + auto result = del({"SPINE"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted")); + EXPECT_TRUE(groups().empty()); + + configure({"SPINE"}); + ASSERT_EQ(groups().size(), 1); + // The recreated group starts clean — no fields leak across the + // delete/recreate boundary. + EXPECT_FALSE(groups()[0].remote_as_4_byte().has_value()); +} + } // namespace facebook::fboss From 99aa8192f3fc3943a555a9ab5d3c3fd496bc6fc8 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 21 Aug 2026 03:24:20 +0000 Subject: [PATCH 3/9] [Nexthop] Deflake BGP and VLAN-default CLI integration tests Two pre-existing CI failures surfaced on every CLI-touching PR: - ConfigBgp*Test: the SetUp gate sampled systemctl is-active once. On fboss-sim bgpd flaps activating->active->failed from boot, so one unlucky sample lands in the brief active window, runs the test, and the bgpd thrift RPC then times out after 90s (seen as SetGracefulRestartTime / SetCountConfedsInAsPathLenFalse failures; green runs are the ones where every sample landed on inactive and skipped). The gate now also requires bgpd to answer its getRunningConfig RPC within 10s, else the test skips like the rest of the flap cycle. - ConfigVlanDefaultTest.ChangeDefaultVlanWithPortInNonDefaultVlan vacated only the test port before changing the default VLAN, but the change-default guard refuses while ANY port still uses the old default VLAN without an interface. On baselines generated before the default VLAN, so the guard always fired. Vacate every eth port from the old default VLAN first and restore them after. Co-Authored-By: Claude Fable 5 --- .../test/integration_test/ConfigBgpTestBase.h | 12 ++++++ .../ConfigVlanDefaultTest.cpp | 40 ++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h index f2ad61eca84cb..7fa3b65eb8953 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h @@ -53,6 +53,18 @@ class ConfigBgpTestBase : public Fboss2IntegrationTest { if (bgpDaemonActiveState() != "active") { GTEST_SKIP() << "skipping because bgp is not active"; } + // systemd "active" is a point sample: on an image where bgpd cannot hold + // its boot config it flaps activating -> active -> failed, and a single + // sample can land inside the brief active window (seen on fboss-sim, + // where the sibling tests' samples all landed on inactive and skipped). + // Require the daemon to actually answer its thrift RPC before running, + // and skip otherwise — the same skip the rest of the flap cycle produces. + try { + readRunningBgpConfigViaRpc(std::chrono::seconds(10)); + } catch (const std::exception& ex) { + GTEST_SKIP() << "skipping because bgpd is active but not answering its " + << "thrift RPC (flapping unit?): " << ex.what(); + } // Snapshot the config bgpd runs with; TearDown restores it, so committed // test objects never outlive the test regardless of where it failed. haveBgpSnapshot_ = folly::readFile(kBgpDaemonConfigPath, bgpSnapshot_); diff --git a/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp index 0b6578cc37436..861da08d48e73 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigVlanDefaultTest.cpp @@ -307,6 +307,34 @@ TEST_F(ConfigVlanDefaultTest, ChangeDefaultVlanWithPortInNonDefaultVlan) { << "Failed to move " << ifName << " to side VLAN " << sideVlan; XLOG(INFO) << "[Step 1] Moved " << ifName << " to VLAN " << sideVlan; + // Vacate every other eth port still parked on the current default VLAN + // (baselines generated before the per-port ingress VLAN change park all + // ports there). The change-default guard refuses while ANY port uses the + // old default VLAN and it has no interface, not just the test port. + std::vector movedPorts; + for (const auto& port : initialConfig["sw"]["ports"]) { + auto name = port.getDefault("name", "").asString(); + if (name.rfind("eth", 0) != 0 || name == ifName) { + continue; + } + if (port.getDefault("ingressVlan", -1).asInt() != currentDefault) { + continue; + } + auto r = runCli( + {"config", + "interface", + name, + "switchport", + "access", + "vlan", + std::to_string(sideVlan)}); + if (r.exitCode == 0) { + movedPorts.push_back(name); + } + } + XLOG(INFO) << "[Step 1b] Vacated " << movedPorts.size() + << " other ports off default VLAN " << currentDefault; + // Set a new default VLAN — the command must succeed even when no ports // remain on the old default VLAN. The target is auto-created, so it must be // an ID the agent can back with a route table (T284228086). @@ -329,7 +357,7 @@ TEST_F(ConfigVlanDefaultTest, ChangeDefaultVlanWithPortInNonDefaultVlan) { waitForAgentReady(); EXPECT_EQ(getSwConfigField("defaultVlan"), newDefault); - // Restore: move the port back and reset defaultVlan. + // Restore: move the ports back and reset defaultVlan. XLOG(INFO) << "[Restore] Moving " << ifName << " back to VLAN " << originalPortVlan; runCli( @@ -340,6 +368,16 @@ TEST_F(ConfigVlanDefaultTest, ChangeDefaultVlanWithPortInNonDefaultVlan) { "access", "vlan", std::to_string(originalPortVlan)}); + for (const auto& name : movedPorts) { + runCli( + {"config", + "interface", + name, + "switchport", + "access", + "vlan", + std::to_string(currentDefault)}); + } auto restoreDefault = runCli({"config", "vlan", "default", std::to_string(currentDefault)}); From c0994f3f470692c636f93361832b23d820d94d93 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 21 Aug 2026 16:48:47 +0000 Subject: [PATCH 4/9] [Nexthop] Bump cmd_config_test to size=large MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite is ~60 files of ConfigSession-backed tests, each doing several fsync-backed writeFileAtomic calls per case; with this PR's added peer-group tests the full run takes ~530s on a dev box and timed out at the CI job's default moderate test timeout (300s) — killed mid-fsync in ConfigSessionTestFixture.commandTrackingMultipleCommands, 50/51 remaining tests green. size=large raises the test timeout to 900s (same precedent as thrift_packet_stream/tests). Co-Authored-By: Claude Fable 5 --- fboss/cli/fboss2/test/config/BUCK | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fboss/cli/fboss2/test/config/BUCK b/fboss/cli/fboss2/test/config/BUCK index 2a062557d1e12..11e757b69483f 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -5,6 +5,10 @@ oncall("fboss_oss") cpp_unittest( name = "cmd_config_test", + # The suite is ~60 files of ConfigSession tests, each doing several + # fsync-backed writeFileAtomic calls; on CI disks the run can exceed the + # default "moderate" (300s) Bazel test timeout. + size = "large", srcs = [ "BgpConfigSessionTest.cpp", "CmdConfigAclRuleTest.cpp", From 4c366a0084e5fdd2cd4af356aaf9912b8490bd03 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 9 Sep 2026 11:36:35 +0000 Subject: [PATCH 5/9] [Nexthop] Seed inherited peer-group timers, expose passive, fix j2c grammar Signed-off-by: hillol-nexthop --- fboss/cli/fboss2/CmdListConfig.cpp | 2 +- .../CmdConfigProtocolBgpPeerGroup.cpp | 304 ++++++++++-------- .../test/config/CmdConfigBgpPeerGroupTest.cpp | 101 +++++- .../ConfigBgpPeerGroupTest.cpp | 23 ++ fboss/cli/fboss2/tools/bgp_json_to_cli.py | 147 ++------- .../tools/tests/test_bgp_json_to_cli.py | 245 ++++++++++---- 6 files changed, 508 insertions(+), 314 deletions(-) diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 72e57947a5d4d..c9e3db1a9f566 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -412,7 +412,7 @@ const CommandTree& kConfigCommandTree() { "description, peer-tag, ingress-policy, " "egress-policy, rr-client, confed-peer, " "redistribute-peer, enhanced-route-refresh, " - "connect-mode, next-hop-self, add-path send|receive, " + "passive, next-hop-self, add-path send|receive, " "afi disable-ipv4-afi|disable-ipv6-afi|" "ipv4-over-ipv6-nh, " "graceful-restart restart-time|stateful-ha, " diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp index a9eede5cdd694..4a6da72e65737 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp @@ -47,7 +47,7 @@ constexpr std::string_view kEgressPolicy = "egress-policy"; constexpr std::string_view kRrClient = "rr-client"; constexpr std::string_view kRedistributePeer = "redistribute-peer"; constexpr std::string_view kEnhancedRouteRefresh = "enhanced-route-refresh"; -constexpr std::string_view kConnectMode = "connect-mode"; +constexpr std::string_view kPassive = "passive"; constexpr std::string_view kAddPathSend = "add-path send"; constexpr std::string_view kAddPathReceive = "add-path receive"; constexpr std::string_view kAfiDisableIpv4Afi = "afi disable-ipv4-afi"; @@ -79,11 +79,14 @@ constexpr std::string_view kMaxRoutePreWarningOnly = constexpr std::string_view kMaxRoutePostWarningOnly = "max-route post-warning-only"; -// connect-mode values -constexpr std::string_view kConnectModePassive = "PASSIVE"; -constexpr std::string_view kConnectModeActive = "ACTIVE"; -constexpr std::string_view kConnectModeBoth = "BOTH"; +// Protocol ranges bgpd enforces (or the wire format silently truncates to): +// the OPEN hold time is 16 bits and bgpd refuses 1-2s; the graceful-restart +// restart time is the 12-bit field of RFC 4724. +constexpr int32_t kHoldTimeMinSeconds = 3; +constexpr int32_t kHoldTimeMaxSeconds = 65535; +constexpr int32_t kGracefulRestartMaxSeconds = 4095; +using BgpConfig = bgp::thrift::BgpConfig; using PeerGroup = bgp::thrift::PeerGroup; using bgpcli::asnAttr; using bgpcli::AttrHandler; @@ -95,6 +98,7 @@ using bgpcli::parseBool; using bgpcli::Result; using bgpcli::routeCountAttr; using bgpcli::secondsAttr; +using bgpcli::SecondsRange; using bgpcli::stringAttr; using bgpcli::Tokens; using facebook::neteng::fboss::bgp_attr::AddPath; @@ -145,6 +149,13 @@ void setEnhancedRouteRefresh(PeerGroup& g, bool v) { g.enhanced_route_refresh() = v; } +// bgpd reads is_passive=true as PASSIVE_ONLY (listen) and false as +// PASSIVE_ACTIVE (listen and connect); there is no active-only mode, so the +// CLI exposes the flag itself rather than inventing connect-mode names. +void setPassive(PeerGroup& g, bool v) { + g.is_passive() = v; +} + void setDisableIpv4Afi(PeerGroup& g, bool v) { g.disable_ipv4_afi() = v; } @@ -211,48 +222,56 @@ void setNextHopSelf(PeerGroup& g, bool v) { g.next_hop_self() = v; } -// ---- hand-written handlers ------------------------------------------------- -// Only for value shapes no factory covers, because the shape is unique to this -// attribute rather than reusable. Both mirror the neighbor dispatcher's -// versions, which write the identically-named BgpPeer fields. - -// connect-mode: the thrift model only has is_passive (listen vs actively -// connect), so this is not an enum mapping — BOTH has no representation and is -// rejected with an explanation rather than silently mapped. -Result connectMode(PeerGroup& group, const Tokens& values) { - if (values.size() != 1) { - return err( - fmt::format( - "Error: {} requires <{}|{}>", - kConnectMode, - kConnectModePassive, - kConnectModeActive)); - } - if (values[0] == kConnectModePassive) { - group.is_passive() = true; - } else if (values[0] == kConnectModeActive) { - group.is_passive() = false; - } else if (values[0] == kConnectModeBoth) { - return err( - fmt::format( - "Error: {} {} has no representation in bgp_config.thrift " - "(PeerGroup only models is_passive); use {} or {}", - kConnectMode, - kConnectModeBoth, - kConnectModePassive, - kConnectModeActive)); - } else { - return err( - fmt::format( - "Error: Invalid {} value '{}'; expected {} or {}", - kConnectMode, - values[0], - kConnectModePassive, - kConnectModeActive)); +// ---- inherited values ------------------------------------------------------ +// bgpd applies a peer group's bgp_peer_timers to its members as a WHOLE +// struct: once the group carries one, every member takes hold, keepalive and +// out-delay from it, and hold_time_seconds / keep_alive_seconds are +// non-optional i32 (default 0). Hold time 0 means no keepalives, so a timers +// struct created for one field would silently disable dead-peer detection for +// the whole group. Before the first timer attribute is set on a group, the +// struct is therefore seeded from what its members use today -- the global +// hold time, with the conventional hold/3 keepalive -- and the requested field +// is then changed on top of it. pre_filter / post_filter need no seeding: a +// group has nothing above it to inherit a RouteLimit from, and the setter's +// ensure() yields the thrift defaults, which is also what bgpd applies to a +// peer without one. + +void seedTimers(const BgpConfig& cfg, PeerGroup& group) { + if (group.bgp_peer_timers()) { + return; } - return ok(fmt::format("Successfully set {} to: {}", kConnectMode, values[0])); + bgp::thrift::BgpPeerTimers timers; + timers.hold_time_seconds() = *cfg.hold_time(); + timers.keep_alive_seconds() = *cfg.hold_time() / 3; + group.bgp_peer_timers() = std::move(timers); +} + +// A peer-group handler sees the whole config (for the global defaults) plus +// the group being edited. The shared factories only know the group, so they +// are adapted; the adapter for timer attributes seeds first. +using PeerGroupHandler = + std::function; + +PeerGroupHandler groupOnly(AttrHandler handler) { + return [handler = std::move(handler)]( + const BgpConfig&, PeerGroup& group, const Tokens& values) { + return handler(group, values); + }; +} + +PeerGroupHandler withTimers(AttrHandler handler) { + return [handler = std::move(handler)]( + const BgpConfig& cfg, PeerGroup& group, const Tokens& values) { + seedTimers(cfg, group); + return handler(group, values); + }; } +// ---- hand-written handlers ------------------------------------------------- +// Only for value shapes no factory covers, because the shape is unique to this +// attribute rather than reusable. Mirrors the neighbor dispatcher's version, +// which writes the identically-named BgpPeer field. + // add-path send/receive: a boolean at the CLI, but the two attributes share // one thrift field — add_path is a single enum whose values form a bitmask by // design (RECEIVE=1, SEND=2, BOTH=3). Each takes the boolAttr text but merges @@ -298,73 +317,102 @@ Result addPathReceive(PeerGroup& group, const Tokens& values) { // handler bodies here by design — if an attribute appears to need one, its // value shape is missing a factory and the fix is to add the factory. -const std::map, std::less<>>& -attrHandlers() { - static const std::map, std::less<>> - kHandlers = { +const std::map>& attrHandlers() { + auto plain = [](AttrHandler h) { return groupOnly(std::move(h)); }; + auto timers = [](AttrHandler h) { + return withTimers(std::move(h)); + }; + static const std::map> kHandlers = + { {std::string(kRemoteAsn), - asnAttr(kRemoteAsn, setRemoteAsn)}, - {std::string(kLocalAsn), asnAttr(kLocalAsn, setLocalAsn)}, + plain(asnAttr(kRemoteAsn, setRemoteAsn))}, + {std::string(kLocalAsn), + plain(asnAttr(kLocalAsn, setLocalAsn))}, {std::string(kDescription), - joinedStringAttr(kDescription, setDescription)}, + plain(joinedStringAttr(kDescription, setDescription))}, {std::string(kPeerTag), - stringAttr(kPeerTag, "string", setPeerTag)}, + plain(stringAttr(kPeerTag, "string", setPeerTag))}, {std::string(kIngressPolicy), - stringAttr( - kIngressPolicy, "policy-name", setIngressPolicy)}, + plain( + stringAttr( + kIngressPolicy, "policy-name", setIngressPolicy))}, {std::string(kEgressPolicy), - stringAttr( - kEgressPolicy, "policy-name", setEgressPolicy)}, - {std::string(kRrClient), boolAttr(kRrClient, setRrClient)}, + plain( + stringAttr( + kEgressPolicy, "policy-name", setEgressPolicy))}, + {std::string(kRrClient), + plain(boolAttr(kRrClient, setRrClient))}, {std::string(kConfedPeer), - boolAttr(kConfedPeer, setConfedPeer)}, + plain(boolAttr(kConfedPeer, setConfedPeer))}, {std::string(kRedistributePeer), - boolAttr(kRedistributePeer, setRedistributePeer)}, + plain(boolAttr(kRedistributePeer, setRedistributePeer))}, {std::string(kEnhancedRouteRefresh), - boolAttr(kEnhancedRouteRefresh, setEnhancedRouteRefresh)}, - {std::string(kConnectMode), connectMode}, - {std::string(kAddPathSend), addPathSend}, - {std::string(kAddPathReceive), addPathReceive}, + plain( + boolAttr( + kEnhancedRouteRefresh, setEnhancedRouteRefresh))}, + {std::string(kPassive), + plain(boolAttr(kPassive, setPassive))}, + {std::string(kAddPathSend), plain(addPathSend)}, + {std::string(kAddPathReceive), plain(addPathReceive)}, {std::string(kAfiDisableIpv4Afi), - boolAttr(kAfiDisableIpv4Afi, setDisableIpv4Afi)}, + plain(boolAttr(kAfiDisableIpv4Afi, setDisableIpv4Afi))}, {std::string(kAfiDisableIpv6Afi), - boolAttr(kAfiDisableIpv6Afi, setDisableIpv6Afi)}, + plain(boolAttr(kAfiDisableIpv6Afi, setDisableIpv6Afi))}, {std::string(kAfiIpv4OverIpv6Nh), - boolAttr(kAfiIpv4OverIpv6Nh, setIpv4OverIpv6Nh)}, + plain(boolAttr(kAfiIpv4OverIpv6Nh, setIpv4OverIpv6Nh))}, {std::string(kGracefulRestartTime), - secondsAttr( - kGracefulRestartTime, setGracefulRestartTime)}, + timers( + secondsAttr( + kGracefulRestartTime, + setGracefulRestartTime, + SecondsRange{0, kGracefulRestartMaxSeconds}))}, {std::string(kGracefulRestartStatefulHa), - boolAttr( - kGracefulRestartStatefulHa, setGracefulRestartStatefulHa)}, + plain( + boolAttr( + kGracefulRestartStatefulHa, setGracefulRestartStatefulHa))}, {std::string(kMaxRoutePreFilter), - routeCountAttr(kMaxRoutePreFilter, setMaxRoutePreFilter)}, + plain( + routeCountAttr( + kMaxRoutePreFilter, setMaxRoutePreFilter))}, {std::string(kMaxRoutePostFilter), - routeCountAttr( - kMaxRoutePostFilter, setMaxRoutePostFilter)}, + plain( + routeCountAttr( + kMaxRoutePostFilter, setMaxRoutePostFilter))}, {std::string(kMaxRoutePreWarningThreshold), - routeCountAttr( - kMaxRoutePreWarningThreshold, setMaxRoutePreWarningThreshold)}, + plain( + routeCountAttr( + kMaxRoutePreWarningThreshold, + setMaxRoutePreWarningThreshold))}, {std::string(kMaxRoutePostWarningThreshold), - routeCountAttr( - kMaxRoutePostWarningThreshold, setMaxRoutePostWarningThreshold)}, + plain( + routeCountAttr( + kMaxRoutePostWarningThreshold, + setMaxRoutePostWarningThreshold))}, {std::string(kMaxRoutePreWarningOnly), - boolAttr( - kMaxRoutePreWarningOnly, setMaxRoutePreWarningOnly)}, + plain( + boolAttr( + kMaxRoutePreWarningOnly, setMaxRoutePreWarningOnly))}, {std::string(kMaxRoutePostWarningOnly), - boolAttr( - kMaxRoutePostWarningOnly, setMaxRoutePostWarningOnly)}, + plain( + boolAttr( + kMaxRoutePostWarningOnly, setMaxRoutePostWarningOnly))}, {std::string(kTimersHoldTime), - secondsAttr(kTimersHoldTime, setTimersHoldTime)}, + timers( + secondsAttr( + kTimersHoldTime, + setTimersHoldTime, + SecondsRange{kHoldTimeMinSeconds, kHoldTimeMaxSeconds}))}, {std::string(kTimersKeepalive), - secondsAttr(kTimersKeepalive, setTimersKeepalive)}, + timers( + secondsAttr(kTimersKeepalive, setTimersKeepalive))}, {std::string(kTimersOutDelay), - secondsAttr(kTimersOutDelay, setTimersOutDelay)}, + timers(secondsAttr(kTimersOutDelay, setTimersOutDelay))}, {std::string(kTimersWithdrawUnprogDelay), - secondsAttr( - kTimersWithdrawUnprogDelay, setTimersWithdrawUnprogDelay)}, + timers( + secondsAttr( + kTimersWithdrawUnprogDelay, setTimersWithdrawUnprogDelay))}, {std::string(kNextHopSelf), - boolAttr(kNextHopSelf, setNextHopSelf)}, + plain(boolAttr(kNextHopSelf, setNextHopSelf))}, }; return kHandlers; } @@ -380,39 +428,20 @@ std::string validAttrList() { return out; } -// Find the peer-group keyed by name, creating it if absent. Setting an -// attribute on a not-yet-created group implicitly creates it, so command -// ordering stays forgiving; a bare `peer-group ` creates one explicitly. -// Unlike a BgpPeer, PeerGroup's only non-optional field is `name`, so no -// address seeding is needed. -bgp::thrift::PeerGroup& findOrCreatePeerGroup( - bgp::thrift::BgpConfig& cfg, +// The group named `groupName`, or end(). Setting an attribute on a +// not-yet-created group implicitly creates it, so command ordering stays +// forgiving; a bare `peer-group ` creates one explicitly. Unlike a +// BgpPeer, PeerGroup's only non-optional field is `name`, so a new group needs +// no address seeding. +std::vector::iterator findPeerGroup( + std::vector& groups, const std::string& groupName) { - auto& groups = cfg.peer_groups().ensure(); - for (auto& group : groups) { - if (*group.name() == groupName) { - return group; + for (auto it = groups.begin(); it != groups.end(); ++it) { + if (*it->name() == groupName) { + return it; } } - groups.emplace_back(); - auto& group = groups.back(); - group.name() = groupName; - return group; -} - -// Whether a peer-group with the given name already exists. -bool peerGroupExists( - const bgp::thrift::BgpConfig& cfg, - const std::string& groupName) { - if (!cfg.peer_groups().has_value()) { - return false; - } - for (const auto& group : *cfg.peer_groups()) { - if (*group.name() == groupName) { - return true; - } - } - return false; + return groups.end(); } } // namespace @@ -465,27 +494,38 @@ CmdConfigProtocolBgpPeerGroup::queryClient( const ObjectArgType& args) { auto& session = ConfigSession::getInstance(); auto& cfg = session.getBgpConfig(); - const bool created = !peerGroupExists(cfg, args.groupName()); - auto& group = findOrCreatePeerGroup(cfg, args.groupName()); - + auto& groups = cfg.peer_groups().ensure(); + auto existing = findPeerGroup(groups, args.groupName()); + + // Edit a copy: a rejected value (or a struct seeded for it) must leave the + // in-memory config exactly as it was, since later lookups in the same + // process see it whether or not it was saved. + PeerGroup group; + if (existing != groups.end()) { + group = *existing; + } else { + group.name() = args.groupName(); + } Result result = args.attr().empty() ? ok(fmt::format( "Successfully created BGP peer-group {}", args.groupName())) // The attribute is guaranteed valid: BgpPeerGroupConfig's constructor // rejects an unknown attribute before we get here. - : attrHandlers().find(args.attr())->second(group, args.values()); - if (result.ok) { - if (!args.attr().empty()) { - result.message += fmt::format(" for peer-group {}", args.groupName()); - } - session.saveBgpConfig(); - result.message += - fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); - } else if (created) { - // A rejected value must not leave a half-created group in the in-memory - // config (visible to later lookups in the same process, e.g. tests). - cfg.peer_groups()->pop_back(); + : attrHandlers().find(args.attr())->second(cfg, group, args.values()); + if (!result.ok) { + return result.message; + } + if (existing != groups.end()) { + *existing = std::move(group); + } else { + groups.push_back(std::move(group)); + } + if (!args.attr().empty()) { + result.message += fmt::format(" for peer-group {}", args.groupName()); } + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); return result.message; } diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp index b894f859b1a90..14ff26a2111ba 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp @@ -44,6 +44,10 @@ class CmdConfigBgpPeerGroupTestFixture : public CmdConfigTestBase { return ConfigSession::getInstance().getBgpConfig().peer_groups().ensure(); } + bgp::thrift::BgpConfig& config() { + return ConfigSession::getInstance().getBgpConfig(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -132,18 +136,20 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, addPathMerge) { EXPECT_FALSE(groups()[0].add_path().has_value()); } -TEST_F(CmdConfigBgpPeerGroupTestFixture, connectMode) { - run({"SPINE", "connect-mode", "PASSIVE"}); +TEST_F(CmdConfigBgpPeerGroupTestFixture, passive) { + // is_passive=true is PASSIVE_ONLY in bgpd, false is PASSIVE_ACTIVE (listen + // and connect); the CLI exposes the flag rather than connect-mode names. + run({"SPINE", "passive", "true"}); EXPECT_TRUE(groups()[0].is_passive().value_or(false)); - run({"SPINE", "connect-mode", "ACTIVE"}); + run({"SPINE", "passive", "false"}); EXPECT_FALSE(groups()[0].is_passive().value_or(true)); - // BOTH has no thrift representation and must be rejected, leaving the - // previous value intact and nothing new persisted. - auto result = run({"SPINE", "connect-mode", "BOTH"}); - EXPECT_THAT(result, HasSubstr("Error")); + auto result = run({"SPINE", "passive", "BOTH"}); + EXPECT_THAT(result, HasSubstr("Invalid")); EXPECT_FALSE(groups()[0].is_passive().value_or(true)); + EXPECT_THROW( + run({"SPINE", "connect-mode", "PASSIVE"}), std::invalid_argument); } TEST_F(CmdConfigBgpPeerGroupTestFixture, boolAndStringAttributes) { @@ -250,6 +256,87 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, namedGroupsAreDistinct) { EXPECT_FALSE(groups()[1].local_as_4_byte().has_value()); } +// ============================================================================== +// Inherited values: bgpd applies a group's timers to its members as a whole +// struct, so the first timer attribute must seed the rest from the global +// defaults instead of leaving them at the thrift default of 0. +// ============================================================================== + +TEST_F(CmdConfigBgpPeerGroupTestFixture, timersSeededFromGlobal) { + config().hold_time() = 180; + run({"SPINE", "graceful-restart", "restart-time", "60"}); + + ASSERT_EQ(groups().size(), 1); + const auto& timers = *groups()[0].bgp_peer_timers(); + EXPECT_EQ(*timers.hold_time_seconds(), 180); + EXPECT_EQ(*timers.keep_alive_seconds(), 60); + EXPECT_EQ(*timers.out_delay_seconds(), 0); + EXPECT_EQ(*timers.withdraw_unprog_delay_seconds(), 0); + EXPECT_EQ(timers.graceful_restart_seconds().value_or(0), 60); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, timersNotReseededOncePresent) { + run({"SPINE", "timers", "hold-time", "90"}); + run({"SPINE", "timers", "keepalive", "30"}); + // Seeding happens once: a later global change does not leak into a group + // that already carries its own timers, and other fields are preserved. + config().hold_time() = 180; + run({"SPINE", "timers", "out-delay", "2"}); + + const auto& timers = *groups()[0].bgp_peer_timers(); + EXPECT_EQ(*timers.hold_time_seconds(), 90); + EXPECT_EQ(*timers.keep_alive_seconds(), 30); + EXPECT_EQ(*timers.out_delay_seconds(), 2); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, routeLimitOtherFieldsKeepDefaults) { + // No level above a group to inherit a RouteLimit from: the untouched fields + // take the thrift defaults bgpd applies to a peer without one. + run({"SPINE", "max-route", "pre-warning-only", "true"}); + const auto& pre = *groups()[0].pre_filter(); + EXPECT_EQ(*pre.max_routes(), 12000); + EXPECT_EQ(*pre.warning_limit(), 0); + EXPECT_TRUE(*pre.warning_only()); + EXPECT_FALSE(groups()[0].post_filter().has_value()); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, timerRanges) { + // Hold time: 0 or 3..65535 (bgpd refuses 1-2; the OPEN field is 16 bits). + EXPECT_THAT(run({"SPINE", "timers", "hold-time", "1"}), HasSubstr("Error")); + EXPECT_THAT(run({"SPINE", "timers", "hold-time", "2"}), HasSubstr("Error")); + EXPECT_THAT( + run({"SPINE", "timers", "hold-time", "65536"}), HasSubstr("Error")); + EXPECT_TRUE(groups().empty()) + << "rejected first attribute must not leave a half-created group"; + run({"SPINE", "timers", "hold-time", "0"}); + EXPECT_EQ(*groups()[0].bgp_peer_timers()->hold_time_seconds(), 0); + run({"SPINE", "timers", "hold-time", "3"}); + EXPECT_EQ(*groups()[0].bgp_peer_timers()->hold_time_seconds(), 3); + run({"SPINE", "timers", "hold-time", "65535"}); + EXPECT_EQ(*groups()[0].bgp_peer_timers()->hold_time_seconds(), 65535); + + // Graceful-restart time: 0..4095 (12-bit field, RFC 4724). + EXPECT_THAT( + run({"SPINE", "graceful-restart", "restart-time", "4096"}), + HasSubstr("Error")); + EXPECT_FALSE(groups()[0].bgp_peer_timers()->graceful_restart_seconds()); + run({"SPINE", "graceful-restart", "restart-time", "4095"}); + EXPECT_EQ( + groups()[0].bgp_peer_timers()->graceful_restart_seconds().value_or(0), + 4095); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, rejectedValueLeavesGroupUntouched) { + run({"SPINE", "remote-asn", "65000"}); + // A rejected timer must not leave behind the timers struct that was seeded + // for it: that would silently pin the group's members to the current + // global hold time. + EXPECT_THAT(run({"SPINE", "timers", "hold-time", "2"}), HasSubstr("Error")); + ASSERT_EQ(groups().size(), 1); + EXPECT_FALSE(groups()[0].bgp_peer_timers().has_value()); + EXPECT_EQ(groups()[0].remote_as_4_byte().value_or(0), 65000); +} + // ============================================================================== // Reject paths — the error is surfaced and nothing is persisted // ============================================================================== diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp index 544a437efea22..92086d85316c7 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp @@ -104,6 +104,29 @@ TEST_F(ConfigBgpPeerGroupTest, SetTimersHoldTimeAndCommit) { << "bgpd's running config has no peer-group " << kGroup; ASSERT_TRUE(group->count("bgp_peer_timers")); EXPECT_EQ((*group)["bgp_peer_timers"]["hold_time_seconds"].asInt(), 90); + // bgpd applies the group's timers struct to its members as a whole, so the + // fields the user did not set must carry the global defaults (hold/3 + // keepalive), not the thrift default of 0 that would disable keepalives. + ASSERT_TRUE(running.count("hold_time")); + EXPECT_EQ( + (*group)["bgp_peer_timers"]["keep_alive_seconds"].asInt(), + running["hold_time"].asInt() / 3); +} + +TEST_F(ConfigBgpPeerGroupTest, SetPassiveAndCommit) { + discardSession(); + clearBgpSession(); + stagePeerGroup({kGroup, "passive", "true"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + auto running = readRunningBgpConfigViaRpc(); + const auto* group = findGroup(running, kGroup); + ASSERT_NE(group, nullptr) + << "bgpd's running config has no peer-group " << kGroup; + ASSERT_TRUE(group->count("is_passive")); + EXPECT_TRUE((*group)["is_passive"].asBool()); } TEST_F(ConfigBgpPeerGroupTest, DeleteGroupAndCommit) { diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index ac61ef33f2c3a..a5fcf94dc0ac4 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -114,121 +114,6 @@ def generate_global_commands(config: dict[str, Any]) -> list[str]: return commands -def _generate_peer_group_basic_commands( - 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: - commands.append( - f"config protocol bgp peer-group {escaped_name} remote-asn {escape_shell_arg(peer_group['remote_as_4_byte'])}" - ) - if "description" in peer_group: - commands.append( - f"config protocol bgp peer-group {escaped_name} description {escape_shell_arg(peer_group['description'])}" - ) - if "ingress_policy_name" in peer_group: - commands.append( - f"config protocol bgp peer-group {escaped_name} ingress-policy {escape_shell_arg(peer_group['ingress_policy_name'])}" - ) - if "egress_policy_name" in peer_group: - commands.append( - f"config protocol bgp peer-group {escaped_name} egress-policy {escape_shell_arg(peer_group['egress_policy_name'])}" - ) - if "peer_tag" in peer_group: - commands.append( - f"config protocol bgp peer-group {escaped_name} peer-tag {escape_shell_arg(peer_group['peer_tag'])}" - ) - return commands - - -def _generate_peer_group_bool_commands( - peer_group: dict[str, Any], escaped_name: str -) -> list[str]: - """Generate boolean flag commands for peer-group.""" - commands = [] - bool_fields = [ - ("is_rr_client", "rr-client"), - ("next_hop_self", "next-hop-self"), - ("is_confed_peer", "confed-peer"), - ("v4_over_v6_nexthop", "v4-over-v6-nh"), - ("disable_ipv4_afi", "disable-ipv4-afi"), - ] - for field, cli_name in bool_fields: - if field in peer_group: - commands.append( - f"config protocol bgp peer-group {escaped_name} {cli_name} {_shell_bool(peer_group[field])}" - ) - return commands - - -def _generate_peer_group_timer_commands( - timers: dict[str, Any], escaped_name: str -) -> list[str]: - """Generate timer commands for peer-group.""" - commands = [] - if timers.get("hold_time_seconds"): - commands.append( - f"config protocol bgp peer-group {escaped_name} timers hold-time {escape_shell_arg(timers['hold_time_seconds'])}" - ) - if timers.get("keep_alive_seconds"): - commands.append( - f"config protocol bgp peer-group {escaped_name} timers keepalive {escape_shell_arg(timers['keep_alive_seconds'])}" - ) - if "out_delay_seconds" in timers: - commands.append( - f"config protocol bgp peer-group {escaped_name} timers out-delay {escape_shell_arg(timers['out_delay_seconds'])}" - ) - if "withdraw_unprog_delay_seconds" in timers: - commands.append( - f"config protocol bgp peer-group {escaped_name} timers withdraw-unprog-delay {escape_shell_arg(timers['withdraw_unprog_delay_seconds'])}" - ) - return commands - - -def _generate_peer_group_prefilter_commands( - pre_filter: dict[str, Any], escaped_name: str -) -> list[str]: - """Generate pre_filter commands for peer-group.""" - commands = [] - if pre_filter.get("max_routes"): - commands.append( - f"config protocol bgp peer-group {escaped_name} max-routes {escape_shell_arg(pre_filter['max_routes'])}" - ) - if "warning_limit" in pre_filter: - commands.append( - f"config protocol bgp peer-group {escaped_name} warning-limit {escape_shell_arg(pre_filter['warning_limit'])}" - ) - if "warning_only" in pre_filter: - commands.append( - f"config protocol bgp peer-group {escaped_name} warning-only {_shell_bool(pre_filter['warning_only'])}" - ) - return commands - - -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: - return [] - - escaped_name = escape_shell_arg(name) - commands = [] - commands.extend(_generate_peer_group_basic_commands(peer_group, escaped_name)) - commands.extend(_generate_peer_group_bool_commands(peer_group, escaped_name)) - commands.extend( - _generate_peer_group_timer_commands( - peer_group.get("bgp_peer_timers", {}), escaped_name - ) - ) - commands.extend( - _generate_peer_group_prefilter_commands( - peer_group.get("pre_filter", {}), escaped_name - ) - ) - return commands - - def format_bandwidth(bps: int) -> str: """Format bandwidth in bps with the largest exact K/M/G suffix. @@ -480,6 +365,38 @@ def generate_peer_commands(peer: dict[str, Any]) -> list[str]: return commands +# (json field, CLI attribute) — PeerGroup-only fields whose value maps 1:1 +# onto a peer-group attribute token. +_PEER_GROUP_SCALAR_FIELDS = [ + ("remote_as_4_byte", "remote-asn"), + ("local_as_4_byte", "local-asn"), + ("description", "description"), + ("peer_tag", "peer-tag"), + ("ingress_policy_name", "ingress-policy"), + ("egress_policy_name", "egress-policy"), +] + + +def generate_peer_group_commands(peer_group: dict[str, Any]) -> list[str]: + """Generate `config protocol bgp peer-group` CLI commands for a peer group. + + The peer-group and neighbor dispatchers share one attribute grammar for + the fields both thrift structs carry, so the per-shape generators are + shared; only the scalar field list differs. + """ + name = peer_group.get("name", "") + if not name: + return [] + + prefix = f"config protocol bgp peer-group {escape_shell_arg(name)}" + commands = [] + commands.extend( + _generate_scalar_commands(peer_group, prefix, _PEER_GROUP_SCALAR_FIELDS) + ) + commands.extend(_generate_session_commands(peer_group, prefix)) + return commands + + def generate_exec_commands(commands: list[str], binary: str = "fboss2") -> list[str]: """Generate executable commands with custom binary.""" exec_commands = [ 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 0cdbb60d3e26c..81791c4b49f0c 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 @@ -233,7 +233,9 @@ def test_empty_config(self) -> None: class GeneratePeerGroupCommandsTest(unittest.TestCase): - """Tests for generate_peer_group_commands function.""" + """Tests for generate_peer_group_commands (peer-group grammar).""" + + PREFIX = "config protocol bgp peer-group TEST" def test_empty_name_returns_empty(self) -> None: """Peer group without name should return empty list.""" @@ -249,42 +251,68 @@ def test_remote_asn(self) -> None: "config protocol bgp peer-group TEST-GROUP remote-asn 65000", commands ) + def test_local_asn(self) -> None: + """local_as_4_byte should generate local-asn command.""" + peer_group = {"name": "TEST", "local_as_4_byte": 64512} + commands = generate_peer_group_commands(peer_group) + self.assertIn(f"{self.PREFIX} local-asn 64512", commands) + def test_description_with_spaces(self) -> None: - """Description with spaces should be properly quoted.""" + """Description with spaces should be quoted.""" peer_group = {"name": "TEST", "description": "Test peer group"} commands = generate_peer_group_commands(peer_group) - self.assertIn( - "config protocol bgp peer-group TEST description 'Test peer group'", - commands, - ) + self.assertIn(f"{self.PREFIX} description 'Test peer group'", commands) def test_boolean_flag_true(self) -> None: - """Boolean flag with true value should generate 'true'.""" + """Boolean true should generate 'true'.""" peer_group = {"name": "TEST", "next_hop_self": True} commands = generate_peer_group_commands(peer_group) - self.assertIn( - "config protocol bgp peer-group TEST next-hop-self true", commands - ) + self.assertIn(f"{self.PREFIX} next-hop-self true", commands) def test_boolean_flag_false(self) -> None: - """Boolean flag with false value should generate 'false' (not omitted).""" + """Boolean false should generate 'false' (not be omitted).""" peer_group = {"name": "TEST", "disable_ipv4_afi": False} commands = generate_peer_group_commands(peer_group) - self.assertIn( - "config protocol bgp peer-group TEST disable-ipv4-afi false", commands - ) + self.assertIn(f"{self.PREFIX} afi disable-ipv4-afi false", commands) + + def test_afi_flags(self) -> None: + """AFI flags use the two-token `afi ...` attributes.""" + peer_group = { + "name": "TEST", + "disable_ipv4_afi": True, + "disable_ipv6_afi": False, + "v4_over_v6_nexthop": True, + } + commands = generate_peer_group_commands(peer_group) + self.assertIn(f"{self.PREFIX} afi disable-ipv4-afi true", commands) + self.assertIn(f"{self.PREFIX} afi disable-ipv6-afi false", commands) + self.assertIn(f"{self.PREFIX} afi ipv4-over-ipv6-nh true", commands) - def test_is_rr_client(self) -> None: - """is_rr_client should generate rr-client command.""" - peer_group = {"name": "TEST", "is_rr_client": True} + def test_session_flags(self) -> None: + """Session flags map onto their single-token boolean attributes.""" + peer_group = { + "name": "TEST", + "is_rr_client": True, + "is_confed_peer": True, + "is_redistribute_peer": False, + "enhanced_route_refresh": True, + "enable_stateful_ha": True, + } commands = generate_peer_group_commands(peer_group) - self.assertIn("config protocol bgp peer-group TEST rr-client true", commands) + self.assertIn(f"{self.PREFIX} rr-client true", commands) + self.assertIn(f"{self.PREFIX} confed-peer true", commands) + self.assertIn(f"{self.PREFIX} redistribute-peer false", commands) + self.assertIn(f"{self.PREFIX} enhanced-route-refresh true", commands) + self.assertIn(f"{self.PREFIX} graceful-restart stateful-ha true", commands) - def test_is_confed_peer(self) -> None: - """is_confed_peer should generate confed-peer command.""" - peer_group = {"name": "TEST", "is_confed_peer": True} + def test_is_passive(self) -> None: + """is_passive should generate the passive flag.""" + peer_group = {"name": "TEST", "is_passive": True} + commands = generate_peer_group_commands(peer_group) + self.assertIn(f"{self.PREFIX} passive true", commands) + peer_group["is_passive"] = False commands = generate_peer_group_commands(peer_group) - self.assertIn("config protocol bgp peer-group TEST confed-peer true", commands) + self.assertIn(f"{self.PREFIX} passive false", commands) def test_policies(self) -> None: """Policy names should generate correct commands.""" @@ -294,23 +322,22 @@ def test_policies(self) -> None: "egress_policy_name": "OUT_POLICY", } commands = generate_peer_group_commands(peer_group) - self.assertIn( - "config protocol bgp peer-group TEST ingress-policy IN_POLICY", commands - ) - self.assertIn( - "config protocol bgp peer-group TEST egress-policy OUT_POLICY", commands - ) + self.assertIn(f"{self.PREFIX} ingress-policy IN_POLICY", commands) + self.assertIn(f"{self.PREFIX} egress-policy OUT_POLICY", commands) - def test_v4_over_v6_nexthop(self) -> None: - """v4_over_v6_nexthop should generate v4-over-v6-nh command.""" - peer_group = {"name": "TEST", "v4_over_v6_nexthop": True} + def test_add_path(self) -> None: + """add_path BOTH should expand to both directions; RECEIVE to one.""" + peer_group = {"name": "TEST", "add_path": 3} commands = generate_peer_group_commands(peer_group) - self.assertIn( - "config protocol bgp peer-group TEST v4-over-v6-nh true", commands - ) + self.assertIn(f"{self.PREFIX} add-path receive true", commands) + self.assertIn(f"{self.PREFIX} add-path send true", commands) + peer_group["add_path"] = "RECEIVE" + commands = generate_peer_group_commands(peer_group) + self.assertIn(f"{self.PREFIX} add-path receive true", commands) + self.assertNotIn(f"{self.PREFIX} add-path send true", commands) def test_timers(self) -> None: - """Timer fields should generate correct commands.""" + """Timers, including zero values, should generate correct commands.""" peer_group = { "name": "TEST", "bgp_peer_timers": { @@ -318,44 +345,142 @@ def test_timers(self) -> None: "keep_alive_seconds": 10, "out_delay_seconds": 5, "withdraw_unprog_delay_seconds": 0, + "graceful_restart_seconds": 120, }, } commands = generate_peer_group_commands(peer_group) - self.assertIn( - "config protocol bgp peer-group TEST timers hold-time 30", commands - ) - self.assertIn( - "config protocol bgp peer-group TEST timers keepalive 10", commands - ) - self.assertIn( - "config protocol bgp peer-group TEST timers out-delay 5", commands - ) - self.assertIn( - "config protocol bgp peer-group TEST timers withdraw-unprog-delay 0", - commands, - ) + self.assertIn(f"{self.PREFIX} timers hold-time 30", commands) + self.assertIn(f"{self.PREFIX} timers keepalive 10", commands) + self.assertIn(f"{self.PREFIX} timers out-delay 5", commands) + self.assertIn(f"{self.PREFIX} timers withdraw-unprog-delay 0", commands) + self.assertIn(f"{self.PREFIX} graceful-restart restart-time 120", commands) + + def test_hold_time_zero_preserved(self) -> None: + """hold_time_seconds: 0 is a real value (no keepalives), not 'unset'.""" + peer_group = {"name": "TEST", "bgp_peer_timers": {"hold_time_seconds": 0}} + commands = generate_peer_group_commands(peer_group) + self.assertIn(f"{self.PREFIX} timers hold-time 0", commands) - def test_pre_filter_max_routes(self) -> None: - """pre_filter.max_routes should generate max-routes command.""" - peer_group = {"name": "TEST", "pre_filter": {"max_routes": 45000}} + def test_pre_filter(self) -> None: + """pre_filter maps onto the max-route pre-* attributes.""" + peer_group = { + "name": "TEST", + "pre_filter": { + "max_routes": 45000, + "warning_limit": 100, + "warning_only": True, + }, + } commands = generate_peer_group_commands(peer_group) - self.assertIn("config protocol bgp peer-group TEST max-routes 45000", commands) + self.assertIn(f"{self.PREFIX} max-route pre-filter 45000", commands) + self.assertIn(f"{self.PREFIX} max-route pre-warning-threshold 100", commands) + self.assertIn(f"{self.PREFIX} max-route pre-warning-only true", commands) - def test_pre_filter_warning_settings(self) -> None: - """pre_filter warning settings should generate correct commands.""" + def test_post_filter(self) -> None: + """post_filter maps onto the max-route post-* attributes.""" peer_group = { "name": "TEST", - "pre_filter": {"warning_limit": 100, "warning_only": True}, + "post_filter": { + "max_routes": 50000, + "warning_limit": 48000, + "warning_only": False, + }, } commands = generate_peer_group_commands(peer_group) - self.assertIn("config protocol bgp peer-group TEST warning-limit 100", commands) - self.assertIn("config protocol bgp peer-group TEST warning-only true", commands) + self.assertIn(f"{self.PREFIX} max-route post-filter 50000", commands) + self.assertIn(f"{self.PREFIX} max-route post-warning-threshold 48000", commands) + self.assertIn(f"{self.PREFIX} max-route post-warning-only false", commands) def test_peer_tag(self) -> None: """peer_tag should generate peer-tag command.""" peer_group = {"name": "TEST", "peer_tag": "FSW"} commands = generate_peer_group_commands(peer_group) - self.assertIn("config protocol bgp peer-group TEST peer-tag FSW", commands) + self.assertIn(f"{self.PREFIX} peer-tag FSW", commands) + + def test_complete_peer_group_loses_no_field(self) -> None: + """Every CLI-settable PeerGroup field produces exactly one command, in + the grammar the peer-group dispatcher accepts.""" + peer_group = { + "name": "TEST", + "remote_as_4_byte": 65000, + "local_as_4_byte": 64512, + "description": "spine group", + "peer_tag": "FSW", + "ingress_policy_name": "IN_POLICY", + "egress_policy_name": "OUT_POLICY", + "is_passive": True, + "is_rr_client": True, + "is_confed_peer": False, + "is_redistribute_peer": True, + "enhanced_route_refresh": True, + "disable_ipv4_afi": True, + "disable_ipv6_afi": False, + "v4_over_v6_nexthop": True, + "enable_stateful_ha": True, + "next_hop_self": True, + "add_path": 3, + "bgp_peer_timers": { + "hold_time_seconds": 90, + "keep_alive_seconds": 30, + "out_delay_seconds": 5, + "withdraw_unprog_delay_seconds": 10, + "graceful_restart_seconds": 120, + }, + "pre_filter": { + "max_routes": 45000, + "warning_limit": 40000, + "warning_only": True, + }, + "post_filter": { + "max_routes": 50000, + "warning_limit": 48000, + "warning_only": False, + }, + } + expected = [ + "remote-asn 65000", + "local-asn 64512", + "description 'spine group'", + "peer-tag FSW", + "ingress-policy IN_POLICY", + "egress-policy OUT_POLICY", + "passive true", + "rr-client true", + "confed-peer false", + "redistribute-peer true", + "enhanced-route-refresh true", + "afi disable-ipv4-afi true", + "afi disable-ipv6-afi false", + "afi ipv4-over-ipv6-nh true", + "graceful-restart stateful-ha true", + "next-hop-self true", + "add-path receive true", + "add-path send true", + "timers hold-time 90", + "timers keepalive 30", + "timers out-delay 5", + "timers withdraw-unprog-delay 10", + "graceful-restart restart-time 120", + "max-route pre-filter 45000", + "max-route pre-warning-threshold 40000", + "max-route pre-warning-only true", + "max-route post-filter 50000", + "max-route post-warning-threshold 48000", + "max-route post-warning-only false", + ] + commands = generate_peer_group_commands(peer_group) + self.assertEqual(commands, [f"{self.PREFIX} {e}" for e in expected]) + # One command per leaf field: nothing in the input was dropped. + leaf_fields = ( + len(peer_group) + - 1 # name is the key, not an attribute + - 3 # the three nested structs are counted by their leaves below + + 1 # add_path BOTH expands to two commands + + len(peer_group["bgp_peer_timers"]) + + len(peer_group["pre_filter"]) + + len(peer_group["post_filter"]) + ) + self.assertEqual(len(commands), leaf_fields) class GeneratePeerCommandsTest(unittest.TestCase): @@ -849,7 +974,9 @@ def test_full_rsw_config(self) -> None: # Verify peer group commands self.assertIn("peer-group RSW-FSW-V6 next-hop-self true", joined) self.assertIn("peer-group RSW-FSW-V6 confed-peer true", joined) - self.assertIn("peer-group RSW-RTSW-V6 disable-ipv4-afi true", joined) + self.assertIn("peer-group RSW-FSW-V6 afi ipv4-over-ipv6-nh true", joined) + self.assertIn("peer-group RSW-FSW-V6 timers hold-time 30", joined) + self.assertIn("peer-group RSW-RTSW-V6 afi disable-ipv4-afi true", joined) # Verify peer commands self.assertIn("neighbor 2401:db00:501c::/64 remote-asn 65000", joined) From a0acc2a5c41ee016b01bf7610a0ef1fbd902742e Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 10 Sep 2026 07:37:15 +0000 Subject: [PATCH 6/9] [Nexthop] Seed unlimited peer-group route limit, close the attribute gap, flag unconverted j2c fields Route limits: a group with no RouteLimit imposes none on its members (bgpd skips the check when the struct is absent), yet the first warning-only or threshold edit created the struct with the thrift default max_routes 12000, a hard cap with session teardown the operator never asked for. The struct is now seeded {max_routes 0, warning_only false, warning_limit 0} (0 = unlimited in bgpd) and only the requested field changes; a rejected value leaves no struct behind. Attributes: add every remaining PeerGroup field bgpd resolves peer > group: remove-private-as, enforce-first-as, route-refresh, ttl-security-hops (1..255), link-bandwidth, advertise-lbw, receive-lbw -- same spellings and validation as the neighbor command. Document the fields deliberately not exposed because bgpd never reads them from a group (local_addr, next_hop4/6, enabled, router_port_id, graceful_restart_end_of_rib_seconds). bgp_json_to_cli.py: the peer-group generator covers the new attributes via the shared tables, maps the deprecated i32 ASNs to the 4-byte attribute, and emits a `# WARNING` line (script + stderr) for any field with no CLI equivalent instead of dropping it. test_complete_peer_group_loses_no_field now asserts 36 commands. --- .../CmdConfigProtocolBgpPeerGroup.cpp | 133 ++++++++++++++++-- fboss/cli/fboss2/docs/BGP_CLI_TEST_PLAN.md | 10 +- .../test/config/CmdConfigBgpPeerGroupTest.cpp | 82 ++++++++++- .../ConfigBgpPeerGroupTest.cpp | 41 ++++++ fboss/cli/fboss2/tools/bgp_json_to_cli.py | 18 ++- .../tools/tests/test_bgp_json_to_cli.py | 74 ++++++++++ 6 files changed, 333 insertions(+), 25 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp index 4a6da72e65737..6393f4f3a1e14 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp @@ -47,6 +47,13 @@ constexpr std::string_view kEgressPolicy = "egress-policy"; constexpr std::string_view kRrClient = "rr-client"; constexpr std::string_view kRedistributePeer = "redistribute-peer"; constexpr std::string_view kEnhancedRouteRefresh = "enhanced-route-refresh"; +constexpr std::string_view kRouteRefresh = "route-refresh"; +constexpr std::string_view kRemovePrivateAs = "remove-private-as"; +constexpr std::string_view kEnforceFirstAs = "enforce-first-as"; +constexpr std::string_view kTtlSecurityHops = "ttl-security-hops"; +constexpr std::string_view kLinkBandwidth = "link-bandwidth"; +constexpr std::string_view kAdvertiseLbw = "advertise-lbw"; +constexpr std::string_view kReceiveLbw = "receive-lbw"; constexpr std::string_view kPassive = "passive"; constexpr std::string_view kAddPathSend = "add-path send"; constexpr std::string_view kAddPathReceive = "add-path receive"; @@ -78,20 +85,30 @@ constexpr std::string_view kMaxRoutePreWarningOnly = "max-route pre-warning-only"; constexpr std::string_view kMaxRoutePostWarningOnly = "max-route post-warning-only"; +// PeerGroup fields deliberately NOT exposed, because bgpd never reads them +// from a group: local_addr, next_hop4, next_hop6 (per-peer only, the +// neighbor command owns them), `enabled` and `router_port_id` (read nowhere) +// and bgp_peer_timers.graceful_restart_end_of_rib_seconds (same). Staging +// them would persist config the daemon ignores. // Protocol ranges bgpd enforces (or the wire format silently truncates to): // the OPEN hold time is 16 bits and bgpd refuses 1-2s; the graceful-restart -// restart time is the 12-bit field of RFC 4724. +// restart time is the 12-bit field of RFC 4724; TTL security hops are +// bgpd's kMin/kMaxTtlSecurityHops (it throws on anything else at load). constexpr int32_t kHoldTimeMinSeconds = 3; constexpr int32_t kHoldTimeMaxSeconds = 65535; constexpr int32_t kGracefulRestartMaxSeconds = 4095; +constexpr int32_t kTtlSecurityHopsMin = 1; +constexpr int32_t kTtlSecurityHopsMax = 255; using BgpConfig = bgp::thrift::BgpConfig; using PeerGroup = bgp::thrift::PeerGroup; using bgpcli::asnAttr; using bgpcli::AttrHandler; +using bgpcli::bitRateAttr; using bgpcli::boolAttr; using bgpcli::err; +using bgpcli::intAttr; using bgpcli::joinedStringAttr; using bgpcli::ok; using bgpcli::parseBool; @@ -100,8 +117,11 @@ using bgpcli::routeCountAttr; using bgpcli::secondsAttr; using bgpcli::SecondsRange; using bgpcli::stringAttr; +using bgpcli::thriftEnumAttr; using bgpcli::Tokens; using facebook::neteng::fboss::bgp_attr::AddPath; +using facebook::neteng::fboss::bgp_attr::AdvertiseLinkBandwidth; +using facebook::neteng::fboss::bgp_attr::ReceiveLinkBandwidth; // ---- per-attribute setters ------------------------------------------------- // Pure thrift assignment: no validation, no messages. Parsing, bounds and the @@ -149,9 +169,39 @@ void setEnhancedRouteRefresh(PeerGroup& g, bool v) { g.enhanced_route_refresh() = v; } +void setRouteRefresh(PeerGroup& g, bool v) { + g.route_refresh() = v; +} + +void setRemovePrivateAs(PeerGroup& g, bool v) { + g.remove_private_as() = v; +} + +void setEnforceFirstAs(PeerGroup& g, bool v) { + g.enforce_first_as() = v; +} + +void setTtlSecurityHops(PeerGroup& g, int32_t v) { + g.ttl_security_hops() = v; +} + +void setLinkBandwidth(PeerGroup& g, const std::string& v) { + g.link_bandwidth_bps() = v; +} + +void setAdvertiseLbw(PeerGroup& g, AdvertiseLinkBandwidth v) { + g.advertise_link_bandwidth() = v; +} + +void setReceiveLbw(PeerGroup& g, ReceiveLinkBandwidth v) { + g.receive_link_bandwidth() = v; +} + // bgpd reads is_passive=true as PASSIVE_ONLY (listen) and false as -// PASSIVE_ACTIVE (listen and connect); there is no active-only mode, so the -// CLI exposes the flag itself rather than inventing connect-mode names. +// PASSIVE_ACTIVE (listen and connect). Its session code also knows an +// ACTIVE_ONLY mode, but the config model has no way to request it (the mode +// is derived from is_passive alone), so the CLI exposes the flag itself +// rather than inventing connect-mode names. void setPassive(PeerGroup& g, bool v) { g.is_passive() = v; } @@ -231,10 +281,15 @@ void setNextHopSelf(PeerGroup& g, bool v) { // the whole group. Before the first timer attribute is set on a group, the // struct is therefore seeded from what its members use today -- the global // hold time, with the conventional hold/3 keepalive -- and the requested field -// is then changed on top of it. pre_filter / post_filter need no seeding: a -// group has nothing above it to inherit a RouteLimit from, and the setter's -// ensure() yields the thrift defaults, which is also what bgpd applies to a -// peer without one. +// is then changed on top of it. +// +// pre_filter / post_filter are seeded too, though a group has nothing above +// it to inherit from: bgpd applies NO limit to a member without a RouteLimit +// (capRoutesPerPeer returns early) and reads max_routes == 0 as unlimited, +// whereas the thrift default the setter's ensure() would leave behind is a +// 12000-route hard cap with session teardown. A warning-only or threshold +// edit must not introduce a cap the operator never asked for, so the struct +// starts as {0, false, 0} and only the requested field changes. void seedTimers(const BgpConfig& cfg, PeerGroup& group) { if (group.bgp_peer_timers()) { @@ -246,9 +301,21 @@ void seedTimers(const BgpConfig& cfg, PeerGroup& group) { group.bgp_peer_timers() = std::move(timers); } +void seedRouteLimit(PeerGroup& group, bool pre) { + auto field = pre ? group.pre_filter() : group.post_filter(); + if (field) { + return; + } + bgp::thrift::RouteLimit unlimited; + unlimited.max_routes() = 0; + unlimited.warning_only() = false; + unlimited.warning_limit() = 0; + field = std::move(unlimited); +} + // A peer-group handler sees the whole config (for the global defaults) plus // the group being edited. The shared factories only know the group, so they -// are adapted; the adapter for timer attributes seeds first. +// are adapted; the adapters for struct-valued attributes seed first. using PeerGroupHandler = std::function; @@ -267,6 +334,14 @@ PeerGroupHandler withTimers(AttrHandler handler) { }; } +PeerGroupHandler withRouteLimit(bool pre, AttrHandler handler) { + return [pre, handler = std::move(handler)]( + const BgpConfig&, PeerGroup& group, const Tokens& values) { + seedRouteLimit(group, pre); + return handler(group, values); + }; +} + // ---- hand-written handlers ------------------------------------------------- // Only for value shapes no factory covers, because the shape is unique to this // attribute rather than reusable. Mirrors the neighbor dispatcher's version, @@ -322,6 +397,12 @@ const std::map>& attrHandlers() { auto timers = [](AttrHandler h) { return withTimers(std::move(h)); }; + auto preLimit = [](AttrHandler h) { + return withRouteLimit(/* pre */ true, std::move(h)); + }; + auto postLimit = [](AttrHandler h) { + return withRouteLimit(/* pre */ false, std::move(h)); + }; static const std::map> kHandlers = { {std::string(kRemoteAsn), @@ -350,6 +431,30 @@ const std::map>& attrHandlers() { plain( boolAttr( kEnhancedRouteRefresh, setEnhancedRouteRefresh))}, + {std::string(kRouteRefresh), + plain(boolAttr(kRouteRefresh, setRouteRefresh))}, + {std::string(kRemovePrivateAs), + plain(boolAttr(kRemovePrivateAs, setRemovePrivateAs))}, + {std::string(kEnforceFirstAs), + plain(boolAttr(kEnforceFirstAs, setEnforceFirstAs))}, + {std::string(kTtlSecurityHops), + plain( + intAttr( + kTtlSecurityHops, + "1-255", + kTtlSecurityHopsMin, + kTtlSecurityHopsMax, + setTtlSecurityHops))}, + {std::string(kLinkBandwidth), + plain(bitRateAttr(kLinkBandwidth, setLinkBandwidth))}, + {std::string(kAdvertiseLbw), + plain( + thriftEnumAttr( + kAdvertiseLbw, setAdvertiseLbw))}, + {std::string(kReceiveLbw), + plain( + thriftEnumAttr( + kReceiveLbw, setReceiveLbw))}, {std::string(kPassive), plain(boolAttr(kPassive, setPassive))}, {std::string(kAddPathSend), plain(addPathSend)}, @@ -371,29 +476,29 @@ const std::map>& attrHandlers() { boolAttr( kGracefulRestartStatefulHa, setGracefulRestartStatefulHa))}, {std::string(kMaxRoutePreFilter), - plain( + preLimit( routeCountAttr( kMaxRoutePreFilter, setMaxRoutePreFilter))}, {std::string(kMaxRoutePostFilter), - plain( + postLimit( routeCountAttr( kMaxRoutePostFilter, setMaxRoutePostFilter))}, {std::string(kMaxRoutePreWarningThreshold), - plain( + preLimit( routeCountAttr( kMaxRoutePreWarningThreshold, setMaxRoutePreWarningThreshold))}, {std::string(kMaxRoutePostWarningThreshold), - plain( + postLimit( routeCountAttr( kMaxRoutePostWarningThreshold, setMaxRoutePostWarningThreshold))}, {std::string(kMaxRoutePreWarningOnly), - plain( + preLimit( boolAttr( kMaxRoutePreWarningOnly, setMaxRoutePreWarningOnly))}, {std::string(kMaxRoutePostWarningOnly), - plain( + postLimit( boolAttr( kMaxRoutePostWarningOnly, setMaxRoutePostWarningOnly))}, {std::string(kTimersHoldTime), diff --git a/fboss/cli/fboss2/docs/BGP_CLI_TEST_PLAN.md b/fboss/cli/fboss2/docs/BGP_CLI_TEST_PLAN.md index ebb8ad6486105..3b995ef3cb4e1 100644 --- a/fboss/cli/fboss2/docs/BGP_CLI_TEST_PLAN.md +++ b/fboss/cli/fboss2/docs/BGP_CLI_TEST_PLAN.md @@ -73,10 +73,10 @@ fboss2 config protocol bgp peer-group RSW-FSW-V6 next-hop-self true fboss2 config protocol bgp peer-group RSW-FSW-V6 confed-peer true fboss2 config protocol bgp peer-group RSW-FSW-V6 ingress-policy PROPAGATE_RSW_FSW_IN fboss2 config protocol bgp peer-group RSW-FSW-V6 egress-policy PROPAGATE_RSW_FSW_OUT -fboss2 config protocol bgp peer-group RSW-FSW-V6 v4-over-v6-nh true +fboss2 config protocol bgp peer-group RSW-FSW-V6 afi ipv4-over-ipv6-nh true fboss2 config protocol bgp peer-group RSW-FSW-V6 timers hold-time 30 fboss2 config protocol bgp peer-group RSW-FSW-V6 timers keepalive 10 -fboss2 config protocol bgp peer-group RSW-RTSW-V6 disable-ipv4-afi true +fboss2 config protocol bgp peer-group RSW-RTSW-V6 afi disable-ipv4-afi true # Peer configuration (with explicit disable_ipv4_afi: false preserved) fboss2 config protocol bgp neighbor 2401:db00:501c::/64 remote-asn 65000 @@ -131,11 +131,11 @@ diff <(jq -S . /tmp/rsw_bgp_nao.txt) <(jq -S . ~/.fboss2/bgp_config.json) | Peer Group | `config protocol bgp peer-group remote-asn ` | Set remote AS | | Peer Group | `config protocol bgp peer-group description ` | Set description | | Peer Group | `config protocol bgp peer-group next-hop-self ` | Enable/disable next-hop-self | -| Peer Group | `config protocol bgp peer-group disable-ipv4-afi ` | Disable IPv4 AFI | -| Peer Group | `config protocol bgp peer-group v4-over-v6-nh ` | Enable v4-over-v6 nexthop | +| Peer Group | `config protocol bgp peer-group afi disable-ipv4-afi ` | Disable IPv4 AFI | +| Peer Group | `config protocol bgp peer-group afi ipv4-over-ipv6-nh ` | Enable v4-over-v6 nexthop | | Peer Group | `config protocol bgp peer-group timers hold-time ` | Set hold time | | Peer Group | `config protocol bgp peer-group timers keepalive ` | Set keepalive interval | -| Peer Group | `config protocol bgp peer-group max-routes ` | Set max routes | +| Peer Group | `config protocol bgp peer-group max-route pre-filter ` | Set max routes (pre-policy; 0 = unlimited) | | Neighbor | `config protocol bgp neighbor remote-asn ` | Set remote AS | | Neighbor | `config protocol bgp neighbor peer-group ` | Assign to peer group | | Neighbor | `config protocol bgp neighbor bind-addr address ` | Set local address | diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp index 14ff26a2111ba..a80c525bb273e 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp @@ -157,6 +157,9 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, boolAndStringAttributes) { run({"SPINE", "confed-peer", "false"}); run({"SPINE", "redistribute-peer", "true"}); run({"SPINE", "enhanced-route-refresh", "true"}); + run({"SPINE", "route-refresh", "true"}); + run({"SPINE", "remove-private-as", "true"}); + run({"SPINE", "enforce-first-as", "false"}); run({"SPINE", "afi", "disable-ipv4-afi", "true"}); run({"SPINE", "afi", "disable-ipv6-afi", "false"}); run({"SPINE", "afi", "ipv4-over-ipv6-nh", "true"}); @@ -174,6 +177,9 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, boolAndStringAttributes) { EXPECT_FALSE(group.is_confed_peer().value_or(true)); EXPECT_TRUE(group.is_redistribute_peer().value_or(false)); EXPECT_TRUE(group.enhanced_route_refresh().value_or(false)); + EXPECT_TRUE(group.route_refresh().value_or(false)); + EXPECT_TRUE(group.remove_private_as().value_or(false)); + EXPECT_FALSE(group.enforce_first_as().value_or(true)); EXPECT_TRUE(group.disable_ipv4_afi().value_or(false)); EXPECT_FALSE(group.disable_ipv6_afi().value_or(true)); EXPECT_TRUE(group.v4_over_v6_nexthop().value_or(false)); @@ -204,6 +210,52 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, routeLimits) { EXPECT_FALSE(*group.post_filter()->warning_only()); } +TEST_F(CmdConfigBgpPeerGroupTestFixture, linkBandwidthAttributes) { + // Same spellings and validation as the neighbor command: bgpd resolves the + // link-bandwidth trio peer > group. + run({"SPINE", "link-bandwidth", "10G"}); + EXPECT_EQ(groups()[0].link_bandwidth_bps().value_or(""), "10G"); + run({"SPINE", "link-bandwidth", "1.5G"}); + EXPECT_EQ(groups()[0].link_bandwidth_bps().value_or(""), "1.5G"); + run({"SPINE", "link-bandwidth", "auto"}); + EXPECT_EQ(groups()[0].link_bandwidth_bps().value_or(""), "auto"); + EXPECT_THAT(run({"SPINE", "link-bandwidth", "10Q"}), HasSubstr("Invalid")); + EXPECT_THAT(run({"SPINE", "link-bandwidth", "10T"}), HasSubstr("Invalid")); + EXPECT_EQ(groups()[0].link_bandwidth_bps().value_or(""), "auto"); + + run({"SPINE", "advertise-lbw", "1"}); + run({"SPINE", "receive-lbw", "ACCEPT"}); // enum value name accepted + const auto& group = groups()[0]; + EXPECT_EQ( + static_cast(group.advertise_link_bandwidth().value_or( + facebook::neteng::fboss::bgp_attr::AdvertiseLinkBandwidth::DISABLE)), + 1); + EXPECT_EQ( + static_cast(group.receive_link_bandwidth().value_or( + facebook::neteng::fboss::bgp_attr::ReceiveLinkBandwidth::DISABLE)), + 1); + EXPECT_THAT( + run({"SPINE", "advertise-lbw", "99"}), HasSubstr("not a valid mode")); + EXPECT_THAT( + run({"SPINE", "receive-lbw", "SOMETIMES"}), + HasSubstr("not a valid mode")); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, ttlSecurityHopsRange) { + // bgpd throws at config load outside kMin/kMaxTtlSecurityHops (1..255). + EXPECT_THAT(run({"SPINE", "ttl-security-hops", "0"}), HasSubstr("Error")); + EXPECT_THAT(run({"SPINE", "ttl-security-hops", "256"}), HasSubstr("Error")); + EXPECT_THAT(run({"SPINE", "ttl-security-hops", "x"}), HasSubstr("Error")); + EXPECT_TRUE(groups().empty()) + << "rejected first attribute must not leave a half-created group"; + run({"SPINE", "ttl-security-hops", "1"}); + EXPECT_EQ(groups()[0].ttl_security_hops().value_or(0), 1); + run({"SPINE", "ttl-security-hops", "255"}); + EXPECT_EQ(groups()[0].ttl_security_hops().value_or(0), 255); + EXPECT_THAT(run({"SPINE", "ttl-security-hops", "256"}), HasSubstr("Error")); + EXPECT_EQ(groups()[0].ttl_security_hops().value_or(0), 255); +} + TEST_F(CmdConfigBgpPeerGroupTestFixture, valueValidation) { // ASN must fit in 4 bytes (RFC 6793). EXPECT_THAT(run({"SPINE", "remote-asn", "4294967296"}), HasSubstr("Invalid")); @@ -289,15 +341,30 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, timersNotReseededOncePresent) { EXPECT_EQ(*timers.out_delay_seconds(), 2); } -TEST_F(CmdConfigBgpPeerGroupTestFixture, routeLimitOtherFieldsKeepDefaults) { - // No level above a group to inherit a RouteLimit from: the untouched fields - // take the thrift defaults bgpd applies to a peer without one. +TEST_F(CmdConfigBgpPeerGroupTestFixture, routeLimitSeededUnlimited) { + // A group with no RouteLimit imposes none on its members (bgpd skips the + // check when the struct is absent), so the struct created for a + // warning-only/threshold edit must keep that: max_routes 0 = unlimited, + // not the 12000 thrift default, which would be a hard cap with teardown. run({"SPINE", "max-route", "pre-warning-only", "true"}); const auto& pre = *groups()[0].pre_filter(); - EXPECT_EQ(*pre.max_routes(), 12000); + EXPECT_EQ(*pre.max_routes(), 0); EXPECT_EQ(*pre.warning_limit(), 0); EXPECT_TRUE(*pre.warning_only()); EXPECT_FALSE(groups()[0].post_filter().has_value()); + + run({"SPINE", "max-route", "post-warning-threshold", "30000"}); + const auto& post = *groups()[0].post_filter(); + EXPECT_EQ(*post.max_routes(), 0); + EXPECT_EQ(*post.warning_limit(), 30000); + EXPECT_FALSE(*post.warning_only()); +} + +TEST_F(CmdConfigBgpPeerGroupTestFixture, routeLimitNotReseededOncePresent) { + run({"SPINE", "max-route", "pre-filter", "45000"}); + run({"SPINE", "max-route", "pre-warning-threshold", "30000"}); + EXPECT_EQ(*groups()[0].pre_filter()->max_routes(), 45000); + EXPECT_EQ(*groups()[0].pre_filter()->warning_limit(), 30000); } TEST_F(CmdConfigBgpPeerGroupTestFixture, timerRanges) { @@ -335,6 +402,13 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, rejectedValueLeavesGroupUntouched) { ASSERT_EQ(groups().size(), 1); EXPECT_FALSE(groups()[0].bgp_peer_timers().has_value()); EXPECT_EQ(groups()[0].remote_as_4_byte().value_or(0), 65000); + // Likewise a rejected route-limit value must not leave a seeded RouteLimit. + EXPECT_THAT( + run({"SPINE", "max-route", "pre-warning-threshold", "-1"}), + HasSubstr("Error")); + EXPECT_FALSE(groups()[0].pre_filter().has_value()); + EXPECT_THAT(run({"SPINE", "ttl-security-hops", "0"}), HasSubstr("Error")); + EXPECT_FALSE(groups()[0].ttl_security_hops().has_value()); } // ============================================================================== diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp index 92086d85316c7..f1ede5067661b 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp @@ -129,6 +129,47 @@ TEST_F(ConfigBgpPeerGroupTest, SetPassiveAndCommit) { EXPECT_TRUE((*group)["is_passive"].asBool()); } +TEST_F(ConfigBgpPeerGroupTest, SessionKnobsAndCommit) { + // The per-session knobs bgpd resolves peer > group (remove-private-as, + // ttl-security-hops) reach the daemon's running config via the same path. + discardSession(); + clearBgpSession(); + stagePeerGroup({kGroup, "remove-private-as", "true"}); + stagePeerGroup({kGroup, "ttl-security-hops", "2"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + auto running = readRunningBgpConfigViaRpc(); + const auto* group = findGroup(running, kGroup); + ASSERT_NE(group, nullptr) + << "bgpd's running config has no peer-group " << kGroup; + ASSERT_TRUE(group->count("remove_private_as")); + EXPECT_TRUE((*group)["remove_private_as"].asBool()); + ASSERT_TRUE(group->count("ttl_security_hops")); + EXPECT_EQ((*group)["ttl_security_hops"].asInt(), 2); +} + +TEST_F(ConfigBgpPeerGroupTest, RouteLimitWarningKeepsUnlimitedCap) { + // A warning threshold on a group with no route limit must not introduce the + // 12000-route thrift default: bgpd reads max_routes 0 as unlimited, which + // is what the group's members had before the edit. + discardSession(); + clearBgpSession(); + stagePeerGroup({kGroup, "max-route", "pre-warning-threshold", "30000"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + auto running = readRunningBgpConfigViaRpc(); + const auto* group = findGroup(running, kGroup); + ASSERT_NE(group, nullptr) + << "bgpd's running config has no peer-group " << kGroup; + ASSERT_TRUE(group->count("pre_filter")); + EXPECT_EQ((*group)["pre_filter"]["max_routes"].asInt(), 0); + EXPECT_EQ((*group)["pre_filter"]["warning_limit"].asInt(), 30000); +} + TEST_F(ConfigBgpPeerGroupTest, DeleteGroupAndCommit) { // Land a peer-group in the system config, then delete it through a second // commit and verify it is gone from bgpd's running config. diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index a5fcf94dc0ac4..82862060984e3 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -366,16 +366,28 @@ def generate_peer_commands(peer: dict[str, Any]) -> list[str]: # (json field, CLI attribute) — PeerGroup-only fields whose value maps 1:1 -# onto a peer-group attribute token. +# onto a peer-group attribute token. Deprecated i32 ASNs map to the 4-byte +# attribute, as for neighbors. PeerGroup also carries local_addr, next_hop4, +# next_hop6, enabled and router_port_id, but bgpd reads none of them from a +# group (the first three per peer only, the last two nowhere), so the +# dispatcher has no attribute for them and they are flagged instead. _PEER_GROUP_SCALAR_FIELDS = [ ("remote_as_4_byte", "remote-asn"), + ("remote_as", "remote-asn"), ("local_as_4_byte", "local-asn"), + ("local_as", "local-asn"), ("description", "description"), ("peer_tag", "peer-tag"), ("ingress_policy_name", "ingress-policy"), ("egress_policy_name", "egress-policy"), ] +_PEER_GROUP_HANDLED_FIELDS = ( + {"name"} + | {field for field, _ in _PEER_GROUP_SCALAR_FIELDS} + | _SHARED_HANDLED_FIELDS +) + def generate_peer_group_commands(peer_group: dict[str, Any]) -> list[str]: """Generate `config protocol bgp peer-group` CLI commands for a peer group. @@ -389,7 +401,9 @@ def generate_peer_group_commands(peer_group: dict[str, Any]) -> list[str]: return [] prefix = f"config protocol bgp peer-group {escape_shell_arg(name)}" - commands = [] + commands = _unconverted_field_warnings( + peer_group, _PEER_GROUP_HANDLED_FIELDS, f"peer-group {_printable(name)}" + ) commands.extend( _generate_scalar_commands(peer_group, prefix, _PEER_GROUP_SCALAR_FIELDS) ) 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 81791c4b49f0c..4cc958f336918 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 @@ -413,11 +413,18 @@ def test_complete_peer_group_loses_no_field(self) -> None: "is_confed_peer": False, "is_redistribute_peer": True, "enhanced_route_refresh": True, + "route_refresh": False, + "remove_private_as": True, + "enforce_first_as": True, "disable_ipv4_afi": True, "disable_ipv6_afi": False, "v4_over_v6_nexthop": True, "enable_stateful_ha": True, "next_hop_self": True, + "ttl_security_hops": 2, + "link_bandwidth_bps": "10G", + "advertise_link_bandwidth": 1, + "receive_link_bandwidth": "ACCEPT", "add_path": 3, "bgp_peer_timers": { "hold_time_seconds": 90, @@ -449,11 +456,18 @@ def test_complete_peer_group_loses_no_field(self) -> None: "confed-peer false", "redistribute-peer true", "enhanced-route-refresh true", + "route-refresh false", + "remove-private-as true", + "enforce-first-as true", "afi disable-ipv4-afi true", "afi disable-ipv6-afi false", "afi ipv4-over-ipv6-nh true", "graceful-restart stateful-ha true", "next-hop-self true", + "ttl-security-hops 2", + "link-bandwidth 10G", + "advertise-lbw 1", + "receive-lbw ACCEPT", "add-path receive true", "add-path send true", "timers hold-time 90", @@ -481,6 +495,66 @@ def test_complete_peer_group_loses_no_field(self) -> None: + len(peer_group["post_filter"]) ) self.assertEqual(len(commands), leaf_fields) + self.assertEqual(len(commands), 36) + + def test_link_bandwidth(self) -> None: + """The link-bandwidth trio bgpd resolves peer > group is generated.""" + peer_group = { + "name": "TEST", + "link_bandwidth_bps": 10_000_000_000, + "advertise_link_bandwidth": "ADVERTISE", + "receive_link_bandwidth": 1, + } + commands = generate_peer_group_commands(peer_group) + self.assertIn(f"{self.PREFIX} link-bandwidth 10G", commands) + self.assertIn(f"{self.PREFIX} advertise-lbw ADVERTISE", commands) + self.assertIn(f"{self.PREFIX} receive-lbw 1", commands) + + def test_deprecated_asn_fields_map_to_4_byte_attribute(self) -> None: + """The deprecated i32 ASNs are carried over, not dropped.""" + peer_group = {"name": "TEST", "remote_as": 65000, "local_as": 64512} + commands = generate_peer_group_commands(peer_group) + self.assertIn(f"{self.PREFIX} remote-asn 65000", commands) + self.assertIn(f"{self.PREFIX} local-asn 64512", commands) + self.assertFalse(any(c.startswith("# WARNING") for c in commands)) + + def test_unrepresentable_peer_group_fields_warn(self) -> None: + """PeerGroup fields bgpd never reads from a group are reported, not + silently dropped, and produce no config command.""" + peer_group = { + "name": "TEST", + "remote_as_4_byte": 65000, + "local_addr": "2001:db8::2", + "next_hop4": "10.1.1.1", + "next_hop6": "2001:db8::a", + "enabled": True, + "router_port_id": 7, + "bgp_peer_timers": { + "hold_time_seconds": 90, + "graceful_restart_end_of_rib_seconds": 30, + }, + } + commands = generate_peer_group_commands(peer_group) + warnings = [c for c in commands if c.startswith("# WARNING")] + self.assertEqual( + warnings, + [ + f"# WARNING: peer-group TEST: field {f} has no CLI equivalent, " + "not converted" + for f in ( + "local_addr", + "next_hop4", + "next_hop6", + "enabled", + "router_port_id", + "bgp_peer_timers.graceful_restart_end_of_rib_seconds", + ) + ], + ) + self.assertEqual( + [c for c in commands if not c.startswith("#")], + [f"{self.PREFIX} remote-asn 65000", f"{self.PREFIX} timers hold-time 90"], + ) class GeneratePeerCommandsTest(unittest.TestCase): From e7ab1805d5c65bf9663c332ceeb2812ac85c4af6 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 16 Sep 2026 07:39:48 +0000 Subject: [PATCH 7/9] Make BGP peer-group delete idempotent Delete mirrors add: an absent group is already the requested end state, so the delete succeeds with a `Warning:` instead of an `Error:`, and nothing is saved. The referenced-by-neighbors guard still errors. Co-Authored-By: Claude Fable 5.1 --- .../CmdDeleteProtocolBgpPeerGroup.cpp | 12 +++++--- .../test/config/CmdDeleteBgpPeerGroupTest.cpp | 30 +++++++++++++++---- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp index b34cd75ece87c..a0f885934bb94 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp @@ -50,17 +50,21 @@ CmdDeleteProtocolBgpPeerGroup::queryClient( const ObjectArgType& args) { auto& session = ConfigSession::getInstance(); auto& cfg = session.getBgpConfig(); + // Delete mirrors add: an absent group is already the requested end state, + // so this is a success with a warning. Nothing is saved, so a typo'd delete + // can't stage an unrelated session change. + auto absent = fmt::format( + "Warning: BGP peer-group {} does not exist; nothing to delete", + args.groupName()); if (!cfg.peer_groups().has_value()) { - // Nothing is persisted for an unknown group, so a typo'd delete can't - // stage an unrelated session change. - return fmt::format("Error: BGP peer-group {} not found", args.groupName()); + return absent; } auto& groups = *cfg.peer_groups(); auto it = std::find_if(groups.begin(), groups.end(), [&](const auto& group) { return *group.name() == args.groupName(); }); if (it == groups.end()) { - return fmt::format("Error: BGP peer-group {} not found", args.groupName()); + return absent; } // A neighbor's peer_group_name resolves against this group by name at // daemon load; erasing the group while a neighbor still points at it would diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp index 5ef75558b0e4e..aff7335b6721e 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp @@ -100,19 +100,39 @@ TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteExistingGroup) { EXPECT_TRUE(sessionFileExists()); } -TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteUnknownGroupRejected) { +// Delete mirrors add: an absent target is a success with a warning, never an +// error, so a replayed script stays idempotent. +TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteUnknownGroupWarns) { auto result = del({"NO-SUCH-GROUP"}); EXPECT_THAT( - result, HasSubstr("Error: BGP peer-group NO-SUCH-GROUP not found")); - // Nothing was persisted for the failed delete. + result, + HasSubstr( + "Warning: BGP peer-group NO-SUCH-GROUP does not exist; nothing to " + "delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); + // Nothing changed, so nothing is staged. EXPECT_FALSE(sessionFileExists()) - << "session file should not exist after rejected delete"; + << "session file should not exist after a no-op delete"; +} + +TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteTwiceIsIdempotent) { + configure({"SPINE", "remote-asn", "65000"}); + EXPECT_THAT(del({"SPINE"}), HasSubstr("Successfully deleted")); + EXPECT_TRUE(groups().empty()); + + auto again = del({"SPINE"}); + EXPECT_THAT( + again, + HasSubstr( + "Warning: BGP peer-group SPINE does not exist; nothing to delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); + EXPECT_TRUE(groups().empty()); } TEST_F(CmdDeleteBgpPeerGroupTestFixture, deleteUnknownLeavesOthersIntact) { configure({"SPINE", "remote-asn", "65000"}); auto result = del({"LEAF"}); - EXPECT_THAT(result, HasSubstr("not found")); + EXPECT_THAT(result, HasSubstr("does not exist")); ASSERT_EQ(groups().size(), 1); EXPECT_EQ(*groups()[0].name(), "SPINE"); } From 387e0da02303fbc2857411157d2647b07ddf3943 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Mon, 21 Sep 2026 03:55:34 +0000 Subject: [PATCH 8/9] [Nexthop] Report an existing neighbor or peer-group on bare re-create A bare `config protocol bgp neighbor ` or `peer-group ` on an object that already exists said "Successfully created" and re-saved the unchanged config. Return "already exists" before touching the session instead, matching the VLAN and ACL config commands. Co-Authored-By: Claude Fable 5.1 --- .../bgp/neighbor/CmdConfigProtocolBgpNeighbor.cpp | 3 +++ .../bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp | 3 +++ .../fboss2/test/config/CmdConfigBgpNeighborTest.cpp | 11 +++++++++++ .../fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp | 11 +++++++++++ 4 files changed, 28 insertions(+) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/neighbor/CmdConfigProtocolBgpNeighbor.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/neighbor/CmdConfigProtocolBgpNeighbor.cpp index 464dbd50b86e9..1ed0e898a0892 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/neighbor/CmdConfigProtocolBgpNeighbor.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/neighbor/CmdConfigProtocolBgpNeighbor.cpp @@ -822,6 +822,9 @@ CmdConfigProtocolBgpNeighbor::queryClient( auto& cfg = session.getBgpConfig(); auto& peers = *cfg.peers(); auto existing = bgpcli::findBgpPeer(cfg, args.peerAddr()); + if (args.attr().empty() && existing != peers.end()) { + return fmt::format("BGP neighbor {} already exists", args.peerAddr()); + } // Edit a copy: a rejected value (or a struct seeded for it) must leave the // in-memory config exactly as it was, since later lookups in the same diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp index 6393f4f3a1e14..a4db7b0813e9d 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp @@ -601,6 +601,9 @@ CmdConfigProtocolBgpPeerGroup::queryClient( auto& cfg = session.getBgpConfig(); auto& groups = cfg.peer_groups().ensure(); auto existing = findPeerGroup(groups, args.groupName()); + if (args.attr().empty() && existing != groups.end()) { + return fmt::format("BGP peer-group {} already exists", args.groupName()); + } // Edit a copy: a rejected value (or a struct seeded for it) must leave the // in-memory config exactly as it was, since later lookups in the same diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpNeighborTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpNeighborTest.cpp index 1a721367e6040..f34a1b503238c 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpNeighborTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpNeighborTest.cpp @@ -129,6 +129,17 @@ TEST_F(CmdConfigBgpNeighborTestFixture, bareCreate) { EXPECT_EQ(*peers()[1].local_addr(), "::"); } +TEST_F(CmdConfigBgpNeighborTestFixture, bareCreateExisting) { + run({"10.0.0.1", "remote-asn", "65000"}); + // A bare re-create of an existing neighbor is a no-op: it must say so + // rather than claim a creation, and leave the neighbor untouched. + auto result = run({"10.0.0.1"}); + EXPECT_THAT(result, HasSubstr("BGP neighbor 10.0.0.1 already exists")); + EXPECT_THAT(result, Not(HasSubstr("Successfully created"))); + ASSERT_EQ(peers().size(), 1); + EXPECT_EQ(peers()[0].remote_as_4_byte().value_or(0), 65000); +} + TEST_F(CmdConfigBgpNeighborTestFixture, setRemoteAsn) { auto result = run({"10.0.0.1", "remote-asn", "65000"}); EXPECT_THAT(result, HasSubstr("65000")); diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp index a80c525bb273e..2ec00a01a8682 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp @@ -100,6 +100,17 @@ TEST_F(CmdConfigBgpPeerGroupTestFixture, bareCreate) { EXPECT_EQ(*groups()[1].name(), "LEAF"); } +TEST_F(CmdConfigBgpPeerGroupTestFixture, bareCreateExisting) { + run({"SPINE", "remote-asn", "65000"}); + // A bare re-create of an existing group is a no-op: it must say so rather + // than claim a creation, and leave the group untouched. + auto result = run({"SPINE"}); + EXPECT_THAT(result, HasSubstr("BGP peer-group SPINE already exists")); + EXPECT_THAT(result, Not(HasSubstr("Successfully created"))); + ASSERT_EQ(groups().size(), 1); + EXPECT_EQ(groups()[0].remote_as_4_byte().value_or(0), 65000); +} + TEST_F(CmdConfigBgpPeerGroupTestFixture, setRemoteAsn) { auto result = run({"SPINE", "remote-asn", "65000"}); EXPECT_THAT(result, HasSubstr("65000")); From a3572fc96c6bf07877725bf060f99283457c09a4 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Mon, 21 Sep 2026 04:47:20 +0000 Subject: [PATCH 9/9] [Nexthop] j2c: emit a bare peer-group create when no attribute command is generated `name` is the only required PeerGroup field and bgpd accepts a group carrying nothing else, so JSON-to-CLI replay must still create it; otherwise a neighbor referring to it names a group that never existed and the replayed config fails to load. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 5 ++++ .../tools/tests/test_bgp_json_to_cli.py | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 82862060984e3..5fdf14d551058 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -408,6 +408,11 @@ def generate_peer_group_commands(peer_group: dict[str, Any]) -> list[str]: _generate_scalar_commands(peer_group, prefix, _PEER_GROUP_SCALAR_FIELDS) ) commands.extend(_generate_session_commands(peer_group, prefix)) + # `name` is the only required PeerGroup field and bgpd accepts a group + # carrying nothing else, so a bare create is needed to bring it into + # existence; otherwise neighbors referring to it fail to load. + if all(command.startswith("#") for command in commands): + commands.append(prefix) return commands 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 4cc958f336918..bdec657dd80c0 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 @@ -243,6 +243,32 @@ def test_empty_name_returns_empty(self) -> None: commands = generate_peer_group_commands(peer_group) self.assertEqual(commands, []) + def test_name_only_emits_bare_create(self) -> None: + """A group with only its required name must still be created, or + neighbors referring to it would name a group that never existed.""" + commands = generate_peer_group_commands({"name": "TEST"}) + self.assertEqual(commands, [self.PREFIX]) + + def test_only_unrepresentable_fields_emits_bare_create(self) -> None: + """Warning comments are not executable commands, so a group whose + fields all lack a CLI equivalent still needs the bare create.""" + commands = generate_peer_group_commands({"name": "TEST", "enabled": True}) + self.assertEqual( + commands, + [ + "# WARNING: peer-group TEST: field enabled has no CLI equivalent, " + "not converted", + self.PREFIX, + ], + ) + + def test_attribute_present_emits_no_bare_create(self) -> None: + """Setting an attribute creates the group implicitly.""" + commands = generate_peer_group_commands( + {"name": "TEST", "remote_as_4_byte": 65000} + ) + self.assertEqual(commands, [f"{self.PREFIX} remote-asn 65000"]) + def test_remote_asn(self) -> None: """remote_as_4_byte should generate correct command.""" peer_group = {"name": "TEST-GROUP", "remote_as_4_byte": 65000}