From 0e1f2f8de9912ef32627a24df6d2223d5dbc286c Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 6 Aug 2026 13:14:22 +0000 Subject: [PATCH 1/8] [Nexthop] NOS-6657..NOS-6670: fboss2 config/delete protocol bgp policy prefix-list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `config protocol bgp policy prefix-list [entry ] [ ...]` and `delete protocol bgp policy prefix-list [entry ]` — the third BGP policy object-type command family, beneath the `policy` grouping node alongside as-path-list and community-list. - prefix-list dispatcher with a two-level key (list name + entry seq-num) writing bgp_policy.BgpPolicies.prefix_lists[] through the typed ConfigSession. List level: boolean-operator, compare-operator (EQ|GE|LE|NE|GT|LT), description, and ip-version (stored as the numeric routing_policy.PrefixList.version, the field the sheet documents). Entry level (routing_policy.PrefixListEntry keyed by seq_num in prefixes[]): base-prefix (validated as , an explicit /len is required), communities (accumulates into the optional set), description, match-logic, max-allowed-subnet-count (-> max_allowed_golden_prefix_subnet_count), regex, and the prefix-len-range compare-operator|value pair targeting the single CompareNumericValue the CLI maintains at prefix_len_ranges[0] (compare-operator additionally accepts RG; value is bounded 0-128). - delete mirrors community-list's two levels: the whole list by name, or a single entry by `entry `. - new intAttr factory in the shared BgpCliAttrHandlers.h (bounded int32, valueDesc taken by value like enumAttr's), used by the two numeric entry attributes. - unit tests (19) + commit-path integration tests mirroring ConfigBgpPolicyCommunityListTest, run on a DUT (all 4 pass). Co-Authored-By: Claude Fable 5 --- cmake/CliFboss2.cmake | 5 + cmake/CliFboss2TestConfig.cmake | 2 + cmake/CliFboss2TestIntegrationTest.cmake | 1 + fboss/cli/fboss2/BUCK | 5 + fboss/cli/fboss2/CmdListConfig.cpp | 21 ++ .../bgp/policy/CmdConfigProtocolBgpPolicy.h | 2 +- .../prefix-list/BgpPrefixListCliUtils.h | 85 ++++++ .../CmdConfigProtocolBgpPolicyPrefixList.cpp | 282 ++++++++++++++++++ .../CmdConfigProtocolBgpPolicyPrefixList.h | 85 ++++++ .../CmdDeleteProtocolBgpPolicyPrefixList.cpp | 140 +++++++++ .../CmdDeleteProtocolBgpPolicyPrefixList.h | 75 +++++ fboss/cli/fboss2/test/config/BUCK | 2 + .../CmdConfigBgpPolicyPrefixListTest.cpp | 196 ++++++++++++ .../CmdDeleteBgpPolicyPrefixListTest.cpp | 206 +++++++++++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + .../ConfigBgpPolicyPrefixListTest.cpp | 124 ++++++++ 16 files changed, 1231 insertions(+), 1 deletion(-) create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp create mode 100644 fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 379e474011f36..e142efcb39e51 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -978,6 +978,9 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.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 @@ -1120,6 +1123,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h fboss/cli/fboss2/commands/delete/protocol/CmdDeleteProtocol.h fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index f9226bda5a1be..ad3bfebb2abfa 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -16,6 +16,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp @@ -55,6 +56,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp + fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.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 7bfff504c46a8..3c586c6cd55f2 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -20,6 +20,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp fboss/cli/fboss2/test/integration_test/ConfigConcurrentSessionsTest.cpp fboss/cli/fboss2/test/integration_test/ConfigHostnameTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 384df2a3ef923..d3708160cde2c 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1212,6 +1212,7 @@ cpp_library( "commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.cpp", "commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp", "commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp", + "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1282,6 +1283,7 @@ cpp_library( "commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp", "commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp", "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp", + "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp", "commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp", "commands/delete/qos/CmdDeleteQos.cpp", @@ -1361,6 +1363,8 @@ cpp_library( "commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h", "commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h", "commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h", + "commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h", + "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", @@ -1432,6 +1436,7 @@ cpp_library( "commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h", "commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h", "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h", + "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.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 5d3fcd0a64bd2..4783aaa0088b0 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -45,6 +45,7 @@ #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/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.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" @@ -112,6 +113,7 @@ #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/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.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" @@ -444,6 +446,17 @@ const CommandTree& kConfigCommandTree() { CmdConfigProtocolBgpPolicyCommunityList>, argRegistrar< CmdConfigProtocolBgpPolicyCommunityListTraits>, + }, + { + "prefix-list", + "Configure BGP prefix-list: " + "[ ...] " + "(boolean-operator, compare-operator, " + "description, ip-version)", + commandHandler< + CmdConfigProtocolBgpPolicyPrefixList>, + argRegistrar< + CmdConfigProtocolBgpPolicyPrefixListTraits>, }}, }, }, @@ -808,6 +821,14 @@ const CommandTree& kConfigCommandTree() { CmdDeleteProtocolBgpPolicyCommunityList>, argRegistrar< CmdDeleteProtocolBgpPolicyCommunityListTraits>, + }, + { + "prefix-list", + "Delete a BGP prefix-list: " + "[entry ]", + commandHandler, + argRegistrar< + CmdDeleteProtocolBgpPolicyPrefixListTraits>, }}, }}, }, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h index 6d0115fb5aa0f..df4e340b6a1ad 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h @@ -20,7 +20,7 @@ namespace facebook::fboss { // Grouping node for `config protocol bgp policy ...`. Holds no // state of its own; the per-object-type dispatchers (as-path-list, -// community-list, and later prefix-list/routing-policy) are its subcommands. +// community-list, prefix-list, and later routing-policy) are its subcommands. // Mirrors CmdConfigProtocolBgp. struct CmdConfigProtocolBgpPolicyTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgp; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h new file mode 100644 index 0000000000000..c7dd90295301f --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h @@ -0,0 +1,85 @@ +/* + * 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/routing_policy_types.h" + +/** + * Lookup/create helpers for the prefix-list CLI family, shared between the + * list-level dispatcher (CmdConfigProtocolBgpPolicyPrefixList), the entry + * subcommand (CmdConfigProtocolBgpPolicyPrefixListEntry), and the delete + * counterparts. A PrefixList is keyed by name; a PrefixListEntry (in + * prefixes[]) is keyed by seq_num. + */ +namespace facebook::fboss::bgpcli { + +inline bool prefixListExists( + const bgp::thrift::BgpConfig& cfg, + const std::string& name) { + if (!cfg.policies().has_value()) { + return false; + } + const auto& lists = *cfg.policies()->prefix_lists(); + return std::any_of(lists.begin(), lists.end(), [&](const auto& list) { + return *list.name() == name; + }); +} + +// Find the prefix-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 `prefix-list ` creates one +// explicitly. PrefixList's only key field is `name`. +inline bgp::routing_policy::PrefixList& findOrCreatePrefixList( + bgp::thrift::BgpConfig& cfg, + const std::string& name) { + auto& lists = *cfg.policies().ensure().prefix_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 prefixListEntryExists( + const bgp::routing_policy::PrefixList& list, + int32_t seqNum) { + const auto& entries = *list.prefixes(); + return std::any_of(entries.begin(), entries.end(), [&](const auto& entry) { + return entry.seq_num().has_value() && *entry.seq_num() == seqNum; + }); +} + +// Find the entry keyed by seq_num within a list's prefixes[], creating it if +// absent. seq_num is the entry's identity. +inline bgp::routing_policy::PrefixListEntry& findOrCreatePrefixListEntry( + bgp::routing_policy::PrefixList& list, + int32_t seqNum) { + auto& entries = *list.prefixes(); + for (auto& entry : entries) { + if (entry.seq_num().has_value() && *entry.seq_num() == seqNum) { + return entry; + } + } + entries.emplace_back(); + auto& entry = entries.back(); + entry.seq_num() = seqNum; + return entry; +} + +} // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp new file mode 100644 index 0000000000000..b11429997ad21 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp @@ -0,0 +1,282 @@ +/* + * 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/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#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/prefix-list/BgpPrefixListCliUtils.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 prefixes +// are their own subcommand, not attributes. +constexpr std::string_view kBooleanOperator = "boolean-operator"; +constexpr std::string_view kCompareOperator = "compare-operator"; +constexpr std::string_view kDescription = "description"; +constexpr std::string_view kIpVersion = "ip-version"; + +// boolean-operator values (routing_policy.BooleanOperator names). +constexpr std::string_view kBooleanOperatorAnd = "AND"; +constexpr std::string_view kBooleanOperatorOr = "OR"; +constexpr std::string_view kBooleanOperatorNot = "NOT"; + +// compare-operator values (routing_policy.ComparisonOperator names). RG +// (range) is only documented for the entry-level prefix-len-range. +constexpr std::string_view kCompareOperatorEq = "EQ"; +constexpr std::string_view kCompareOperatorGe = "GE"; +constexpr std::string_view kCompareOperatorLe = "LE"; +constexpr std::string_view kCompareOperatorNe = "NE"; +constexpr std::string_view kCompareOperatorGt = "GT"; +constexpr std::string_view kCompareOperatorLt = "LT"; +constexpr std::string_view kCompareOperatorRg = "RG"; + +// ip-version values, stored as the numeric IP version in PrefixList.version +// (the documented thrift field for this command). +constexpr std::string_view kIpVersionV4 = "v4"; +constexpr std::string_view kIpVersionV6 = "v6"; +constexpr int32_t kIpVersionV4Value = 4; +constexpr int32_t kIpVersionV6Value = 6; + +using BooleanOperator = bgp::routing_policy::BooleanOperator; +using ComparisonOperator = bgp::routing_policy::ComparisonOperator; +using PrefixList = bgp::routing_policy::PrefixList; +using bgpcli::AttrHandler; +using bgpcli::enumAttr; +using bgpcli::joinedStringAttr; +using bgpcli::ok; +using bgpcli::Result; + +std::optional lookupComparisonOperator( + const std::string& s) { + if (s == kCompareOperatorEq) { + return ComparisonOperator::EQ; + } + if (s == kCompareOperatorGe) { + return ComparisonOperator::GE; + } + if (s == kCompareOperatorLe) { + return ComparisonOperator::LE; + } + if (s == kCompareOperatorNe) { + return ComparisonOperator::NE; + } + if (s == kCompareOperatorGt) { + return ComparisonOperator::GT; + } + if (s == kCompareOperatorLt) { + return ComparisonOperator::LT; + } + return std::nullopt; +} + +// ---- value lookups ---------------------------------------------------------- + +std::optional lookupBooleanOperator(const std::string& s) { + if (s == kBooleanOperatorAnd) { + return BooleanOperator::AND; + } + if (s == kBooleanOperatorOr) { + return BooleanOperator::OR; + } + if (s == kBooleanOperatorNot) { + return BooleanOperator::NOT; + } + return std::nullopt; +} + +std::optional lookupIpVersion(const std::string& s) { + if (s == kIpVersionV4) { + return kIpVersionV4Value; + } + if (s == kIpVersionV6) { + return kIpVersionV6Value; + } + return std::nullopt; +} + +// ---- list-level setters ----------------------------------------------------- +// Each writes one already-parsed, already-validated value. Parsing and message +// text belong to the shared factories in BgpCliAttrHandlers.h. + +void setListBooleanOperator(PrefixList& list, BooleanOperator op) { + list.boolean_operator() = op; +} + +void setListCompareOperator(PrefixList& list, ComparisonOperator op) { + list.compare_operator() = op; +} + +void setListDescription(PrefixList& list, const std::string& description) { + list.description() = description; +} + +// ip-version is stored as the numeric PrefixList.version, the field the +// command is documented against. +void setListIpVersion(PrefixList& list, int32_t version) { + list.version() = version; +} + +// ---- 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, kBooleanOperatorNot); + static const std::string kCompareOperatorValues = fmt::format( + "{}|{}|{}|{}|{}|{}", + kCompareOperatorEq, + kCompareOperatorGe, + kCompareOperatorLe, + kCompareOperatorNe, + kCompareOperatorGt, + kCompareOperatorLt); + static const std::string kIpVersionValues = + fmt::format("{}|{}", kIpVersionV4, kIpVersionV6); + static const std::map, std::less<>> + kHandlers = { + {std::string(kBooleanOperator), + enumAttr( + kBooleanOperator, + kBooleanOperatorValues, + lookupBooleanOperator, + setListBooleanOperator)}, + {std::string(kCompareOperator), + enumAttr( + kCompareOperator, + kCompareOperatorValues, + lookupComparisonOperator, + setListCompareOperator)}, + {std::string(kDescription), + joinedStringAttr(kDescription, setListDescription)}, + {std::string(kIpVersion), + enumAttr( + kIpVersion, + kIpVersionValues, + lookupIpVersion, + setListIpVersion)}, + }; + return kHandlers; +} + +// The attr factories keep their display name as a string_view, so the +// composed `prefix-len-range ` names need static storage — a +// fmt::format temporary at the factory call site would dangle (the same +// hazard enumAttr's valueDesc used to have). +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 BgpAsPathListConfig). +BgpPrefixListConfig::BgpPrefixListConfig(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: prefix-list is required, optionally followed by an " + " "); + } + if (v[0].empty()) { + throw std::invalid_argument("Error: prefix-list name must not be empty"); + } + listName_ = v[0]; + + if (v.size() == 1) { + return; // bare `prefix-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 prefix-list attribute '{}'. Valid attributes: {}", + attr_, + validAttrList())); + } +} + +CmdConfigProtocolBgpPolicyPrefixListTraits::RetType +CmdConfigProtocolBgpPolicyPrefixList::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool listCreated = !bgpcli::prefixListExists(cfg, args.listName()); + auto& list = bgpcli::findOrCreatePrefixList(cfg, args.listName()); + + Result result = args.attr().empty() + ? ok(listCreated + ? fmt::format( + "Successfully created BGP prefix-list {}", args.listName()) + : fmt::format( + "BGP prefix-list {} already exists", args.listName())) + // The attribute is guaranteed valid: BgpPrefixListConfig'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 prefix-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()->prefix_lists()->pop_back(); + } + return result.message; +} + +void CmdConfigProtocolBgpPolicyPrefixList::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdConfigProtocolBgpPolicyPrefixList, + CmdConfigProtocolBgpPolicyPrefixListTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h new file mode 100644 index 0000000000000..030b9c5d74228 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h @@ -0,0 +1,85 @@ +/* + * 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 `prefix-list [ ...]`, validated at +// construction. A PrefixList is keyed by . Its prefixes[] entries are +// their own subcommand (CmdConfigProtocolBgpPolicyPrefixListEntry), not +// attributes. Mirrors BgpAsPathListConfig. +// +// Grammar (from the FBOSS proposed syntax): +// prefix-list (create/select list) +// prefix-list boolean-operator (list attribute) +// prefix-list compare-operator (list attribute) +// prefix-list description (list attribute) +// prefix-list ip-version (list attribute) +class BgpPrefixListConfig : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpPrefixListConfig(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_; +}; + +// Single handler for the whole `config protocol bgp policy prefix-list` +// family; dispatch and per-attribute parsing live in the .cpp so a new tunable +// is a one-entry change. Mirrors CmdConfigProtocolBgpPolicyAsPathList. +struct CmdConfigProtocolBgpPolicyPrefixListTraits : 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 CmdConfigProtocolBgpPolicyAsPathListTraits. + cmd.add_option("args", args, " [ ...]"); + } + using ObjectArgType = BgpPrefixListConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyPrefixList + : public CmdHandler< + CmdConfigProtocolBgpPolicyPrefixList, + CmdConfigProtocolBgpPolicyPrefixListTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyPrefixListTraits::ObjectArgType; + using RetType = CmdConfigProtocolBgpPolicyPrefixListTraits::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/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp new file mode 100644 index 0000000000000..df31e0a6accd8 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp @@ -0,0 +1,140 @@ +/* + * 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/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" +#include "fmt/format.h" + +namespace facebook::fboss { + +namespace { +// CLI keyword selecting the nested entry, matching the config command's +// grammar. +constexpr std::string_view kObjectName = "prefix-list"; +constexpr std::string_view kEntryKeyword = "entry"; +} // namespace + +// Parse + validate at construction so queryClient stays a thin dispatch. +BgpPrefixListRef::BgpPrefixListRef(std::vector v) + : utils::BaseObjectArgType(v) { + // Pre-empt the generic member-selector message for a missing seq-num: the + // shared parser would call the member a , but here it is a number. + if (v.size() >= 2 && v[1] == kEntryKeyword && + (v.size() < 3 || v[2].empty())) { + throw std::invalid_argument("Error: `entry` requires a "); + } + auto selector = bgpcli::parseListMemberSelector( + v, + kObjectName, + kEntryKeyword, + "Error: delete protocol bgp policy prefix-list requires , " + "optionally followed by `entry `"); + // Unlike the config grammar, nothing may follow the parsed prefix: there + // are no attributes to delete through this command. + if (selector.restStart < v.size()) { + throw std::invalid_argument( + selector.memberName + ? fmt::format( + "Error: unexpected token '{}' after entry ", + v[selector.restStart]) + : fmt::format( + "Error: unexpected token '{}'. Usage: delete protocol bgp " + "policy prefix-list [entry ]", + v[selector.restStart])); + } + listName_ = std::move(selector.listName); + if (selector.memberName) { + auto seq = bgpcli::parseNonNegInt32(*selector.memberName); + if (!seq) { + throw std::invalid_argument( + fmt::format( + "Error: entry must be a non-negative integer, got '{}'", + *selector.memberName)); + } + hasEntry_ = true; + seqNum_ = *seq; + } +} + +CmdDeleteProtocolBgpPolicyPrefixListTraits::RetType +CmdDeleteProtocolBgpPolicyPrefixList::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 prefix-list {} not found", args.listName()); + } + auto& lists = *cfg.policies()->prefix_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 prefix-list {} not found", args.listName()); + } + if (args.hasEntry()) { + // Delete a single entry; the list itself stays. + auto& entries = *it->prefixes(); + auto entryIt = + std::find_if(entries.begin(), entries.end(), [&](const auto& entry) { + return entry.seq_num().has_value() && + *entry.seq_num() == args.seqNum(); + }); + if (entryIt == entries.end()) { + return fmt::format( + "Error: BGP prefix-list {} entry {} not found", + args.listName(), + args.seqNum()); + } + entries.erase(entryIt); + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP prefix-list {} entry {}\n" + "Config saved to: {}", + args.listName(), + args.seqNum(), + session.getBgpSessionConfigPath()); + } + lists.erase(it); + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP prefix-list {}\nConfig saved to: {}", + args.listName(), + session.getBgpSessionConfigPath()); +} + +void CmdDeleteProtocolBgpPolicyPrefixList::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdDeleteProtocolBgpPolicyPrefixList, + CmdDeleteProtocolBgpPolicyPrefixListTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h new file mode 100644 index 0000000000000..21e73f3589f15 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h @@ -0,0 +1,75 @@ +/* + * 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 "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 prefix-list [entry ]`, +// validated at construction. The list name and entry seq-num are the +// identities the config command stores. Without the `entry` selector the +// whole list is deleted; with it, only that entry. Mirrors +// BgpCommunityListRef, with a seq-num-keyed second level in place of the +// name-keyed one. +class BgpPrefixListRef : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpPrefixListRef(std::vector v); + const std::string& listName() const { + return listName_; + } + bool hasEntry() const { + return hasEntry_; + } + int32_t seqNum() const { + return seqNum_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::string listName_; + bool hasEntry_{false}; + int32_t seqNum_{0}; +}; + +struct CmdDeleteProtocolBgpPolicyPrefixListTraits : public WriteCommandTraits { + using ParentCmd = CmdDeleteProtocolBgpPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("args", args, " [entry ]"); + } + using ObjectArgType = BgpPrefixListRef; + using RetType = std::string; +}; + +class CmdDeleteProtocolBgpPolicyPrefixList + : public CmdHandler< + CmdDeleteProtocolBgpPolicyPrefixList, + CmdDeleteProtocolBgpPolicyPrefixListTraits> { + public: + using ObjectArgType = + CmdDeleteProtocolBgpPolicyPrefixListTraits::ObjectArgType; + using RetType = CmdDeleteProtocolBgpPolicyPrefixListTraits::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 3267f3eed9a45..c74372c510f28 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -17,6 +17,7 @@ cpp_unittest( "CmdConfigBgpPeerGroupTest.cpp", "CmdConfigBgpPolicyAsPathListTest.cpp", "CmdConfigBgpPolicyCommunityListTest.cpp", + "CmdConfigBgpPolicyPrefixListTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", "CmdConfigDhcpTest.cpp", @@ -56,6 +57,7 @@ cpp_unittest( "CmdDeleteBgpPeerGroupTest.cpp", "CmdDeleteBgpPolicyAsPathListTest.cpp", "CmdDeleteBgpPolicyCommunityListTest.cpp", + "CmdDeleteBgpPolicyPrefixListTest.cpp", "CmdDeleteConfigInterfaceTest.cpp", "CmdDeleteCoppQueueTest.cpp", "CmdDeleteCoppReasonTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp new file mode 100644 index 0000000000000..7b544f32729fc --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp @@ -0,0 +1,196 @@ +// (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/routing_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +using namespace ::testing; +using facebook::bgp::routing_policy::BooleanOperator; +using facebook::bgp::routing_policy::ComparisonOperator; + +namespace facebook::fboss { + +// The prefix-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 +// CmdConfigBgpPolicyCommunityListTest). +class CmdConfigBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { + public: + CmdConfigBgpPolicyPrefixListTestFixture() + : CmdConfigTestBase("bgp_prefix_list_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string run(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyPrefixList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPrefixListConfig(tokens)); + } + + const std::vector& lists() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .prefix_lists(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpPrefixListConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, argValidation) { + // Bare create list. + auto bareList = BgpPrefixListConfig({"PL100"}); + EXPECT_EQ(bareList.listName(), "PL100"); + EXPECT_TRUE(bareList.attr().empty()); + + // List-level attribute. + auto listAttr = BgpPrefixListConfig({"PL100", "description", "my", "list"}); + EXPECT_EQ(listAttr.attr(), "description"); + EXPECT_EQ(listAttr.values(), std::vector({"my", "list"})); + + // Invalid: empty, empty name, unknown attribute. + EXPECT_THROW(BgpPrefixListConfig({}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListConfig({""}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListConfig({"PL100", "no-such-attr", "1"}), + std::invalid_argument); + // base-prefix is an entry attribute, not a list attribute. + EXPECT_THROW( + BgpPrefixListConfig({"PL100", "base-prefix", "10.0.0.0/8"}), + std::invalid_argument); +} + +// ============================================================================== +// List-level handlers +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, bareCreateList) { + auto result = run({"PL100"}); + EXPECT_THAT(result, HasSubstr("Successfully created BGP prefix-list PL100")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); + EXPECT_TRUE(lists()[0].prefixes()->empty()); + EXPECT_TRUE(sessionFileExists()); + + run({"PL200"}); + ASSERT_EQ(lists().size(), 2); + EXPECT_EQ(*lists()[1].name(), "PL200"); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setListDescription) { + auto result = run({"PL100", "description", "spine", "prefixes"}); + EXPECT_THAT(result, HasSubstr("Successfully set description")); + ASSERT_EQ(lists().size(), 1); + // Multi-token description is re-joined. + EXPECT_EQ(*lists()[0].description(), "spine prefixes"); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setBooleanOperator) { + auto result = run({"PL100", "boolean-operator", "AND"}); + EXPECT_THAT(result, HasSubstr("Successfully set boolean-operator")); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::AND); + + run({"PL100", "boolean-operator", "NOT"}); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::NOT); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, booleanOperatorDefaultIsOr) { + run({"PL100"}); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setCompareOperator) { + auto result = run({"PL100", "compare-operator", "GE"}); + EXPECT_THAT(result, HasSubstr("Successfully set compare-operator")); + ASSERT_TRUE(lists()[0].compare_operator().has_value()); + EXPECT_EQ(*lists()[0].compare_operator(), ComparisonOperator::GE); + + // RG is only documented for the entry-level prefix-len-range. + auto rejected = run({"PL100", "compare-operator", "RG"}); + EXPECT_THAT( + rejected, + HasSubstr( + "Invalid compare-operator value 'RG'; expected EQ|GE|LE|NE|GT|LT")); + EXPECT_EQ(*lists()[0].compare_operator(), ComparisonOperator::GE); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setIpVersion) { + auto result = run({"PL100", "ip-version", "v4"}); + EXPECT_THAT(result, HasSubstr("Successfully set ip-version")); + ASSERT_TRUE(lists()[0].version().has_value()); + EXPECT_EQ(*lists()[0].version(), 4); + + run({"PL100", "ip-version", "v6"}); + EXPECT_EQ(*lists()[0].version(), 6); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, namedListsAreDistinct) { + run({"PL100", "description", "one"}); + run({"PL200", "description", "two"}); + ASSERT_EQ(lists().size(), 2); + // Re-referencing an existing list by name updates it, not appends. + run({"PL100", "description", "one-updated"}); + ASSERT_EQ(lists().size(), 2); + EXPECT_EQ(*lists()[0].description(), "one-updated"); +} + +// ============================================================================== +// Entry-level handlers +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, reReferenceReportsExisting) { + run({"PL100"}); + // A second bare reference must not claim to have created it again. + EXPECT_THAT(run({"PL100"}), HasSubstr("already exists")); + ASSERT_EQ(lists().size(), 1); +} + +// ============================================================================== +// Reject paths — the error is surfaced and nothing is persisted +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyPrefixListTestFixture, + invalidBooleanOperatorRejected) { + auto result = run({"PL100", "boolean-operator", "XOR"}); + EXPECT_THAT( + result, + HasSubstr("Invalid boolean-operator value 'XOR'; expected AND|OR|NOT")); + // The rejected value must not leave a phantom list behind. + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, invalidIpVersionRejected) { + auto result = run({"PL100", "ip-version", "v5"}); + EXPECT_THAT( + result, HasSubstr("Invalid ip-version value 'v5'; expected v4|v6")); + 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/CmdDeleteBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp new file mode 100644 index 0000000000000..0943c1f4457fe --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp @@ -0,0 +1,206 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.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 +// CmdDeleteBgpPolicyCommunityListTest). +class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { + public: + CmdDeleteBgpPolicyPrefixListTestFixture() + : CmdConfigTestBase("bgp_prefix_list_delete_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string configure(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyPrefixList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPrefixListConfig(tokens)); + } + + // Seed an entry. The entry level is its own subcommand + // (CmdConfigProtocolBgpPolicyPrefixListEntry), which lands above this + // command in the stack — so stage it through the shared helpers rather than + // depending on that handler. + void configureEntry( + const std::string& listName, + int32_t seqNum, + const std::string& basePrefix) { + auto& session = ConfigSession::getInstance(); + auto& list = + bgpcli::findOrCreatePrefixList(session.getBgpConfig(), listName); + bgpcli::findOrCreatePrefixListEntry(list, seqNum).base_prefix() = + basePrefix; + session.saveBgpConfig(); + } + + std::string del(const std::vector& tokens) { + CmdDeleteProtocolBgpPolicyPrefixList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPrefixListRef(tokens)); + } + + const std::vector& lists() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .prefix_lists(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpPrefixListRef (arg) validation +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { + auto listOnly = BgpPrefixListRef({"PL100"}); + EXPECT_EQ(listOnly.listName(), "PL100"); + EXPECT_FALSE(listOnly.hasEntry()); + + auto withEntry = BgpPrefixListRef({"PL100", "entry", "10"}); + EXPECT_EQ(withEntry.listName(), "PL100"); + EXPECT_TRUE(withEntry.hasEntry()); + EXPECT_EQ(withEntry.seqNum(), 10); + + // Invalid: empty, empty name, non-`entry` second token, missing seq-num, + // non-integer and negative seq-nums, extra tokens. + EXPECT_THROW(BgpPrefixListRef({}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListRef({""}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListRef({"PL100", "PL200"}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListRef({"PL100", "entry"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListRef({"PL100", "entry", "ten"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListRef({"PL100", "entry", "-1"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListRef({"PL100", "entry", "10", "extra"}), + std::invalid_argument); +} + +// ============================================================================== +// queryClient: whole-list deletion +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingList) { + configureEntry("PL100", 10, "10.0.0.0/8"); + configure({"PL200", "description", "keep"}); + ASSERT_EQ(lists().size(), 2); + + auto result = del({"PL100"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted BGP prefix-list PL100")); + // The other list survives untouched. + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL200"); + EXPECT_EQ(*lists()[0].description(), "keep"); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownListRejected) { + auto result = del({"NO-SUCH-LIST"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP prefix-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( + CmdDeleteBgpPolicyPrefixListTestFixture, + deleteUnknownLeavesOthersIntact) { + configure({"PL100", "description", "one"}); + auto result = del({"PL200"}); + EXPECT_THAT(result, HasSubstr("not found")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); +} + +// ============================================================================== +// queryClient: single entry deletion +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingEntry) { + configureEntry("PL100", 10, "10.0.0.0/8"); + configureEntry("PL100", 20, "192.168.0.0/16"); + + auto result = del({"PL100", "entry", "10"}); + EXPECT_THAT( + result, HasSubstr("Successfully deleted BGP prefix-list PL100 entry 10")); + // The list and its other entry survive. + ASSERT_EQ(lists().size(), 1); + ASSERT_EQ(lists()[0].prefixes()->size(), 1); + EXPECT_EQ(*(*lists()[0].prefixes())[0].seq_num(), 20); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteLastEntryKeepsList) { + configureEntry("PL100", 10, "10.0.0.0/8"); + + auto result = del({"PL100", "entry", "10"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted")); + // The list stays, shaped like one that never had entries (prefixes is a + // non-optional list, so empty — not unset — is that shape). + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); + EXPECT_TRUE(lists()[0].prefixes()->empty()); +} + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownEntryRejected) { + configureEntry("PL100", 10, "10.0.0.0/8"); + // Remove the session file created by configure so its absence afterwards + // proves the rejected delete did not persist anything new. + ASSERT_TRUE(sessionFileExists()); + std::filesystem::remove( + ConfigSession::getInstance().getBgpSessionConfigPath()); + + auto result = del({"PL100", "entry", "99"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP prefix-list PL100 entry 99 not found")); + EXPECT_FALSE(sessionFileExists()) + << "rejected delete must not persist a session file"; + // The existing entry is untouched. + ASSERT_EQ(lists().size(), 1); + ASSERT_EQ(lists()[0].prefixes()->size(), 1); + EXPECT_EQ(*(*lists()[0].prefixes())[0].seq_num(), 10); +} + +TEST_F( + CmdDeleteBgpPolicyPrefixListTestFixture, + deleteEntryFromUnknownListRejected) { + auto result = del({"NO-SUCH-LIST", "entry", "10"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP prefix-list NO-SUCH-LIST not found")); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected delete"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 358dedfb6fc65..01a74ad36e009 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -27,6 +27,7 @@ cpp_binary( "ConfigBgpPeerGroupTest.cpp", "ConfigBgpPolicyAsPathListTest.cpp", "ConfigBgpPolicyCommunityListTest.cpp", + "ConfigBgpPolicyPrefixListTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", "ConfigCoppTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp new file mode 100644 index 0000000000000..af43793b1a497 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp @@ -0,0 +1,124 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy prefix-list + * [ ...]`. + * + * Scope: the prefix-list level (its seq-num-keyed entries are covered by + * ConfigBgpPolicyPrefixListEntryTest). Every test stages the + * change AND commits it, then asserts the value landed at the correct thrift + * field path inside the matching .policies.prefix_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 prefix-list name, unlikely to collide with a real list in the +// device's running BGP config. +const std::string kList = "FBOSS2-TEST-PREFIXES"; +} // namespace + +class ConfigBgpPolicyPrefixListTest : public ConfigBgpTestBase { + protected: + // Stage `config protocol bgp policy prefix-list ` WITHOUT + // clearing the staged session, so attributes can accumulate across + // invocations. Returns the staged session JSON. + folly::dynamic stagePrefixList(const std::vector& tokens) { + std::vector args = { + "config", "protocol", "bgp", "policy", "prefix-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.prefix_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("prefix_lists") == 0) { + return nullptr; + } + for (const auto& list : config["policies"]["prefix_lists"]) { + if (list.count("name") && list["name"].asString() == name) { + return &list; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyPrefixListTest, SetListAttributesAndCommit) { + discardSession(); + clearBgpSession(); + stagePrefixList({kList, "description", "test", "spine", "prefixes"}); + stagePrefixList({kList, "boolean-operator", "AND"}); + stagePrefixList({kList, "ip-version", "v4"}); + 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 prefix-list " << kList; + EXPECT_EQ((*list)["description"].asString(), "test spine prefixes"); + + auto running = readRunningBgpConfigViaRpc(); + const auto* runningList = findList(running, kList); + ASSERT_NE(runningList, nullptr) + << "bgpd's running config has no prefix-list " << kList; + EXPECT_EQ((*runningList)["description"].asString(), "test spine prefixes"); + // Enums ride the SimpleJSON wire format as integers: + // routing_policy.BooleanOperator.AND = 1. + EXPECT_EQ((*runningList)["boolean_operator"].asInt(), 1); + EXPECT_EQ((*runningList)["version"].asInt(), 4); +} + +TEST_F(ConfigBgpPolicyPrefixListTest, DeleteListAndCommit) { + // Land a prefix-list in the system config, then delete it through a second + // commit and verify it is gone from bgpd's running config. + discardSession(); + clearBgpSession(); + stagePrefixList({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 prefix-list in bgpd's running config"; + + clearBgpSession(); + auto result = + runCli({"delete", "protocol", "bgp", "policy", "prefix-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 prefix-list still present in bgpd's running config"; +} From 431d1f7dcece950ff32e0766de7657f40e133465 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:18:35 +0000 Subject: [PATCH 2/8] Guard prefix-list delete against routing-policy references Deleting a prefix-list that a routing-policy term still names in prefix_filters.prefix_list_names committed a dangling 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 and as-path-list deletes apply. Deleting a single `entry ` stays allowed: the name the terms resolve remains defined. The scanner is file-local: the term/match CLIs live above this change, so the tests seed the reference through the thrift structs directly. Co-Authored-By: Claude Fable 5.1 --- .../CmdDeleteProtocolBgpPolicyPrefixList.cpp | 50 ++++++++++ .../CmdDeleteBgpPolicyPrefixListTest.cpp | 91 +++++++++++++++++++ 2 files changed, 141 insertions(+) diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp index df31e0a6accd8..24a47f1b350dc 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp @@ -13,6 +13,7 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include +#include #include #include #include @@ -35,6 +36,43 @@ namespace { // grammar. constexpr std::string_view kObjectName = "prefix-list"; constexpr std::string_view kEntryKeyword = "entry"; + +// Every routing-policy term whose PREFIX_LIST match names `listName` in +// prefix_filters.prefix_list_names (the field bgpd resolves at config load), +// labelled the way the CLI addresses the term. +std::vector findTermsReferencingPrefixList( + const bgp::thrift::BgpConfig& cfg, + const std::string& listName) { + std::vector referrers; + if (!cfg.policies().has_value()) { + return referrers; + } + for (const auto& policy : *cfg.policies()->bgp_policy_statements()) { + for (const auto& term : *policy.policy_entries()) { + if (!term.policy_match_entries().has_value()) { + continue; + } + for (const auto& match : *term.policy_match_entries()->match_entries()) { + if (!match.prefix_filters().has_value()) { + continue; + } + const auto& names = *match.prefix_filters()->prefix_list_names(); + if (std::find(names.begin(), names.end(), listName) == names.end()) { + continue; + } + referrers.push_back( + term.sequence_number().has_value() + ? fmt::format( + "policy {} term {}", + *policy.name(), + *term.sequence_number()) + : fmt::format( + "policy {} term {}", *policy.name(), *term.name())); + } + } + } + return referrers; +} } // namespace // Parse + validate at construction so queryClient stays a thin dispatch. @@ -120,6 +158,18 @@ CmdDeleteProtocolBgpPolicyPrefixList::queryClient( args.seqNum(), session.getBgpSessionConfigPath()); } + // A term's prefix_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. + // Deleting a single entry above is safe: the name stays defined. + auto referrers = findTermsReferencingPrefixList(cfg, args.listName()); + if (!referrers.empty()) { + return fmt::format( + "Error: BGP prefix-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/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp index 0943c1f4457fe..daed31653afe1 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp @@ -4,8 +4,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -72,6 +74,35 @@ class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { .prefix_lists(); } + // Seed a routing-policy term whose PREFIX_LIST match names `listName` (the + // term/match CLIs live in higher PRs), the way bgpd reads the reference: + // prefix_filters.prefix_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::PREFIX_LIST; + matches.back().prefix_filters().ensure().prefix_list_names() = {listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -203,4 +234,64 @@ TEST_F( << "session file should not exist after rejected delete"; } +// ============================================================================== +// Reference guard — a list a routing-policy term still names is not deletable +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteReferencedListRejected) { + configure({"PL100", "description", "in-use"}); + addPolicyTermMatching("RM100", 10, "PL100"); + + auto result = del({"PL100"}); + 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(), "PL100"); + EXPECT_EQ(*lists()[0].description(), "in-use"); +} + +TEST_F( + CmdDeleteBgpPolicyPrefixListTestFixture, + deleteReferencedListNamesEveryReferrer) { + configure({"PL100"}); + addPolicyTermMatching("RM100", 10, "PL100"); + addPolicyTermMatching("RM200", 20, "PL100"); + + auto result = del({"PL100"}); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + EXPECT_THAT(result, HasSubstr("policy RM200 term 20")); + ASSERT_EQ(lists().size(), 1); +} + +TEST_F( + CmdDeleteBgpPolicyPrefixListTestFixture, + referenceToOtherListDoesNotBlockDelete) { + configure({"PL100"}); + configure({"PL200"}); + // A term referencing PL100 must not block deleting PL200. + addPolicyTermMatching("RM100", 10, "PL100"); + + auto result = del({"PL200"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted BGP prefix-list PL200")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); +} + +TEST_F( + CmdDeleteBgpPolicyPrefixListTestFixture, + entryDeleteOnReferencedListAllowed) { + // Removing one entry keeps the name defined, so the reference stays valid. + configureEntry("PL100", 10, "10.0.0.0/8"); + configureEntry("PL100", 20, "192.168.0.0/16"); + addPolicyTermMatching("RM100", 10, "PL100"); + + auto result = del({"PL100", "entry", "10"}); + EXPECT_THAT( + result, HasSubstr("Successfully deleted BGP prefix-list PL100 entry 10")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(lists()[0].prefixes()->size(), 1); +} + } // namespace facebook::fboss From c724be6d96868dd44f579d2c84585de27419b1f8 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:21:13 +0000 Subject: [PATCH 3/8] Emit prefix-list commands from bgp_json_to_cli.py Adds generate_prefix_list_commands (description, boolean-operator when not the OR default, compare-operator, ip-version from the `version` field) and hooks it into the policy block after the community-lists. RG at the list level, `prefix_list_names`, an unexpected `version` and the `ip_version` enum field have no CLI spelling and are emitted as `# WARNING:` lines. Entries follow in the entry subcommand change. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 90 +++++++++++++++++++ .../tools/tests/test_bgp_json_to_cli.py | 71 +++++++++++++++ 2 files changed, 161 insertions(+) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 30fc6e9c0a472..b47588c8b56a2 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -543,6 +543,94 @@ def generate_community_list_commands(community_list: dict[str, Any]) -> list[str return commands +_COMPARISON_OPERATOR_NAMES = { + 1: "EQ", + 2: "GE", + 3: "LE", + 4: "NE", + 5: "GT", + 6: "LT", + 7: "RG", +} +_IP_VERSION_KEYWORDS = {4: "v4", 6: "v6"} + + +def _comparison_operator_name(raw: Any) -> str: + if isinstance(raw, str): + return raw + return _COMPARISON_OPERATOR_NAMES.get(int(raw), str(raw)) + + +def _prefix_list_warnings(name: str, prefix_list: dict[str, Any]) -> list[str]: + """Warnings for PrefixList fields that have no CLI spelling.""" + warnings = [] + if prefix_list.get("prefix_list_names"): + warnings.append( + _warning( + f"prefix-list {name}: prefix_list_names has no CLI equivalent; " + "not emitted" + ) + ) + if "ip_version" in prefix_list: + warnings.append( + _warning( + f"prefix-list {name}: ip_version has no CLI equivalent (the CLI " + "writes `version`); not emitted" + ) + ) + return warnings + + +def generate_prefix_list_commands(prefix_list: dict[str, Any]) -> list[str]: + """Generate `config protocol bgp policy prefix-list` commands for one list. + + `ip-version` is the CLI spelling of the `version` field (4/6). The + `prefix_list_names` references and the `ip_version` enum field have no + CLI spelling and surface as warnings. + """ + name = prefix_list.get("name", "") + if not name: + return [] + + prefix = f"config protocol bgp policy prefix-list {escape_shell_arg(name)}" + commands = [] + if prefix_list.get("description"): + commands.append( + f"{prefix} description {escape_shell_arg(prefix_list['description'])}" + ) + if "boolean_operator" in prefix_list: + operator = _boolean_operator_name(prefix_list["boolean_operator"]) + if operator != "OR": + commands.append(f"{prefix} boolean-operator {escape_shell_arg(operator)}") + if "compare_operator" in prefix_list: + operator = _comparison_operator_name(prefix_list["compare_operator"]) + if operator == "RG": + commands.append( + _warning( + f"prefix-list {name}: compare_operator RG is not accepted at " + "the list level; not emitted" + ) + ) + else: + commands.append(f"{prefix} compare-operator {escape_shell_arg(operator)}") + if "version" in prefix_list: + keyword = _IP_VERSION_KEYWORDS.get(prefix_list["version"]) + if keyword is None: + commands.append( + _warning( + f"prefix-list {name}: version {prefix_list['version']} is " + "neither 4 nor 6; not emitted" + ) + ) + else: + commands.append(f"{prefix} ip-version {keyword}") + commands.extend(_prefix_list_warnings(name, prefix_list)) + if not commands: + # Nothing to set: still recreate the (empty) prefix-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. @@ -556,6 +644,8 @@ def generate_policy_commands(config: dict[str, Any]) -> list[str]: commands.extend(generate_as_path_list_commands(as_path_list)) for community_list in policies.get("community_lists", []): commands.extend(generate_community_list_commands(community_list)) + for prefix_list in policies.get("prefix_lists", []): + commands.extend(generate_prefix_list_commands(prefix_list)) return commands diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index c1c240e7f9e6b..84f44836f1a80 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 @@ -29,6 +29,7 @@ generate_peer_commands, generate_peer_group_commands, generate_policy_commands, + generate_prefix_list_commands, json_to_cli, ) @@ -1184,6 +1185,76 @@ def test_policy_block_lists_after_as_path_lists(self) -> None: ) +class GeneratePrefixListCommandsTest(unittest.TestCase): + """Tests for generate_prefix_list_commands (prefix-list grammar).""" + + PREFIX = "config protocol bgp policy prefix-list PL" + + def test_empty_name_returns_empty(self) -> None: + self.assertEqual(generate_prefix_list_commands({"version": 4}), []) + + def test_bare_list_is_recreated(self) -> None: + self.assertEqual(generate_prefix_list_commands({"name": "PL"}), [self.PREFIX]) + + def test_scalar_attributes(self) -> None: + commands = generate_prefix_list_commands( + { + "name": "PL", + "description": "loopbacks", + "boolean_operator": "AND", + "compare_operator": 2, + "version": 6, + } + ) + self.assertEqual( + commands, + [ + f"{self.PREFIX} description loopbacks", + f"{self.PREFIX} boolean-operator AND", + f"{self.PREFIX} compare-operator GE", + f"{self.PREFIX} ip-version v6", + ], + ) + + def test_or_default_omitted(self) -> None: + self.assertEqual( + generate_prefix_list_commands({"name": "PL", "boolean_operator": 2}), + [self.PREFIX], + ) + + def test_unexpressible_values_warn(self) -> None: + commands = generate_prefix_list_commands( + { + "name": "PL", + "compare_operator": 7, + "version": 5, + "prefix_list_names": ["X"], + "ip_version": 1, + } + ) + self.assertEqual(len(commands), 4) + for c in commands: + self.assertTrue(c.startswith("# WARNING:"), c) + + def test_policy_block_order(self) -> None: + config = { + "policies": { + "aspath_lists": [{"name": "A"}], + "community_lists": [{"name": "CL"}], + "prefix_lists": [{"name": "PL"}], + } + } + commands = generate_policy_commands(config) + self.assertEqual( + commands, + [ + "config protocol bgp policy as-path-list A", + "config protocol bgp policy community-list CL", + "config protocol bgp policy prefix-list PL", + ], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands.""" From c3ab1c420b772ace206a41b7b935c8c6a9eeec27 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 16 Sep 2026 07:45:34 +0000 Subject: [PATCH 4/8] Make BGP prefix-list delete idempotent Delete mirrors add: deleting an absent list or an absent entry is a success with a `Warning:` and saves nothing, so a replayed script stays idempotent. The term-reference guard still errors. Co-Authored-By: Claude Fable 5.1 --- .../CmdDeleteProtocolBgpPolicyPrefixList.cpp | 14 ++-- .../CmdDeleteBgpPolicyPrefixListTest.cpp | 66 +++++++++++++++---- 2 files changed, 63 insertions(+), 17 deletions(-) diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp index 24a47f1b350dc..3fe66b85fc359 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp @@ -123,17 +123,21 @@ CmdDeleteProtocolBgpPolicyPrefixList::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 prefix-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 prefix-list {} not found", args.listName()); + return absent; } auto& lists = *cfg.policies()->prefix_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 prefix-list {} not found", args.listName()); + return absent; } if (args.hasEntry()) { // Delete a single entry; the list itself stays. @@ -145,7 +149,7 @@ CmdDeleteProtocolBgpPolicyPrefixList::queryClient( }); if (entryIt == entries.end()) { return fmt::format( - "Error: BGP prefix-list {} entry {} not found", + "Warning: BGP prefix-list {} has no entry {}; nothing to delete", args.listName(), args.seqNum()); } diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp index daed31653afe1..109616abc6bbd 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp @@ -156,13 +156,33 @@ TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingList) { EXPECT_TRUE(sessionFileExists()); } -TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownListRejected) { +// Delete mirrors add: an absent target is a success with a warning, never an +// error, so a replayed script stays idempotent. +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownListWarns) { auto result = del({"NO-SUCH-LIST"}); EXPECT_THAT( - result, HasSubstr("Error: BGP prefix-list NO-SUCH-LIST not found")); - // Nothing was persisted for the failed delete. + result, + HasSubstr( + "Warning: BGP prefix-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(CmdDeleteBgpPolicyPrefixListTestFixture, deleteTwiceIsIdempotent) { + configure({"PL100", "description", "delete-me"}); + EXPECT_THAT(del({"PL100"}), HasSubstr("Successfully deleted")); + EXPECT_TRUE(lists().empty()); + + auto again = del({"PL100"}); + EXPECT_THAT( + again, + HasSubstr( + "Warning: BGP prefix-list PL100 does not exist; nothing to delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); + EXPECT_TRUE(lists().empty()); } TEST_F( @@ -170,7 +190,7 @@ TEST_F( deleteUnknownLeavesOthersIntact) { configure({"PL100", "description", "one"}); auto result = del({"PL200"}); - EXPECT_THAT(result, HasSubstr("not found")); + EXPECT_THAT(result, HasSubstr("does not exist")); ASSERT_EQ(lists().size(), 1); EXPECT_EQ(*lists()[0].name(), "PL100"); } @@ -205,33 +225,55 @@ TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteLastEntryKeepsList) { EXPECT_TRUE(lists()[0].prefixes()->empty()); } -TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownEntryRejected) { +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownEntryWarns) { configureEntry("PL100", 10, "10.0.0.0/8"); // Remove the session file created by configure so its absence afterwards - // proves the rejected delete did not persist anything new. + // proves the no-op delete did not persist anything new. ASSERT_TRUE(sessionFileExists()); std::filesystem::remove( ConfigSession::getInstance().getBgpSessionConfigPath()); auto result = del({"PL100", "entry", "99"}); EXPECT_THAT( - result, HasSubstr("Error: BGP prefix-list PL100 entry 99 not found")); + result, + HasSubstr( + "Warning: BGP prefix-list PL100 has no entry 99; nothing to delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); EXPECT_FALSE(sessionFileExists()) - << "rejected delete must not persist a session file"; + << "no-op delete must not persist a session file"; // The existing entry is untouched. ASSERT_EQ(lists().size(), 1); ASSERT_EQ(lists()[0].prefixes()->size(), 1); EXPECT_EQ(*(*lists()[0].prefixes())[0].seq_num(), 10); } +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteEntryTwiceIsIdempotent) { + configureEntry("PL100", 10, "10.0.0.0/8"); + EXPECT_THAT(del({"PL100", "entry", "10"}), HasSubstr("Successfully")); + EXPECT_TRUE(lists()[0].prefixes()->empty()); + + auto again = del({"PL100", "entry", "10"}); + EXPECT_THAT( + again, + HasSubstr( + "Warning: BGP prefix-list PL100 has no entry 10; nothing to delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); + ASSERT_EQ(lists().size(), 1); + EXPECT_TRUE(lists()[0].prefixes()->empty()); +} + TEST_F( CmdDeleteBgpPolicyPrefixListTestFixture, - deleteEntryFromUnknownListRejected) { + deleteEntryFromUnknownListWarns) { auto result = del({"NO-SUCH-LIST", "entry", "10"}); EXPECT_THAT( - result, HasSubstr("Error: BGP prefix-list NO-SUCH-LIST not found")); + result, + HasSubstr( + "Warning: BGP prefix-list NO-SUCH-LIST does not exist; nothing to " + "delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); EXPECT_FALSE(sessionFileExists()) - << "session file should not exist after rejected delete"; + << "session file should not exist after a no-op delete"; } // ============================================================================== From cbb75bab9ad649645f2bd0b82f673f7823fcc1e1 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 8 Oct 2026 08:15:20 +0000 Subject: [PATCH 5/8] j2c: do not crash on a malformed BGP enum value _boolean_operator_name() and _comparison_operator_name() call int() on the JSON value, so a null or a list in boolean_operator/compare_operator aborted the whole conversion with a Python traceback. j2c is deliberately not a validator: the fboss2 dispatchers are the single place that decides what is legal, and the script's warnings are reserved for valid thrift the CLI cannot express. So the helpers now catch TypeError and ValueError and return the raw value, which reaches the generated script quoted and is rejected by the CLI on replay with its normal message. Values int() accepts (floats, bools) are left alone for the same reason. _boolean_operator_name() predates the prefix-list work; the fix is here because this PR adds its sibling and the review landed on both. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 17 +++++++++++++--- .../tools/tests/test_bgp_json_to_cli.py | 20 +++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index b47588c8b56a2..3681d3b3781c0 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -431,10 +431,17 @@ def _warning(text: str) -> str: def _boolean_operator_name(raw: Any) -> str: - """routing_policy.BooleanOperator as its name, from the int or the name.""" + """routing_policy.BooleanOperator as its name, from the int or the name. + + Not a validator: an unrecognised value is returned as-is so the CLI + rejects it on replay with its own message. + """ if isinstance(raw, str): return raw - return _BOOLEAN_OPERATOR_NAMES.get(int(raw), str(raw)) + try: + return _BOOLEAN_OPERATOR_NAMES.get(int(raw), str(raw)) + except (TypeError, ValueError): + return str(raw) def generate_as_path_list_commands(as_path_list: dict[str, Any]) -> list[str]: @@ -556,9 +563,13 @@ def generate_community_list_commands(community_list: dict[str, Any]) -> list[str def _comparison_operator_name(raw: Any) -> str: + """routing_policy.ComparisonOperator as its name; see _boolean_operator_name.""" if isinstance(raw, str): return raw - return _COMPARISON_OPERATOR_NAMES.get(int(raw), str(raw)) + try: + return _COMPARISON_OPERATOR_NAMES.get(int(raw), str(raw)) + except (TypeError, ValueError): + return str(raw) def _prefix_list_warnings(name: str, prefix_list: dict[str, Any]) -> list[str]: 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 84f44836f1a80..3954dac441fd6 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 @@ -1047,6 +1047,16 @@ def test_boolean_operator_not_warns(self) -> None: self.assertTrue(commands[0].startswith("# WARNING:"), commands[0]) self.assertIn("NOT", commands[0]) + def test_malformed_boolean_operator_passes_through(self) -> None: + """j2c does not validate enum values; the CLI rejects them on replay.""" + for raw, rendered in ((None, "None"), ([1], "'[1]'")): + commands = generate_as_path_list_commands( + {"name": "ASPL", "boolean_operator": raw} + ) + self.assertEqual( + commands, [f"{self.PREFIX} boolean-operator {rendered}"], raw + ) + def test_dead_fields_warn(self) -> None: """Fields bgpd never reads surface as warnings, not silently dropped.""" commands = generate_as_path_list_commands( @@ -1236,6 +1246,16 @@ def test_unexpressible_values_warn(self) -> None: for c in commands: self.assertTrue(c.startswith("# WARNING:"), c) + def test_malformed_compare_operator_passes_through(self) -> None: + """j2c does not validate enum values; the CLI rejects them on replay.""" + for raw, rendered in ((None, "None"), ([2], "'[2]'")): + commands = generate_prefix_list_commands( + {"name": "PL", "compare_operator": raw} + ) + self.assertEqual( + commands, [f"{self.PREFIX} compare-operator {rendered}"], raw + ) + def test_policy_block_order(self) -> None: config = { "policies": { From bb5010cf83a8adb5267ddf9430b33bdfd52ddffa Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 8 Oct 2026 11:25:52 +0000 Subject: [PATCH 6/8] Restrict prefix-list operators to what bgpd accepts; harden j2c warnings bgpd validates a PrefixList only when a routing-policy term references it (PrefixTreeMatch::validateAndCreatePrefixTree): any list-level compare_operator throws "Unsupported PrefixList configuration", and any boolean_operator other than OR throws "PrefixList BooleanOperator can only be OR". The CLI offered six compare operators and three boolean operators, so a committed list could be rejected wholesale the moment a policy used it, and the orphan-list integration test could not see that. Drop the compare-operator attribute and its unused RG constant, offer only boolean-operator OR, and make bgp_json_to_cli.py warn-and-skip instead of emitting the values bgpd rejects. The integration test stages OR and asserts the daemon reads back 2. Also drops a stale comment about composed prefix-len-range attribute names that this list-level file never had. Two more converter fixes from the same review: warning text now goes through _printable() inside _warning(), so a control character in a JSON name cannot end the # comment line and become shell input on replay; and generate_prefix_list_commands warns with the count when it drops prefixes[] (the as-path-list and community-list emitters already warn for theirs) until the entry subcommand adds real emission. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/CmdListConfig.cpp | 4 +- .../CmdConfigProtocolBgpPolicyPrefixList.cpp | 90 ++++--------------- .../CmdConfigProtocolBgpPolicyPrefixList.h | 5 +- .../CmdConfigBgpPolicyPrefixListTest.cpp | 48 +++++----- .../ConfigBgpPolicyPrefixListTest.cpp | 9 +- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 38 +++++--- .../tools/tests/test_bgp_json_to_cli.py | 59 ++++++++++-- 7 files changed, 135 insertions(+), 118 deletions(-) diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 4783aaa0088b0..631ea3f51afd3 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -451,8 +451,8 @@ const CommandTree& kConfigCommandTree() { "prefix-list", "Configure BGP prefix-list: " "[ ...] " - "(boolean-operator, compare-operator, " - "description, ip-version)", + "(boolean-operator, description, " + "ip-version)", commandHandler< CmdConfigProtocolBgpPolicyPrefixList>, argRegistrar< diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp index b11429997ad21..f1d55bd4aea7c 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp @@ -13,18 +13,15 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include -#include #include #include #include #include #include -#include #include #include #include #include -#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/prefix-list/BgpPrefixListCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" @@ -32,6 +29,13 @@ #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fmt/format.h" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + namespace facebook::fboss { namespace { @@ -40,24 +44,15 @@ namespace { // valid-attribute set and the handler table stay in sync. The list's prefixes // are their own subcommand, not attributes. constexpr std::string_view kBooleanOperator = "boolean-operator"; -constexpr std::string_view kCompareOperator = "compare-operator"; constexpr std::string_view kDescription = "description"; constexpr std::string_view kIpVersion = "ip-version"; -// boolean-operator values (routing_policy.BooleanOperator names). -constexpr std::string_view kBooleanOperatorAnd = "AND"; +// boolean-operator values (routing_policy.BooleanOperator names). Only OR is +// offered: bgpd's PrefixTreeMatch rejects any other value ("PrefixList +// BooleanOperator can only be OR") once a policy references the list, and +// it rejects a list-level compare_operator outright, so that attribute is +// not offered at all. constexpr std::string_view kBooleanOperatorOr = "OR"; -constexpr std::string_view kBooleanOperatorNot = "NOT"; - -// compare-operator values (routing_policy.ComparisonOperator names). RG -// (range) is only documented for the entry-level prefix-len-range. -constexpr std::string_view kCompareOperatorEq = "EQ"; -constexpr std::string_view kCompareOperatorGe = "GE"; -constexpr std::string_view kCompareOperatorLe = "LE"; -constexpr std::string_view kCompareOperatorNe = "NE"; -constexpr std::string_view kCompareOperatorGt = "GT"; -constexpr std::string_view kCompareOperatorLt = "LT"; -constexpr std::string_view kCompareOperatorRg = "RG"; // ip-version values, stored as the numeric IP version in PrefixList.version // (the documented thrift field for this command). @@ -67,7 +62,6 @@ constexpr int32_t kIpVersionV4Value = 4; constexpr int32_t kIpVersionV6Value = 6; using BooleanOperator = bgp::routing_policy::BooleanOperator; -using ComparisonOperator = bgp::routing_policy::ComparisonOperator; using PrefixList = bgp::routing_policy::PrefixList; using bgpcli::AttrHandler; using bgpcli::enumAttr; @@ -75,41 +69,12 @@ using bgpcli::joinedStringAttr; using bgpcli::ok; using bgpcli::Result; -std::optional lookupComparisonOperator( - const std::string& s) { - if (s == kCompareOperatorEq) { - return ComparisonOperator::EQ; - } - if (s == kCompareOperatorGe) { - return ComparisonOperator::GE; - } - if (s == kCompareOperatorLe) { - return ComparisonOperator::LE; - } - if (s == kCompareOperatorNe) { - return ComparisonOperator::NE; - } - if (s == kCompareOperatorGt) { - return ComparisonOperator::GT; - } - if (s == kCompareOperatorLt) { - return ComparisonOperator::LT; - } - return std::nullopt; -} - // ---- value lookups ---------------------------------------------------------- std::optional lookupBooleanOperator(const std::string& s) { - if (s == kBooleanOperatorAnd) { - return BooleanOperator::AND; - } if (s == kBooleanOperatorOr) { return BooleanOperator::OR; } - if (s == kBooleanOperatorNot) { - return BooleanOperator::NOT; - } return std::nullopt; } @@ -131,10 +96,6 @@ void setListBooleanOperator(PrefixList& list, BooleanOperator op) { list.boolean_operator() = op; } -void setListCompareOperator(PrefixList& list, ComparisonOperator op) { - list.compare_operator() = op; -} - void setListDescription(PrefixList& list, const std::string& description) { list.description() = description; } @@ -150,16 +111,8 @@ void setListIpVersion(PrefixList& list, int32_t version) { // the setter that stores it. const std::map, std::less<>>& listAttrHandlers() { - static const std::string kBooleanOperatorValues = fmt::format( - "{}|{}|{}", kBooleanOperatorAnd, kBooleanOperatorOr, kBooleanOperatorNot); - static const std::string kCompareOperatorValues = fmt::format( - "{}|{}|{}|{}|{}|{}", - kCompareOperatorEq, - kCompareOperatorGe, - kCompareOperatorLe, - kCompareOperatorNe, - kCompareOperatorGt, - kCompareOperatorLt); + static const std::string kBooleanOperatorValues = + std::string(kBooleanOperatorOr); static const std::string kIpVersionValues = fmt::format("{}|{}", kIpVersionV4, kIpVersionV6); static const std::map, std::less<>> @@ -170,12 +123,6 @@ listAttrHandlers() { kBooleanOperatorValues, lookupBooleanOperator, setListBooleanOperator)}, - {std::string(kCompareOperator), - enumAttr( - kCompareOperator, - kCompareOperatorValues, - lookupComparisonOperator, - setListCompareOperator)}, {std::string(kDescription), joinedStringAttr(kDescription, setListDescription)}, {std::string(kIpVersion), @@ -188,10 +135,6 @@ listAttrHandlers() { return kHandlers; } -// The attr factories keep their display name as a string_view, so the -// composed `prefix-len-range ` names need static storage — a -// fmt::format temporary at the factory call site would dangle (the same -// hazard enumAttr's valueDesc used to have). std::string validAttrList() { std::string out; for (const auto& [name, _] : listAttrHandlers()) { @@ -244,6 +187,7 @@ CmdConfigProtocolBgpPolicyPrefixList::queryClient( auto& cfg = session.getBgpConfig(); const bool listCreated = !bgpcli::prefixListExists(cfg, args.listName()); auto& list = bgpcli::findOrCreatePrefixList(cfg, args.listName()); + auto& lists = *cfg.policies().ensure().prefix_lists(); Result result = args.attr().empty() ? ok(listCreated @@ -262,10 +206,10 @@ CmdConfigProtocolBgpPolicyPrefixList::queryClient( session.saveBgpConfig(); result.message += fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); - } else if (listCreated) { + } else if (listCreated && !lists.empty()) { // Drop the phantom list so a rejected value is not visible to later // lookups in the same process. - cfg.policies()->prefix_lists()->pop_back(); + lists.pop_back(); } return result.message; } diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h index 030b9c5d74228..d6c87608c2ac4 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h @@ -27,8 +27,9 @@ namespace facebook::fboss { // // Grammar (from the FBOSS proposed syntax): // prefix-list (create/select list) -// prefix-list boolean-operator (list attribute) -// prefix-list compare-operator (list attribute) +// prefix-list boolean-operator OR (list attribute; bgpd accepts +// only OR, and no list-level +// compare-operator at all) // prefix-list description (list attribute) // prefix-list ip-version (list attribute) class BgpPrefixListConfig : public utils::BaseObjectArgType { diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp index 7b544f32729fc..66e3a64f4f717 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp @@ -1,6 +1,7 @@ // (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. #include +#include #include #include @@ -17,7 +18,6 @@ using namespace ::testing; using facebook::bgp::routing_policy::BooleanOperator; -using facebook::bgp::routing_policy::ComparisonOperator; namespace facebook::fboss { @@ -80,6 +80,11 @@ TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, argValidation) { EXPECT_THROW( BgpPrefixListConfig({"PL100", "base-prefix", "10.0.0.0/8"}), std::invalid_argument); + // compare-operator is not offered: bgpd rejects a list-level + // compare_operator ("Unsupported PrefixList configuration"). + EXPECT_THROW( + BgpPrefixListConfig({"PL100", "compare-operator", "GE"}), + std::invalid_argument); } // ============================================================================== @@ -108,12 +113,21 @@ TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setListDescription) { } TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setBooleanOperator) { - auto result = run({"PL100", "boolean-operator", "AND"}); + auto result = run({"PL100", "boolean-operator", "OR"}); EXPECT_THAT(result, HasSubstr("Successfully set boolean-operator")); - EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::AND); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); - run({"PL100", "boolean-operator", "NOT"}); - EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::NOT); + // bgpd: "PrefixList BooleanOperator can only be OR". AND and NOT are + // refused and leave the stored value untouched. + for (const auto* op : {"AND", "NOT"}) { + auto rejected = run({"PL100", "boolean-operator", op}); + EXPECT_THAT( + rejected, + HasSubstr( + fmt::format( + "Invalid boolean-operator value '{}'; expected OR", op))); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); + } } TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, booleanOperatorDefaultIsOr) { @@ -121,19 +135,14 @@ TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, booleanOperatorDefaultIsOr) { EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); } -TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setCompareOperator) { - auto result = run({"PL100", "compare-operator", "GE"}); - EXPECT_THAT(result, HasSubstr("Successfully set compare-operator")); - ASSERT_TRUE(lists()[0].compare_operator().has_value()); - EXPECT_EQ(*lists()[0].compare_operator(), ComparisonOperator::GE); - - // RG is only documented for the entry-level prefix-len-range. - auto rejected = run({"PL100", "compare-operator", "RG"}); - EXPECT_THAT( - rejected, - HasSubstr( - "Invalid compare-operator value 'RG'; expected EQ|GE|LE|NE|GT|LT")); - EXPECT_EQ(*lists()[0].compare_operator(), ComparisonOperator::GE); +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, compareOperatorNeverSet) { + // Nothing at the list level writes compare_operator; bgpd rejects the + // field whenever it is present. + run({"PL100", "description", "x"}); + run({"PL100", "boolean-operator", "OR"}); + run({"PL100", "ip-version", "v4"}); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].compare_operator().has_value()); } TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setIpVersion) { @@ -176,8 +185,7 @@ TEST_F( invalidBooleanOperatorRejected) { auto result = run({"PL100", "boolean-operator", "XOR"}); EXPECT_THAT( - result, - HasSubstr("Invalid boolean-operator value 'XOR'; expected AND|OR|NOT")); + result, HasSubstr("Invalid boolean-operator value 'XOR'; expected OR")); // The rejected value must not leave a phantom list behind. EXPECT_TRUE(lists().empty()); EXPECT_FALSE(sessionFileExists()) diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp index af43793b1a497..4978f0263c097 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp @@ -74,7 +74,7 @@ TEST_F(ConfigBgpPolicyPrefixListTest, SetListAttributesAndCommit) { discardSession(); clearBgpSession(); stagePrefixList({kList, "description", "test", "spine", "prefixes"}); - stagePrefixList({kList, "boolean-operator", "AND"}); + stagePrefixList({kList, "boolean-operator", "OR"}); stagePrefixList({kList, "ip-version", "v4"}); commitAndGetSha(); ASSERT_TRUE(waitForBgpDaemonActive()) @@ -93,8 +93,11 @@ TEST_F(ConfigBgpPolicyPrefixListTest, SetListAttributesAndCommit) { << "bgpd's running config has no prefix-list " << kList; EXPECT_EQ((*runningList)["description"].asString(), "test spine prefixes"); // Enums ride the SimpleJSON wire format as integers: - // routing_policy.BooleanOperator.AND = 1. - EXPECT_EQ((*runningList)["boolean_operator"].asInt(), 1); + // routing_policy.BooleanOperator.OR = 2. bgpd only validates list + // operators from PrefixTreeMatch::PopulateReferences, i.e. once a + // routing-policy term references the list, so an end-to-end rejection + // test belongs with the term match command. + EXPECT_EQ((*runningList)["boolean_operator"].asInt(), 2); EXPECT_EQ((*runningList)["version"].asInt(), 4); } diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 3681d3b3781c0..a23d5d24b0b72 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -424,7 +424,9 @@ def generate_peer_group_commands(peer_group: dict[str, Any]) -> list[str]: def _warning(text: str) -> str: - return f"{_WARNING_PREFIX} {text}" + # Warning text interpolates JSON-sourced names and values; a control + # character in one would end the `#` comment and become shell input. + return f"{_WARNING_PREFIX} {_printable(text)}" _BOOLEAN_OPERATOR_NAMES = {1: "AND", 2: "OR", 3: "NOT"} @@ -595,9 +597,12 @@ def _prefix_list_warnings(name: str, prefix_list: dict[str, Any]) -> list[str]: def generate_prefix_list_commands(prefix_list: dict[str, Any]) -> list[str]: """Generate `config protocol bgp policy prefix-list` commands for one list. - `ip-version` is the CLI spelling of the `version` field (4/6). The - `prefix_list_names` references and the `ip_version` enum field have no - CLI spelling and surface as warnings. + `ip-version` is the CLI spelling of the `version` field (4/6). bgpd + accepts only `boolean_operator` OR and no list-level `compare_operator` + (PrefixTreeMatch::validateAndCreatePrefixTree), and the CLI offers nothing + else, so other values surface as warnings rather than commands. The + `prefix_list_names` references, the `ip_version` enum field and (until the + entry subcommand lands) `prefixes` have no CLI spelling and also warn. """ name = prefix_list.get("name", "") if not name: @@ -612,18 +617,20 @@ def generate_prefix_list_commands(prefix_list: dict[str, Any]) -> list[str]: if "boolean_operator" in prefix_list: operator = _boolean_operator_name(prefix_list["boolean_operator"]) if operator != "OR": - commands.append(f"{prefix} boolean-operator {escape_shell_arg(operator)}") - if "compare_operator" in prefix_list: - operator = _comparison_operator_name(prefix_list["compare_operator"]) - if operator == "RG": commands.append( _warning( - f"prefix-list {name}: compare_operator RG is not accepted at " - "the list level; not emitted" + f"prefix-list {name}: boolean_operator {operator} is not " + "accepted by bgpd (only OR); not emitted" ) ) - else: - commands.append(f"{prefix} compare-operator {escape_shell_arg(operator)}") + if "compare_operator" in prefix_list: + operator = _comparison_operator_name(prefix_list["compare_operator"]) + commands.append( + _warning( + f"prefix-list {name}: list-level compare_operator {operator} is " + "not accepted by bgpd; not emitted" + ) + ) if "version" in prefix_list: keyword = _IP_VERSION_KEYWORDS.get(prefix_list["version"]) if keyword is None: @@ -635,6 +642,13 @@ def generate_prefix_list_commands(prefix_list: dict[str, Any]) -> list[str]: ) else: commands.append(f"{prefix} ip-version {keyword}") + if prefix_list.get("prefixes"): + commands.append( + _warning( + f"prefix-list {name}: {len(prefix_list['prefixes'])} prefixes " + "are not emitted until the entry subcommand lands" + ) + ) commands.extend(_prefix_list_warnings(name, prefix_list)) if not commands: # Nothing to set: still recreate the (empty) prefix-list by name. 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 3954dac441fd6..d0f091594e29b 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 @@ -1220,12 +1220,44 @@ def test_scalar_attributes(self) -> None: commands, [ f"{self.PREFIX} description loopbacks", - f"{self.PREFIX} boolean-operator AND", - f"{self.PREFIX} compare-operator GE", + "# WARNING: prefix-list PL: boolean_operator AND is not accepted " + "by bgpd (only OR); not emitted", + "# WARNING: prefix-list PL: list-level compare_operator GE is " + "not accepted by bgpd; not emitted", f"{self.PREFIX} ip-version v6", ], ) + def test_boolean_operator_not_or_warns(self) -> None: + """bgpd: "PrefixList BooleanOperator can only be OR".""" + for raw, rendered in ((1, "AND"), (3, "NOT"), ("NOT", "NOT")): + commands = generate_prefix_list_commands( + {"name": "PL", "boolean_operator": raw} + ) + self.assertEqual( + commands, + [ + f"# WARNING: prefix-list PL: boolean_operator {rendered} is " + "not accepted by bgpd (only OR); not emitted" + ], + raw, + ) + + def test_prefixes_warn_until_entry_subcommand(self) -> None: + commands = generate_prefix_list_commands( + { + "name": "PL", + "prefixes": [{"base_prefix": "10.0.0.0/8"}, {"base_prefix": "::/0"}], + } + ) + self.assertEqual( + commands, + [ + "# WARNING: prefix-list PL: 2 prefixes are not emitted until " + "the entry subcommand lands" + ], + ) + def test_or_default_omitted(self) -> None: self.assertEqual( generate_prefix_list_commands({"name": "PL", "boolean_operator": 2}), @@ -1246,16 +1278,31 @@ def test_unexpressible_values_warn(self) -> None: for c in commands: self.assertTrue(c.startswith("# WARNING:"), c) - def test_malformed_compare_operator_passes_through(self) -> None: - """j2c does not validate enum values; the CLI rejects them on replay.""" - for raw, rendered in ((None, "None"), ([2], "'[2]'")): + def test_any_compare_operator_warns(self) -> None: + """bgpd rejects a list-level compare_operator whatever its value.""" + for raw, rendered in ((2, "GE"), (7, "RG"), (None, "None"), ([2], "[2]")): commands = generate_prefix_list_commands( {"name": "PL", "compare_operator": raw} ) self.assertEqual( - commands, [f"{self.PREFIX} compare-operator {rendered}"], raw + commands, + [ + f"# WARNING: prefix-list PL: list-level compare_operator " + f"{rendered} is not accepted by bgpd; not emitted" + ], + raw, ) + def test_warning_text_is_single_comment_line(self) -> None: + """A control character in a JSON name must not end the # comment.""" + commands = generate_prefix_list_commands( + {"name": "PL\nrm -rf /", "prefix_list_names": ["X"]} + ) + self.assertEqual(len(commands), 1) + self.assertTrue(commands[0].startswith("# WARNING:"), commands[0]) + self.assertNotIn("\n", commands[0]) + self.assertIn("PL?rm -rf /", commands[0]) + def test_policy_block_order(self) -> None: config = { "policies": { From 5e4290261dbfe279bdfb640a3b7e95e49da2be5d Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 8 Oct 2026 11:25:52 +0000 Subject: [PATCH 7/8] Keep the prefix-list delete at the list level bgpd rejects any PrefixListEntry that carries seq_num (PolicyMatch.cpp, "Unsupported Prefix configuration: seq_num"), yet the delete command's `entry ` selector and the shared findOrCreatePrefixListEntry() helper used seq_num as the entry's stored identity. Rather than ship a delete path whose config bgpd refuses, this PR stops at the list level: `delete protocol bgp policy prefix-list ` only. The entry selector, the entry helpers and their tests move to the entry subcommand PR, which also decides the entry's identity (re-key by base_prefix, or relax bgpd). Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/CmdListConfig.cpp | 3 +- .../prefix-list/BgpPrefixListCliUtils.h | 44 ++---- .../CmdDeleteProtocolBgpPolicyPrefixList.cpp | 88 +++--------- .../CmdDeleteProtocolBgpPolicyPrefixList.h | 22 +-- .../CmdDeleteBgpPolicyPrefixListTest.cpp | 133 +----------------- .../ConfigBgpPolicyPrefixListTest.cpp | 12 +- 6 files changed, 48 insertions(+), 254 deletions(-) diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 631ea3f51afd3..94d83960a4920 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -824,8 +824,7 @@ const CommandTree& kConfigCommandTree() { }, { "prefix-list", - "Delete a BGP prefix-list: " - "[entry ]", + "Delete a BGP prefix-list: ", commandHandler, argRegistrar< CmdDeleteProtocolBgpPolicyPrefixListTraits>, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h index c7dd90295301f..acbe11d08af78 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h @@ -10,18 +10,22 @@ #pragma once -#include #include -#include #include -#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" + +#ifndef IS_OSS +#include +#include +#else +#include +#endif /** * Lookup/create helpers for the prefix-list CLI family, shared between the - * list-level dispatcher (CmdConfigProtocolBgpPolicyPrefixList), the entry - * subcommand (CmdConfigProtocolBgpPolicyPrefixListEntry), and the delete - * counterparts. A PrefixList is keyed by name; a PrefixListEntry (in - * prefixes[]) is keyed by seq_num. + * list-level dispatcher (CmdConfigProtocolBgpPolicyPrefixList) and its delete + * counterpart. A PrefixList is keyed by name. Entry helpers arrive with the + * entry subcommand, which decides the entry's identity (bgpd rejects a + * PrefixListEntry carrying seq_num). */ namespace facebook::fboss::bgpcli { @@ -56,30 +60,4 @@ inline bgp::routing_policy::PrefixList& findOrCreatePrefixList( return list; } -inline bool prefixListEntryExists( - const bgp::routing_policy::PrefixList& list, - int32_t seqNum) { - const auto& entries = *list.prefixes(); - return std::any_of(entries.begin(), entries.end(), [&](const auto& entry) { - return entry.seq_num().has_value() && *entry.seq_num() == seqNum; - }); -} - -// Find the entry keyed by seq_num within a list's prefixes[], creating it if -// absent. seq_num is the entry's identity. -inline bgp::routing_policy::PrefixListEntry& findOrCreatePrefixListEntry( - bgp::routing_policy::PrefixList& list, - int32_t seqNum) { - auto& entries = *list.prefixes(); - for (auto& entry : entries) { - if (entry.seq_num().has_value() && *entry.seq_num() == seqNum) { - return entry; - } - } - entries.emplace_back(); - auto& entry = entries.back(); - entry.seq_num() = seqNum; - return entry; -} - } // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp index 3fe66b85fc359..23bb9bcbbc32c 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp @@ -14,29 +14,25 @@ #include #include -#include #include #include -#include #include #include -#include -#include #include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fmt/format.h" +#ifndef IS_OSS +#include +#else +#include +#endif + namespace facebook::fboss { namespace { -// CLI keyword selecting the nested entry, matching the config command's -// grammar. -constexpr std::string_view kObjectName = "prefix-list"; -constexpr std::string_view kEntryKeyword = "entry"; - // Every routing-policy term whose PREFIX_LIST match names `listName` in // prefix_filters.prefix_list_names (the field bgpd resolves at config load), // labelled the way the CLI addresses the term. @@ -78,43 +74,23 @@ std::vector findTermsReferencingPrefixList( // Parse + validate at construction so queryClient stays a thin dispatch. BgpPrefixListRef::BgpPrefixListRef(std::vector v) : utils::BaseObjectArgType(v) { - // Pre-empt the generic member-selector message for a missing seq-num: the - // shared parser would call the member a , but here it is a number. - if (v.size() >= 2 && v[1] == kEntryKeyword && - (v.size() < 3 || v[2].empty())) { - throw std::invalid_argument("Error: `entry` requires a "); - } - auto selector = bgpcli::parseListMemberSelector( - v, - kObjectName, - kEntryKeyword, - "Error: delete protocol bgp policy prefix-list requires , " - "optionally followed by `entry `"); - // Unlike the config grammar, nothing may follow the parsed prefix: there - // are no attributes to delete through this command. - if (selector.restStart < v.size()) { + if (v.empty()) { throw std::invalid_argument( - selector.memberName - ? fmt::format( - "Error: unexpected token '{}' after entry ", - v[selector.restStart]) - : fmt::format( - "Error: unexpected token '{}'. Usage: delete protocol bgp " - "policy prefix-list [entry ]", - v[selector.restStart])); + "Error: delete protocol bgp policy prefix-list requires "); } - listName_ = std::move(selector.listName); - if (selector.memberName) { - auto seq = bgpcli::parseNonNegInt32(*selector.memberName); - if (!seq) { - throw std::invalid_argument( - fmt::format( - "Error: entry must be a non-negative integer, got '{}'", - *selector.memberName)); - } - hasEntry_ = true; - seqNum_ = *seq; + if (v[0].empty()) { + throw std::invalid_argument("Error: prefix-list name must not be empty"); } + // Nothing may follow the name: there are no attributes to delete through + // this command, and entries are deleted through the entry subcommand. + if (v.size() > 1) { + throw std::invalid_argument( + fmt::format( + "Error: unexpected token '{}'. Usage: delete protocol bgp policy " + "prefix-list ", + v[1])); + } + listName_ = v[0]; } CmdDeleteProtocolBgpPolicyPrefixListTraits::RetType @@ -139,33 +115,9 @@ CmdDeleteProtocolBgpPolicyPrefixList::queryClient( if (it == lists.end()) { return absent; } - if (args.hasEntry()) { - // Delete a single entry; the list itself stays. - auto& entries = *it->prefixes(); - auto entryIt = - std::find_if(entries.begin(), entries.end(), [&](const auto& entry) { - return entry.seq_num().has_value() && - *entry.seq_num() == args.seqNum(); - }); - if (entryIt == entries.end()) { - return fmt::format( - "Warning: BGP prefix-list {} has no entry {}; nothing to delete", - args.listName(), - args.seqNum()); - } - entries.erase(entryIt); - session.saveBgpConfig(); - return fmt::format( - "Successfully deleted BGP prefix-list {} entry {}\n" - "Config saved to: {}", - args.listName(), - args.seqNum(), - session.getBgpSessionConfigPath()); - } // A term's prefix_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. - // Deleting a single entry above is safe: the name stays defined. auto referrers = findTermsReferencingPrefixList(cfg, args.listName()); if (!referrers.empty()) { return fmt::format( diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h index 21e73f3589f15..06193e1604112 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h @@ -10,7 +10,6 @@ #pragma once -#include #include #include #include "CLI/App.hpp" @@ -21,12 +20,11 @@ namespace facebook::fboss { -// Parsed `delete protocol bgp policy prefix-list [entry ]`, -// validated at construction. The list name and entry seq-num are the -// identities the config command stores. Without the `entry` selector the -// whole list is deleted; with it, only that entry. Mirrors -// BgpCommunityListRef, with a seq-num-keyed second level in place of the -// name-keyed one. +// Parsed `delete protocol bgp policy prefix-list `, validated at +// construction. The list name is the only identity the config command +// stores at this level; deleting a single entry arrives with the entry +// subcommand (CmdConfigProtocolBgpPolicyPrefixListEntry), where the entry's +// identity is decided. class BgpPrefixListRef : public utils::BaseObjectArgType { public: // NOLINTNEXTLINE(google-explicit-constructor) @@ -34,25 +32,17 @@ class BgpPrefixListRef : public utils::BaseObjectArgType { const std::string& listName() const { return listName_; } - bool hasEntry() const { - return hasEntry_; - } - int32_t seqNum() const { - return seqNum_; - } const static utils::ObjectArgTypeId id = utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; private: std::string listName_; - bool hasEntry_{false}; - int32_t seqNum_{0}; }; struct CmdDeleteProtocolBgpPolicyPrefixListTraits : public WriteCommandTraits { using ParentCmd = CmdDeleteProtocolBgpPolicy; static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("args", args, " [entry ]"); + cmd.add_option("args", args, ""); } using ObjectArgType = BgpPrefixListRef; using RetType = std::string; diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp index 109616abc6bbd..d1040069b4352 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp @@ -13,7 +13,6 @@ #include #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h" #include "fboss/cli/fboss2/session/ConfigSession.h" @@ -44,22 +43,6 @@ class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { return cmd.queryClient(hostInfo, BgpPrefixListConfig(tokens)); } - // Seed an entry. The entry level is its own subcommand - // (CmdConfigProtocolBgpPolicyPrefixListEntry), which lands above this - // command in the stack — so stage it through the shared helpers rather than - // depending on that handler. - void configureEntry( - const std::string& listName, - int32_t seqNum, - const std::string& basePrefix) { - auto& session = ConfigSession::getInstance(); - auto& list = - bgpcli::findOrCreatePrefixList(session.getBgpConfig(), listName); - bgpcli::findOrCreatePrefixListEntry(list, seqNum).base_prefix() = - basePrefix; - session.saveBgpConfig(); - } - std::string del(const std::vector& tokens) { CmdDeleteProtocolBgpPolicyPrefixList cmd; HostInfo hostInfo("testhost"); @@ -116,26 +99,14 @@ class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { auto listOnly = BgpPrefixListRef({"PL100"}); EXPECT_EQ(listOnly.listName(), "PL100"); - EXPECT_FALSE(listOnly.hasEntry()); - auto withEntry = BgpPrefixListRef({"PL100", "entry", "10"}); - EXPECT_EQ(withEntry.listName(), "PL100"); - EXPECT_TRUE(withEntry.hasEntry()); - EXPECT_EQ(withEntry.seqNum(), 10); - - // Invalid: empty, empty name, non-`entry` second token, missing seq-num, - // non-integer and negative seq-nums, extra tokens. + // Invalid: empty, empty name, any second token (entries are deleted + // through the entry subcommand, not here). EXPECT_THROW(BgpPrefixListRef({}), std::invalid_argument); EXPECT_THROW(BgpPrefixListRef({""}), std::invalid_argument); EXPECT_THROW(BgpPrefixListRef({"PL100", "PL200"}), std::invalid_argument); - EXPECT_THROW(BgpPrefixListRef({"PL100", "entry"}), std::invalid_argument); - EXPECT_THROW( - BgpPrefixListRef({"PL100", "entry", "ten"}), std::invalid_argument); EXPECT_THROW( - BgpPrefixListRef({"PL100", "entry", "-1"}), std::invalid_argument); - EXPECT_THROW( - BgpPrefixListRef({"PL100", "entry", "10", "extra"}), - std::invalid_argument); + BgpPrefixListRef({"PL100", "entry", "10"}), std::invalid_argument); } // ============================================================================== @@ -143,7 +114,7 @@ TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { // ============================================================================== TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingList) { - configureEntry("PL100", 10, "10.0.0.0/8"); + configure({"PL100", "description", "drop"}); configure({"PL200", "description", "keep"}); ASSERT_EQ(lists().size(), 2); @@ -195,87 +166,6 @@ TEST_F( EXPECT_EQ(*lists()[0].name(), "PL100"); } -// ============================================================================== -// queryClient: single entry deletion -// ============================================================================== - -TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingEntry) { - configureEntry("PL100", 10, "10.0.0.0/8"); - configureEntry("PL100", 20, "192.168.0.0/16"); - - auto result = del({"PL100", "entry", "10"}); - EXPECT_THAT( - result, HasSubstr("Successfully deleted BGP prefix-list PL100 entry 10")); - // The list and its other entry survive. - ASSERT_EQ(lists().size(), 1); - ASSERT_EQ(lists()[0].prefixes()->size(), 1); - EXPECT_EQ(*(*lists()[0].prefixes())[0].seq_num(), 20); - EXPECT_TRUE(sessionFileExists()); -} - -TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteLastEntryKeepsList) { - configureEntry("PL100", 10, "10.0.0.0/8"); - - auto result = del({"PL100", "entry", "10"}); - EXPECT_THAT(result, HasSubstr("Successfully deleted")); - // The list stays, shaped like one that never had entries (prefixes is a - // non-optional list, so empty — not unset — is that shape). - ASSERT_EQ(lists().size(), 1); - EXPECT_EQ(*lists()[0].name(), "PL100"); - EXPECT_TRUE(lists()[0].prefixes()->empty()); -} - -TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownEntryWarns) { - configureEntry("PL100", 10, "10.0.0.0/8"); - // 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({"PL100", "entry", "99"}); - EXPECT_THAT( - result, - HasSubstr( - "Warning: BGP prefix-list PL100 has no entry 99; nothing to delete")); - EXPECT_THAT(result, Not(HasSubstr("Error:"))); - EXPECT_FALSE(sessionFileExists()) - << "no-op delete must not persist a session file"; - // The existing entry is untouched. - ASSERT_EQ(lists().size(), 1); - ASSERT_EQ(lists()[0].prefixes()->size(), 1); - EXPECT_EQ(*(*lists()[0].prefixes())[0].seq_num(), 10); -} - -TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteEntryTwiceIsIdempotent) { - configureEntry("PL100", 10, "10.0.0.0/8"); - EXPECT_THAT(del({"PL100", "entry", "10"}), HasSubstr("Successfully")); - EXPECT_TRUE(lists()[0].prefixes()->empty()); - - auto again = del({"PL100", "entry", "10"}); - EXPECT_THAT( - again, - HasSubstr( - "Warning: BGP prefix-list PL100 has no entry 10; nothing to delete")); - EXPECT_THAT(again, Not(HasSubstr("Error:"))); - ASSERT_EQ(lists().size(), 1); - EXPECT_TRUE(lists()[0].prefixes()->empty()); -} - -TEST_F( - CmdDeleteBgpPolicyPrefixListTestFixture, - deleteEntryFromUnknownListWarns) { - auto result = del({"NO-SUCH-LIST", "entry", "10"}); - EXPECT_THAT( - result, - HasSubstr( - "Warning: BGP prefix-list NO-SUCH-LIST does not exist; nothing to " - "delete")); - EXPECT_THAT(result, Not(HasSubstr("Error:"))); - EXPECT_FALSE(sessionFileExists()) - << "session file should not exist after a no-op delete"; -} - // ============================================================================== // Reference guard — a list a routing-policy term still names is not deletable // ============================================================================== @@ -321,19 +211,4 @@ TEST_F( EXPECT_EQ(*lists()[0].name(), "PL100"); } -TEST_F( - CmdDeleteBgpPolicyPrefixListTestFixture, - entryDeleteOnReferencedListAllowed) { - // Removing one entry keeps the name defined, so the reference stays valid. - configureEntry("PL100", 10, "10.0.0.0/8"); - configureEntry("PL100", 20, "192.168.0.0/16"); - addPolicyTermMatching("RM100", 10, "PL100"); - - auto result = del({"PL100", "entry", "10"}); - EXPECT_THAT( - result, HasSubstr("Successfully deleted BGP prefix-list PL100 entry 10")); - ASSERT_EQ(lists().size(), 1); - EXPECT_EQ(lists()[0].prefixes()->size(), 1); -} - } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp index 4978f0263c097..aefb8a9845807 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp @@ -4,12 +4,12 @@ * End-to-end tests for `fboss2-dev config protocol bgp policy prefix-list * [ ...]`. * - * Scope: the prefix-list level (its seq-num-keyed entries are covered by - * ConfigBgpPolicyPrefixListEntryTest). Every test stages the - * change AND commits it, then asserts the value landed at the correct thrift - * field path inside the matching .policies.prefix_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 + * Scope: the prefix-list level (its entries are covered by + * ConfigBgpPolicyPrefixListEntryTest in the entry subcommand's PR). Every test + * stages the change AND commits it, then asserts the value landed at the + * correct thrift field path inside the matching .policies.prefix_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. * From 04be951e8c5cf561f6924a2ae60f75b52ec25019 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 8 Oct 2026 11:25:52 +0000 Subject: [PATCH 8/8] Guard community-list delete and sync referencing filters bgpd resolves a community-list reference by name at config load, from a term's communities_filter.community_list_names (CommunityMatch) or its community_action_list_names (CommunityAction), and fails the load on a dangling one. The delete command erased a list without looking, and it also reset communities to unset after removing the last value, which a community action rejects ("Missing communities in CommunityList"). bgpd further requires a term's inline copy of the list to carry the same boolean_operator as the named list ("Conflicting boolean_operator from the reference") and reads exact_match only from that inline copy, so setting either on the named list alone produced a config bgpd rejects or ignores. Add findTermsReferencingCommunityList() beside the as-path-list one; refuse a whole-list delete while referenced, refuse removing the last community while a community action references the list, and write boolean_operator and exact_match through to every referencing match filter. Mirrors what the as-path-list command already does. Co-Authored-By: Claude Fable 5.1 --- .../community-list/BgpCommunityListCliUtils.h | 65 +++++++++ ...mdConfigProtocolBgpPolicyCommunityList.cpp | 16 +++ ...mdDeleteProtocolBgpPolicyCommunityList.cpp | 38 ++++++ .../CmdConfigBgpPolicyCommunityListTest.cpp | 77 +++++++++++ .../CmdDeleteBgpPolicyCommunityListTest.cpp | 127 ++++++++++++++++++ 5 files changed, 323 insertions(+) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h index 6bea30b09ffa0..b7bcdd6f9622d 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h @@ -10,8 +10,10 @@ #pragma once +#include #include #include +#include #ifndef IS_OSS #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" @@ -60,4 +62,67 @@ inline bgp::bgp_policy::CommunityList& findOrCreateCommunityList( return list; } +// A routing-policy term that names a community-list. A match reference +// (`filter` set) is a COMMUNITY match whose +// communities_filter.community_list_names carries the name; an action +// reference (`filter` null, `isAction`) is a community action whose +// community_action_list_names carries it. `label` identifies the term the +// way the CLI addresses it (`policy

term `). +struct CommunityListReference { + bgp::bgp_policy::CommunityList* filter; + std::string label; + bool isAction; +}; + +// Every reference to `listName`. bgpd resolves both kinds by name at config +// load (CommunityMatch / CommunityAction::PopulateReferences) and fails the +// load on a dangling one, so the delete command refuses while this is +// non-empty. A match's inline copy must also carry the same boolean_operator +// as the list ("Conflicting boolean_operator from the reference") and bgpd +// reads exact_match only from that inline copy, so the config command writes +// both through to every match reference. +inline std::vector findTermsReferencingCommunityList( + bgp::thrift::BgpConfig& cfg, + const std::string& listName) { + std::vector refs; + if (!cfg.policies().has_value()) { + return refs; + } + auto names = [&](const auto& list) { + return std::find(list.begin(), list.end(), listName) != list.end(); + }; + for (auto& policy : *cfg.policies()->bgp_policy_statements()) { + for (auto& term : *policy.policy_entries()) { + auto label = term.sequence_number().has_value() + ? fmt::format( + "policy {} term {}", *policy.name(), *term.sequence_number()) + : fmt::format("policy {} term {}", *policy.name(), *term.name()); + if (term.policy_match_entries().has_value()) { + for (auto& match : *term.policy_match_entries()->match_entries()) { + if (!match.communities_filter().has_value() || + !match.communities_filter()->community_list_names().has_value()) { + continue; + } + auto& filter = *match.communities_filter(); + if (names(*filter.community_list_names())) { + refs.push_back({&filter, label, false}); + } + } + } + for (auto& action : *term.policy_action_entries()) { + if (!action.community_action().has_value() || + !action.community_action() + ->community_action_list_names() + .has_value()) { + continue; + } + if (names(*action.community_action()->community_action_list_names())) { + refs.push_back({nullptr, label, true}); + } + } + } + } + return refs; +} + } // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp index fb0eaf1dd6248..79cdcdb57f12d 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp @@ -237,6 +237,22 @@ CmdConfigProtocolBgpPolicyCommunityList::queryClient( if (!args.attr().empty()) { result.message += fmt::format(" for community-list {}", args.listName()); } + if (args.attr() == kBooleanOperator || args.attr() == kExactMatch) { + // bgpd requires a term's inline copy of the list to carry the same + // boolean_operator ("Conflicting boolean_operator from the reference") + // and reads exact_match only from that copy, so keep every match + // reference in step with the named list. + for (auto& ref : + bgpcli::findTermsReferencingCommunityList(cfg, args.listName())) { + if (ref.filter == nullptr) { + continue; + } + ref.filter->boolean_operator() = *list.boolean_operator(); + if (list.exact_match().has_value()) { + ref.filter->exact_match() = *list.exact_match(); + } + } + } session.saveBgpConfig(); result.message += fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp index 6caa2350a22a6..26f6df1eebde2 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp @@ -13,12 +13,14 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include +#include #include #include #include #include #include #include +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" @@ -92,6 +94,7 @@ CmdDeleteProtocolBgpPolicyCommunityList::queryClient( if (it == lists.end()) { return absent; } + auto refs = bgpcli::findTermsReferencingCommunityList(cfg, args.listName()); if (args.hasCommunity()) { // Remove one value from communities; the list itself stays. auto& list = *it; @@ -100,6 +103,26 @@ CmdDeleteProtocolBgpPolicyCommunityList::queryClient( auto valueIt = std::find(communities.begin(), communities.end(), args.community()); if (valueIt != communities.end()) { + if (communities.size() == 1) { + // A community action resolves the list's values at daemon load and + // fails on an empty one ("Missing communities in CommunityList"); + // a match reference tolerates it. + std::vector actions; + for (const auto& ref : refs) { + if (ref.isAction) { + actions.push_back(ref.label); + } + } + if (!actions.empty()) { + return fmt::format( + "Error: {} is the last community of BGP community-list {}, " + "which is still used by the community action of {}; remove " + "those actions first", + args.community(), + args.listName(), + folly::join(", ", actions)); + } + } communities.erase(valueIt); if (communities.empty()) { // Leave the list looking like one that never had values, so a @@ -121,6 +144,21 @@ CmdDeleteProtocolBgpPolicyCommunityList::queryClient( args.listName(), args.community()); } + // A term's community_list_names / community_action_list_names resolve + // 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. + if (!refs.empty()) { + std::vector referrers; + for (const auto& ref : refs) { + referrers.push_back(ref.label); + } + return fmt::format( + "Error: BGP community-list {} is still referenced by {}; remove those " + "matches and actions first", + args.listName(), + folly::join(", ", referrers)); + } lists.erase(it); session.saveBgpConfig(); return fmt::format( diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp index 6bc17b32ffc9d..80c45b1ac9370 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp @@ -3,6 +3,8 @@ #include #include +#include +#include #include #include #include @@ -54,6 +56,44 @@ class CmdConfigBgpPolicyCommunityListTestFixture : public CmdConfigTestBase { .community_lists(); } + // Seed a routing-policy term whose COMMUNITY match names `listName`, the + // way bgpd reads the reference. Read the inline filter back via filterOf(): + // the vectors may reallocate on the next seed. + 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(); + 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::COMMUNITY_LIST; + matches.back().communities_filter().ensure().community_list_names() = { + listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + + const bgp::bgp_policy::CommunityList& filterOf(const std::string& policy) { + auto& policies = *ConfigSession::getInstance() + .getBgpConfig() + .policies() + ->bgp_policy_statements(); + auto it = std::find_if(policies.begin(), policies.end(), [&](auto& p) { + return *p.name() == policy; + }); + return *(*(*it->policy_entries())[0] + .policy_match_entries() + ->match_entries())[0] + .communities_filter(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -263,4 +303,41 @@ TEST_F( HasSubstr("requires ")); } +// ============================================================================== +// Reference sync — bgpd reads the operator and exact_match from the term's +// inline copy of the list +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyCommunityListTestFixture, + setBooleanOperatorSyncsReferencingFilters) { + run({"CL100"}); + run({"CL200"}); + addPolicyTermMatching("RM100", 10, "CL100"); + addPolicyTermMatching("RM200", 20, "CL200"); + + run({"CL100", "boolean-operator", "AND"}); + EXPECT_EQ(*filterOf("RM100").boolean_operator(), BooleanOperator::AND); + // A filter naming another list is untouched. + EXPECT_EQ(*filterOf("RM200").boolean_operator(), BooleanOperator::OR); + + run({"CL100", "boolean-operator", "OR"}); + EXPECT_EQ(*filterOf("RM100").boolean_operator(), BooleanOperator::OR); +} + +TEST_F( + CmdConfigBgpPolicyCommunityListTestFixture, + setExactMatchSyncsReferencingFilters) { + run({"CL100"}); + addPolicyTermMatching("RM100", 10, "CL100"); + EXPECT_FALSE(filterOf("RM100").exact_match().has_value()); + + run({"CL100", "exact-match", "true"}); + ASSERT_TRUE(filterOf("RM100").exact_match().has_value()); + EXPECT_TRUE(*filterOf("RM100").exact_match()); + + run({"CL100", "exact-match", "false"}); + EXPECT_FALSE(*filterOf("RM100").exact_match()); +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp index 05cce49b00cea..3e3c39b63f56c 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp @@ -3,7 +3,10 @@ #include #include +#include +#include #include +#include #include #include #include @@ -60,6 +63,56 @@ class CmdDeleteBgpPolicyCommunityListTestFixture : public CmdConfigTestBase { .community_lists(); } + // Seed a routing-policy term whose COMMUNITY match names `listName` (the + // term/match CLIs live in higher PRs), the way bgpd reads the reference: + // communities_filter.community_list_names. + void addPolicyTermMatching( + const std::string& policy, + int64_t seq, + const std::string& listName) { + auto& term = addTerm(policy, seq); + auto& matches = *term.policy_match_entries().ensure().match_entries(); + matches.emplace_back(); + matches.back().type() = + bgp::bgp_policy::BgpPolicyAtomicMatchType::COMMUNITY_LIST; + matches.back().communities_filter().ensure().community_list_names() = { + listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + + // Seed a term whose community action names `listName` + // (community_action.community_action_list_names). + void addPolicyTermAction( + const std::string& policy, + int64_t seq, + const std::string& listName) { + auto& term = addTerm(policy, seq); + auto& actions = *term.policy_action_entries(); + actions.emplace_back(); + actions.back().community_action().ensure().community_action_list_names() = { + listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + + bgp::bgp_policy::BgpPolicyTerm& addTerm( + const std::string& policy, + int64_t seq) { + 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; + return terms.back(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -251,4 +304,78 @@ TEST_F( << "session file should not exist after a no-op delete"; } +// ============================================================================== +// Reference guard — a list a routing-policy term still names is not deletable +// ============================================================================== + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteReferencedListRejected) { + configure({"CL100", "description", "in-use"}); + addPolicyTermMatching("RM100", 10, "CL100"); + + auto result = del({"CL100"}); + EXPECT_THAT(result, HasSubstr("still referenced")); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].description(), "in-use"); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteActionReferencedListRejected) { + configure({"CL100", "community", "65000:100"}); + addPolicyTermAction("RM100", 10, "CL100"); + + auto result = del({"CL100"}); + EXPECT_THAT(result, HasSubstr("still referenced")); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + ASSERT_EQ(lists().size(), 1); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteLastCommunityOnActionReferencedListRejected) { + // bgpd: "Missing communities in CommunityList" for an action reference + // to a valueless list. + configure({"CL100", "community", "65000:100"}); + configure({"CL100", "community", "65000:200"}); + addPolicyTermAction("RM100", 10, "CL100"); + + EXPECT_THAT(del({"CL100", "community", "65000:100"}), HasSubstr("Success")); + auto result = del({"CL100", "community", "65000:200"}); + EXPECT_THAT(result, HasSubstr("is the last community")); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].communities(), std::vector({"65000:200"})); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteLastCommunityOnMatchReferencedListAllowed) { + // A match reference tolerates a valueless list. + configure({"CL100", "community", "65000:100"}); + addPolicyTermMatching("RM100", 10, "CL100"); + + auto result = del({"CL100", "community", "65000:100"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted")); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].communities().has_value()); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + referenceToOtherListDoesNotBlockDelete) { + configure({"CL100"}); + configure({"CL200"}); + addPolicyTermMatching("RM100", 10, "CL100"); + addPolicyTermAction("RM100", 20, "CL100"); + + auto result = del({"CL200"}); + EXPECT_THAT( + result, HasSubstr("Successfully deleted BGP community-list CL200")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "CL100"); +} + } // namespace facebook::fboss