From b3c6f8aec0c00cbcd66adc9619a9fd81e4716157 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 7 Aug 2026 18:31:22 +0000 Subject: [PATCH 1/5] [Nexthop] NOS-6643..NOS-6648: fboss2 config/delete protocol bgp policy as-path-list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the first BGP policy object-type command family, under a new `policy` grouping node beneath `protocol bgp` (sibling to global/neighbor/peer-group): config protocol bgp policy as-path-list [description ] delete protocol bgp policy as-path-list The list's `entry ` level is a CLI11 subcommand of its own and lands in the next commit, so this one owns only the list level. - Follows the dispatcher shape the neighbor/peer-group families use: one factory per value shape from the shared BgpCliAttrHandlers.h, named setters that do nothing but assign the thrift field, and a registry that is one line per attribute. Writes bgp_policy.BgpPolicies.aspath_lists[] through the typed ConfigSession — no new session plumbing, since BgpConfig.policies already exists. - Lookup/create helpers live in BgpAsPathListCliUtils.h, including the entry-level helpers, so the delete command and the entry subcommand in the next commit share them rather than re-deriving the scan. - A rejected value leaves nothing staged: a list implicitly created for the failed command is rolled back before returning. - There is no per-entry delete; `delete ... as-path-list ` removes the whole list. - unit tests (10): CmdConfigBgpPolicyAsPathListTest (6) covering arg validation, bare create, the description round-trip, named lists staying distinct, re-reference reporting the existing list and unknown-attribute rejection, plus CmdDeleteBgpPolicyAsPathListTest (4). - integration tests (2): ConfigBgpPolicyAsPathListTest stages and commits, then asserts against bgpd's running config via the getRunningConfig RPC, confirming bgpd accepts and adopts the .policies blob end to end. Co-Authored-By: Claude Fable 5 --- cmake/AgentHwSaiStoreTest.cmake | 1 - cmake/AgentState.cmake | 1 - cmake/AgentTestAgentHwTests.cmake | 1 - cmake/CliFboss2.cmake | 11 +- cmake/CliFboss2TestConfig.cmake | 2 + cmake/CliFboss2TestIntegrationTest.cmake | 1 + cmake/ThriftCowVisitors.cmake | 1 - fboss/cli/fboss2/BUCK | 9 + fboss/cli/fboss2/CmdListConfig.cpp | 32 ++++ .../bgp/policy/CmdConfigProtocolBgpPolicy.cpp | 30 ++++ .../bgp/policy/CmdConfigProtocolBgpPolicy.h | 40 +++++ .../as-path-list/BgpAsPathListCliUtils.h | 87 ++++++++++ .../CmdConfigProtocolBgpPolicyAsPathList.cpp | 157 ++++++++++++++++++ .../CmdConfigProtocolBgpPolicyAsPathList.h | 82 +++++++++ .../bgp/policy/CmdDeleteProtocolBgpPolicy.cpp | 32 ++++ .../bgp/policy/CmdDeleteProtocolBgpPolicy.h | 43 +++++ .../CmdDeleteProtocolBgpPolicyAsPathList.cpp | 80 +++++++++ .../CmdDeleteProtocolBgpPolicyAsPathList.h | 63 +++++++ fboss/cli/fboss2/test/config/BUCK | 2 + .../CmdConfigBgpPolicyAsPathListTest.cpp | 137 +++++++++++++++ .../CmdDeleteBgpPolicyAsPathListTest.cpp | 113 +++++++++++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + .../ConfigBgpPolicyAsPathListTest.cpp | 118 +++++++++++++ 23 files changed, 1038 insertions(+), 6 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.cpp create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp create mode 100644 fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp diff --git a/cmake/AgentHwSaiStoreTest.cmake b/cmake/AgentHwSaiStoreTest.cmake index f79e6ca747c71..83cb27cf9a899 100644 --- a/cmake/AgentHwSaiStoreTest.cmake +++ b/cmake/AgentHwSaiStoreTest.cmake @@ -35,7 +35,6 @@ add_executable(store_test fboss/agent/hw/sai/store/tests/UdfStoreTest.cpp fboss/agent/hw/sai/store/tests/VlanStoreTest.cpp fboss/agent/hw/sai/store/tests/WredStoreTest.cpp - fboss/agent/hw/sai/store/tests/UdfStoreTest.cpp ) target_link_libraries(store_test diff --git a/cmake/AgentState.cmake b/cmake/AgentState.cmake index 29cd67cbffa20..f0da7e9a7307c 100644 --- a/cmake/AgentState.cmake +++ b/cmake/AgentState.cmake @@ -126,7 +126,6 @@ add_library(state fboss/agent/state/UdfGroupMap.cpp fboss/agent/state/UdfPacketMatcher.cpp fboss/agent/state/UdfPacketMatcherMap.cpp - fboss/agent/state/IpTunnel.cpp fboss/agent/state/IpTunnelMap.cpp fboss/agent/state/Vlan.cpp fboss/agent/state/VlanMap.cpp diff --git a/cmake/AgentTestAgentHwTests.cmake b/cmake/AgentTestAgentHwTests.cmake index a134b629255b6..b5e1ed4dd0c4a 100644 --- a/cmake/AgentTestAgentHwTests.cmake +++ b/cmake/AgentTestAgentHwTests.cmake @@ -218,7 +218,6 @@ add_library(agent_hw_test_src fboss/agent/test/agent_hw_tests/AgentRollbackTests.cpp fboss/agent/test/agent_hw_tests/AgentRouteRollbackTests.cpp fboss/agent/test/agent_hw_tests/AgentPacketStreamHandlerTests.cpp - fboss/agent/test/agent_hw_tests/AgentAclTableGroupTests.cpp ) target_link_libraries(agent_hw_test_src diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 23c59a1341f23..07238e5dd4769 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -968,6 +968,11 @@ 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/policy/CmdConfigProtocolBgpPolicy.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.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 @@ -1104,6 +1109,10 @@ add_library(fboss2_config_lib 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/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.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 @@ -1117,8 +1126,6 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/delete/copp/queue/CmdDeleteCoppQueue.h fboss/cli/fboss2/commands/delete/copp/reason/CmdDeleteCoppReason.cpp fboss/cli/fboss2/commands/delete/copp/reason/CmdDeleteCoppReason.h - fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.cpp - fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.cpp fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicy.h fboss/cli/fboss2/commands/delete/qos/policy/CmdDeleteQosPolicyMap.cpp diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index 34829a21d3f3e..de0fce8ebefc7 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -14,6 +14,7 @@ add_executable(fboss2_cmd_config_test 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/CmdConfigBgpPolicyAsPathListTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp @@ -50,6 +51,7 @@ add_executable(fboss2_cmd_config_test 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/CmdDeleteBgpPolicyAsPathListTest.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 7c5b4fc6560d5..ea9253e0d21e5 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -18,6 +18,7 @@ add_executable(fboss2_integration_test 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/ConfigBgpPolicyAsPathListTest.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/cmake/ThriftCowVisitors.cmake b/cmake/ThriftCowVisitors.cmake index 96e030c9a517a..3098c99790a73 100644 --- a/cmake/ThriftCowVisitors.cmake +++ b/cmake/ThriftCowVisitors.cmake @@ -33,7 +33,6 @@ add_library( thrift_cow_visitors fboss/thrift_cow/visitors/DeltaVisitor.h fboss/thrift_cow/visitors/ExtendedPathVisitor.h - fboss/thrift_cow/visitors/ExtendedPathVisitor.h fboss/thrift_cow/visitors/PathVisitor.h fboss/thrift_cow/visitors/RecurseVisitor.h fboss/thrift_cow/visitors/VisitorUtils.h diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 677a736a47a23..be3d268986c52 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1177,6 +1177,8 @@ 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/policy/CmdConfigProtocolBgpPolicy.cpp", + "commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1244,6 +1246,8 @@ cpp_library( "commands/delete/protocol/bgp/CmdDeleteProtocolBgp.cpp", "commands/delete/protocol/bgp/neighbor/CmdDeleteProtocolBgpNeighbor.cpp", "commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp", + "commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp", + "commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp", "commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp", "commands/delete/qos/CmdDeleteQos.cpp", @@ -1317,6 +1321,9 @@ 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/policy/CmdConfigProtocolBgpPolicy.h", + "commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h", + "commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", @@ -1385,6 +1392,8 @@ cpp_library( "commands/delete/protocol/bgp/CmdDeleteProtocolBgp.h", "commands/delete/protocol/bgp/neighbor/CmdDeleteProtocolBgpNeighbor.h", "commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h", + "commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h", + "commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.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 1b193ab848fb7..278ddee598715 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -45,6 +45,8 @@ #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/policy/CmdConfigProtocolBgpPolicy.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.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" @@ -109,6 +111,8 @@ #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/bgp/policy/CmdDeleteProtocolBgpPolicy.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.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" @@ -435,6 +439,21 @@ const CommandTree& kConfigCommandTree() { commandHandler, argRegistrar, }, + { + "policy", + "Configure BGP policy objects", + commandHandler, + argRegistrar, + {{ + "as-path-list", + "Configure BGP AS-path list: " + "[ ...] (description)", + commandHandler< + CmdConfigProtocolBgpPolicyAsPathList>, + argRegistrar< + CmdConfigProtocolBgpPolicyAsPathListTraits>, + }}, + }, }, }, { @@ -776,6 +795,19 @@ const CommandTree& kConfigCommandTree() { "Delete a BGP peer-group: ", commandHandler, argRegistrar, + }, + { + "policy", + "Delete BGP policy objects", + commandHandler, + argTypeHandler, + {{ + "as-path-list", + "Delete a BGP AS-path list: ", + commandHandler, + argRegistrar< + CmdDeleteProtocolBgpPolicyAsPathListTraits>, + }}, }}, }, { diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.cpp new file mode 100644 index 0000000000000..ace33a2b0c2ce --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.cpp @@ -0,0 +1,30 @@ +/* + * 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/policy/CmdConfigProtocolBgpPolicy.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +namespace facebook::fboss { + +CmdConfigProtocolBgpPolicyTraits::RetType +CmdConfigProtocolBgpPolicy::queryClient(const HostInfo& /* hostInfo */) { + return "BGP policy configuration. Use subcommands: as-path-list"; +} + +void CmdConfigProtocolBgpPolicy::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void +CmdHandler::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h new file mode 100644 index 0000000000000..4830fe77f5545 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h @@ -0,0 +1,40 @@ +/* + * 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/CmdConfigProtocolBgp.h" + +namespace facebook::fboss { + +// Grouping node for `config protocol bgp policy ...`. Holds no +// state of its own; the per-object-type dispatchers (as-path-list, and later +// community-list/prefix-list/routing-policy) are its subcommands. Mirrors +// CmdConfigProtocolBgp. +struct CmdConfigProtocolBgpPolicyTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigProtocolBgp; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicy : public CmdHandler< + CmdConfigProtocolBgpPolicy, + CmdConfigProtocolBgpPolicyTraits> { + public: + using ObjectArgType = CmdConfigProtocolBgpPolicyTraits::ObjectArgType; + using RetType = CmdConfigProtocolBgpPolicyTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h new file mode 100644 index 0000000000000..5c627006a629f --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h @@ -0,0 +1,87 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#include +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" + +/** + * Lookup/create helpers for the as-path-list CLI family, shared between the + * list-level dispatcher (CmdConfigProtocolBgpPolicyAsPathList), the entry + * subcommand (CmdConfigProtocolBgpPolicyAsPathListEntry), and the delete + * counterparts. An AsPathList is keyed by name; an AsPathListEntry (in + * as_path_list[]) is keyed by sequence_number. + */ +namespace facebook::fboss::bgpcli { + +inline bool asPathListExists( + const bgp::thrift::BgpConfig& cfg, + const std::string& name) { + if (!cfg.policies().has_value()) { + return false; + } + const auto& lists = *cfg.policies()->aspath_lists(); + return std::any_of(lists.begin(), lists.end(), [&](const auto& list) { + return *list.name() == name; + }); +} + +// Find the as-path-list keyed by name, creating it if absent. Setting an +// attribute on a not-yet-created list implicitly creates it, so command +// ordering stays forgiving; a bare `as-path-list ` creates one +// explicitly. AsPathList's only key field is `name`. +inline bgp::bgp_policy::AsPathList& findOrCreateAsPathList( + bgp::thrift::BgpConfig& cfg, + const std::string& name) { + auto& lists = *cfg.policies().ensure().aspath_lists(); + for (auto& list : lists) { + if (*list.name() == name) { + return list; + } + } + lists.emplace_back(); + auto& list = lists.back(); + list.name() = name; + return list; +} + +inline bool asPathListEntryExists( + const bgp::bgp_policy::AsPathList& list, + int64_t seqNum) { + const auto& entries = *list.as_path_list(); + return std::any_of(entries.begin(), entries.end(), [&](const auto& entry) { + return entry.sequence_number().has_value() && + *entry.sequence_number() == seqNum; + }); +} + +// Find the entry keyed by sequence_number within a list, creating it if +// absent. sequence_number is the entry's identity. +inline bgp::bgp_policy::AsPathListEntry& findOrCreateAsPathListEntry( + bgp::bgp_policy::AsPathList& list, + int64_t seqNum) { + auto& entries = *list.as_path_list(); + for (auto& entry : entries) { + if (entry.sequence_number().has_value() && + *entry.sequence_number() == seqNum) { + return entry; + } + } + entries.emplace_back(); + auto& entry = entries.back(); + entry.sequence_number() = seqNum; + return entry; +} + +} // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp new file mode 100644 index 0000000000000..e4c6edbc7c0ec --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp @@ -0,0 +1,157 @@ +/* + * 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/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.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 { + +// The attribute names, exactly as documented. Kept here so the +// valid-attribute set and the handler table stay in sync. The list's entries +// are their own subcommand, not attributes. +constexpr std::string_view kDescription = "description"; + +using AsPathList = bgp::bgp_policy::AsPathList; +using bgpcli::AttrHandler; +using bgpcli::joinedStringAttr; +using bgpcli::ok; +using bgpcli::Result; + +// ---- setters ---------------------------------------------------------------- +// Each writes one already-parsed, already-validated value. Parsing and message +// text belong to the shared factories in BgpCliAttrHandlers.h. + +void setListDescription(AsPathList& list, const std::string& description) { + list.description() = description; +} + +// ---- list-level attribute registry ------------------------------------------ +// One line per documented attribute: its dispatch key, its value shape, and +// the setter that stores it. +const std::map, std::less<>>& +listAttrHandlers() { + static const std::map, std::less<>> + kHandlers = { + {std::string(kDescription), + joinedStringAttr(kDescription, setListDescription)}, + }; + return kHandlers; +} + +std::string validAttrList() { + std::string out; + for (const auto& [name, _] : listAttrHandlers()) { + if (!out.empty()) { + out += ", "; + } + out += name; + } + return out; +} + +} // namespace + +// Parse + validate at construction so queryClient stays a thin dispatch. +// Throwing std::invalid_argument is how the framework surfaces arg parse +// errors (same mechanism as BgpPeerGroupConfig). +BgpAsPathListConfig::BgpAsPathListConfig(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: as-path-list is required, optionally followed by an " + " "); + } + if (v[0].empty()) { + throw std::invalid_argument("Error: as-path-list name must not be empty"); + } + listName_ = v[0]; + + if (v.size() == 1) { + return; // bare `as-path-list `: create the list + } + + attr_ = v[1]; + values_.assign(v.begin() + 2, v.end()); + + if (listAttrHandlers().find(attr_) == listAttrHandlers().end()) { + throw std::invalid_argument( + fmt::format( + "Error: unknown as-path-list attribute '{}'. Valid attributes: {}", + attr_, + validAttrList())); + } +} + +CmdConfigProtocolBgpPolicyAsPathListTraits::RetType +CmdConfigProtocolBgpPolicyAsPathList::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool listCreated = !bgpcli::asPathListExists(cfg, args.listName()); + auto& list = bgpcli::findOrCreateAsPathList(cfg, args.listName()); + + Result result = args.attr().empty() + ? ok(listCreated + ? fmt::format( + "Successfully created BGP as-path-list {}", + args.listName()) + : fmt::format( + "BGP as-path-list {} already exists", args.listName())) + // The attribute is guaranteed valid: BgpAsPathListConfig's constructor + // rejects an unknown attribute before we get here. + : listAttrHandlers().find(args.attr())->second(list, args.values()); + + if (result.ok) { + if (!args.attr().empty()) { + result.message += fmt::format(" for as-path-list {}", args.listName()); + } + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } else if (listCreated) { + // Drop the phantom list so a rejected value is not visible to later + // lookups in the same process. + cfg.policies()->aspath_lists()->pop_back(); + } + return result.message; +} + +void CmdConfigProtocolBgpPolicyAsPathList::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdConfigProtocolBgpPolicyAsPathList, + CmdConfigProtocolBgpPolicyAsPathListTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h new file mode 100644 index 0000000000000..7fe25d5f548de --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h @@ -0,0 +1,82 @@ +/* + * 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/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `as-path-list [ ...]`, validated at +// construction. An AsPathList is keyed by . The list's entries are their +// own subcommand (CmdConfigProtocolBgpPolicyAsPathListEntry), not attributes. +// +// Grammar (from the FBOSS proposed syntax): +// as-path-list (create/select list) +// as-path-list description (list attribute) +class BgpAsPathListConfig : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpAsPathListConfig(std::vector v); + const std::string& listName() const { + return listName_; + } + 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 listName_; + std::string attr_; // matched dispatch key ("" = bare create) + std::vector values_; +}; + +// The list level of the `config protocol bgp policy as-path-list` family; +// dispatch and per-attribute parsing live in the .cpp so a new tunable is a +// one-entry change. Mirrors CmdConfigProtocolBgpPolicyRoutingPolicy, whose +// nested level (term / entry) is likewise its own subcommand. +struct CmdConfigProtocolBgpPolicyAsPathListTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigProtocolBgpPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // No positionals_at_end() here: CLI11 must stay free to classify the + // `entry` token as this command's subcommand rather than swallowing it + // into args. See CmdConfigProtocolBgpPolicyRoutingPolicyTraits. + cmd.add_option("args", args, " [ ...]"); + } + using ObjectArgType = BgpAsPathListConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyAsPathList + : public CmdHandler< + CmdConfigProtocolBgpPolicyAsPathList, + CmdConfigProtocolBgpPolicyAsPathListTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyAsPathListTraits::ObjectArgType; + using RetType = CmdConfigProtocolBgpPolicyAsPathListTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& args); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp new file mode 100644 index 0000000000000..bc15db0a326f4 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp @@ -0,0 +1,32 @@ +/* + * 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/policy/CmdDeleteProtocolBgpPolicy.h" +#include +#include + +#include "fboss/cli/fboss2/CmdHandler.cpp" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +CmdDeleteProtocolBgpPolicyTraits::RetType +CmdDeleteProtocolBgpPolicy::queryClient(const HostInfo& /* hostInfo */) { + return "Delete BGP policy objects. Use subcommands: as-path-list"; +} + +void CmdDeleteProtocolBgpPolicy::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +template void +CmdHandler::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h new file mode 100644 index 0000000000000..d658fee78a4ba --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h @@ -0,0 +1,43 @@ +/* + * 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 "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 { + +// Grouping node for `delete protocol bgp policy `. Holds no +// state of its own; the per-object-type delete commands are its subcommands. +struct CmdDeleteProtocolBgpPolicyTraits : public WriteCommandTraits { + using ParentCmd = CmdDeleteProtocolBgp; + static constexpr utils::ObjectArgTypeId ObjectArgTypeId = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_NONE; + using ObjectArgType = std::monostate; + using RetType = std::string; +}; + +class CmdDeleteProtocolBgpPolicy : public CmdHandler< + CmdDeleteProtocolBgpPolicy, + CmdDeleteProtocolBgpPolicyTraits> { + public: + using ObjectArgType = CmdDeleteProtocolBgpPolicyTraits::ObjectArgType; + using RetType = CmdDeleteProtocolBgpPolicyTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp new file mode 100644 index 0000000000000..b9f3bfb68dab5 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp @@ -0,0 +1,80 @@ +/* + * 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/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.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. +BgpAsPathListRef::BgpAsPathListRef(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.size() != 1) { + throw std::invalid_argument( + "Error: delete protocol bgp policy as-path-list requires exactly one " + ""); + } + if (v[0].empty()) { + throw std::invalid_argument("Error: as-path-list name must not be empty"); + } + listName_ = std::move(v[0]); +} + +CmdDeleteProtocolBgpPolicyAsPathListTraits::RetType +CmdDeleteProtocolBgpPolicyAsPathList::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + if (!cfg.policies().has_value()) { + // Nothing is persisted for an unknown list, so a typo'd delete can't stage + // an unrelated session change. + return fmt::format("Error: BGP as-path-list {} not found", args.listName()); + } + auto& lists = *cfg.policies()->aspath_lists(); + auto it = std::find_if(lists.begin(), lists.end(), [&](const auto& list) { + return *list.name() == args.listName(); + }); + if (it == lists.end()) { + return fmt::format("Error: BGP as-path-list {} not found", args.listName()); + } + lists.erase(it); + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP as-path-list {}\nConfig saved to: {}", + args.listName(), + session.getBgpSessionConfigPath()); +} + +void CmdDeleteProtocolBgpPolicyAsPathList::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdDeleteProtocolBgpPolicyAsPathList, + CmdDeleteProtocolBgpPolicyAsPathListTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h new file mode 100644 index 0000000000000..4249bdd3546a4 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h @@ -0,0 +1,63 @@ +/* + * 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/policy/CmdDeleteProtocolBgpPolicy.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `delete protocol bgp policy as-path-list `, validated at +// construction. The name is the list's identity, matched exactly the same way +// the config command stores it. Mirrors BgpPeerGroupRef. +class BgpAsPathListRef : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpAsPathListRef(std::vector v); + const std::string& listName() const { + return listName_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::string listName_; +}; + +struct CmdDeleteProtocolBgpPolicyAsPathListTraits : public WriteCommandTraits { + using ParentCmd = CmdDeleteProtocolBgpPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("args", args, ""); + } + using ObjectArgType = BgpAsPathListRef; + using RetType = std::string; +}; + +class CmdDeleteProtocolBgpPolicyAsPathList + : public CmdHandler< + CmdDeleteProtocolBgpPolicyAsPathList, + CmdDeleteProtocolBgpPolicyAsPathListTraits> { + public: + using ObjectArgType = + CmdDeleteProtocolBgpPolicyAsPathListTraits::ObjectArgType; + using RetType = CmdDeleteProtocolBgpPolicyAsPathListTraits::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 82ed40f48e62a..19a8f08bfd082 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -15,6 +15,7 @@ cpp_unittest( "CmdConfigBgpGlobalTest.cpp", "CmdConfigBgpNeighborTest.cpp", "CmdConfigBgpPeerGroupTest.cpp", + "CmdConfigBgpPolicyAsPathListTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", "CmdConfigDhcpTest.cpp", @@ -68,6 +69,7 @@ cpp_unittest( "CmdDeleteTunnelIpInIpTest.cpp", "CmdDeleteVlanTest.cpp", "ConfigSessionSystemdTest.cpp", + "CmdDeleteBgpPolicyAsPathListTest.cpp", "ProfileValidationTest.cpp", ], headers = [ diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp new file mode 100644 index 0000000000000..c0e96f09a023d --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp @@ -0,0 +1,137 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include + +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +using namespace ::testing; + +namespace facebook::fboss { + +// The as-path-list 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 +// CmdConfigBgpPeerGroupTest). +class CmdConfigBgpPolicyAsPathListTestFixture : public CmdConfigTestBase { + public: + CmdConfigBgpPolicyAsPathListTestFixture() + : CmdConfigTestBase("bgp_aspath_list_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string run(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyAsPathList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpAsPathListConfig(tokens)); + } + + const std::vector& lists() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .aspath_lists(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpAsPathListConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, argValidation) { + // Bare create list. + auto bareList = BgpAsPathListConfig({"AS100"}); + EXPECT_EQ(bareList.listName(), "AS100"); + EXPECT_TRUE(bareList.attr().empty()); + + // List-level attribute. + auto listAttr = BgpAsPathListConfig({"AS100", "description", "my", "list"}); + EXPECT_EQ(listAttr.attr(), "description"); + EXPECT_EQ(listAttr.values(), std::vector({"my", "list"})); + + // Invalid: empty, empty name, unknown attribute. + EXPECT_THROW(BgpAsPathListConfig({}), std::invalid_argument); + EXPECT_THROW(BgpAsPathListConfig({""}), std::invalid_argument); + EXPECT_THROW( + BgpAsPathListConfig({"AS100", "no-such-attr", "1"}), + std::invalid_argument); + // asn-regexp is an entry attribute, not a list attribute. + EXPECT_THROW( + BgpAsPathListConfig({"AS100", "asn-regexp", "^65000_"}), + std::invalid_argument); +} + +// ============================================================================== +// List-level handlers +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, bareCreateList) { + auto result = run({"AS100"}); + EXPECT_THAT(result, HasSubstr("Successfully created BGP as-path-list AS100")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "AS100"); + EXPECT_TRUE(lists()[0].as_path_list()->empty()); + EXPECT_TRUE(sessionFileExists()); + + run({"AS200"}); + ASSERT_EQ(lists().size(), 2); + EXPECT_EQ(*lists()[1].name(), "AS200"); +} + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, setListDescription) { + auto result = run({"AS100", "description", "spine", "as-paths"}); + EXPECT_THAT(result, HasSubstr("Successfully set description")); + ASSERT_EQ(lists().size(), 1); + // Multi-token description is re-joined. + EXPECT_EQ(*lists()[0].description(), "spine as-paths"); +} + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, namedListsAreDistinct) { + run({"AS100", "description", "one"}); + run({"AS200", "description", "two"}); + ASSERT_EQ(lists().size(), 2); + // Re-referencing an existing list by name updates it, not appends. + run({"AS100", "description", "one-updated"}); + ASSERT_EQ(lists().size(), 2); + EXPECT_EQ(*lists()[0].description(), "one-updated"); +} + +// ============================================================================== +// Reject paths — the error is surfaced and nothing is persisted +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, reReferenceReportsExisting) { + run({"AS100"}); + // A second bare reference must not claim to have created it again. + EXPECT_THAT(run({"AS100"}), HasSubstr("already exists")); + ASSERT_EQ(lists().size(), 1); +} + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, unknownAttributeRejected) { + EXPECT_THROW(run({"AS100", "no-such-attr", "1"}), std::invalid_argument); + // The rejected command must not leave a phantom list behind. + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp new file mode 100644 index 0000000000000..7c575a4dd51e0 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp @@ -0,0 +1,113 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include + +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.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 CmdDeleteBgpPolicyAsPathListTestFixture : public CmdConfigTestBase { + public: + CmdDeleteBgpPolicyAsPathListTestFixture() + : CmdConfigTestBase("bgp_aspath_list_delete_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string configure(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyAsPathList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpAsPathListConfig(tokens)); + } + + std::string del(const std::vector& tokens) { + CmdDeleteProtocolBgpPolicyAsPathList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpAsPathListRef(tokens)); + } + + const std::vector& lists() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .aspath_lists(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpAsPathListRef (arg) validation +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, argValidation) { + EXPECT_EQ(BgpAsPathListRef({"AS100"}).listName(), "AS100"); + + // Invalid: empty, empty name, extra tokens. + EXPECT_THROW(BgpAsPathListRef({}), std::invalid_argument); + EXPECT_THROW(BgpAsPathListRef({""}), std::invalid_argument); + EXPECT_THROW(BgpAsPathListRef({"AS100", "AS200"}), std::invalid_argument); +} + +// ============================================================================== +// queryClient +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteExistingList) { + configure({"AS100", "description", "delete-me"}); + configure({"AS200", "description", "keep"}); + ASSERT_EQ(lists().size(), 2); + + auto result = del({"AS100"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted BGP as-path-list AS100")); + // The other list survives untouched. + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "AS200"); + EXPECT_EQ(*lists()[0].description(), "keep"); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteUnknownListRejected) { + auto result = del({"NO-SUCH-LIST"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP as-path-list NO-SUCH-LIST not found")); + // Nothing was persisted for the failed delete. + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected delete"; +} + +TEST_F( + CmdDeleteBgpPolicyAsPathListTestFixture, + deleteUnknownLeavesOthersIntact) { + configure({"AS100", "description", "one"}); + auto result = del({"AS200"}); + EXPECT_THAT(result, HasSubstr("not found")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "AS100"); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index b8ebe6a138482..f8fe039d26a41 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -25,6 +25,7 @@ cpp_binary( "ConfigBgpGlobalTest.cpp", "ConfigBgpNeighborTest.cpp", "ConfigBgpPeerGroupTest.cpp", + "ConfigBgpPolicyAsPathListTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", "ConfigCoppTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp new file mode 100644 index 0000000000000..62fb1bc07cb11 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp @@ -0,0 +1,118 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy as-path-list + * [ ...]`. + * + * Scope: the AS-path list level (its entries are covered by + * ConfigBgpPolicyAsPathListEntryTest). Every test stages the change AND + * commits it, then asserts the value landed at the correct thrift field path + * inside the matching .policies.aspath_lists[] entry of bgpd's running config + * (via getRunningConfig RPC) — which also confirms bgpd accepts and adopts a + * `.policies` blob at all. 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 AS-path-list names, unlikely to collide with a real list in the +// device's running BGP config. +const std::string kList = "FBOSS2-TEST-ASPATH"; +} // namespace + +class ConfigBgpPolicyAsPathListTest : public ConfigBgpTestBase { + protected: + // Stage `config protocol bgp policy as-path-list ` WITHOUT + // clearing the staged session, so attributes can accumulate across + // invocations. Returns the staged session JSON. + folly::dynamic stageAsPathList(const std::vector& tokens) { + std::vector args = { + "config", "protocol", "bgp", "policy", "as-path-list"}; + 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 .policies.aspath_lists[] entry named `name`, or nullptr. + static const folly::dynamic* findList( + const folly::dynamic& config, + const std::string& name) { + if (config.count("policies") == 0 || + config["policies"].count("aspath_lists") == 0) { + return nullptr; + } + for (const auto& list : config["policies"]["aspath_lists"]) { + if (list.count("name") && list["name"].asString() == name) { + return &list; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyAsPathListTest, SetListDescriptionAndCommit) { + discardSession(); + clearBgpSession(); + stageAsPathList({kList, "description", "test", "spine", "as-paths"}); + 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 the .policies blob. + auto config = readSystemBgpConfig(); + const auto* list = findList(config, kList); + ASSERT_NE(list, nullptr) << "committed config has no as-path-list " << kList; + EXPECT_EQ((*list)["description"].asString(), "test spine as-paths"); + + auto running = readRunningBgpConfigViaRpc(); + const auto* runningList = findList(running, kList); + ASSERT_NE(runningList, nullptr) + << "bgpd's running config has no as-path-list " << kList; + EXPECT_EQ((*runningList)["description"].asString(), "test spine as-paths"); +} + +TEST_F(ConfigBgpPolicyAsPathListTest, DeleteListAndCommit) { + // Land an as-path-list in the system config, then delete it through a second + // commit and verify it is gone from bgpd's running config. + discardSession(); + clearBgpSession(); + stageAsPathList({kList, "description", "delete-me"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + ASSERT_NE(findList(readRunningBgpConfigViaRpc(), kList), nullptr) + << "setup commit did not land the as-path-list in bgpd's running config"; + + clearBgpSession(); + auto result = + runCli({"delete", "protocol", "bgp", "policy", "as-path-list", kList}); + EXPECT_THAT(result.stdout, HasSubstr("Successfully deleted")); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after delete commit; state=" + << bgpDaemonActiveState(); + EXPECT_EQ(findList(readRunningBgpConfigViaRpc(), kList), nullptr) + << "deleted as-path-list still present in bgpd's running config"; +} From d5373ea16aa98c850cd00fceb11cb51dd9e58429 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:13:27 +0000 Subject: [PATCH 2/5] Guard as-path-list delete, add regex/boolean-operator, delete one regex Deleting an as-path-list that a routing-policy term still names committed a dangling as_path_list_names reference; bgpd fails the config load on it and crash-loops. The whole-list delete now refuses and names every referencing `policy

term `, the same rule the peer-group delete applies to neighbors. findTermsReferencingAsPathList lives in BgpAsPathListCliUtils.h because the boolean-operator setter uses the same walk. The list gained the two fields bgpd actually matches on: `regex ` appends one boost::regex to as_paths (compiled here so a pattern bgpd would reject at load never reaches the session; whitespace is refused since bgpd renders the AS path `_`-joined), and `boolean-operator ` (NOT is not offered: bgpd treats anything but OR as AND). The operator is written through to every referencing term's inline copy, which bgpd requires to agree with the list ("Conflicting boolean_operator"). `delete ... as-path-list regex ` removes a single pattern and is allowed on a referenced list. Adds boost_regex to the fboss2 config lib (BUCK: boost_regex; cmake: Boost::regex, regex component) and the unit/integration coverage for each path. Co-Authored-By: Claude Fable 5.1 --- CMakeLists.txt | 2 +- cmake/CliFboss2.cmake | 1 + fboss/cli/fboss2/BUCK | 3 + fboss/cli/fboss2/CmdListConfig.cpp | 6 +- .../as-path-list/BgpAsPathListCliUtils.h | 65 ++++++++ .../CmdConfigProtocolBgpPolicyAsPathList.cpp | 86 ++++++++++ .../CmdConfigProtocolBgpPolicyAsPathList.h | 5 + .../CmdDeleteProtocolBgpPolicyAsPathList.cpp | 75 ++++++++- .../CmdDeleteProtocolBgpPolicyAsPathList.h | 18 ++- .../CmdConfigBgpPolicyAsPathListTest.cpp | 127 +++++++++++++++ .../CmdDeleteBgpPolicyAsPathListTest.cpp | 152 +++++++++++++++++- .../ConfigBgpPolicyAsPathListTest.cpp | 44 +++++ 12 files changed, 572 insertions(+), 12 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5499f65692912..428fd1b512144 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -291,7 +291,7 @@ find_package(Glog REQUIRED) include_directories(${GLOG_INCLUDE_DIRS}) find_package(folly CONFIG REQUIRED) include_directories(${FOLLY_INCLUDE_DIR}) -find_package(Boost REQUIRED COMPONENTS container filesystem) +find_package(Boost REQUIRED COMPONENTS container filesystem regex) include_directories(${Boost_INCLUDE_DIR}) find_package(fb303 CONFIG REQUIRED) find_package(fizz CONFIG REQUIRED) diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 07238e5dd4769..a7068d9f2920a 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -1168,6 +1168,7 @@ target_link_libraries(fboss2_config_lib bgp_config_cpp2 switchinfo_utils platform_mapping + Boost::regex Folly::folly ) diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index be3d268986c52..405d3951fe673 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1466,6 +1466,9 @@ cpp_library( "//thrift/lib/cpp2:field_ref", "//thrift/lib/cpp2/gen:module_types_h", ], + external_deps = [ + ("boost", None, "boost_regex"), + ], ) export_file( diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 278ddee598715..a17746a115306 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -447,7 +447,8 @@ const CommandTree& kConfigCommandTree() { {{ "as-path-list", "Configure BGP AS-path list: " - "[ ...] (description)", + "[ ...] (description, " + "regex, boolean-operator)", commandHandler< CmdConfigProtocolBgpPolicyAsPathList>, argRegistrar< @@ -803,7 +804,8 @@ const CommandTree& kConfigCommandTree() { argTypeHandler, {{ "as-path-list", - "Delete a BGP AS-path list: ", + "Delete a BGP AS-path list, or one of its regexes: " + " [regex ]", commandHandler, argRegistrar< CmdDeleteProtocolBgpPolicyAsPathListTraits>, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h index 5c627006a629f..cb0f40eb5a0b2 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h @@ -10,10 +10,12 @@ #pragma once +#include #include #include #include #include +#include #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" /** @@ -84,4 +86,67 @@ inline bgp::bgp_policy::AsPathListEntry& findOrCreateAsPathListEntry( return entry; } +inline const bgp::bgp_policy::AsPathList* findAsPathList( + const bgp::thrift::BgpConfig& cfg, + const std::string& name) { + if (!cfg.policies().has_value()) { + return nullptr; + } + const auto& lists = *cfg.policies()->aspath_lists(); + auto it = std::find_if(lists.begin(), lists.end(), [&](const auto& list) { + return *list.name() == name; + }); + return it == lists.end() ? nullptr : &*it; +} + +// A routing-policy term whose AS_PATH match names an as-path-list. `label` +// identifies the term the way the CLI addresses it (`policy

term `; +// the term name when it has no sequence number) so a rejection can be acted +// on directly. +struct AsPathListReference { + bgp::bgp_policy::BgpPolicyAtomicMatch* match; + std::string label; +}; + +// Every AS_PATH match whose as_path_filters.as_path_list_names carries +// `listName`. bgpd resolves those names at config load and fails the load on +// a dangling one, so the delete command refuses while this is non-empty, and +// the boolean-operator setter writes through to each match (bgpd requires the +// referencing copy to carry the same operator as the list). +inline std::vector findTermsReferencingAsPathList( + bgp::thrift::BgpConfig& cfg, + const std::string& listName) { + std::vector refs; + if (!cfg.policies().has_value()) { + return refs; + } + for (auto& policy : *cfg.policies()->bgp_policy_statements()) { + for (auto& term : *policy.policy_entries()) { + if (!term.policy_match_entries().has_value()) { + continue; + } + for (auto& match : *term.policy_match_entries()->match_entries()) { + if (!match.as_path_filters().has_value() || + !match.as_path_filters()->as_path_list_names().has_value()) { + continue; + } + const auto& names = *match.as_path_filters()->as_path_list_names(); + if (std::find(names.begin(), names.end(), listName) == names.end()) { + continue; + } + refs.push_back( + {&match, + term.sequence_number().has_value() + ? fmt::format( + "policy {} term {}", + *policy.name(), + *term.sequence_number()) + : fmt::format( + "policy {} term {}", *policy.name(), *term.name())}); + } + } + } + return refs; +} + } // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp index e4c6edbc7c0ec..4392e824d916d 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp @@ -12,17 +12,22 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" +#include #include #include +#include +#include #include #include #include +#include #include #include #include #include #include #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" @@ -37,31 +42,102 @@ namespace { // The attribute names, exactly as documented. Kept here so the // valid-attribute set and the handler table stay in sync. The list's entries // are their own subcommand, not attributes. +constexpr std::string_view kBooleanOperator = "boolean-operator"; constexpr std::string_view kDescription = "description"; +constexpr std::string_view kRegex = "regex"; + +constexpr std::string_view kBooleanOperatorAnd = "AND"; +constexpr std::string_view kBooleanOperatorOr = "OR"; using AsPathList = bgp::bgp_policy::AsPathList; +using BooleanOperator = bgp::routing_policy::BooleanOperator; using bgpcli::AttrHandler; +using bgpcli::enumAttr; +using bgpcli::err; using bgpcli::joinedStringAttr; using bgpcli::ok; using bgpcli::Result; +using bgpcli::Tokens; + +// NOT is deliberately not offered: bgpd treats every operator other than OR +// as "all must match", so NOT would silently behave as AND. +std::optional lookupBooleanOperator(const std::string& s) { + if (s == kBooleanOperatorAnd) { + return BooleanOperator::AND; + } + if (s == kBooleanOperatorOr) { + return BooleanOperator::OR; + } + return std::nullopt; +} // ---- setters ---------------------------------------------------------------- // Each writes one already-parsed, already-validated value. Parsing and message // text belong to the shared factories in BgpCliAttrHandlers.h. +void setListBooleanOperator(AsPathList& list, BooleanOperator op) { + list.boolean_operator() = op; +} + void setListDescription(AsPathList& list, const std::string& description) { list.description() = description; } +// Hand-written because no factory covers an accumulating, validated set: +// each `regex` appends one boost::regex to as_paths (the field bgpd matches +// against the `_`-joined AS path), compiled here so a pattern bgpd would +// reject at load never reaches the session. Idempotent on repeats. +Result regex(AsPathList& list, const Tokens& values) { + if (values.size() != 1 || values[0].empty()) { + return err( + fmt::format( + "Error: {} requires (one token; join AS numbers with `_`, " + "e.g. ^65000_65001$)", + kRegex)); + } + const auto& pattern = values[0]; + if (std::any_of(pattern.begin(), pattern.end(), [](unsigned char c) { + return std::isspace(c) != 0; + })) { + return err( + fmt::format( + "Error: {} '{}' must not contain whitespace; bgpd renders the AS " + "path with `_` between AS numbers", + kRegex, + pattern)); + } + try { + boost::regex compiled(pattern); + } catch (const boost::regex_error& e) { + return err( + fmt::format("Error: Malformed {} '{}': {}", kRegex, pattern, e.what())); + } + auto& paths = list.as_paths().ensure(); + if (std::find(paths.begin(), paths.end(), pattern) != paths.end()) { + return ok(fmt::format("{} {} already present", kRegex, pattern)); + } + paths.push_back(pattern); + return ok(fmt::format("Successfully added {} {}", kRegex, pattern)); +} + // ---- list-level attribute registry ------------------------------------------ // One line per documented attribute: its dispatch key, its value shape, and // the setter that stores it. const std::map, std::less<>>& listAttrHandlers() { + static const std::string kBooleanOperatorValues = + fmt::format("{}|{}", kBooleanOperatorAnd, kBooleanOperatorOr); static const std::map, std::less<>> kHandlers = { + {std::string(kBooleanOperator), + enumAttr( + kBooleanOperator, + kBooleanOperatorValues, + lookupBooleanOperator, + setListBooleanOperator)}, {std::string(kDescription), joinedStringAttr(kDescription, setListDescription)}, + {std::string(kRegex), regex}, }; return kHandlers; } @@ -134,6 +210,16 @@ CmdConfigProtocolBgpPolicyAsPathList::queryClient( if (!args.attr().empty()) { result.message += fmt::format(" for as-path-list {}", args.listName()); } + if (args.attr() == kBooleanOperator) { + // bgpd rejects a term whose inline copy of the list carries a different + // operator ("Conflicting boolean_operator"), so keep every referencing + // match in step with the list. + for (auto& ref : + bgpcli::findTermsReferencingAsPathList(cfg, args.listName())) { + ref.match->as_path_filters()->boolean_operator() = + *list.boolean_operator(); + } + } session.saveBgpConfig(); result.message += fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h index 7fe25d5f548de..c3f574ae0693e 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h @@ -27,6 +27,11 @@ namespace facebook::fboss { // Grammar (from the FBOSS proposed syntax): // as-path-list (create/select list) // as-path-list description (list attribute) +// as-path-list regex (append to as_paths, the +// field bgpd matches on) +// as-path-list boolean-operator (also written through to +// every term referencing +// the list) class BgpAsPathListConfig : public utils::BaseObjectArgType { public: // NOLINTNEXTLINE(google-explicit-constructor) diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp index b9f3bfb68dab5..9b025078f29dd 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp @@ -13,14 +13,17 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include +#include #include #include #include #include #include #include +#include #include #include +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" @@ -28,18 +31,41 @@ namespace facebook::fboss { +namespace { +constexpr std::string_view kRegexKeyword = "regex"; +} // namespace + // Parse + validate at construction so queryClient stays a thin dispatch. BgpAsPathListRef::BgpAsPathListRef(std::vector v) : utils::BaseObjectArgType(v) { - if (v.size() != 1) { + if (v.empty()) { throw std::invalid_argument( - "Error: delete protocol bgp policy as-path-list requires exactly one " - ""); + "Error: delete protocol bgp policy as-path-list requires , " + "optionally followed by `regex `"); } if (v[0].empty()) { throw std::invalid_argument("Error: as-path-list name must not be empty"); } - listName_ = std::move(v[0]); + listName_ = v[0]; + if (v.size() == 1) { + return; + } + if (v[1] != kRegexKeyword) { + throw std::invalid_argument( + fmt::format( + "Error: unexpected token '{}'. Usage: delete protocol bgp policy " + "as-path-list [regex ]", + v[1])); + } + if (v.size() < 3 || v[2].empty()) { + throw std::invalid_argument("Error: `regex` requires a "); + } + if (v.size() > 3) { + throw std::invalid_argument( + fmt::format("Error: unexpected token '{}' after regex ", v[3])); + } + hasRegex_ = true; + regex_ = v[2]; } CmdDeleteProtocolBgpPolicyAsPathListTraits::RetType @@ -60,6 +86,47 @@ CmdDeleteProtocolBgpPolicyAsPathList::queryClient( if (it == lists.end()) { return fmt::format("Error: BGP as-path-list {} not found", args.listName()); } + if (args.hasRegex()) { + // Removing one pattern from a referenced list is safe: the name the terms + // resolve stays defined. + auto& list = *it; + if (list.as_paths().has_value()) { + auto& paths = *list.as_paths(); + auto pathIt = std::find(paths.begin(), paths.end(), args.regex()); + if (pathIt != paths.end()) { + paths.erase(pathIt); + if (paths.empty()) { + list.as_paths().reset(); + } + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP as-path-list {} regex {}\n" + "Config saved to: {}", + args.listName(), + args.regex(), + session.getBgpSessionConfigPath()); + } + } + return fmt::format( + "Error: BGP as-path-list {} regex {} not found", + args.listName(), + args.regex()); + } + // A term's as_path_list_names resolves against this list by name at daemon + // load; erasing the list while a term still names it would commit a + // dangling reference and fail the load. Refuse and name the terms instead. + std::vector referrers; + for (const auto& ref : + bgpcli::findTermsReferencingAsPathList(cfg, args.listName())) { + referrers.push_back(ref.label); + } + if (!referrers.empty()) { + return fmt::format( + "Error: BGP as-path-list {} is still referenced by {}; remove those " + "matches first", + args.listName(), + folly::join(", ", referrers)); + } lists.erase(it); session.saveBgpConfig(); return fmt::format( diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h index 4249bdd3546a4..874460157ef52 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h @@ -20,9 +20,11 @@ namespace facebook::fboss { -// Parsed `delete protocol bgp policy as-path-list `, validated at -// construction. The name is the list's identity, matched exactly the same way -// the config command stores it. Mirrors BgpPeerGroupRef. +// Parsed `delete protocol bgp policy as-path-list [regex ]`, +// validated at construction. The name is the list's identity, matched exactly +// the same way the config command stores it; the optional `regex` selector +// removes one as_paths pattern instead of the whole list. Mirrors +// BgpPeerGroupRef. class BgpAsPathListRef : public utils::BaseObjectArgType { public: // NOLINTNEXTLINE(google-explicit-constructor) @@ -30,17 +32,25 @@ class BgpAsPathListRef : public utils::BaseObjectArgType { const std::string& listName() const { return listName_; } + bool hasRegex() const { + return hasRegex_; + } + const std::string& regex() const { + return regex_; + } const static utils::ObjectArgTypeId id = utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; private: std::string listName_; + bool hasRegex_{false}; + std::string regex_; }; struct CmdDeleteProtocolBgpPolicyAsPathListTraits : public WriteCommandTraits { using ParentCmd = CmdDeleteProtocolBgpPolicy; static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("args", args, ""); + cmd.add_option("args", args, " [regex ]"); } using ObjectArgType = BgpAsPathListRef; using RetType = std::string; diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp index c0e96f09a023d..2d6ef6bc4fa16 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp @@ -10,6 +10,8 @@ #include #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" @@ -47,6 +49,30 @@ class CmdConfigBgpPolicyAsPathListTestFixture : public CmdConfigTestBase { .aspath_lists(); } + // Seed a routing-policy term whose AS_PATH match names `listName` (the + // term/match CLIs live in higher PRs), the way bgpd reads the reference: + // as_path_filters.as_path_list_names. Returns the seeded atomic match. + bgp::bgp_policy::BgpPolicyAtomicMatch& addPolicyTermMatching( + const std::string& policy, + int64_t seq, + const std::string& listName) { + auto& cfg = ConfigSession::getInstance().getBgpConfig(); + auto& policies = *cfg.policies().ensure().bgp_policy_statements(); + policies.emplace_back(); + policies.back().name() = policy; + auto& terms = *policies.back().policy_entries(); + terms.emplace_back(); + terms.back().sequence_number() = seq; + auto& matches = + *terms.back().policy_match_entries().ensure().match_entries(); + matches.emplace_back(); + matches.back().type() = bgp::bgp_policy::BgpPolicyAtomicMatchType::AS_PATH; + matches.back().as_path_filters().ensure().as_path_list_names().ensure() = { + listName}; + ConfigSession::getInstance().saveBgpConfig(); + return matches.back(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -78,6 +104,12 @@ TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, argValidation) { EXPECT_THROW( BgpAsPathListConfig({"AS100", "asn-regexp", "^65000_"}), std::invalid_argument); + + // List-level regex and boolean-operator are recognized. + EXPECT_EQ(BgpAsPathListConfig({"AS100", "regex", "^65000_"}).attr(), "regex"); + EXPECT_EQ( + BgpAsPathListConfig({"AS100", "boolean-operator", "AND"}).attr(), + "boolean-operator"); } // ============================================================================== @@ -134,4 +166,99 @@ TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, unknownAttributeRejected) { << "session file should not exist after rejected input"; } +// ============================================================================== +// regex — as_paths, the field bgpd matches against +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, regexAppends) { + auto result = run({"AS100", "regex", "^65000_"}); + EXPECT_THAT(result, HasSubstr("Successfully added regex ^65000_")); + EXPECT_THAT(result, HasSubstr("for as-path-list AS100")); + run({"AS100", "regex", "_65001$"}); + ASSERT_EQ(lists().size(), 1); + ASSERT_TRUE(lists()[0].as_paths().has_value()); + EXPECT_EQ( + *lists()[0].as_paths(), std::vector({"^65000_", "_65001$"})); + // The dead entry list is left alone. + EXPECT_TRUE(lists()[0].as_path_list()->empty()); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, regexAddAgainIsIdempotent) { + run({"AS100", "regex", "^65000_"}); + auto result = run({"AS100", "regex", "^65000_"}); + EXPECT_THAT(result, HasSubstr("already present")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(lists()[0].as_paths()->size(), 1); +} + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, malformedRegexRejected) { + auto result = run({"AS100", "regex", "^65000_("}); + EXPECT_THAT(result, HasSubstr("Error: Malformed regex")); + // A rejected first attribute leaves no phantom list and nothing on disk. + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, regexWithWhitespaceRejected) { + run({"AS100"}); + auto result = run({"AS100", "regex", "65000 65001"}); + EXPECT_THAT(result, HasSubstr("must not contain whitespace")); + EXPECT_THAT(result, HasSubstr("_")); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].as_paths().has_value()); + + EXPECT_THAT(run({"AS100", "regex"}), HasSubstr("requires ")); + EXPECT_THAT( + run({"AS100", "regex", "^65000_", "_65001$"}), + HasSubstr("requires ")); +} + +// ============================================================================== +// boolean-operator — AND|OR, written through to referencing terms +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, booleanOperatorAccepted) { + auto result = run({"AS100", "boolean-operator", "AND"}); + EXPECT_THAT(result, HasSubstr("Successfully set boolean-operator to: AND")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ( + *lists()[0].boolean_operator(), + bgp::routing_policy::BooleanOperator::AND); + run({"AS100", "boolean-operator", "OR"}); + EXPECT_EQ( + *lists()[0].boolean_operator(), bgp::routing_policy::BooleanOperator::OR); +} + +TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, booleanOperatorNotRejected) { + auto result = run({"AS100", "boolean-operator", "NOT"}); + EXPECT_THAT(result, HasSubstr("Invalid boolean-operator value 'NOT'")); + EXPECT_THAT(result, HasSubstr("AND|OR")); + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()); +} + +TEST_F( + CmdConfigBgpPolicyAsPathListTestFixture, + booleanOperatorPropagatesToReferencingTerms) { + run({"AS100"}); + run({"AS200"}); + const auto& match = addPolicyTermMatching("RM100", 10, "AS100"); + const auto& other = addPolicyTermMatching("RM100", 20, "AS200"); + EXPECT_EQ( + *match.as_path_filters()->boolean_operator(), + bgp::routing_policy::BooleanOperator::OR); + + run({"AS100", "boolean-operator", "AND"}); + // bgpd requires the term's inline copy to carry the list's operator. + EXPECT_EQ( + *match.as_path_filters()->boolean_operator(), + bgp::routing_policy::BooleanOperator::AND); + // A term referencing a different list is untouched. + EXPECT_EQ( + *other.as_path_filters()->boolean_operator(), + bgp::routing_policy::BooleanOperator::OR); +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp index 7c575a4dd51e0..a8337a0ba9a6c 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp @@ -4,7 +4,9 @@ #include #include +#include #include +#include #include #include #include @@ -54,6 +56,35 @@ class CmdDeleteBgpPolicyAsPathListTestFixture : public CmdConfigTestBase { .aspath_lists(); } + // Seed a routing-policy term whose AS_PATH match names `listName` (the + // term/match CLIs live in higher PRs), the way bgpd reads the reference: + // as_path_filters.as_path_list_names. + void addPolicyTermMatching( + const std::string& policy, + int64_t seq, + const std::string& listName) { + auto& cfg = ConfigSession::getInstance().getBgpConfig(); + auto& policies = *cfg.policies().ensure().bgp_policy_statements(); + auto it = std::find_if(policies.begin(), policies.end(), [&](auto& p) { + return *p.name() == policy; + }); + if (it == policies.end()) { + policies.emplace_back(); + policies.back().name() = policy; + it = std::prev(policies.end()); + } + auto& terms = *it->policy_entries(); + terms.emplace_back(); + terms.back().sequence_number() = seq; + auto& matches = + *terms.back().policy_match_entries().ensure().match_entries(); + matches.emplace_back(); + matches.back().type() = bgp::bgp_policy::BgpPolicyAtomicMatchType::AS_PATH; + matches.back().as_path_filters().ensure().as_path_list_names().ensure() = { + listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -66,11 +97,23 @@ class CmdDeleteBgpPolicyAsPathListTestFixture : public CmdConfigTestBase { TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, argValidation) { EXPECT_EQ(BgpAsPathListRef({"AS100"}).listName(), "AS100"); + EXPECT_FALSE(BgpAsPathListRef({"AS100"}).hasRegex()); - // Invalid: empty, empty name, extra tokens. + // Optional `regex ` selector. + auto withRegex = BgpAsPathListRef({"AS100", "regex", "^65000_"}); + EXPECT_EQ(withRegex.listName(), "AS100"); + EXPECT_TRUE(withRegex.hasRegex()); + EXPECT_EQ(withRegex.regex(), "^65000_"); + + // Invalid: empty, empty name, extra tokens, incomplete or trailing selector. EXPECT_THROW(BgpAsPathListRef({}), std::invalid_argument); EXPECT_THROW(BgpAsPathListRef({""}), std::invalid_argument); EXPECT_THROW(BgpAsPathListRef({"AS100", "AS200"}), std::invalid_argument); + EXPECT_THROW(BgpAsPathListRef({"AS100", "regex"}), std::invalid_argument); + EXPECT_THROW(BgpAsPathListRef({"AS100", "regex", ""}), std::invalid_argument); + EXPECT_THROW( + BgpAsPathListRef({"AS100", "regex", "^65000_", "extra"}), + std::invalid_argument); } // ============================================================================== @@ -110,4 +153,111 @@ TEST_F( EXPECT_EQ(*lists()[0].name(), "AS100"); } +// ============================================================================== +// Reference guard — a list a routing-policy term still names is not deletable +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteReferencedListRejected) { + configure({"AS100", "description", "in-use"}); + addPolicyTermMatching("RM100", 10, "AS100"); + + auto result = del({"AS100"}); + EXPECT_THAT(result, HasSubstr("still referenced")); + // The refusal names the policy and term so the user can act on it. + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + EXPECT_THAT(result, HasSubstr("remove those matches first")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "AS100"); + EXPECT_EQ(*lists()[0].description(), "in-use"); +} + +TEST_F( + CmdDeleteBgpPolicyAsPathListTestFixture, + deleteReferencedListNamesEveryReferrer) { + configure({"AS100"}); + addPolicyTermMatching("RM100", 10, "AS100"); + addPolicyTermMatching("RM200", 20, "AS100"); + + auto result = del({"AS100"}); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + EXPECT_THAT(result, HasSubstr("policy RM200 term 20")); + ASSERT_EQ(lists().size(), 1); +} + +TEST_F( + CmdDeleteBgpPolicyAsPathListTestFixture, + referenceToOtherListDoesNotBlockDelete) { + configure({"AS100"}); + configure({"AS200"}); + // A term referencing AS100 must not block deleting AS200. + addPolicyTermMatching("RM100", 10, "AS100"); + + auto result = del({"AS200"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted BGP as-path-list AS200")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "AS100"); +} + +TEST_F( + CmdDeleteBgpPolicyAsPathListTestFixture, + rejectedDeleteDoesNotStageWhenNothingStaged) { + // Seed the reference without going through the session file, then confirm + // the refused delete writes nothing. + auto& cfg = ConfigSession::getInstance().getBgpConfig(); + auto& list = cfg.policies().ensure().aspath_lists()->emplace_back(); + list.name() = "AS100"; + auto& policies = *cfg.policies()->bgp_policy_statements(); + policies.emplace_back(); + policies.back().name() = "RM100"; + auto& term = policies.back().policy_entries()->emplace_back(); + term.sequence_number() = 10; + auto& match = + term.policy_match_entries().ensure().match_entries()->emplace_back(); + match.type() = bgp::bgp_policy::BgpPolicyAtomicMatchType::AS_PATH; + match.as_path_filters().ensure().as_path_list_names().ensure() = {"AS100"}; + ASSERT_FALSE(sessionFileExists()); + + auto result = del({"AS100"}); + EXPECT_THAT(result, HasSubstr("still referenced")); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected delete"; +} + +// ============================================================================== +// regex selector — remove one pattern, keep the list +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteOneRegex) { + configure({"AS100", "regex", "^65000_"}); + configure({"AS100", "regex", "_65001$"}); + // Removing a pattern from a referenced list is fine: the name stays defined. + addPolicyTermMatching("RM100", 10, "AS100"); + + auto result = del({"AS100", "regex", "^65000_"}); + EXPECT_THAT( + result, + HasSubstr("Successfully deleted BGP as-path-list AS100 regex ^65000_")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].as_paths(), std::vector({"_65001$"})); + + // Deleting the last pattern clears as_paths but keeps the list. + del({"AS100", "regex", "_65001$"}); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].as_paths().has_value()); +} + +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteUnknownRegexRejected) { + configure({"AS100", "regex", "^65000_"}); + auto result = del({"AS100", "regex", "^65002_"}); + EXPECT_THAT( + result, + HasSubstr("Error: BGP as-path-list AS100 regex ^65002_ not found")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(lists()[0].as_paths()->size(), 1); + + EXPECT_THAT( + del({"NO-SUCH-LIST", "regex", "^65000_"}), + HasSubstr("Error: BGP as-path-list NO-SUCH-LIST not found")); +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp index 62fb1bc07cb11..7597b59716247 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp @@ -20,6 +20,7 @@ */ #include +#include #include #include @@ -92,6 +93,49 @@ TEST_F(ConfigBgpPolicyAsPathListTest, SetListDescriptionAndCommit) { EXPECT_EQ((*runningList)["description"].asString(), "test spine as-paths"); } +TEST_F(ConfigBgpPolicyAsPathListTest, SetRegexAndOperatorAndCommit) { + discardSession(); + clearBgpSession(); + stageAsPathList({kList, "regex", "^65000_"}); + stageAsPathList({kList, "regex", "_65001$"}); + stageAsPathList({kList, "boolean-operator", "AND"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + // bgpd matches on as_paths + boolean_operator (not the entry list); the + // running config proves it parsed and adopted both. + auto running = readRunningBgpConfigViaRpc(); + const auto* list = findList(running, kList); + ASSERT_NE(list, nullptr) << "bgpd's running config has no as-path-list " + << kList; + ASSERT_TRUE(list->count("as_paths")); + std::vector paths; + for (const auto& p : (*list)["as_paths"]) { + paths.push_back(p.asString()); + } + EXPECT_EQ(paths, std::vector({"^65000_", "_65001$"})); + ASSERT_TRUE(list->count("boolean_operator")); + // routing_policy.BooleanOperator.AND == 1 + EXPECT_EQ((*list)["boolean_operator"].asInt(), 1); +} + +TEST_F(ConfigBgpPolicyAsPathListTest, MalformedRegexRejected) { + clearBgpSession(); + auto result = runCli( + {"config", + "protocol", + "bgp", + "policy", + "as-path-list", + kList, + "regex", + "^65000_("}); + EXPECT_THAT(result.stdout, HasSubstr("Malformed regex")); + EXPECT_FALSE(std::filesystem::exists(bgpSessionPath())) + << "session file should not exist after rejected input"; +} + TEST_F(ConfigBgpPolicyAsPathListTest, DeleteListAndCommit) { // Land an as-path-list in the system config, then delete it through a second // commit and verify it is gone from bgpd's running config. From 007445fc53364bd5d1bf92df8362c41e5e643960 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:13:48 +0000 Subject: [PATCH 3/5] Emit as-path-list commands from bgp_json_to_cli.py bgp_json_to_cli.py rebuilt global, peer-group and peer settings only; every policy object in the input was dropped, so a replayed script could commit peers naming policies that no longer exist. This adds generate_as_path_list_commands (description, one `regex` line per as_paths pattern, `boolean-operator` when not the OR default) and a generate_policy_commands block that the nested families hook into. `--raw` and the script wrapper now share one generate_commands sequence: global, policy objects, then the peer-groups and peers that reference them, so the emitted order never stages a dangling name. Fields the CLI cannot express (as_path_list_names, the unread as_path_list entries, NOT) are emitted as `# WARNING:` comment lines, which the script wrapper passes through unprefixed instead of silently dropping the data. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 86 +++++++++++ .../tools/tests/test_bgp_json_to_cli.py | 136 ++++++++++++++++++ 2 files changed, 222 insertions(+) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index ca48e06d9f490..71ed0f5d8f607 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -418,6 +418,90 @@ def generate_peer_group_commands(peer_group: dict[str, Any]) -> list[str]: return commands +# Emitted verbatim (never prefixed with the binary) for JSON the CLI cannot +# express, so a replayed script neither fails nor silently drops the field. +_WARNING_PREFIX = "# WARNING:" + + +def _warning(text: str) -> str: + return f"{_WARNING_PREFIX} {text}" + + +_BOOLEAN_OPERATOR_NAMES = {1: "AND", 2: "OR", 3: "NOT"} + + +def _boolean_operator_name(raw: Any) -> str: + """routing_policy.BooleanOperator as its name, from the int or the name.""" + if isinstance(raw, str): + return raw + return _BOOLEAN_OPERATOR_NAMES.get(int(raw), str(raw)) + + +def generate_as_path_list_commands(as_path_list: dict[str, Any]) -> list[str]: + """Generate `config protocol bgp policy as-path-list` commands for one list. + + bgpd matches on `as_paths` (one `regex` line each) and `boolean_operator` + (emitted only when it differs from the OR default). `as_path_list_names` + and the `as_path_list` entries are never read by bgpd and have no CLI + spelling here, so they surface as warnings rather than vanishing. + """ + name = as_path_list.get("name", "") + if not name: + return [] + + prefix = f"config protocol bgp policy as-path-list {escape_shell_arg(name)}" + commands = [] + if as_path_list.get("description"): + commands.append( + f"{prefix} description {escape_shell_arg(as_path_list['description'])}" + ) + for regex in as_path_list.get("as_paths") or []: + commands.append(f"{prefix} regex {escape_shell_arg(regex)}") + if "boolean_operator" in as_path_list: + operator = _boolean_operator_name(as_path_list["boolean_operator"]) + if operator == "NOT": + commands.append( + _warning( + f"as-path-list {name}: boolean_operator NOT is not " + "accepted by the CLI (bgpd treats it as AND); not emitted" + ) + ) + elif operator != "OR": + commands.append(f"{prefix} boolean-operator {escape_shell_arg(operator)}") + if as_path_list.get("as_path_list_names"): + commands.append( + _warning( + f"as-path-list {name}: as_path_list_names is not read by bgpd " + "and has no CLI equivalent; not emitted" + ) + ) + if as_path_list.get("as_path_list"): + commands.append( + _warning( + f"as-path-list {name}: as_path_list entries are not read by " + "bgpd (bgpd matches as_paths); not emitted" + ) + ) + if not commands: + # Nothing to set: still recreate the (empty) list by name. + commands.append(prefix) + return commands + + +def generate_policy_commands(config: dict[str, Any]) -> list[str]: + """Generate the `config protocol bgp policy ...` commands. + + Lists come before the routing-policies that reference them, and the whole + block precedes the peer-group/peer commands that name a policy, so a + replayed script never stages a dangling reference. + """ + policies = config.get("policies", {}) + commands = [] + for as_path_list in policies.get("aspath_lists", []): + commands.extend(generate_as_path_list_commands(as_path_list)) + return commands + + def generate_exec_commands(commands: list[str], binary: str = "fboss2") -> list[str]: """Generate executable commands with custom binary.""" exec_commands = [ @@ -447,6 +531,8 @@ def generate_commands(config: dict[str, Any]) -> list[str]: for fields that have no CLI equivalent).""" commands = [] commands.extend(generate_global_commands(config)) + # Policy objects before the peer-group/peer lines that reference them. + commands.extend(generate_policy_commands(config)) for peer_group in config.get("peer_groups", []): commands.extend(generate_peer_group_commands(peer_group)) for peer in config.get("peers", []): 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 462c100792f33..0ced31be80c67 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 @@ -21,11 +21,13 @@ from fboss.cli.fboss2.tools.bgp_json_to_cli import ( escape_shell_arg, format_bandwidth, + generate_as_path_list_commands, generate_commands, generate_exec_commands, generate_global_commands, generate_peer_commands, generate_peer_group_commands, + generate_policy_commands, json_to_cli, ) @@ -985,6 +987,100 @@ def peer_config(peer: dict) -> dict: return {"peers": [peer]} +class GenerateAsPathListCommandsTest(unittest.TestCase): + """Tests for generate_as_path_list_commands (as-path-list grammar).""" + + PREFIX = "config protocol bgp policy as-path-list ASPL" + + def test_empty_name_returns_empty(self) -> None: + self.assertEqual(generate_as_path_list_commands({"as_paths": ["^1_"]}), []) + + def test_bare_list_is_recreated(self) -> None: + """A list with nothing to set still comes back by name.""" + self.assertEqual( + generate_as_path_list_commands({"name": "ASPL"}), [self.PREFIX] + ) + + def test_description(self) -> None: + commands = generate_as_path_list_commands( + {"name": "ASPL", "description": "spine as paths"} + ) + self.assertEqual(commands, [f"{self.PREFIX} description 'spine as paths'"]) + + def test_empty_description_not_emitted(self) -> None: + """bgpd's running config carries the "" default; nothing to replay.""" + commands = generate_as_path_list_commands({"name": "ASPL", "description": ""}) + self.assertEqual(commands, [self.PREFIX]) + + def test_regex_per_as_path(self) -> None: + """Each as_paths pattern is one quoted `regex` line, in order.""" + commands = generate_as_path_list_commands( + {"name": "ASPL", "as_paths": ["^65000_", "_65001$"]} + ) + self.assertEqual( + commands, + [f"{self.PREFIX} regex '^65000_'", f"{self.PREFIX} regex '_65001$'"], + ) + + def test_boolean_operator_and(self) -> None: + for raw in (1, "AND"): + commands = generate_as_path_list_commands( + {"name": "ASPL", "boolean_operator": raw} + ) + self.assertEqual(commands, [f"{self.PREFIX} boolean-operator AND"], raw) + + def test_boolean_operator_or_is_default(self) -> None: + """OR is the thrift default and the CLI's; nothing to emit.""" + for raw in (2, "OR"): + commands = generate_as_path_list_commands( + {"name": "ASPL", "boolean_operator": raw} + ) + self.assertEqual(commands, [self.PREFIX], raw) + + def test_boolean_operator_not_warns(self) -> None: + commands = generate_as_path_list_commands( + {"name": "ASPL", "boolean_operator": 3} + ) + self.assertEqual(len(commands), 1) + self.assertTrue(commands[0].startswith("# WARNING:"), commands[0]) + self.assertIn("NOT", commands[0]) + + def test_dead_fields_warn(self) -> None: + """Fields bgpd never reads surface as warnings, not silently dropped.""" + commands = generate_as_path_list_commands( + { + "name": "ASPL", + "as_path_list_names": ["OTHER"], + "as_path_list": [{"sequence_number": 10}], + } + ) + self.assertEqual(len(commands), 2) + self.assertIn("as_path_list_names", commands[0]) + self.assertIn("as_path_list entries", commands[1]) + for c in commands: + self.assertTrue(c.startswith("# WARNING:"), c) + + def test_injection_in_regex_neutralized(self) -> None: + commands = generate_as_path_list_commands( + {"name": "ASPL", "as_paths": ["$(reboot)"]} + ) + self.assertEqual(commands, [f"{self.PREFIX} regex '$(reboot)'"]) + + def test_policy_block_orders_lists_first(self) -> None: + config = {"policies": {"aspath_lists": [{"name": "A"}, {"name": "B"}]}} + commands = generate_policy_commands(config) + self.assertEqual( + commands, + [ + "config protocol bgp policy as-path-list A", + "config protocol bgp policy as-path-list B", + ], + ) + + def test_no_policies_key(self) -> None: + self.assertEqual(generate_policy_commands({}), []) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands.""" @@ -996,6 +1092,15 @@ def test_exec_commands_contains_header(self) -> None: # Check for set -e with any comment self.assertTrue(any("set -e" in cmd for cmd in result)) + def test_exec_commands_pass_warnings_through(self) -> None: + """Warning comments are never prefixed with the binary.""" + result = generate_exec_commands( + ["config protocol bgp global hold-time 90", "# WARNING: x"], "fboss2" + ) + self.assertIn("fboss2 config protocol bgp global hold-time 90", result) + self.assertIn("# WARNING: x", result) + self.assertNotIn("fboss2 # WARNING: x", result) + def test_exec_commands_prepends_binary(self) -> None: """Exec commands should prepend binary to each command.""" commands = ["config protocol bgp global router-id 10.0.0.1"] @@ -1021,6 +1126,37 @@ def test_generate_commands_carries_warnings_in_raw_output(self) -> None: self.assertTrue(commands[0].startswith("# WARNING: neighbor 2001:db8::1")) +class GenerateCommandsOrderTest(unittest.TestCase): + """generate_commands is the single generator sequence for --raw and the + script: global, then policy objects, then the peer-groups and peers that + reference them.""" + + def test_policies_between_global_and_peer_groups(self) -> None: + config = { + "router_id": "10.0.0.1", + "policies": {"aspath_lists": [{"name": "ASPL", "as_paths": ["^1_"]}]}, + "peer_groups": [{"name": "PG", "remote_as_4_byte": 65000}], + "peers": [{"peer_addr": "10.0.0.2", "peer_group_name": "PG"}], + } + commands = generate_commands(config) + idx = { + "global": next(i for i, c in enumerate(commands) if "global" in c), + "policy": next(i for i, c in enumerate(commands) if "as-path-list" in c), + "peer_group": next( + i for i, c in enumerate(commands) if "peer-group PG" in c + ), + "peer": next(i for i, c in enumerate(commands) if "neighbor" in c), + } + self.assertLess(idx["global"], idx["policy"]) + self.assertLess(idx["policy"], idx["peer_group"]) + self.assertLess(idx["peer_group"], idx["peer"]) + + def test_script_wraps_same_sequence(self) -> None: + config = {"policies": {"aspath_lists": [{"name": "ASPL"}]}} + script = json_to_cli(config, binary="fboss2") + self.assertIn("fboss2 config protocol bgp policy as-path-list ASPL", script) + + class JsonToCliIntegrationTest(unittest.TestCase): """Integration tests for json_to_cli function.""" From c91affed41a0957c8fbc48590662b5a88a10494f Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 16 Sep 2026 07:40:29 +0000 Subject: [PATCH 4/5] Make as-path-list delete idempotent; document the flat regex grammar Delete mirrors add: deleting an absent list or an absent regex is a success with a `Warning:` and saves nothing, so a replayed script stays idempotent. The term-reference guard still errors. bgpd matches on the flat `as_paths` string list and never reads the structured `as_path_list[]` entries, so the helpers that keyed entries by sequence number are dead; drop them and describe the flat grammar in the CLI doc. `positionals_at_end()` is restored on the leaf command now that it has no subcommand. Co-Authored-By: Claude Fable 5.1 --- docs/docs/debugging/fboss_cli.md | 2 +- .../as-path-list/BgpAsPathListCliUtils.h | 37 ++-------- .../CmdConfigProtocolBgpPolicyAsPathList.cpp | 8 ++- .../CmdConfigProtocolBgpPolicyAsPathList.h | 11 +-- .../CmdDeleteProtocolBgpPolicyAsPathList.cpp | 15 ++-- .../CmdConfigBgpPolicyAsPathListTest.cpp | 4 +- .../CmdDeleteBgpPolicyAsPathListTest.cpp | 69 ++++++++++++++++--- .../ConfigBgpPolicyAsPathListTest.cpp | 4 +- 8 files changed, 89 insertions(+), 61 deletions(-) diff --git a/docs/docs/debugging/fboss_cli.md b/docs/docs/debugging/fboss_cli.md index 53dcc7e3ddfc8..e8276233611cd 100644 --- a/docs/docs/debugging/fboss_cli.md +++ b/docs/docs/debugging/fboss_cli.md @@ -1249,7 +1249,7 @@ The BGP command families are: - **`config protocol bgp global [ ...]`**: process-wide settings such as `router-id`, `local-asn`, `hold-time`, `confed-asn`, `cluster-id`, `network6` (advertised networks), and `switch-limit` overload-protection settings. - **`config protocol bgp neighbor [ ...]`**: per-neighbor settings such as `remote-asn`, `local-asn`, `peer-group`, `description`, `bind-addr address`, `next-hop4`/`next-hop6`/`next-hop-self`, `passive `, `afi disable-ipv4-afi`/`disable-ipv6-afi`, `rr-client`, `confed-peer`, `type`, `ingress-policy`/`egress-policy`, `add-path send|receive`, `link-bandwidth`, `graceful-restart restart-time`, `timers hold-time|keepalive|out-delay|withdraw-unprog-delay`, and `max-route pre-filter|post-filter` limits. A bare `neighbor ` creates the neighbor. - **`config protocol bgp peer-group [ ...]`**: the same attribute family applied to a peer group, which neighbors can then reference via `peer-group`. -- **`config protocol bgp policy as-path-list [entry ] [ ...]`**: BGP routing policy objects. `as-path-list` entries support `asn-regexp`, `description`, and `match-logic `. More policy object types (community-list, prefix-list, routing-policy) will follow the same pattern. +- **`config protocol bgp policy as-path-list [ ...]`**: BGP routing policy objects. An `as-path-list` is a flat set of AS-path regexes: `regex ` appends one pattern (repeat the command per pattern; join AS numbers with `_`, e.g. `^65000_65001$`), plus `description` and `boolean-operator `. `delete ... as-path-list regex ` removes one pattern. More policy object types (community-list, prefix-list, routing-policy) follow the same pattern. Each family has a `delete` counterpart, e.g. `fboss2-dev delete protocol bgp neighbor `. diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h index cb0f40eb5a0b2..f267990aac1c3 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h @@ -13,17 +13,16 @@ #include #include #include -#include #include #include #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" /** * Lookup/create helpers for the as-path-list CLI family, shared between the - * list-level dispatcher (CmdConfigProtocolBgpPolicyAsPathList), the entry - * subcommand (CmdConfigProtocolBgpPolicyAsPathListEntry), and the delete - * counterparts. An AsPathList is keyed by name; an AsPathListEntry (in - * as_path_list[]) is keyed by sequence_number. + * list-level dispatcher (CmdConfigProtocolBgpPolicyAsPathList) and its delete + * counterpart. An AsPathList is keyed by name. Its patterns live in the flat + * as_paths string list, which is the field bgpd matches against; the + * structured as_path_list[] entries are never read by bgpd and have no CLI. */ namespace facebook::fboss::bgpcli { @@ -58,34 +57,6 @@ inline bgp::bgp_policy::AsPathList& findOrCreateAsPathList( return list; } -inline bool asPathListEntryExists( - const bgp::bgp_policy::AsPathList& list, - int64_t seqNum) { - const auto& entries = *list.as_path_list(); - return std::any_of(entries.begin(), entries.end(), [&](const auto& entry) { - return entry.sequence_number().has_value() && - *entry.sequence_number() == seqNum; - }); -} - -// Find the entry keyed by sequence_number within a list, creating it if -// absent. sequence_number is the entry's identity. -inline bgp::bgp_policy::AsPathListEntry& findOrCreateAsPathListEntry( - bgp::bgp_policy::AsPathList& list, - int64_t seqNum) { - auto& entries = *list.as_path_list(); - for (auto& entry : entries) { - if (entry.sequence_number().has_value() && - *entry.sequence_number() == seqNum) { - return entry; - } - } - entries.emplace_back(); - auto& entry = entries.back(); - entry.sequence_number() = seqNum; - return entry; -} - inline const bgp::bgp_policy::AsPathList* findAsPathList( const bgp::thrift::BgpConfig& cfg, const std::string& name) { diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp index 4392e824d916d..b9c520ad5e32e 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp @@ -12,7 +12,7 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" -#include +#include // NOLINT(misc-include-cleaner) #include #include #include @@ -40,8 +40,10 @@ namespace facebook::fboss { namespace { // The attribute names, exactly as documented. Kept here so the -// valid-attribute set and the handler table stay in sync. The list's entries -// are their own subcommand, not attributes. +// valid-attribute set and the handler table stay in sync. Patterns are flat +// `regex` attributes because bgpd stores them as a flat string list; nested +// entries are reserved for families where the daemon stores objects +// (prefix-list, routing-policy). constexpr std::string_view kBooleanOperator = "boolean-operator"; constexpr std::string_view kDescription = "description"; constexpr std::string_view kRegex = "regex"; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h index c3f574ae0693e..74cc1d275dfde 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h @@ -21,8 +21,9 @@ namespace facebook::fboss { // Parsed `as-path-list [ ...]`, validated at -// construction. An AsPathList is keyed by . The list's entries are their -// own subcommand (CmdConfigProtocolBgpPolicyAsPathListEntry), not attributes. +// construction. An AsPathList is keyed by . Its patterns are a flat +// string list in the daemon, so they are flat `regex` attributes here (one per +// invocation), not nested entries. // // Grammar (from the FBOSS proposed syntax): // as-path-list (create/select list) @@ -61,9 +62,9 @@ class BgpAsPathListConfig : public utils::BaseObjectArgType { struct CmdConfigProtocolBgpPolicyAsPathListTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgpPolicy; static void addCliArg(CLI::App& cmd, std::vector& args) { - // No positionals_at_end() here: CLI11 must stay free to classify the - // `entry` token as this command's subcommand rather than swallowing it - // into args. See CmdConfigProtocolBgpPolicyRoutingPolicyTraits. + // Stops CLI11 from classifying attribute tokens as subcommands once the + // list name is consumed. See CmdConfigProtocolBgpNeighborTraits. + cmd.positionals_at_end(); cmd.add_option("args", args, " [ ...]"); } using ObjectArgType = BgpAsPathListConfig; diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp index 9b025078f29dd..f701be2f68690 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp @@ -21,7 +21,6 @@ #include #include #include -#include #include #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" @@ -74,17 +73,21 @@ CmdDeleteProtocolBgpPolicyAsPathList::queryClient( const ObjectArgType& args) { auto& session = ConfigSession::getInstance(); auto& cfg = session.getBgpConfig(); + // Delete mirrors add: an absent target 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 as-path-list {} does not exist; nothing to delete", + args.listName()); if (!cfg.policies().has_value()) { - // Nothing is persisted for an unknown list, so a typo'd delete can't stage - // an unrelated session change. - return fmt::format("Error: BGP as-path-list {} not found", args.listName()); + return absent; } auto& lists = *cfg.policies()->aspath_lists(); auto it = std::find_if(lists.begin(), lists.end(), [&](const auto& list) { return *list.name() == args.listName(); }); if (it == lists.end()) { - return fmt::format("Error: BGP as-path-list {} not found", args.listName()); + return absent; } if (args.hasRegex()) { // Removing one pattern from a referenced list is safe: the name the terms @@ -108,7 +111,7 @@ CmdDeleteProtocolBgpPolicyAsPathList::queryClient( } } return fmt::format( - "Error: BGP as-path-list {} regex {} not found", + "Warning: BGP as-path-list {} has no regex {}; nothing to delete", args.listName(), args.regex()); } diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp index 2d6ef6bc4fa16..2f1e87f49d5d3 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp @@ -11,7 +11,6 @@ #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" @@ -100,7 +99,8 @@ TEST_F(CmdConfigBgpPolicyAsPathListTestFixture, argValidation) { EXPECT_THROW( BgpAsPathListConfig({"AS100", "no-such-attr", "1"}), std::invalid_argument); - // asn-regexp is an entry attribute, not a list attribute. + // asn-regexp was the removed entry subcommand's attribute; bgpd never read + // the entries it wrote, so it must not be accepted anywhere. EXPECT_THROW( BgpAsPathListConfig({"AS100", "asn-regexp", "^65000_"}), std::invalid_argument); diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp index a8337a0ba9a6c..7cf9ab0d0387b 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp @@ -134,13 +134,33 @@ TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteExistingList) { EXPECT_TRUE(sessionFileExists()); } -TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteUnknownListRejected) { +// Delete mirrors add: an absent target is a success with a warning, never an +// error, so a replayed script stays idempotent. +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteUnknownListWarns) { auto result = del({"NO-SUCH-LIST"}); EXPECT_THAT( - result, HasSubstr("Error: BGP as-path-list NO-SUCH-LIST not found")); - // Nothing was persisted for the failed delete. + result, + HasSubstr( + "Warning: BGP as-path-list NO-SUCH-LIST 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(CmdDeleteBgpPolicyAsPathListTestFixture, deleteTwiceIsIdempotent) { + configure({"AS100", "description", "delete-me"}); + EXPECT_THAT(del({"AS100"}), HasSubstr("Successfully deleted")); + EXPECT_TRUE(lists().empty()); + + auto again = del({"AS100"}); + EXPECT_THAT( + again, + HasSubstr( + "Warning: BGP as-path-list AS100 does not exist; nothing to delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); + EXPECT_TRUE(lists().empty()); } TEST_F( @@ -148,7 +168,7 @@ TEST_F( deleteUnknownLeavesOthersIntact) { configure({"AS100", "description", "one"}); auto result = del({"AS200"}); - EXPECT_THAT(result, HasSubstr("not found")); + EXPECT_THAT(result, HasSubstr("does not exist")); ASSERT_EQ(lists().size(), 1); EXPECT_EQ(*lists()[0].name(), "AS100"); } @@ -246,18 +266,49 @@ TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteOneRegex) { EXPECT_FALSE(lists()[0].as_paths().has_value()); } -TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteUnknownRegexRejected) { +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteUnknownRegexWarns) { configure({"AS100", "regex", "^65000_"}); + // Remove the session file created by configure so its absence afterwards + // proves the no-op delete did not persist anything new. + ASSERT_TRUE(sessionFileExists()); + std::filesystem::remove( + ConfigSession::getInstance().getBgpSessionConfigPath()); + auto result = del({"AS100", "regex", "^65002_"}); EXPECT_THAT( result, - HasSubstr("Error: BGP as-path-list AS100 regex ^65002_ not found")); + HasSubstr( + "Warning: BGP as-path-list AS100 has no regex ^65002_; nothing to " + "delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); + EXPECT_FALSE(sessionFileExists()) + << "no-op delete must not persist a session file"; ASSERT_EQ(lists().size(), 1); EXPECT_EQ(lists()[0].as_paths()->size(), 1); + auto unknownList = del({"NO-SUCH-LIST", "regex", "^65000_"}); EXPECT_THAT( - del({"NO-SUCH-LIST", "regex", "^65000_"}), - HasSubstr("Error: BGP as-path-list NO-SUCH-LIST not found")); + unknownList, + HasSubstr( + "Warning: BGP as-path-list NO-SUCH-LIST does not exist; nothing to " + "delete")); + EXPECT_THAT(unknownList, Not(HasSubstr("Error:"))); +} + +TEST_F(CmdDeleteBgpPolicyAsPathListTestFixture, deleteRegexTwiceIsIdempotent) { + configure({"AS100", "regex", "^65000_"}); + EXPECT_THAT(del({"AS100", "regex", "^65000_"}), HasSubstr("Successfully")); + EXPECT_FALSE(lists()[0].as_paths().has_value()); + + auto again = del({"AS100", "regex", "^65000_"}); + EXPECT_THAT( + again, + HasSubstr( + "Warning: BGP as-path-list AS100 has no regex ^65000_; nothing to " + "delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].as_paths().has_value()); } } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp index 7597b59716247..f4acbce84c8e2 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp @@ -4,8 +4,8 @@ * End-to-end tests for `fboss2-dev config protocol bgp policy as-path-list * [ ...]`. * - * Scope: the AS-path list level (its entries are covered by - * ConfigBgpPolicyAsPathListEntryTest). Every test stages the change AND + * Scope: the whole AS-path list, including its flat `regex` patterns (the + * as_paths field bgpd matches on). Every test stages the change AND * commits it, then asserts the value landed at the correct thrift field path * inside the matching .policies.aspath_lists[] entry of bgpd's running config * (via getRunningConfig RPC) — which also confirms bgpd accepts and adopts a From d982da17213e915d90bb0a15c4ea01b6cdb84624 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 22 Sep 2026 05:25:50 +0000 Subject: [PATCH 5/5] Trigger CI after bot restack (empty commit)