From 350df6e2b2e8946b0966b521d9bafcd6990199db Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 7 Aug 2026 19:19:49 +0000 Subject: [PATCH 1/7] [Nexthop] NOS-6657..NOS-6670: fboss2 bgp policy prefix-list entry subcommand Splits the `entry` level of `config protocol bgp policy prefix-list` into its own CLI11 subcommand, mirroring `as-path-list entry `. The previous commit ships the list level and the whole delete command; this one ships the entry level of the config command: prefix-list entry -> prefixes[], keyed by seq_num ... entry base-prefix -> PrefixListEntry.base_prefix ... entry match-logic -> PrefixListEntry.match_logic ... entry prefix-len-range compare-operator -> prefix_len_ranges[0].compare_operator ... entry prefix-len-range value <0-128> -> prefix_len_ranges[0].value ... entry communities -> PrefixListEntry.communities ... entry max-allowed-subnet-count -> PrefixListEntry.max_allowed_golden_prefix_subnet_count ... entry regex -> PrefixListEntry.regex ... entry description -> PrefixListEntry.description - The list name arrives through the ancestor-args tuple, and lookup/create is shared with the parent through BgpPrefixListCliUtils.h. A list attribute typed alongside `entry` is rejected rather than silently dropped, because only the leaf command runs. - base-prefix is validated with folly::IPAddress::tryCreateNetwork and additionally requires an explicit /len, because folly silently fills in an address-family default mask for a bare address. The string is stored as typed, not normalized. - The thrift models a list of accepted length ranges (prefix_len_ranges), but the documented CLI exposes a single range, so prefix-len-range compare-operator|value both target the one CompareNumericValue the CLI maintains at prefix_len_ranges[0], created on first use and rolled back if the value is rejected. - communities is a thrift set: repeated invocations accumulate members, and re-adding an existing member reports it without duplicating. - Four value shapes have no factory in BgpCliAttrHandlers.h (base-prefix, communities, prefix-len-range, regex) and get hand-written handlers; the rest go through the shared factories. The composed `prefix-len-range ` display names get static storage so the factories' string_view name capture cannot dangle. - `delete ... prefix-list entry ` already ships in the previous commit and is untouched here. - unit tests (14): CmdConfigBgpPolicyPrefixListEntryTest covering arg validation, bare entry create, seq-num keying, the entry-level attribute round-trips, communities accumulation, the composed prefix-len-range sub-attributes, the rejection messages (explicit-/len requirement, out-of-range mask, second slash, v6 accept, the 0-128 range bound), and phantom-rollback for entry and range. - integration tests (2): ConfigBgpPolicyPrefixListEntryTest stages and commits, then verifies through bgpd's getRunningConfig RPC including prefix_len_ranges[0] and communities, with the sibling entry surviving the single-entry delete. Co-Authored-By: Claude Fable 5 --- cmake/CliFboss2.cmake | 2 + cmake/CliFboss2TestConfig.cmake | 1 + cmake/CliFboss2TestIntegrationTest.cmake | 1 + fboss/cli/fboss2/BUCK | 2 + fboss/cli/fboss2/CmdListConfig.cpp | 13 + ...ConfigProtocolBgpPolicyPrefixListEntry.cpp | 426 ++++++++++++++++++ ...mdConfigProtocolBgpPolicyPrefixListEntry.h | 95 ++++ fboss/cli/fboss2/test/config/BUCK | 1 + .../CmdConfigBgpPolicyPrefixListEntryTest.cpp | 289 ++++++++++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + .../ConfigBgpPolicyPrefixListEntryTest.cpp | 176 ++++++++ 11 files changed, 1007 insertions(+) create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 86d1efda6834f..a019608ead973 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -984,6 +984,8 @@ add_library(fboss2_config_lib 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/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.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 diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index ad3bfebb2abfa..983ed0e18e451 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -17,6 +17,7 @@ add_executable(fboss2_cmd_config_test 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/CmdConfigBgpPolicyPrefixListEntryTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index 3c586c6cd55f2..02d8e1add9da4 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -21,6 +21,7 @@ add_executable(fboss2_integration_test 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/ConfigBgpPolicyPrefixListEntryTest.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 edc65d609d290..2ec9628e1fe32 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1218,6 +1218,7 @@ cpp_library( "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/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1370,6 +1371,7 @@ cpp_library( "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/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 94d83960a4920..1ebf883405018 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -46,6 +46,7 @@ #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/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.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" @@ -457,6 +458,18 @@ const CommandTree& kConfigCommandTree() { CmdConfigProtocolBgpPolicyPrefixList>, argRegistrar< CmdConfigProtocolBgpPolicyPrefixListTraits>, + {{ + "entry", + "Configure a prefix-list entry: " + "[ ...] (base-prefix, " + "communities, description, match-logic, " + "max-allowed-subnet-count, " + "prefix-len-range, regex)", + commandHandler< + CmdConfigProtocolBgpPolicyPrefixListEntry>, + argRegistrar< + CmdConfigProtocolBgpPolicyPrefixListEntryTraits>, + }}, }}, }, }, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp new file mode 100644 index 0000000000000..49961cf97deb2 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp @@ -0,0 +1,426 @@ +/* + * 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/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#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/BgpCliValueParsers.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. +constexpr std::string_view kBasePrefix = "base-prefix"; +constexpr std::string_view kCommunities = "communities"; +constexpr std::string_view kCompareOperator = "compare-operator"; +constexpr std::string_view kDescription = "description"; +constexpr std::string_view kMatchLogic = "match-logic"; +constexpr std::string_view kMaxAllowedSubnetCount = "max-allowed-subnet-count"; +constexpr std::string_view kPrefixLenRange = "prefix-len-range"; +constexpr std::string_view kRegex = "regex"; +constexpr std::string_view kValue = "value"; + +// prefix-len-range compare-operator values +// (routing_policy.ComparisonOperator names). Unlike the list level, the range +// additionally accepts RG (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"; + +// match-logic values (routing_policy.MatchValueLogicOperator names). +constexpr std::string_view kMatchLogicEqual = "EQUAL"; +constexpr std::string_view kMatchLogicNotEqual = "NOT_EQUAL"; + +// prefix-len-range value bounds: a prefix length (v6 caps it at 128). +constexpr int32_t kPrefixLenMax = 128; + +using ComparisonOperator = bgp::routing_policy::ComparisonOperator; +using CompareNumericValue = bgp::routing_policy::CompareNumericValue; +using MatchValueLogicOperator = bgp::routing_policy::MatchValueLogicOperator; +using PrefixListEntry = bgp::routing_policy::PrefixListEntry; +using bgpcli::AttrHandler; +using bgpcli::enumAttr; +using bgpcli::err; +using bgpcli::intAttr; +using bgpcli::joinedStringAttr; +using bgpcli::ok; +using bgpcli::Result; +using bgpcli::Tokens; + +// ---- value lookups ---------------------------------------------------------- + +std::optional lookupMatchLogic(const std::string& s) { + if (s == kMatchLogicEqual) { + return MatchValueLogicOperator::EQUAL; + } + if (s == kMatchLogicNotEqual) { + return MatchValueLogicOperator::NOT_EQUAL; + } + return std::nullopt; +} + +std::optional lookupPrefixLenRangeOperator( + 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; + } + if (s == kCompareOperatorRg) { + return ComparisonOperator::RG; + } + return std::nullopt; +} + +// The prefix-len-range sub-attributes report themselves with the two-token +// name the user typed, so the message names the command rather than the +// sub-key alone. Hoisted so the string_view the factory captures outlives it. +const std::string& prefixLenRangeCompareOperatorName() { + static const std::string kName = + fmt::format("{} {}", kPrefixLenRange, kCompareOperator); + return kName; +} + +const std::string& prefixLenRangeValueName() { + static const std::string kName = + fmt::format("{} {}", kPrefixLenRange, kValue); + return kName; +} + +// ---- prefix-len-range setters ----------------------------------------------- + +void setRangeCompareOperator( + CompareNumericValue& range, + ComparisonOperator op) { + range.compare_operator() = op; +} + +void setRangeValue(CompareNumericValue& range, int32_t value) { + range.value() = value; +} + +// ---- prefix-len-range sub-attribute registry -------------------------------- +// The thrift models a list of accepted length ranges (prefix_len_ranges); the +// documented CLI exposes a single range, so these target the one +// CompareNumericValue the entry handler maintains at prefix_len_ranges[0]. +const std::map, std::less<>>& +prefixLenRangeAttrHandlers() { + static const std::string kRangeOperatorValues = fmt::format( + "{}|{}|{}|{}|{}|{}|{}", + kCompareOperatorEq, + kCompareOperatorGe, + kCompareOperatorLe, + kCompareOperatorNe, + kCompareOperatorGt, + kCompareOperatorLt, + kCompareOperatorRg); + static const std::string kValueRange = fmt::format("0-{}", kPrefixLenMax); + static const std:: + map, std::less<>> + kHandlers = { + {std::string(kCompareOperator), + enumAttr( + prefixLenRangeCompareOperatorName(), + kRangeOperatorValues, + lookupPrefixLenRangeOperator, + setRangeCompareOperator)}, + {std::string(kValue), + intAttr( + prefixLenRangeValueName(), + kValueRange, + 0, + kPrefixLenMax, + setRangeValue)}, + }; + return kHandlers; +} + +const std::string& prefixLenRangeUsage() { + static const std::string kUsage = fmt::format( + "Error: {} requires <{}|{}> ", + kPrefixLenRange, + kCompareOperator, + kValue); + return kUsage; +} + +// ---- entry-level setters ---------------------------------------------------- + +void setDescription(PrefixListEntry& entry, const std::string& description) { + entry.description() = description; +} + +void setMatchLogic(PrefixListEntry& entry, MatchValueLogicOperator op) { + entry.match_logic() = op; +} + +void setMaxAllowedSubnetCount(PrefixListEntry& entry, int32_t count) { + entry.max_allowed_golden_prefix_subnet_count() = count; +} + +// ---- hand-written handlers -------------------------------------------------- +// These four remain full handlers because their value shape has no factory: a +// prefix requiring an explicit mask, an accumulating set, a composed +// sub-attribute, and a stored-verbatim regex. + +Result basePrefix(PrefixListEntry& entry, const Tokens& values) { + if (values.size() != 1) { + return err(fmt::format("Error: {} requires ", kBasePrefix)); + } + // Require an explicit /len — folly fills in a default mask for a bare + // address, so its absence must be checked separately. The string is stored + // as typed, not normalized. + if (values[0].find('/') == std::string::npos || + folly::IPAddress::tryCreateNetwork(values[0]).hasError()) { + return err( + fmt::format( + "Error: Invalid {} value '{}'; expected ", + kBasePrefix, + values[0])); + } + entry.base_prefix() = values[0]; + return ok(fmt::format("Successfully set {} to: {}", kBasePrefix, values[0])); +} + +Result communities(PrefixListEntry& entry, const Tokens& values) { + if (values.size() != 1) { + return err( + fmt::format("Error: {} requires ", kCommunities)); + } + // The accepted communities are a set; repeated invocations accumulate + // members rather than replacing the set. + const bool inserted = entry.communities().ensure().insert(values[0]).second; + return ok( + inserted + ? fmt::format("Successfully added {} to {}", values[0], kCommunities) + : fmt::format("{} already contains {}", kCommunities, values[0])); +} + +Result prefixLenRange(PrefixListEntry& entry, const Tokens& values) { + if (values.empty()) { + return err(prefixLenRangeUsage()); + } + auto handler = prefixLenRangeAttrHandlers().find(values[0]); + if (handler == prefixLenRangeAttrHandlers().end()) { + return err(prefixLenRangeUsage()); + } + auto& ranges = *entry.prefix_len_ranges(); + const bool rangeCreated = ranges.empty(); + if (rangeCreated) { + ranges.emplace_back(); + } + auto result = + handler->second(ranges.front(), Tokens(values.begin() + 1, values.end())); + // A rejected value must not leave a phantom range behind. + if (!result.ok && rangeCreated) { + ranges.pop_back(); + } + return result; +} + +Result regex(PrefixListEntry& entry, const Tokens& values) { + if (values.size() != 1) { + return err(fmt::format("Error: {} requires ", kRegex)); + } + // The thrift documents regex as an alternative to prefix_len_ranges; the CLI + // stores what is given and leaves precedence to the daemon. + entry.regex() = values[0]; + return ok(fmt::format("Successfully set {} to: {}", kRegex, values[0])); +} + +// ---- entry-level attribute registry ---------------------------------------- +// One line per documented attribute: its dispatch key, its value shape, and +// the setter or handler that stores it. +const std::map, std::less<>>& +entryAttrHandlers() { + static const std::string kMatchLogicValues = + fmt::format("{}|{}", kMatchLogicEqual, kMatchLogicNotEqual); + static const std::map, std::less<>> + kHandlers = { + {std::string(kBasePrefix), basePrefix}, + {std::string(kCommunities), communities}, + {std::string(kDescription), + joinedStringAttr(kDescription, setDescription)}, + {std::string(kMatchLogic), + enumAttr( + kMatchLogic, + kMatchLogicValues, + lookupMatchLogic, + setMatchLogic)}, + {std::string(kMaxAllowedSubnetCount), + intAttr( + kMaxAllowedSubnetCount, + "non-negative integer", + 0, + std::numeric_limits::max(), + setMaxAllowedSubnetCount)}, + {std::string(kPrefixLenRange), prefixLenRange}, + {std::string(kRegex), regex}, + }; + return kHandlers; +} + +std::string validAttrList() { + std::string out; + for (const auto& [name, _] : entryAttrHandlers()) { + 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 BgpPrefixListConfig). +BgpPrefixListEntryConfig::BgpPrefixListEntryConfig(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: entry is required, optionally followed by an " + " "); + } + auto seq = bgpcli::parseNonNegInt32(v[0]); + if (!seq) { + throw std::invalid_argument( + fmt::format( + "Error: entry must be a non-negative integer, got '{}'", + v[0])); + } + seqNum_ = *seq; + if (v.size() == 1) { + return; // bare `entry `: create it + } + + attr_ = v[1]; + values_.assign(v.begin() + 2, v.end()); + + if (entryAttrHandlers().find(attr_) == entryAttrHandlers().end()) { + throw std::invalid_argument( + fmt::format( + "Error: unknown prefix-list entry attribute '{}'. Valid " + "attributes: {}", + attr_, + validAttrList())); + } +} + +CmdConfigProtocolBgpPolicyPrefixListEntryTraits::RetType +CmdConfigProtocolBgpPolicyPrefixListEntry::queryClient( + const HostInfo& /* hostInfo */, + const BgpPrefixListConfig& listArgs, + const ObjectArgType& args) { + // The parent parse accepts `prefix-list ... entry + // ...`, but only the leaf (this command) runs — silently dropping the + // list-level attribute would look like it was staged. Reject the mix. + if (!listArgs.attr().empty()) { + return fmt::format( + "Error: configure prefix-list attributes and entry in separate " + "commands (got prefix-list attribute '{}' alongside entry {})", + listArgs.attr(), + args.seqNum()); + } + + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool listCreated = !bgpcli::prefixListExists(cfg, listArgs.listName()); + auto& list = bgpcli::findOrCreatePrefixList(cfg, listArgs.listName()); + const bool entryCreated = !bgpcli::prefixListEntryExists(list, args.seqNum()); + auto& entry = bgpcli::findOrCreatePrefixListEntry(list, args.seqNum()); + + Result result = args.attr().empty() + ? ok(entryCreated + ? fmt::format( + "Successfully created BGP prefix-list {} entry {}", + listArgs.listName(), + args.seqNum()) + : fmt::format( + "BGP prefix-list {} entry {} already exists", + listArgs.listName(), + args.seqNum())) + // The attribute is guaranteed valid: BgpPrefixListEntryConfig's + // constructor rejects an unknown attribute before we get here. + : entryAttrHandlers().find(args.attr())->second(entry, args.values()); + + if (result.ok) { + if (!args.attr().empty()) { + result.message += fmt::format( + " for prefix-list {} entry {}", listArgs.listName(), args.seqNum()); + } + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } else { + // A rejected value must not leave a phantom entry (or a phantom list + // implicitly created for it) visible to later lookups in this process. + if (entryCreated) { + list.prefixes()->pop_back(); + } + if (listCreated) { + cfg.policies()->prefix_lists()->pop_back(); + } + } + return result.message; +} + +void CmdConfigProtocolBgpPolicyPrefixListEntry::printOutput( + const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdConfigProtocolBgpPolicyPrefixListEntry, + CmdConfigProtocolBgpPolicyPrefixListEntryTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h new file mode 100644 index 0000000000000..25f0aebcd67cd --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h @@ -0,0 +1,95 @@ +/* + * 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/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `entry [ ...]`, validated at +// construction. An entry (routing_policy.PrefixListEntry in +// PrefixList.prefixes[]) is keyed by ; the list it belongs to is +// supplied by the parent command's args. +// +// Grammar (from the FBOSS proposed syntax): +// ... prefix-list entry (create/select) +// ... prefix-list entry base-prefix +// ... prefix-list entry communities +// ... prefix-list entry description +// ... prefix-list entry match-logic +// ... prefix-list entry max-allowed-subnet-count +// ... prefix-list entry prefix-len-range compare-operator +// +// ... prefix-list entry prefix-len-range value <0-128> +// ... prefix-list entry regex +class BgpPrefixListEntryConfig : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpPrefixListEntryConfig(std::vector v); + int32_t seqNum() const { + return seqNum_; + } + 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: + int32_t seqNum_{0}; + std::string attr_; // matched dispatch key ("" = bare create) + std::vector values_; +}; + +// The entry level of the prefix-list family as its own CLI11 subcommand; the +// parent's parsed args arrive through the ancestor-args tuple, mirroring +// CmdConfigProtocolBgpPolicyAsPathListEntry. +struct CmdConfigProtocolBgpPolicyPrefixListEntryTraits + : public WriteCommandTraits { + using ParentCmd = CmdConfigProtocolBgpPolicyPrefixList; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // Entry has no nested subcommands; stop CLI11's parent-chain fallthrough + // from stealing a value token that spells `entry` (e.g. in a description). + cmd.positionals_at_end(); + cmd.add_option("args", args, " [ ...]"); + } + using ObjectArgType = BgpPrefixListEntryConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyPrefixListEntry + : public CmdHandler< + CmdConfigProtocolBgpPolicyPrefixListEntry, + CmdConfigProtocolBgpPolicyPrefixListEntryTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyPrefixListEntryTraits::ObjectArgType; + using RetType = CmdConfigProtocolBgpPolicyPrefixListEntryTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BgpPrefixListConfig& listArgs, + 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 c74372c510f28..813a638517e1f 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", + "CmdConfigBgpPolicyPrefixListEntryTest.cpp", "CmdConfigBgpPolicyPrefixListTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp new file mode 100644 index 0000000000000..3584a47283641 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp @@ -0,0 +1,289 @@ +// (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/CmdConfigProtocolBgpPolicyPrefixList.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.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::ComparisonOperator; +using facebook::bgp::routing_policy::MatchValueLogicOperator; + +namespace facebook::fboss { + +// The entry 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 +// CmdConfigBgpPolicyPrefixListTest). +class CmdConfigBgpPolicyPrefixListEntryTestFixture : public CmdConfigTestBase { + public: + CmdConfigBgpPolicyPrefixListEntryTestFixture() + : CmdConfigTestBase("bgp_prefix_list_entry_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + // Invoke the entry handler the way the framework does: the parent's parsed + // args plus the entry's own tokens. + std::string runEntry( + const std::vector& listTokens, + const std::vector& entryTokens) { + CmdConfigProtocolBgpPolicyPrefixListEntry cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient( + hostInfo, + BgpPrefixListConfig(listTokens), + BgpPrefixListEntryConfig(entryTokens)); + } + + std::string runList(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(); + } + + // Entry `idx` of list `listIdx`. + const bgp::routing_policy::PrefixListEntry& entry( + size_t listIdx, + size_t idx) { + return lists()[listIdx].prefixes()->at(idx); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpPrefixListEntryConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, argValidation) { + // Bare create. + auto bare = BgpPrefixListEntryConfig({"10"}); + EXPECT_EQ(bare.seqNum(), 10); + EXPECT_TRUE(bare.attr().empty()); + + // Attribute with values. + auto attr = BgpPrefixListEntryConfig({"10", "base-prefix", "10.0.0.0/8"}); + EXPECT_EQ(attr.seqNum(), 10); + EXPECT_EQ(attr.attr(), "base-prefix"); + EXPECT_EQ(attr.values(), std::vector({"10.0.0.0/8"})); + + // Invalid: empty, non-integer and negative seq-nums, unknown attribute. + EXPECT_THROW(BgpPrefixListEntryConfig({}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListEntryConfig({"ten"}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListEntryConfig({"-1"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListEntryConfig({"10", "no-such-attr", "1"}), + std::invalid_argument); + // ip-version is a list attribute, not an entry attribute. + EXPECT_THROW( + BgpPrefixListEntryConfig({"10", "ip-version", "v4"}), + std::invalid_argument); +} + +// ============================================================================== +// Entry-level handlers, and the reject paths that must persist nothing +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, bareCreateEntry) { + auto result = runEntry({"PL100"}, {"10"}); + EXPECT_THAT( + result, HasSubstr("Successfully created BGP prefix-list PL100 entry 10")); + ASSERT_EQ(lists().size(), 1); + ASSERT_EQ(lists()[0].prefixes()->size(), 1); + ASSERT_TRUE(entry(0, 0).seq_num().has_value()); + EXPECT_EQ(*entry(0, 0).seq_num(), 10); +} + +TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setAttributes) { + runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); + runEntry({"PL100"}, {"10", "description", "spine", "block"}); + runEntry({"PL100"}, {"10", "match-logic", "NOT_EQUAL"}); + runEntry({"PL100"}, {"10", "max-allowed-subnet-count", "64"}); + runEntry({"PL100"}, {"10", "regex", "^10\\..*"}); + + ASSERT_EQ(lists().size(), 1); + ASSERT_EQ(lists()[0].prefixes()->size(), 1); + const auto& e = entry(0, 0); + EXPECT_EQ(*e.seq_num(), 10); + EXPECT_EQ(*e.base_prefix(), "10.0.0.0/8"); + EXPECT_EQ(*e.description(), "spine block"); + EXPECT_EQ(*e.match_logic(), MatchValueLogicOperator::NOT_EQUAL); + EXPECT_EQ(*e.max_allowed_golden_prefix_subnet_count(), 64); + EXPECT_EQ(*e.regex(), "^10\\..*"); +} + +TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setPrefixLenRange) { + runEntry({"PL100"}, {"10", "prefix-len-range", "compare-operator", "RG"}); + runEntry({"PL100"}, {"10", "prefix-len-range", "value", "24"}); + + // Both sub-attributes land on the single prefix_len_ranges[0] element. + ASSERT_EQ(entry(0, 0).prefix_len_ranges()->size(), 1); + const auto& range = entry(0, 0).prefix_len_ranges()->front(); + EXPECT_EQ(*range.compare_operator(), ComparisonOperator::RG); + EXPECT_EQ(*range.value(), 24); +} + +TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, communitiesAccumulate) { + auto result = runEntry({"PL100"}, {"10", "communities", "65000:100"}); + EXPECT_THAT(result, HasSubstr("Successfully added 65000:100 to communities")); + runEntry({"PL100"}, {"10", "communities", "65000:200"}); + + ASSERT_TRUE(entry(0, 0).communities().has_value()); + EXPECT_THAT( + *entry(0, 0).communities(), + UnorderedElementsAre("65000:100", "65000:200")); + + // Re-adding an existing member reports it without duplicating. + auto repeated = runEntry({"PL100"}, {"10", "communities", "65000:100"}); + EXPECT_THAT(repeated, HasSubstr("communities already contains 65000:100")); + EXPECT_EQ(entry(0, 0).communities()->size(), 2); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + entriesAccumulateAndAreKeyed) { + runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); + runEntry({"PL100"}, {"20", "base-prefix", "192.168.0.0/16"}); + ASSERT_EQ(lists().size(), 1); + ASSERT_EQ(lists()[0].prefixes()->size(), 2); + + // Re-referencing an existing entry by seq-num updates it, not appends. + runEntry({"PL100"}, {"10", "match-logic", "EQUAL"}); + EXPECT_EQ(lists()[0].prefixes()->size(), 2); + EXPECT_EQ(*entry(0, 0).base_prefix(), "10.0.0.0/8"); + EXPECT_EQ(*entry(0, 0).match_logic(), MatchValueLogicOperator::EQUAL); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + invalidBasePrefixRejected) { + // A base-prefix without an explicit /len is rejected, as is a non-address. + auto noLen = runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0"}); + EXPECT_THAT( + noLen, + HasSubstr("Invalid base-prefix value '10.0.0.0'; expected ")); + auto garbage = runEntry({"PL100"}, {"10", "base-prefix", "not-a-prefix"}); + EXPECT_THAT(garbage, HasSubstr("Invalid base-prefix value")); + // folly rejects the rest: an out-of-range mask and a second slash. + auto badMask = runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/99"}); + EXPECT_THAT(badMask, HasSubstr("Invalid base-prefix value")); + auto twoSlash = runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8/8"}); + EXPECT_THAT(twoSlash, HasSubstr("Invalid base-prefix value")); + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, basePrefixAcceptsV6) { + auto result = runEntry({"PL100"}, {"10", "base-prefix", "2001:db8::/32"}); + EXPECT_THAT(result, HasSubstr("Successfully set base-prefix")); + // Stored as typed, not normalized. + EXPECT_EQ(*entry(0, 0).base_prefix(), "2001:db8::/32"); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + invalidPrefixLenRangeRejected) { + // Unknown sub-attribute. + auto badSub = runEntry({"PL100"}, {"10", "prefix-len-range", "min", "8"}); + EXPECT_THAT( + badSub, + HasSubstr( + "Error: prefix-len-range requires ")); + // Out-of-range length. + auto badLen = runEntry({"PL100"}, {"10", "prefix-len-range", "value", "129"}); + EXPECT_THAT( + badLen, + HasSubstr("Invalid prefix-len-range value value '129'; expected 0-128")); + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + rejectedPrefixLenRangeKeepsNoPhantomRange) { + // Land the entry first, then reject a range value on it: the entry survives + // but no phantom prefix_len_ranges element may appear. + runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); + auto result = runEntry({"PL100"}, {"10", "prefix-len-range", "value", "300"}); + EXPECT_THAT(result, HasSubstr("Invalid")); + ASSERT_EQ(lists()[0].prefixes()->size(), 1); + EXPECT_TRUE(entry(0, 0).prefix_len_ranges()->empty()); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + rejectedEntryOnExistingListKeepsList) { + // Create the list first, then reject an entry value on it. + runList({"PL100", "description", "keep-me"}); + ASSERT_EQ(lists().size(), 1); + + auto result = runEntry({"PL100"}, {"10", "match-logic", "MAYBE"}); + EXPECT_THAT(result, HasSubstr("Invalid")); + // The pre-existing list survives; only the phantom entry is rolled back. + ASSERT_EQ(lists().size(), 1); + EXPECT_TRUE(lists()[0].prefixes()->empty()); + EXPECT_EQ(*lists()[0].description(), "keep-me"); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + rejectedEntryKeepsExistingEntries) { + runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); + ASSERT_EQ(lists()[0].prefixes()->size(), 1); + + auto result = runEntry({"PL100"}, {"20", "match-logic", "MAYBE"}); + EXPECT_THAT(result, HasSubstr("Invalid")); + // Only the phantom entry is rolled back; the existing one survives. + ASSERT_EQ(lists()[0].prefixes()->size(), 1); + EXPECT_EQ(*entry(0, 0).seq_num(), 10); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + reReferenceReportsExisting) { + runEntry({"PL100"}, {"10"}); + EXPECT_THAT( + runEntry({"PL100"}, {"10"}), HasSubstr("entry 10 already exists")); + EXPECT_EQ(lists()[0].prefixes()->size(), 1); +} + +// A list-level attribute alongside an entry must be rejected: only the leaf +// command runs, so the list attribute would be silently dropped. +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + listAttributeMixedWithEntryRejected) { + auto result = runEntry({"PL100", "description", "mixed"}, {"10"}); + EXPECT_THAT(result, HasSubstr("separate commands")); + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 01a74ad36e009..91dfdccc697f3 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", + "ConfigBgpPolicyPrefixListEntryTest.cpp", "ConfigBgpPolicyPrefixListTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp new file mode 100644 index 0000000000000..2b708587b8234 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp @@ -0,0 +1,176 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy prefix-list + * entry [ ...]`. + * + * Scope: the entry level. 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[].prefixes[] entry of bgpd's running config + * (via getRunningConfig RPC). Stage-only behavior (attribute parsing, + * validation, rejection) is covered by the unit tests; an integration test + * that never commits exercises no daemon. + * + * Requirements: + * - The fboss2-dev binary under test (config subcommand tree). + * - HOME is set (the session file lives under $HOME/.fboss2). + * - bgpd is installed/active (commit restarts it). + */ + +#include +#include +#include +#include +#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 / entry keys, unlikely to collide with a real list in +// the device's running BGP config. +const std::string kList = "FBOSS2-TEST-PREFIXES-ENTRY"; +const std::string kEntrySeq = "10"; +const std::string kEntrySeq2 = "20"; +} // namespace + +class ConfigBgpPolicyPrefixListEntryTest : public ConfigBgpTestBase { + protected: + // Stage `config protocol bgp policy prefix-list entry ` + // WITHOUT clearing the staged session, so attributes can accumulate across + // invocations. Returns the staged session JSON. + folly::dynamic stageEntry(const std::vector& tokens) { + std::vector args = { + "config", "protocol", "bgp", "policy", "prefix-list", kList, "entry"}; + 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; + } + + // The .prefixes[] entry with seq_num `seqNum` inside `list`, or nullptr. + static const folly::dynamic* findEntry( + const folly::dynamic& list, + int64_t seqNum) { + if (list.count("prefixes") == 0) { + return nullptr; + } + for (const auto& entry : list["prefixes"]) { + if (entry.count("seq_num") && entry["seq_num"].asInt() == seqNum) { + return &entry; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyPrefixListEntryTest, SetAttributesAndCommit) { + discardSession(); + clearBgpSession(); + stageEntry({kEntrySeq, "base-prefix", "10.0.0.0/8"}); + stageEntry({kEntrySeq, "match-logic", "NOT_EQUAL"}); + stageEntry({kEntrySeq, "prefix-len-range", "compare-operator", "GE"}); + stageEntry({kEntrySeq, "prefix-len-range", "value", "24"}); + stageEntry({kEntrySeq, "communities", "65000:100"}); + stageEntry({kEntrySeq, "description", "test"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + // Verify the nested entry paths .prefixes[].{base_prefix, match_logic, + // prefix_len_ranges[0], communities, description} through the daemon's own + // view of its config. + auto running = readRunningBgpConfigViaRpc(); + const auto* list = findList(running, kList); + ASSERT_NE(list, nullptr) << "bgpd's running config has no prefix-list " + << kList; + const auto* entry = findEntry(*list, 10); + ASSERT_NE(entry, nullptr) << "running config has no entry " << kEntrySeq; + EXPECT_EQ((*entry)["base_prefix"].asString(), "10.0.0.0/8"); + // routing_policy.MatchValueLogicOperator.NOT_EQUAL = 1 (integer on the + // SimpleJSON wire). + EXPECT_EQ((*entry)["match_logic"].asInt(), 1); + ASSERT_EQ((*entry)["prefix_len_ranges"].size(), 1); + // routing_policy.ComparisonOperator.GE = 2. + EXPECT_EQ((*entry)["prefix_len_ranges"][0]["compare_operator"].asInt(), 2); + EXPECT_EQ((*entry)["prefix_len_ranges"][0]["value"].asInt(), 24); + ASSERT_EQ((*entry)["communities"].size(), 1); + EXPECT_EQ((*entry)["communities"][0].asString(), "65000:100"); + EXPECT_EQ((*entry)["description"].asString(), "test"); +} + +TEST_F(ConfigBgpPolicyPrefixListEntryTest, DeleteEntryAndCommit) { + // Land a prefix-list with two entries, delete one entry through a second + // commit, and verify the list and the other entry survive in bgpd's running + // config. + discardSession(); + clearBgpSession(); + stageEntry({kEntrySeq, "base-prefix", "10.0.0.0/8"}); + stageEntry({kEntrySeq2, "base-prefix", "192.168.0.0/16"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + { + auto running = readRunningBgpConfigViaRpc(); + const auto* list = findList(running, kList); + ASSERT_NE(list, nullptr) + << "setup commit did not land the prefix-list in bgpd's running " + "config"; + ASSERT_NE(findEntry(*list, 10), nullptr); + ASSERT_NE(findEntry(*list, 20), nullptr); + } + + clearBgpSession(); + auto result = runCli( + {"delete", + "protocol", + "bgp", + "policy", + "prefix-list", + kList, + "entry", + kEntrySeq}); + EXPECT_THAT( + result.stdout, + HasSubstr( + fmt::format( + "Successfully deleted BGP prefix-list {} entry {}", + kList, + kEntrySeq))); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after delete commit; state=" + << bgpDaemonActiveState(); + auto running = readRunningBgpConfigViaRpc(); + const auto* list = findList(running, kList); + ASSERT_NE(list, nullptr) + << "deleting one entry must not delete the prefix-list"; + EXPECT_EQ(findEntry(*list, 10), nullptr) + << "deleted entry still present in bgpd's running config"; + EXPECT_NE(findEntry(*list, 20), nullptr) + << "surviving entry missing from bgpd's running config"; +} From af8590328fbf7f296ba8f252347fcfad24c91088 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:24:38 +0000 Subject: [PATCH 2/7] Emit prefix-list entry commands from bgp_json_to_cli.py Each prefix-list entry becomes `... prefix-list entry ` lines (base-prefix, description, match-logic when not EQUAL, max-allowed-subnet-count, the single prefix-len-range the CLI expresses, regex, one `communities` line per value), emitted inside the list generator. An entry without seq_num, extra prefix_len_ranges and ip_version have no CLI spelling and are emitted as `# WARNING:` lines. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 94 +++++++++++++++++++ .../tools/tests/test_bgp_json_to_cli.py | 80 ++++++++++++++++ 2 files changed, 174 insertions(+) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index a159f42b21eb7..ffe618407f687 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -653,12 +653,106 @@ def generate_prefix_list_commands(prefix_list: dict[str, Any]) -> list[str]: ) ) commands.extend(_prefix_list_warnings(name, prefix_list)) + for entry in prefix_list.get("prefixes") or []: + commands.extend(generate_prefix_list_entry_commands(name, entry)) if all(command.startswith("#") for command in commands): # Warnings do not recreate the list; preserve it with a bare command. commands.append(prefix) return commands +_MATCH_LOGIC_NAMES = {0: "EQUAL", 1: "NOT_EQUAL"} + + +def _prefix_list_entry_scalar_commands(prefix: str, entry: dict[str, Any]) -> list[str]: + """The single-valued entry attributes, in the CLI's attribute order.""" + commands = [] + if entry.get("base_prefix"): + commands.append( + f"{prefix} base-prefix {escape_shell_arg(entry['base_prefix'])}" + ) + if entry.get("description"): + commands.append( + f"{prefix} description {escape_shell_arg(entry['description'])}" + ) + if "match_logic" in entry: + raw = entry["match_logic"] + logic = ( + raw if isinstance(raw, str) else _MATCH_LOGIC_NAMES.get(int(raw), str(raw)) + ) + if logic != "EQUAL": + commands.append(f"{prefix} match-logic {escape_shell_arg(logic)}") + if "max_allowed_golden_prefix_subnet_count" in entry: + commands.append( + f"{prefix} max-allowed-subnet-count " + f"{escape_shell_arg(entry['max_allowed_golden_prefix_subnet_count'])}" + ) + return commands + + +def _prefix_list_entry_range_commands( + prefix: str, label: str, entry: dict[str, Any] +) -> list[str]: + """`prefix-len-range` lines for the single range the CLI can express.""" + ranges = entry.get("prefix_len_ranges") or [] + if not ranges: + return [] + first = ranges[0] + commands = [] + if "compare_operator" in first: + commands.append( + f"{prefix} prefix-len-range compare-operator " + f"{escape_shell_arg(_comparison_operator_name(first['compare_operator']))}" + ) + if "value" in first: + commands.append( + f"{prefix} prefix-len-range value {escape_shell_arg(first['value'])}" + ) + if len(ranges) > 1: + commands.append( + _warning( + f"{label}: only the first of {len(ranges)} prefix_len_ranges is " + "expressible; the rest are not emitted" + ) + ) + return commands + + +def generate_prefix_list_entry_commands( + list_name: str, entry: dict[str, Any] +) -> list[str]: + """Generate `... prefix-list entry ` commands for one entry. + + The CLI keys entries by seq_num and supports a single prefix_len_range; + anything beyond that surfaces as a warning. + """ + if "seq_num" not in entry: + return [ + _warning( + f"prefix-list {list_name}: entry '{entry.get('base_prefix', '')}' " + "has no seq_num and cannot be addressed by the CLI; not emitted" + ) + ] + label = f"prefix-list {list_name} entry {entry['seq_num']}" + prefix = ( + f"config protocol bgp policy prefix-list {escape_shell_arg(list_name)} " + f"entry {escape_shell_arg(entry['seq_num'])}" + ) + commands = _prefix_list_entry_scalar_commands(prefix, entry) + commands.extend(_prefix_list_entry_range_commands(prefix, label, entry)) + if entry.get("regex"): + commands.append(f"{prefix} regex {escape_shell_arg(entry['regex'])}") + for community in sorted(entry.get("communities") or []): + commands.append(f"{prefix} communities {escape_shell_arg(community)}") + if "ip_version" in entry: + commands.append( + _warning(f"{label}: ip_version has no CLI equivalent; not emitted") + ) + if not commands: + commands.append(prefix) + return commands + + def generate_policy_commands(config: dict[str, Any]) -> list[str]: """Generate the `config protocol bgp policy ...` 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 2947ad31597e5..7b9e64c1e1449 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 @@ -30,6 +30,7 @@ generate_peer_group_commands, generate_policy_commands, generate_prefix_list_commands, + generate_prefix_list_entry_commands, json_to_cli, ) @@ -1342,6 +1343,85 @@ def test_policy_block_order(self) -> None: ) +class GeneratePrefixListEntryCommandsTest(unittest.TestCase): + """Tests for generate_prefix_list_entry_commands (entry grammar).""" + + PREFIX = "config protocol bgp policy prefix-list PL entry 10" + + def test_full_entry(self) -> None: + commands = generate_prefix_list_entry_commands( + "PL", + { + "seq_num": 10, + "base_prefix": "10.0.0.0/8", + "description": "rfc1918 a", + "match_logic": 1, + "max_allowed_golden_prefix_subnet_count": 4, + "prefix_len_ranges": [{"compare_operator": 3, "value": 24}], + "regex": "^10\\.", + "communities": ["65000:2", "65000:1"], + }, + ) + self.assertEqual( + commands, + [ + f"{self.PREFIX} base-prefix 10.0.0.0/8", + f"{self.PREFIX} description 'rfc1918 a'", + f"{self.PREFIX} match-logic NOT_EQUAL", + f"{self.PREFIX} max-allowed-subnet-count 4", + f"{self.PREFIX} prefix-len-range compare-operator LE", + f"{self.PREFIX} prefix-len-range value 24", + f"{self.PREFIX} regex '^10\\.'", + f"{self.PREFIX} communities 65000:1", + f"{self.PREFIX} communities 65000:2", + ], + ) + + def test_equal_default_omitted(self) -> None: + commands = generate_prefix_list_entry_commands( + "PL", {"seq_num": 10, "match_logic": 0} + ) + self.assertEqual(commands, [self.PREFIX]) + + def test_missing_seq_num_warns(self) -> None: + commands = generate_prefix_list_entry_commands( + "PL", {"base_prefix": "10.0.0.0/8"} + ) + self.assertEqual(len(commands), 1) + self.assertTrue(commands[0].startswith("# WARNING:")) + self.assertIn("10.0.0.0/8", commands[0]) + + def test_extra_ranges_and_ip_version_warn(self) -> None: + commands = generate_prefix_list_entry_commands( + "PL", + { + "seq_num": 10, + "prefix_len_ranges": [{"value": 24}, {"value": 32}], + "ip_version": 1, + }, + ) + self.assertEqual(commands[0], f"{self.PREFIX} prefix-len-range value 24") + self.assertEqual(len(commands), 3) + self.assertTrue(commands[1].startswith("# WARNING:")) + self.assertTrue(commands[2].startswith("# WARNING:")) + + def test_entries_emitted_inside_list(self) -> None: + commands = generate_prefix_list_commands( + { + "name": "PL", + "version": 4, + "prefixes": [{"seq_num": 10, "base_prefix": "10.0.0.0/8"}], + } + ) + self.assertEqual( + commands, + [ + "config protocol bgp policy prefix-list PL ip-version v4", + f"{self.PREFIX} base-prefix 10.0.0.0/8", + ], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands.""" From 226ee0d1fa5ef91a055fc925fc7c7ee53626c7ca Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 16 Sep 2026 07:46:01 +0000 Subject: [PATCH 3/7] Explain why prefix-list entries stay nested The nested as-path-list entry subcommand this comment pointed at does not exist; state the rule instead: nested entries only where bgpd stores objects. Co-Authored-By: Claude Fable 5.1 --- .../entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h index 25f0aebcd67cd..542fdb2b72efc 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h @@ -60,8 +60,9 @@ class BgpPrefixListEntryConfig : public utils::BaseObjectArgType { }; // The entry level of the prefix-list family as its own CLI11 subcommand; the -// parent's parsed args arrive through the ancestor-args tuple, mirroring -// CmdConfigProtocolBgpPolicyAsPathListEntry. +// parent's parsed args arrive through the ancestor-args tuple. Prefix-list +// entries are nested (unlike as-path-list/community-list values) because bgpd +// reads them as structured objects, not a flat string list. struct CmdConfigProtocolBgpPolicyPrefixListEntryTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgpPolicyPrefixList; From 93b5a24c17d03eb117898fe1096feaa5485c8066 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 8 Oct 2026 11:25:53 +0000 Subject: [PATCH 4/7] Carry the entry-level delete and entry helpers in this PR The list-level PR stopped at `delete protocol bgp policy prefix-list ` because bgpd rejects a PrefixListEntry carrying seq_num, and the entry's identity is this PR's call. Restore here the `entry ` delete selector, prefixListEntryExists()/findOrCreatePrefixListEntry() and their tests, unchanged from the list-level PR, so the entry subcommand and its delete side change together when the identity is settled. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/CmdListConfig.cpp | 3 +- .../prefix-list/BgpPrefixListCliUtils.h | 35 ++++- .../CmdDeleteProtocolBgpPolicyPrefixList.cpp | 80 +++++++++-- .../CmdDeleteProtocolBgpPolicyPrefixList.h | 22 ++- .../CmdDeleteBgpPolicyPrefixListTest.cpp | 133 +++++++++++++++++- .../ConfigBgpPolicyPrefixListTest.cpp | 12 +- 6 files changed, 250 insertions(+), 35 deletions(-) diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 1ebf883405018..d702279c613c5 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -837,7 +837,8 @@ const CommandTree& kConfigCommandTree() { }, { "prefix-list", - "Delete a BGP prefix-list: ", + "Delete a BGP prefix-list: " + "[entry ]", 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 4d3b6a69bdd63..b1699137d0d78 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 @@ -11,6 +11,7 @@ #pragma once #include +#include #include #ifndef IS_OSS @@ -22,10 +23,10 @@ /** * Lookup/create helpers for the prefix-list CLI family, shared between the - * 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). + * 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 { @@ -60,4 +61,30 @@ 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 ce04e7803bcef..e9e1fc3377ddc 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 @@ -18,7 +18,10 @@ #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" @@ -33,6 +36,11 @@ 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. @@ -75,23 +83,43 @@ std::vector findTermsReferencingPrefixList( // Parse + validate at construction so queryClient stays a thin dispatch. BgpPrefixListRef::BgpPrefixListRef(std::vector v) : utils::BaseObjectArgType(v) { - if (v.empty()) { - throw std::invalid_argument( - "Error: delete protocol bgp policy prefix-list requires "); - } - if (v[0].empty()) { - throw std::invalid_argument("Error: prefix-list name must not be empty"); + // 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 "); } - // 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) { + 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( - fmt::format( - "Error: unexpected token '{}'. Usage: delete protocol bgp policy " - "prefix-list ", - v[1])); + 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; } - listName_ = v[0]; } CmdDeleteProtocolBgpPolicyPrefixListTraits::RetType @@ -118,9 +146,33 @@ 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 f05e7bcf1e1f2..bac932b51f802 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,6 +10,7 @@ #pragma once +#include #include #include #include "CLI/App.hpp" @@ -20,11 +21,12 @@ namespace facebook::fboss { -// 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. +// 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) @@ -32,17 +34,25 @@ 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, ""); + cmd.add_option("args", args, " [entry ]"); } 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 4bb40be22a255..93d5f950aad42 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp @@ -11,6 +11,7 @@ #include #include +#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" @@ -48,6 +49,22 @@ 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"); @@ -111,14 +128,26 @@ class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { auto listOnly = BgpPrefixListRef({"PL100"}); EXPECT_EQ(listOnly.listName(), "PL100"); + EXPECT_FALSE(listOnly.hasEntry()); - // Invalid: empty, empty name, any second token (entries are deleted - // through the entry subcommand, not here). + 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", "10"}), std::invalid_argument); + BgpPrefixListRef({"PL100", "entry", "-1"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListRef({"PL100", "entry", "10", "extra"}), + std::invalid_argument); } // ============================================================================== @@ -126,7 +155,7 @@ TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { // ============================================================================== TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingList) { - configure({"PL100", "description", "drop"}); + configureEntry("PL100", 10, "10.0.0.0/8"); configure({"PL200", "description", "keep"}); ASSERT_EQ(lists().size(), 2); @@ -178,6 +207,87 @@ 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 // ============================================================================== @@ -237,4 +347,19 @@ 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 885b5ae7a5ca7..a8ee4534b4713 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 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 + * 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. * From 9b1174c4f46609c38183cc56d0dee06505afec80 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 8 Oct 2026 11:25:53 +0000 Subject: [PATCH 5/7] Restrict entry values to what bgpd accepts bgpd's PrefixTreeMatch throws "Unsupported Prefix configuration: match_logic" for anything but EQUAL, and toPolicyComparisonOperator() throws on RG, so the entry subcommand offered two values that make bgpd reject the whole config once a policy references the list. Offer only match-logic EQUAL and the EQ..LT range operators; bgp_json_to_cli.py warns and skips instead of emitting the rejected values. max-allowed-subnet-count is kept but documented as read by bgpd only for golden-prefix policies. The list-level "prefixes are not emitted" warning is dropped now that entries are emitted here. Co-Authored-By: Claude Fable 5.1 --- ...ConfigProtocolBgpPolicyPrefixListEntry.cpp | 26 ++++------ ...mdConfigProtocolBgpPolicyPrefixListEntry.h | 6 ++- .../CmdConfigBgpPolicyPrefixListEntryTest.cpp | 25 ++++++++-- .../ConfigBgpPolicyPrefixListEntryTest.cpp | 9 ++-- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 49 ++++++++++++++----- .../tools/tests/test_bgp_json_to_cli.py | 36 +++++++------- 6 files changed, 95 insertions(+), 56 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp index 49961cf97deb2..15647aede8098 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp @@ -51,19 +51,19 @@ constexpr std::string_view kRegex = "regex"; constexpr std::string_view kValue = "value"; // prefix-len-range compare-operator values -// (routing_policy.ComparisonOperator names). Unlike the list level, the range -// additionally accepts RG (range). +// (routing_policy.ComparisonOperator names). RG is not offered: bgpd's +// toPolicyComparisonOperator() throws on it. 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"; -// match-logic values (routing_policy.MatchValueLogicOperator names). +// match-logic values (routing_policy.MatchValueLogicOperator names). Only +// EQUAL is offered: bgpd throws "Unsupported Prefix configuration: +// match_logic" for anything else once a policy references the list. constexpr std::string_view kMatchLogicEqual = "EQUAL"; -constexpr std::string_view kMatchLogicNotEqual = "NOT_EQUAL"; // prefix-len-range value bounds: a prefix length (v6 caps it at 128). constexpr int32_t kPrefixLenMax = 128; @@ -87,9 +87,6 @@ std::optional lookupMatchLogic(const std::string& s) { if (s == kMatchLogicEqual) { return MatchValueLogicOperator::EQUAL; } - if (s == kMatchLogicNotEqual) { - return MatchValueLogicOperator::NOT_EQUAL; - } return std::nullopt; } @@ -113,9 +110,6 @@ std::optional lookupPrefixLenRangeOperator( if (s == kCompareOperatorLt) { return ComparisonOperator::LT; } - if (s == kCompareOperatorRg) { - return ComparisonOperator::RG; - } return std::nullopt; } @@ -153,14 +147,13 @@ void setRangeValue(CompareNumericValue& range, int32_t value) { const std::map, std::less<>>& prefixLenRangeAttrHandlers() { static const std::string kRangeOperatorValues = fmt::format( - "{}|{}|{}|{}|{}|{}|{}", + "{}|{}|{}|{}|{}|{}", kCompareOperatorEq, kCompareOperatorGe, kCompareOperatorLe, kCompareOperatorNe, kCompareOperatorGt, - kCompareOperatorLt, - kCompareOperatorRg); + kCompareOperatorLt); static const std::string kValueRange = fmt::format("0-{}", kPrefixLenMax); static const std:: map, std::less<>> @@ -201,6 +194,8 @@ void setMatchLogic(PrefixListEntry& entry, MatchValueLogicOperator op) { entry.match_logic() = op; } +// bgpd only honours this for golden-prefix policies; elsewhere it is stored +// and ignored. void setMaxAllowedSubnetCount(PrefixListEntry& entry, int32_t count) { entry.max_allowed_golden_prefix_subnet_count() = count; } @@ -280,8 +275,7 @@ Result regex(PrefixListEntry& entry, const Tokens& values) { // the setter or handler that stores it. const std::map, std::less<>>& entryAttrHandlers() { - static const std::string kMatchLogicValues = - fmt::format("{}|{}", kMatchLogicEqual, kMatchLogicNotEqual); + static const std::string kMatchLogicValues = std::string(kMatchLogicEqual); static const std::map, std::less<>> kHandlers = { {std::string(kBasePrefix), basePrefix}, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h index 542fdb2b72efc..cfb7195b81dc2 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h @@ -31,10 +31,12 @@ namespace facebook::fboss { // ... prefix-list entry base-prefix // ... prefix-list entry communities // ... prefix-list entry description -// ... prefix-list entry match-logic +// ... prefix-list entry match-logic EQUAL +// (bgpd rejects NOT_EQUAL) // ... prefix-list entry max-allowed-subnet-count +// (bgpd reads it only for golden-prefix policies) // ... prefix-list entry prefix-len-range compare-operator -// +// (bgpd rejects RG) // ... prefix-list entry prefix-len-range value <0-128> // ... prefix-list entry regex class BgpPrefixListEntryConfig : public utils::BaseObjectArgType { diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp index 3584a47283641..290807b4fa856 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp @@ -123,7 +123,7 @@ TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, bareCreateEntry) { TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setAttributes) { runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); runEntry({"PL100"}, {"10", "description", "spine", "block"}); - runEntry({"PL100"}, {"10", "match-logic", "NOT_EQUAL"}); + runEntry({"PL100"}, {"10", "match-logic", "EQUAL"}); runEntry({"PL100"}, {"10", "max-allowed-subnet-count", "64"}); runEntry({"PL100"}, {"10", "regex", "^10\\..*"}); @@ -133,22 +133,39 @@ TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setAttributes) { EXPECT_EQ(*e.seq_num(), 10); EXPECT_EQ(*e.base_prefix(), "10.0.0.0/8"); EXPECT_EQ(*e.description(), "spine block"); - EXPECT_EQ(*e.match_logic(), MatchValueLogicOperator::NOT_EQUAL); + EXPECT_EQ(*e.match_logic(), MatchValueLogicOperator::EQUAL); EXPECT_EQ(*e.max_allowed_golden_prefix_subnet_count(), 64); EXPECT_EQ(*e.regex(), "^10\\..*"); } TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setPrefixLenRange) { - runEntry({"PL100"}, {"10", "prefix-len-range", "compare-operator", "RG"}); + runEntry({"PL100"}, {"10", "prefix-len-range", "compare-operator", "GE"}); runEntry({"PL100"}, {"10", "prefix-len-range", "value", "24"}); // Both sub-attributes land on the single prefix_len_ranges[0] element. ASSERT_EQ(entry(0, 0).prefix_len_ranges()->size(), 1); const auto& range = entry(0, 0).prefix_len_ranges()->front(); - EXPECT_EQ(*range.compare_operator(), ComparisonOperator::RG); + EXPECT_EQ(*range.compare_operator(), ComparisonOperator::GE); EXPECT_EQ(*range.value(), 24); } +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + valuesBgpdRejectsAreRefused) { + runEntry({"PL100"}, {"10", "prefix-len-range", "compare-operator", "GE"}); + // bgpd: toPolicyComparisonOperator() throws on RG. + auto rg = + runEntry({"PL100"}, {"10", "prefix-len-range", "compare-operator", "RG"}); + EXPECT_THAT(rg, HasSubstr("expected EQ|GE|LE|NE|GT|LT")); + EXPECT_EQ( + *entry(0, 0).prefix_len_ranges()->front().compare_operator(), + ComparisonOperator::GE); + // bgpd: "Unsupported Prefix configuration: match_logic" unless EQUAL. + auto ne = runEntry({"PL100"}, {"10", "match-logic", "NOT_EQUAL"}); + EXPECT_THAT(ne, HasSubstr("expected EQUAL")); + EXPECT_EQ(*entry(0, 0).match_logic(), MatchValueLogicOperator::EQUAL); +} + TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, communitiesAccumulate) { auto result = runEntry({"PL100"}, {"10", "communities", "65000:100"}); EXPECT_THAT(result, HasSubstr("Successfully added 65000:100 to communities")); diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp index 2b708587b8234..879add72795df 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp @@ -91,7 +91,7 @@ TEST_F(ConfigBgpPolicyPrefixListEntryTest, SetAttributesAndCommit) { discardSession(); clearBgpSession(); stageEntry({kEntrySeq, "base-prefix", "10.0.0.0/8"}); - stageEntry({kEntrySeq, "match-logic", "NOT_EQUAL"}); + stageEntry({kEntrySeq, "match-logic", "EQUAL"}); stageEntry({kEntrySeq, "prefix-len-range", "compare-operator", "GE"}); stageEntry({kEntrySeq, "prefix-len-range", "value", "24"}); stageEntry({kEntrySeq, "communities", "65000:100"}); @@ -110,9 +110,10 @@ TEST_F(ConfigBgpPolicyPrefixListEntryTest, SetAttributesAndCommit) { const auto* entry = findEntry(*list, 10); ASSERT_NE(entry, nullptr) << "running config has no entry " << kEntrySeq; EXPECT_EQ((*entry)["base_prefix"].asString(), "10.0.0.0/8"); - // routing_policy.MatchValueLogicOperator.NOT_EQUAL = 1 (integer on the - // SimpleJSON wire). - EXPECT_EQ((*entry)["match_logic"].asInt(), 1); + // routing_policy.MatchValueLogicOperator.EQUAL = 0 (integer on the + // SimpleJSON wire). bgpd rejects any other value once a policy references + // the list. + EXPECT_EQ((*entry)["match_logic"].asInt(), 0); ASSERT_EQ((*entry)["prefix_len_ranges"].size(), 1); // routing_policy.ComparisonOperator.GE = 2. EXPECT_EQ((*entry)["prefix_len_ranges"][0]["compare_operator"].asInt(), 2); diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index ffe618407f687..e909c5474c51e 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -645,13 +645,6 @@ 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)) for entry in prefix_list.get("prefixes") or []: commands.extend(generate_prefix_list_entry_commands(name, entry)) @@ -664,8 +657,14 @@ def generate_prefix_list_commands(prefix_list: dict[str, Any]) -> list[str]: _MATCH_LOGIC_NAMES = {0: "EQUAL", 1: "NOT_EQUAL"} -def _prefix_list_entry_scalar_commands(prefix: str, entry: dict[str, Any]) -> list[str]: - """The single-valued entry attributes, in the CLI's attribute order.""" +def _prefix_list_entry_scalar_commands( + prefix: str, label: str, entry: dict[str, Any] +) -> list[str]: + """The single-valued entry attributes, in the CLI's attribute order. + + bgpd accepts only match_logic EQUAL (PrefixTreeMatch), so any other value + is a warning, not a command. + """ commands = [] if entry.get("base_prefix"): commands.append( @@ -681,7 +680,12 @@ def _prefix_list_entry_scalar_commands(prefix: str, entry: dict[str, Any]) -> li raw if isinstance(raw, str) else _MATCH_LOGIC_NAMES.get(int(raw), str(raw)) ) if logic != "EQUAL": - commands.append(f"{prefix} match-logic {escape_shell_arg(logic)}") + commands.append( + _warning( + f"{label}: match_logic {logic} is not accepted by bgpd " + "(only EQUAL); not emitted" + ) + ) if "max_allowed_golden_prefix_subnet_count" in entry: commands.append( f"{prefix} max-allowed-subnet-count " @@ -700,9 +704,28 @@ def _prefix_list_entry_range_commands( first = ranges[0] commands = [] if "compare_operator" in first: + operator = _comparison_operator_name(first["compare_operator"]) + if operator == "RG": + # bgpd's toPolicyComparisonOperator() throws on RG, and a range + # replayed without its operator would store an unset enum that + # bgpd rejects just the same, so the whole range is dropped. + commands.append( + _warning( + f"{label}: prefix_len_ranges compare_operator RG is not " + "accepted by bgpd; the range is not emitted" + ) + ) + if len(ranges) > 1: + commands.append( + _warning( + f"{label}: only the first of {len(ranges)} " + "prefix_len_ranges is expressible; the rest are not " + "emitted" + ) + ) + return commands commands.append( - f"{prefix} prefix-len-range compare-operator " - f"{escape_shell_arg(_comparison_operator_name(first['compare_operator']))}" + f"{prefix} prefix-len-range compare-operator {escape_shell_arg(operator)}" ) if "value" in first: commands.append( @@ -738,7 +761,7 @@ def generate_prefix_list_entry_commands( f"config protocol bgp policy prefix-list {escape_shell_arg(list_name)} " f"entry {escape_shell_arg(entry['seq_num'])}" ) - commands = _prefix_list_entry_scalar_commands(prefix, entry) + commands = _prefix_list_entry_scalar_commands(prefix, label, entry) commands.extend(_prefix_list_entry_range_commands(prefix, label, entry)) if entry.get("regex"): commands.append(f"{prefix} regex {escape_shell_arg(entry['regex'])}") 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 7b9e64c1e1449..b27fb1abc3889 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 @@ -1245,22 +1245,6 @@ def test_boolean_operator_not_or_warns(self) -> None: 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", - self.PREFIX, - ], - ) - def test_or_default_omitted(self) -> None: self.assertEqual( generate_prefix_list_commands({"name": "PL", "boolean_operator": 2}), @@ -1367,7 +1351,8 @@ def test_full_entry(self) -> None: [ f"{self.PREFIX} base-prefix 10.0.0.0/8", f"{self.PREFIX} description 'rfc1918 a'", - f"{self.PREFIX} match-logic NOT_EQUAL", + "# WARNING: prefix-list PL entry 10: match_logic NOT_EQUAL is not " + "accepted by bgpd (only EQUAL); not emitted", f"{self.PREFIX} max-allowed-subnet-count 4", f"{self.PREFIX} prefix-len-range compare-operator LE", f"{self.PREFIX} prefix-len-range value 24", @@ -1383,6 +1368,23 @@ def test_equal_default_omitted(self) -> None: ) self.assertEqual(commands, [self.PREFIX]) + def test_range_operator_rg_warns(self) -> None: + commands = generate_prefix_list_entry_commands( + "PL", + { + "seq_num": 10, + "prefix_len_ranges": [{"compare_operator": 7, "value": 24}], + }, + ) + self.assertEqual( + commands, + [ + "# WARNING: prefix-list PL entry 10: prefix_len_ranges " + "compare_operator RG is not accepted by bgpd; the range is not " + "emitted" + ], + ) + def test_missing_seq_num_warns(self) -> None: commands = generate_prefix_list_entry_commands( "PL", {"base_prefix": "10.0.0.0/8"} From 174a72fcee29f15693afe059f0fc6a8781b9f832 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 9 Oct 2026 18:00:50 +0000 Subject: [PATCH 6/7] Key prefix-list entries by prefix instead of seq-num bgpd identifies a PrefixListEntry by base_prefix: it merges entries that share one, ignores order, and throws "Unsupported Prefix configuration: seq_num" on any entry carrying seq_num. The entry subcommand keyed entries by `entry ` and stored that number in seq_num, so every entry it wrote was rejected the moment a policy referenced the list. Spell the entry level as `prefix `: the prefix is the entry's identity and its base_prefix, so the separate base-prefix attribute goes away and nothing writes seq_num. The delete selector becomes `prefix `, the shared helpers key on base_prefix, and bgp_json_to_cli.py emits `prefix ` per entry, warning when a JSON entry carries seq_num (dropped) or lacks base_prefix (skipped). One entry per prefix is the only shape the CLI needs: the documented grammar already exposes a single prefix-len-range, and bgpd merges the rest. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/CmdListConfig.cpp | 12 +- .../prefix-list/BgpPrefixListCliUtils.h | 30 ++-- .../CmdConfigProtocolBgpPolicyPrefixList.h | 2 +- ...ConfigProtocolBgpPolicyPrefixListEntry.cpp | 82 ++++------ ...mdConfigProtocolBgpPolicyPrefixListEntry.h | 42 +++--- .../CmdDeleteProtocolBgpPolicyPrefixList.cpp | 44 +++--- .../CmdDeleteProtocolBgpPolicyPrefixList.h | 27 ++-- .../CmdConfigBgpPolicyPrefixListEntryTest.cpp | 142 ++++++++++-------- .../CmdDeleteBgpPolicyPrefixListTest.cpp | 78 +++++----- .../ConfigBgpPolicyPrefixListEntryTest.cpp | 57 +++---- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 35 +++-- .../tools/tests/test_bgp_json_to_cli.py | 53 ++++--- 12 files changed, 316 insertions(+), 288 deletions(-) diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index d702279c613c5..3a1ef56a11529 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -459,10 +459,10 @@ const CommandTree& kConfigCommandTree() { argRegistrar< CmdConfigProtocolBgpPolicyPrefixListTraits>, {{ - "entry", - "Configure a prefix-list entry: " - "[ ...] (base-prefix, " - "communities, description, match-logic, " + "prefix", + "Configure a prefix-list entry: " + " [ ...] " + "(communities, description, match-logic, " "max-allowed-subnet-count, " "prefix-len-range, regex)", commandHandler< @@ -837,8 +837,8 @@ const CommandTree& kConfigCommandTree() { }, { "prefix-list", - "Delete a BGP prefix-list: " - "[entry ]", + "Delete a BGP prefix-list, or one of its " + "entries: [prefix ]", 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 b1699137d0d78..51b8e5857205c 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,8 +10,8 @@ #pragma once +#include #include -#include #include #ifndef IS_OSS @@ -23,10 +23,11 @@ /** * Lookup/create helpers for the prefix-list CLI family, shared between the - * list-level dispatcher (CmdConfigProtocolBgpPolicyPrefixList), the entry + * list-level dispatcher (CmdConfigProtocolBgpPolicyPrefixList), the prefix * subcommand (CmdConfigProtocolBgpPolicyPrefixListEntry), and the delete * counterparts. A PrefixList is keyed by name; a PrefixListEntry (in - * prefixes[]) is keyed by seq_num. + * prefixes[]) is keyed by base_prefix, the only identity bgpd keeps (it + * merges entries by prefix and rejects seq_num). */ namespace facebook::fboss::bgpcli { @@ -61,29 +62,38 @@ inline bgp::routing_policy::PrefixList& findOrCreatePrefixList( return list; } +// A prefix with an explicit /len, as every entry key must be. folly fills in +// a default mask for a bare address, so the slash is checked separately. The +// string is stored as typed, not normalized. +inline bool isPrefixWithLength(const std::string& s) { + return s.find('/') != std::string::npos && + !folly::IPAddress::tryCreateNetwork(s).hasError(); +} + inline bool prefixListEntryExists( const bgp::routing_policy::PrefixList& list, - int32_t seqNum) { + const std::string& basePrefix) { 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; + return *entry.base_prefix() == basePrefix; }); } -// Find the entry keyed by seq_num within a list's prefixes[], creating it if -// absent. seq_num is the entry's identity. +// Find the entry keyed by base_prefix within a list's prefixes[], creating it +// if absent. bgpd merges entries that share a base_prefix, so one entry per +// prefix is the only shape the CLI needs to address. inline bgp::routing_policy::PrefixListEntry& findOrCreatePrefixListEntry( bgp::routing_policy::PrefixList& list, - int32_t seqNum) { + const std::string& basePrefix) { auto& entries = *list.prefixes(); for (auto& entry : entries) { - if (entry.seq_num().has_value() && *entry.seq_num() == seqNum) { + if (*entry.base_prefix() == basePrefix) { return entry; } } entries.emplace_back(); auto& entry = entries.back(); - entry.seq_num() = seqNum; + entry.base_prefix() = basePrefix; return entry; } 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 4c8111016ddb4..493043db2a75b 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 @@ -61,7 +61,7 @@ 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 + // `prefix` token as this command's subcommand rather than swallowing it // into args. See CmdConfigProtocolBgpPolicyAsPathListTraits. cmd.add_option("args", args, " [ ...]"); } diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp index 15647aede8098..c0253764224fd 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp @@ -13,34 +13,37 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include -#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/BgpCliValueParsers.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" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + namespace facebook::fboss { namespace { // The attribute names, exactly as documented. Kept here so the -// valid-attribute set and the handler table stay in sync. -constexpr std::string_view kBasePrefix = "base-prefix"; +// valid-attribute set and the handler table stay in sync. base-prefix is not +// an attribute: it is the entry's key, given as the `prefix ` +// selector. constexpr std::string_view kCommunities = "communities"; constexpr std::string_view kCompareOperator = "compare-operator"; constexpr std::string_view kDescription = "description"; @@ -201,28 +204,8 @@ void setMaxAllowedSubnetCount(PrefixListEntry& entry, int32_t count) { } // ---- hand-written handlers -------------------------------------------------- -// These four remain full handlers because their value shape has no factory: a -// prefix requiring an explicit mask, an accumulating set, a composed -// sub-attribute, and a stored-verbatim regex. - -Result basePrefix(PrefixListEntry& entry, const Tokens& values) { - if (values.size() != 1) { - return err(fmt::format("Error: {} requires ", kBasePrefix)); - } - // Require an explicit /len — folly fills in a default mask for a bare - // address, so its absence must be checked separately. The string is stored - // as typed, not normalized. - if (values[0].find('/') == std::string::npos || - folly::IPAddress::tryCreateNetwork(values[0]).hasError()) { - return err( - fmt::format( - "Error: Invalid {} value '{}'; expected ", - kBasePrefix, - values[0])); - } - entry.base_prefix() = values[0]; - return ok(fmt::format("Successfully set {} to: {}", kBasePrefix, values[0])); -} +// These three remain full handlers because their value shape has no factory: +// an accumulating set, a composed sub-attribute, and a stored-verbatim regex. Result communities(PrefixListEntry& entry, const Tokens& values) { if (values.size() != 1) { @@ -278,7 +261,6 @@ entryAttrHandlers() { static const std::string kMatchLogicValues = std::string(kMatchLogicEqual); static const std::map, std::less<>> kHandlers = { - {std::string(kBasePrefix), basePrefix}, {std::string(kCommunities), communities}, {std::string(kDescription), joinedStringAttr(kDescription, setDescription)}, @@ -321,19 +303,16 @@ BgpPrefixListEntryConfig::BgpPrefixListEntryConfig(std::vector v) : utils::BaseObjectArgType(v) { if (v.empty()) { throw std::invalid_argument( - "Error: entry is required, optionally followed by an " + "Error: prefix is required, optionally followed by an " " "); } - auto seq = bgpcli::parseNonNegInt32(v[0]); - if (!seq) { + if (!bgpcli::isPrefixWithLength(v[0])) { throw std::invalid_argument( - fmt::format( - "Error: entry must be a non-negative integer, got '{}'", - v[0])); + fmt::format("Error: Invalid prefix '{}'; expected ", v[0])); } - seqNum_ = *seq; + basePrefix_ = v[0]; if (v.size() == 1) { - return; // bare `entry `: create it + return; // bare `prefix `: create it } attr_ = v[1]; @@ -342,7 +321,7 @@ BgpPrefixListEntryConfig::BgpPrefixListEntryConfig(std::vector v) if (entryAttrHandlers().find(attr_) == entryAttrHandlers().end()) { throw std::invalid_argument( fmt::format( - "Error: unknown prefix-list entry attribute '{}'. Valid " + "Error: unknown prefix-list prefix attribute '{}'. Valid " "attributes: {}", attr_, validAttrList())); @@ -354,34 +333,35 @@ CmdConfigProtocolBgpPolicyPrefixListEntry::queryClient( const HostInfo& /* hostInfo */, const BgpPrefixListConfig& listArgs, const ObjectArgType& args) { - // The parent parse accepts `prefix-list ... entry + // The parent parse accepts `prefix-list ... prefix // ...`, but only the leaf (this command) runs — silently dropping the // list-level attribute would look like it was staged. Reject the mix. if (!listArgs.attr().empty()) { return fmt::format( - "Error: configure prefix-list attributes and entry in separate " - "commands (got prefix-list attribute '{}' alongside entry {})", + "Error: configure prefix-list attributes and prefix in separate " + "commands (got prefix-list attribute '{}' alongside prefix {})", listArgs.attr(), - args.seqNum()); + args.basePrefix()); } auto& session = ConfigSession::getInstance(); auto& cfg = session.getBgpConfig(); const bool listCreated = !bgpcli::prefixListExists(cfg, listArgs.listName()); auto& list = bgpcli::findOrCreatePrefixList(cfg, listArgs.listName()); - const bool entryCreated = !bgpcli::prefixListEntryExists(list, args.seqNum()); - auto& entry = bgpcli::findOrCreatePrefixListEntry(list, args.seqNum()); + const bool entryCreated = + !bgpcli::prefixListEntryExists(list, args.basePrefix()); + auto& entry = bgpcli::findOrCreatePrefixListEntry(list, args.basePrefix()); Result result = args.attr().empty() ? ok(entryCreated ? fmt::format( - "Successfully created BGP prefix-list {} entry {}", + "Successfully created BGP prefix-list {} prefix {}", listArgs.listName(), - args.seqNum()) + args.basePrefix()) : fmt::format( - "BGP prefix-list {} entry {} already exists", + "BGP prefix-list {} prefix {} already exists", listArgs.listName(), - args.seqNum())) + args.basePrefix())) // The attribute is guaranteed valid: BgpPrefixListEntryConfig's // constructor rejects an unknown attribute before we get here. : entryAttrHandlers().find(args.attr())->second(entry, args.values()); @@ -389,7 +369,9 @@ CmdConfigProtocolBgpPolicyPrefixListEntry::queryClient( if (result.ok) { if (!args.attr().empty()) { result.message += fmt::format( - " for prefix-list {} entry {}", listArgs.listName(), args.seqNum()); + " for prefix-list {} prefix {}", + listArgs.listName(), + args.basePrefix()); } session.saveBgpConfig(); result.message += diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h index cfb7195b81dc2..1a4cf34485e5b 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h @@ -10,7 +10,6 @@ #pragma once -#include #include #include #include "CLI/App.hpp" @@ -21,30 +20,32 @@ namespace facebook::fboss { -// Parsed `entry [ ...]`, validated at +// Parsed `prefix [ ...]`, validated at // construction. An entry (routing_policy.PrefixListEntry in -// PrefixList.prefixes[]) is keyed by ; the list it belongs to is +// PrefixList.prefixes[]) is keyed by its base_prefix: that is the identity +// bgpd keeps (it merges entries by prefix and rejects seq_num), so the +// documented `entry ` level is spelled `prefix ` and the +// prefix doubles as the entry's base_prefix. The list it belongs to is // supplied by the parent command's args. // -// Grammar (from the FBOSS proposed syntax): -// ... prefix-list entry (create/select) -// ... prefix-list entry base-prefix -// ... prefix-list entry communities -// ... prefix-list entry description -// ... prefix-list entry match-logic EQUAL +// Grammar: +// ... prefix-list prefix (create/select) +// ... prefix-list prefix communities +// ... prefix-list prefix description +// ... prefix-list prefix match-logic EQUAL // (bgpd rejects NOT_EQUAL) -// ... prefix-list entry max-allowed-subnet-count +// ... prefix-list prefix max-allowed-subnet-count // (bgpd reads it only for golden-prefix policies) -// ... prefix-list entry prefix-len-range compare-operator -// (bgpd rejects RG) -// ... prefix-list entry prefix-len-range value <0-128> -// ... prefix-list entry regex +// ... prefix-list prefix prefix-len-range +// compare-operator (bgpd rejects RG) +// ... prefix-list prefix prefix-len-range value <0-128> +// ... prefix-list prefix regex class BgpPrefixListEntryConfig : public utils::BaseObjectArgType { public: // NOLINTNEXTLINE(google-explicit-constructor) /* implicit */ BgpPrefixListEntryConfig(std::vector v); - int32_t seqNum() const { - return seqNum_; + const std::string& basePrefix() const { + return basePrefix_; } const std::string& attr() const { return attr_; @@ -56,7 +57,7 @@ class BgpPrefixListEntryConfig : public utils::BaseObjectArgType { utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; private: - int32_t seqNum_{0}; + std::string basePrefix_; std::string attr_; // matched dispatch key ("" = bare create) std::vector values_; }; @@ -69,10 +70,11 @@ struct CmdConfigProtocolBgpPolicyPrefixListEntryTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgpPolicyPrefixList; static void addCliArg(CLI::App& cmd, std::vector& args) { - // Entry has no nested subcommands; stop CLI11's parent-chain fallthrough - // from stealing a value token that spells `entry` (e.g. in a description). + // The prefix level has no nested subcommands; stop CLI11's parent-chain + // fallthrough from stealing a value token that spells `prefix` (e.g. in a + // description). cmd.positionals_at_end(); - cmd.add_option("args", args, " [ ...]"); + cmd.add_option("args", args, " [ ...]"); } using ObjectArgType = BgpPrefixListEntryConfig; using RetType = std::string; 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 e9e1fc3377ddc..70edcd0da7f1d 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 @@ -22,6 +22,7 @@ #include #include #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.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" @@ -36,10 +37,10 @@ namespace facebook::fboss { namespace { -// CLI keyword selecting the nested entry, matching the config command's -// grammar. +// CLI keyword selecting the nested entry by its base prefix, matching the +// config command's grammar. constexpr std::string_view kObjectName = "prefix-list"; -constexpr std::string_view kEntryKeyword = "entry"; +constexpr std::string_view kPrefixKeyword = "prefix"; // Every routing-policy term whose PREFIX_LIST match names `listName` in // prefix_filters.prefix_list_names (the field bgpd resolves at config load), @@ -83,42 +84,40 @@ 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 && + // Pre-empt the generic member-selector message for a missing prefix: the + // shared parser would call the member a , but here it is a prefix. + if (v.size() >= 2 && v[1] == kPrefixKeyword && (v.size() < 3 || v[2].empty())) { - throw std::invalid_argument("Error: `entry` requires a "); + throw std::invalid_argument("Error: `prefix` requires a "); } auto selector = bgpcli::parseListMemberSelector( v, kObjectName, - kEntryKeyword, + kPrefixKeyword, "Error: delete protocol bgp policy prefix-list requires , " - "optionally followed by `entry `"); + "optionally followed by `prefix `"); // 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 ", + "Error: unexpected token '{}' after prefix ", v[selector.restStart]) : fmt::format( "Error: unexpected token '{}'. Usage: delete protocol bgp " - "policy prefix-list [entry ]", + "policy prefix-list [prefix ]", v[selector.restStart])); } listName_ = std::move(selector.listName); if (selector.memberName) { - auto seq = bgpcli::parseNonNegInt32(*selector.memberName); - if (!seq) { + if (!bgpcli::isPrefixWithLength(*selector.memberName)) { throw std::invalid_argument( fmt::format( - "Error: entry must be a non-negative integer, got '{}'", + "Error: Invalid prefix '{}'; expected ", *selector.memberName)); } - hasEntry_ = true; - seqNum_ = *seq; + basePrefix_ = *selector.memberName; } } @@ -146,27 +145,26 @@ CmdDeleteProtocolBgpPolicyPrefixList::queryClient( if (it == lists.end()) { return absent(); } - if (args.hasEntry()) { + if (args.hasPrefix()) { // 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(); + return *entry.base_prefix() == args.basePrefix(); }); if (entryIt == entries.end()) { return fmt::format( - "Warning: BGP prefix-list {} has no entry {}; nothing to delete", + "Warning: BGP prefix-list {} has no prefix {}; nothing to delete", args.listName(), - args.seqNum()); + args.basePrefix()); } entries.erase(entryIt); session.saveBgpConfig(); return fmt::format( - "Successfully deleted BGP prefix-list {} entry {}\n" + "Successfully deleted BGP prefix-list {} prefix {}\n" "Config saved to: {}", args.listName(), - args.seqNum(), + args.basePrefix(), session.getBgpSessionConfigPath()); } // A term's prefix_list_names resolves against this list by name at daemon 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 bac932b51f802..d1c06f2d6b808 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,7 @@ #pragma once -#include +#include #include #include #include "CLI/App.hpp" @@ -21,12 +21,12 @@ 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 [prefix ]`, +// validated at construction. The list name and the entry's base prefix are +// the identities the config command stores. Without the `prefix` selector +// the whole list is deleted; with it, only that entry. Mirrors +// BgpCommunityListRef, with a prefix-keyed second level in place of the +// value-keyed one. class BgpPrefixListRef : public utils::BaseObjectArgType { public: // NOLINTNEXTLINE(google-explicit-constructor) @@ -34,25 +34,24 @@ class BgpPrefixListRef : public utils::BaseObjectArgType { const std::string& listName() const { return listName_; } - bool hasEntry() const { - return hasEntry_; + bool hasPrefix() const { + return basePrefix_.has_value(); } - int32_t seqNum() const { - return seqNum_; + const std::string& basePrefix() const { + return *basePrefix_; } const static utils::ObjectArgTypeId id = utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; private: std::string listName_; - bool hasEntry_{false}; - int32_t seqNum_{0}; + std::optional basePrefix_; }; 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, " [prefix ]"); } using ObjectArgType = BgpPrefixListRef; using RetType = std::string; diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp index 290807b4fa856..5fce629afbb74 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp @@ -83,26 +83,41 @@ class CmdConfigBgpPolicyPrefixListEntryTestFixture : public CmdConfigTestBase { TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, argValidation) { // Bare create. - auto bare = BgpPrefixListEntryConfig({"10"}); - EXPECT_EQ(bare.seqNum(), 10); + auto bare = BgpPrefixListEntryConfig({"10.0.0.0/8"}); + EXPECT_EQ(bare.basePrefix(), "10.0.0.0/8"); EXPECT_TRUE(bare.attr().empty()); // Attribute with values. - auto attr = BgpPrefixListEntryConfig({"10", "base-prefix", "10.0.0.0/8"}); - EXPECT_EQ(attr.seqNum(), 10); - EXPECT_EQ(attr.attr(), "base-prefix"); - EXPECT_EQ(attr.values(), std::vector({"10.0.0.0/8"})); + auto attr = BgpPrefixListEntryConfig({"10.0.0.0/8", "description", "a", "b"}); + EXPECT_EQ(attr.basePrefix(), "10.0.0.0/8"); + EXPECT_EQ(attr.attr(), "description"); + EXPECT_EQ(attr.values(), std::vector({"a", "b"})); - // Invalid: empty, non-integer and negative seq-nums, unknown attribute. + // The prefix is stored as typed, including v6. + EXPECT_EQ( + BgpPrefixListEntryConfig({"2001:db8::/32"}).basePrefix(), + "2001:db8::/32"); + + // Invalid: empty; a prefix without an explicit /len (folly would default + // the mask), a non-address, an out-of-range mask, a second slash; unknown + // attribute. EXPECT_THROW(BgpPrefixListEntryConfig({}), std::invalid_argument); - EXPECT_THROW(BgpPrefixListEntryConfig({"ten"}), std::invalid_argument); - EXPECT_THROW(BgpPrefixListEntryConfig({"-1"}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListEntryConfig({"10.0.0.0"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListEntryConfig({"not-a-prefix"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListEntryConfig({"10.0.0.0/99"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListEntryConfig({"10.0.0.0/8/8"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListEntryConfig({"10.0.0.0/8", "no-such-attr", "1"}), + std::invalid_argument); + // base-prefix is the key, not an attribute; ip-version is a list attribute. EXPECT_THROW( - BgpPrefixListEntryConfig({"10", "no-such-attr", "1"}), + BgpPrefixListEntryConfig({"10.0.0.0/8", "base-prefix", "10.0.0.0/8"}), std::invalid_argument); - // ip-version is a list attribute, not an entry attribute. EXPECT_THROW( - BgpPrefixListEntryConfig({"10", "ip-version", "v4"}), + BgpPrefixListEntryConfig({"10.0.0.0/8", "ip-version", "v4"}), std::invalid_argument); } @@ -111,26 +126,29 @@ TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, argValidation) { // ============================================================================== TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, bareCreateEntry) { - auto result = runEntry({"PL100"}, {"10"}); + auto result = runEntry({"PL100"}, {"10.0.0.0/8"}); EXPECT_THAT( - result, HasSubstr("Successfully created BGP prefix-list PL100 entry 10")); + result, + HasSubstr( + "Successfully created BGP prefix-list PL100 prefix 10.0.0.0/8")); ASSERT_EQ(lists().size(), 1); ASSERT_EQ(lists()[0].prefixes()->size(), 1); - ASSERT_TRUE(entry(0, 0).seq_num().has_value()); - EXPECT_EQ(*entry(0, 0).seq_num(), 10); + // The key is the entry's base_prefix; nothing writes seq_num, which bgpd + // rejects. + EXPECT_EQ(*entry(0, 0).base_prefix(), "10.0.0.0/8"); + EXPECT_FALSE(entry(0, 0).seq_num().has_value()); } TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setAttributes) { - runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); - runEntry({"PL100"}, {"10", "description", "spine", "block"}); - runEntry({"PL100"}, {"10", "match-logic", "EQUAL"}); - runEntry({"PL100"}, {"10", "max-allowed-subnet-count", "64"}); - runEntry({"PL100"}, {"10", "regex", "^10\\..*"}); + runEntry({"PL100"}, {"10.0.0.0/8", "description", "spine", "block"}); + runEntry({"PL100"}, {"10.0.0.0/8", "match-logic", "EQUAL"}); + runEntry({"PL100"}, {"10.0.0.0/8", "max-allowed-subnet-count", "64"}); + runEntry({"PL100"}, {"10.0.0.0/8", "regex", "^10\\..*"}); ASSERT_EQ(lists().size(), 1); ASSERT_EQ(lists()[0].prefixes()->size(), 1); const auto& e = entry(0, 0); - EXPECT_EQ(*e.seq_num(), 10); + EXPECT_FALSE(e.seq_num().has_value()); EXPECT_EQ(*e.base_prefix(), "10.0.0.0/8"); EXPECT_EQ(*e.description(), "spine block"); EXPECT_EQ(*e.match_logic(), MatchValueLogicOperator::EQUAL); @@ -139,8 +157,9 @@ TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setAttributes) { } TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setPrefixLenRange) { - runEntry({"PL100"}, {"10", "prefix-len-range", "compare-operator", "GE"}); - runEntry({"PL100"}, {"10", "prefix-len-range", "value", "24"}); + runEntry( + {"PL100"}, {"10.0.0.0/8", "prefix-len-range", "compare-operator", "GE"}); + runEntry({"PL100"}, {"10.0.0.0/8", "prefix-len-range", "value", "24"}); // Both sub-attributes land on the single prefix_len_ranges[0] element. ASSERT_EQ(entry(0, 0).prefix_len_ranges()->size(), 1); @@ -152,24 +171,25 @@ TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setPrefixLenRange) { TEST_F( CmdConfigBgpPolicyPrefixListEntryTestFixture, valuesBgpdRejectsAreRefused) { - runEntry({"PL100"}, {"10", "prefix-len-range", "compare-operator", "GE"}); + runEntry( + {"PL100"}, {"10.0.0.0/8", "prefix-len-range", "compare-operator", "GE"}); // bgpd: toPolicyComparisonOperator() throws on RG. - auto rg = - runEntry({"PL100"}, {"10", "prefix-len-range", "compare-operator", "RG"}); + auto rg = runEntry( + {"PL100"}, {"10.0.0.0/8", "prefix-len-range", "compare-operator", "RG"}); EXPECT_THAT(rg, HasSubstr("expected EQ|GE|LE|NE|GT|LT")); EXPECT_EQ( *entry(0, 0).prefix_len_ranges()->front().compare_operator(), ComparisonOperator::GE); // bgpd: "Unsupported Prefix configuration: match_logic" unless EQUAL. - auto ne = runEntry({"PL100"}, {"10", "match-logic", "NOT_EQUAL"}); + auto ne = runEntry({"PL100"}, {"10.0.0.0/8", "match-logic", "NOT_EQUAL"}); EXPECT_THAT(ne, HasSubstr("expected EQUAL")); EXPECT_EQ(*entry(0, 0).match_logic(), MatchValueLogicOperator::EQUAL); } TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, communitiesAccumulate) { - auto result = runEntry({"PL100"}, {"10", "communities", "65000:100"}); + auto result = runEntry({"PL100"}, {"10.0.0.0/8", "communities", "65000:100"}); EXPECT_THAT(result, HasSubstr("Successfully added 65000:100 to communities")); - runEntry({"PL100"}, {"10", "communities", "65000:200"}); + runEntry({"PL100"}, {"10.0.0.0/8", "communities", "65000:200"}); ASSERT_TRUE(entry(0, 0).communities().has_value()); EXPECT_THAT( @@ -177,7 +197,8 @@ TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, communitiesAccumulate) { UnorderedElementsAre("65000:100", "65000:200")); // Re-adding an existing member reports it without duplicating. - auto repeated = runEntry({"PL100"}, {"10", "communities", "65000:100"}); + auto repeated = + runEntry({"PL100"}, {"10.0.0.0/8", "communities", "65000:100"}); EXPECT_THAT(repeated, HasSubstr("communities already contains 65000:100")); EXPECT_EQ(entry(0, 0).communities()->size(), 2); } @@ -185,13 +206,13 @@ TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, communitiesAccumulate) { TEST_F( CmdConfigBgpPolicyPrefixListEntryTestFixture, entriesAccumulateAndAreKeyed) { - runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); - runEntry({"PL100"}, {"20", "base-prefix", "192.168.0.0/16"}); + runEntry({"PL100"}, {"10.0.0.0/8", "description", "a"}); + runEntry({"PL100"}, {"192.168.0.0/16", "description", "b"}); ASSERT_EQ(lists().size(), 1); ASSERT_EQ(lists()[0].prefixes()->size(), 2); - // Re-referencing an existing entry by seq-num updates it, not appends. - runEntry({"PL100"}, {"10", "match-logic", "EQUAL"}); + // Re-referencing an existing entry by prefix updates it, not appends. + runEntry({"PL100"}, {"10.0.0.0/8", "match-logic", "EQUAL"}); EXPECT_EQ(lists()[0].prefixes()->size(), 2); EXPECT_EQ(*entry(0, 0).base_prefix(), "10.0.0.0/8"); EXPECT_EQ(*entry(0, 0).match_logic(), MatchValueLogicOperator::EQUAL); @@ -199,27 +220,20 @@ TEST_F( TEST_F( CmdConfigBgpPolicyPrefixListEntryTestFixture, - invalidBasePrefixRejected) { - // A base-prefix without an explicit /len is rejected, as is a non-address. - auto noLen = runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0"}); - EXPECT_THAT( - noLen, - HasSubstr("Invalid base-prefix value '10.0.0.0'; expected ")); - auto garbage = runEntry({"PL100"}, {"10", "base-prefix", "not-a-prefix"}); - EXPECT_THAT(garbage, HasSubstr("Invalid base-prefix value")); - // folly rejects the rest: an out-of-range mask and a second slash. - auto badMask = runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/99"}); - EXPECT_THAT(badMask, HasSubstr("Invalid base-prefix value")); - auto twoSlash = runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8/8"}); - EXPECT_THAT(twoSlash, HasSubstr("Invalid base-prefix value")); + invalidPrefixRejectedBeforeAnythingIsCreated) { + // The prefix is validated at construction (see argValidation), so a bad + // one never reaches queryClient and never creates a list or an entry. + EXPECT_THROW( + runEntry({"PL100"}, {"10.0.0.0", "description", "x"}), + std::invalid_argument); EXPECT_TRUE(lists().empty()); EXPECT_FALSE(sessionFileExists()) << "session file should not exist after rejected input"; } -TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, basePrefixAcceptsV6) { - auto result = runEntry({"PL100"}, {"10", "base-prefix", "2001:db8::/32"}); - EXPECT_THAT(result, HasSubstr("Successfully set base-prefix")); +TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, prefixAcceptsV6) { + auto result = runEntry({"PL100"}, {"2001:db8::/32"}); + EXPECT_THAT(result, HasSubstr("Successfully created")); // Stored as typed, not normalized. EXPECT_EQ(*entry(0, 0).base_prefix(), "2001:db8::/32"); } @@ -228,13 +242,15 @@ TEST_F( CmdConfigBgpPolicyPrefixListEntryTestFixture, invalidPrefixLenRangeRejected) { // Unknown sub-attribute. - auto badSub = runEntry({"PL100"}, {"10", "prefix-len-range", "min", "8"}); + auto badSub = + runEntry({"PL100"}, {"10.0.0.0/8", "prefix-len-range", "min", "8"}); EXPECT_THAT( badSub, HasSubstr( "Error: prefix-len-range requires ")); // Out-of-range length. - auto badLen = runEntry({"PL100"}, {"10", "prefix-len-range", "value", "129"}); + auto badLen = + runEntry({"PL100"}, {"10.0.0.0/8", "prefix-len-range", "value", "129"}); EXPECT_THAT( badLen, HasSubstr("Invalid prefix-len-range value value '129'; expected 0-128")); @@ -248,8 +264,9 @@ TEST_F( rejectedPrefixLenRangeKeepsNoPhantomRange) { // Land the entry first, then reject a range value on it: the entry survives // but no phantom prefix_len_ranges element may appear. - runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); - auto result = runEntry({"PL100"}, {"10", "prefix-len-range", "value", "300"}); + runEntry({"PL100"}, {"10.0.0.0/8"}); + auto result = + runEntry({"PL100"}, {"10.0.0.0/8", "prefix-len-range", "value", "300"}); EXPECT_THAT(result, HasSubstr("Invalid")); ASSERT_EQ(lists()[0].prefixes()->size(), 1); EXPECT_TRUE(entry(0, 0).prefix_len_ranges()->empty()); @@ -262,7 +279,7 @@ TEST_F( runList({"PL100", "description", "keep-me"}); ASSERT_EQ(lists().size(), 1); - auto result = runEntry({"PL100"}, {"10", "match-logic", "MAYBE"}); + auto result = runEntry({"PL100"}, {"10.0.0.0/8", "match-logic", "MAYBE"}); EXPECT_THAT(result, HasSubstr("Invalid")); // The pre-existing list survives; only the phantom entry is rolled back. ASSERT_EQ(lists().size(), 1); @@ -273,22 +290,23 @@ TEST_F( TEST_F( CmdConfigBgpPolicyPrefixListEntryTestFixture, rejectedEntryKeepsExistingEntries) { - runEntry({"PL100"}, {"10", "base-prefix", "10.0.0.0/8"}); + runEntry({"PL100"}, {"10.0.0.0/8"}); ASSERT_EQ(lists()[0].prefixes()->size(), 1); - auto result = runEntry({"PL100"}, {"20", "match-logic", "MAYBE"}); + auto result = runEntry({"PL100"}, {"192.168.0.0/16", "match-logic", "MAYBE"}); EXPECT_THAT(result, HasSubstr("Invalid")); // Only the phantom entry is rolled back; the existing one survives. ASSERT_EQ(lists()[0].prefixes()->size(), 1); - EXPECT_EQ(*entry(0, 0).seq_num(), 10); + EXPECT_EQ(*entry(0, 0).base_prefix(), "10.0.0.0/8"); } TEST_F( CmdConfigBgpPolicyPrefixListEntryTestFixture, reReferenceReportsExisting) { - runEntry({"PL100"}, {"10"}); + runEntry({"PL100"}, {"10.0.0.0/8"}); EXPECT_THAT( - runEntry({"PL100"}, {"10"}), HasSubstr("entry 10 already exists")); + runEntry({"PL100"}, {"10.0.0.0/8"}), + HasSubstr("prefix 10.0.0.0/8 already exists")); EXPECT_EQ(lists()[0].prefixes()->size(), 1); } @@ -297,7 +315,7 @@ TEST_F( TEST_F( CmdConfigBgpPolicyPrefixListEntryTestFixture, listAttributeMixedWithEntryRejected) { - auto result = runEntry({"PL100", "description", "mixed"}, {"10"}); + auto result = runEntry({"PL100", "description", "mixed"}, {"10.0.0.0/8"}); EXPECT_THAT(result, HasSubstr("separate commands")); EXPECT_TRUE(lists().empty()); EXPECT_FALSE(sessionFileExists()); diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp index 93d5f950aad42..98b3cf1f66846 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp @@ -55,13 +55,11 @@ class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { // 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; + bgpcli::findOrCreatePrefixListEntry(list, basePrefix); session.saveBgpConfig(); } @@ -128,25 +126,26 @@ class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { auto listOnly = BgpPrefixListRef({"PL100"}); EXPECT_EQ(listOnly.listName(), "PL100"); - EXPECT_FALSE(listOnly.hasEntry()); + EXPECT_FALSE(listOnly.hasPrefix()); - auto withEntry = BgpPrefixListRef({"PL100", "entry", "10"}); - EXPECT_EQ(withEntry.listName(), "PL100"); - EXPECT_TRUE(withEntry.hasEntry()); - EXPECT_EQ(withEntry.seqNum(), 10); + auto withPrefix = BgpPrefixListRef({"PL100", "prefix", "10.0.0.0/8"}); + EXPECT_EQ(withPrefix.listName(), "PL100"); + EXPECT_TRUE(withPrefix.hasPrefix()); + EXPECT_EQ(withPrefix.basePrefix(), "10.0.0.0/8"); + EXPECT_FALSE(BgpPrefixListRef({"PL100"}).hasPrefix()); - // Invalid: empty, empty name, non-`entry` second token, missing seq-num, - // non-integer and negative seq-nums, extra tokens. + // Invalid: empty, empty name, non-`prefix` second token, missing prefix, + // a prefix without /len or malformed, 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", "prefix"}), std::invalid_argument); EXPECT_THROW( - BgpPrefixListRef({"PL100", "entry", "ten"}), std::invalid_argument); + BgpPrefixListRef({"PL100", "prefix", "10.0.0.0"}), std::invalid_argument); EXPECT_THROW( - BgpPrefixListRef({"PL100", "entry", "-1"}), std::invalid_argument); + BgpPrefixListRef({"PL100", "prefix", "ten"}), std::invalid_argument); EXPECT_THROW( - BgpPrefixListRef({"PL100", "entry", "10", "extra"}), + BgpPrefixListRef({"PL100", "prefix", "10.0.0.0/8", "extra"}), std::invalid_argument); } @@ -155,7 +154,7 @@ TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { // ============================================================================== TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingList) { - configureEntry("PL100", 10, "10.0.0.0/8"); + configureEntry("PL100", "10.0.0.0/8"); configure({"PL200", "description", "keep"}); ASSERT_EQ(lists().size(), 2); @@ -208,27 +207,29 @@ TEST_F( } // ============================================================================== -// queryClient: single entry deletion +// queryClient: single entry (prefix) deletion // ============================================================================== TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingEntry) { - configureEntry("PL100", 10, "10.0.0.0/8"); - configureEntry("PL100", 20, "192.168.0.0/16"); + configureEntry("PL100", "10.0.0.0/8"); + configureEntry("PL100", "192.168.0.0/16"); - auto result = del({"PL100", "entry", "10"}); + auto result = del({"PL100", "prefix", "10.0.0.0/8"}); EXPECT_THAT( - result, HasSubstr("Successfully deleted BGP prefix-list PL100 entry 10")); + result, + HasSubstr( + "Successfully deleted BGP prefix-list PL100 prefix 10.0.0.0/8")); // 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_EQ(*(*lists()[0].prefixes())[0].base_prefix(), "192.168.0.0/16"); EXPECT_TRUE(sessionFileExists()); } TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteLastEntryKeepsList) { - configureEntry("PL100", 10, "10.0.0.0/8"); + configureEntry("PL100", "10.0.0.0/8"); - auto result = del({"PL100", "entry", "10"}); + auto result = del({"PL100", "prefix", "10.0.0.0/8"}); 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). @@ -238,37 +239,40 @@ TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteLastEntryKeepsList) { } TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownEntryWarns) { - configureEntry("PL100", 10, "10.0.0.0/8"); + configureEntry("PL100", "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"}); + auto result = del({"PL100", "prefix", "172.16.0.0/12"}); EXPECT_THAT( result, HasSubstr( - "Warning: BGP prefix-list PL100 has no entry 99; nothing to delete")); + "Warning: BGP prefix-list PL100 has no prefix 172.16.0.0/12; 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); + EXPECT_EQ(*(*lists()[0].prefixes())[0].base_prefix(), "10.0.0.0/8"); } TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteEntryTwiceIsIdempotent) { - configureEntry("PL100", 10, "10.0.0.0/8"); - EXPECT_THAT(del({"PL100", "entry", "10"}), HasSubstr("Successfully")); + configureEntry("PL100", "10.0.0.0/8"); + EXPECT_THAT( + del({"PL100", "prefix", "10.0.0.0/8"}), HasSubstr("Successfully")); EXPECT_TRUE(lists()[0].prefixes()->empty()); - auto again = del({"PL100", "entry", "10"}); + auto again = del({"PL100", "prefix", "10.0.0.0/8"}); EXPECT_THAT( again, HasSubstr( - "Warning: BGP prefix-list PL100 has no entry 10; nothing to delete")); + "Warning: BGP prefix-list PL100 has no prefix 10.0.0.0/8; nothing to " + "delete")); EXPECT_THAT(again, Not(HasSubstr("Error:"))); ASSERT_EQ(lists().size(), 1); EXPECT_TRUE(lists()[0].prefixes()->empty()); @@ -277,7 +281,7 @@ TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteEntryTwiceIsIdempotent) { TEST_F( CmdDeleteBgpPolicyPrefixListTestFixture, deleteEntryFromUnknownListWarns) { - auto result = del({"NO-SUCH-LIST", "entry", "10"}); + auto result = del({"NO-SUCH-LIST", "prefix", "10.0.0.0/8"}); EXPECT_THAT( result, HasSubstr( @@ -351,13 +355,15 @@ 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"); + configureEntry("PL100", "10.0.0.0/8"); + configureEntry("PL100", "192.168.0.0/16"); addPolicyTermMatching("RM100", 10, "PL100"); - auto result = del({"PL100", "entry", "10"}); + auto result = del({"PL100", "prefix", "10.0.0.0/8"}); EXPECT_THAT( - result, HasSubstr("Successfully deleted BGP prefix-list PL100 entry 10")); + result, + HasSubstr( + "Successfully deleted BGP prefix-list PL100 prefix 10.0.0.0/8")); ASSERT_EQ(lists().size(), 1); EXPECT_EQ(lists()[0].prefixes()->size(), 1); } diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp index 879add72795df..def8205bad0b0 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp @@ -2,7 +2,7 @@ /** * End-to-end tests for `fboss2-dev config protocol bgp policy prefix-list - * entry [ ...]`. + * prefix [ ...]`. * * Scope: the entry level. Every test stages the change AND commits it, then * asserts the value landed at the correct thrift field path inside the @@ -19,7 +19,6 @@ #include #include -#include #include #include @@ -35,18 +34,18 @@ namespace { // Test-only prefix-list / entry keys, unlikely to collide with a real list in // the device's running BGP config. const std::string kList = "FBOSS2-TEST-PREFIXES-ENTRY"; -const std::string kEntrySeq = "10"; -const std::string kEntrySeq2 = "20"; +const std::string kPrefix = "10.0.0.0/8"; +const std::string kPrefix2 = "192.168.0.0/16"; } // namespace class ConfigBgpPolicyPrefixListEntryTest : public ConfigBgpTestBase { protected: - // Stage `config protocol bgp policy prefix-list entry ` + // Stage `config protocol bgp policy prefix-list prefix ` // WITHOUT clearing the staged session, so attributes can accumulate across // invocations. Returns the staged session JSON. folly::dynamic stageEntry(const std::vector& tokens) { std::vector args = { - "config", "protocol", "bgp", "policy", "prefix-list", kList, "entry"}; + "config", "protocol", "bgp", "policy", "prefix-list", kList, "prefix"}; args.insert(args.end(), tokens.begin(), tokens.end()); auto result = runCli(args); EXPECT_EQ(result.exitCode, 0) @@ -71,15 +70,16 @@ class ConfigBgpPolicyPrefixListEntryTest : public ConfigBgpTestBase { return nullptr; } - // The .prefixes[] entry with seq_num `seqNum` inside `list`, or nullptr. + // The .prefixes[] entry with base_prefix `prefix` inside `list`, or nullptr. static const folly::dynamic* findEntry( const folly::dynamic& list, - int64_t seqNum) { + const std::string& prefix) { if (list.count("prefixes") == 0) { return nullptr; } for (const auto& entry : list["prefixes"]) { - if (entry.count("seq_num") && entry["seq_num"].asInt() == seqNum) { + if (entry.count("base_prefix") && + entry["base_prefix"].asString() == prefix) { return &entry; } } @@ -90,12 +90,11 @@ class ConfigBgpPolicyPrefixListEntryTest : public ConfigBgpTestBase { TEST_F(ConfigBgpPolicyPrefixListEntryTest, SetAttributesAndCommit) { discardSession(); clearBgpSession(); - stageEntry({kEntrySeq, "base-prefix", "10.0.0.0/8"}); - stageEntry({kEntrySeq, "match-logic", "EQUAL"}); - stageEntry({kEntrySeq, "prefix-len-range", "compare-operator", "GE"}); - stageEntry({kEntrySeq, "prefix-len-range", "value", "24"}); - stageEntry({kEntrySeq, "communities", "65000:100"}); - stageEntry({kEntrySeq, "description", "test"}); + stageEntry({kPrefix, "match-logic", "EQUAL"}); + stageEntry({kPrefix, "prefix-len-range", "compare-operator", "GE"}); + stageEntry({kPrefix, "prefix-len-range", "value", "24"}); + stageEntry({kPrefix, "communities", "65000:100"}); + stageEntry({kPrefix, "description", "test"}); commitAndGetSha(); ASSERT_TRUE(waitForBgpDaemonActive()) << "bgpd did not return active after commit; state=" @@ -107,9 +106,11 @@ TEST_F(ConfigBgpPolicyPrefixListEntryTest, SetAttributesAndCommit) { const auto* list = findList(running, kList); ASSERT_NE(list, nullptr) << "bgpd's running config has no prefix-list " << kList; - const auto* entry = findEntry(*list, 10); - ASSERT_NE(entry, nullptr) << "running config has no entry " << kEntrySeq; - EXPECT_EQ((*entry)["base_prefix"].asString(), "10.0.0.0/8"); + const auto* entry = findEntry(*list, kPrefix); + ASSERT_NE(entry, nullptr) << "running config has no prefix " << kPrefix; + EXPECT_EQ((*entry)["base_prefix"].asString(), kPrefix); + // The key is the prefix; nothing writes seq_num, which bgpd rejects. + EXPECT_EQ(entry->count("seq_num"), 0); // routing_policy.MatchValueLogicOperator.EQUAL = 0 (integer on the // SimpleJSON wire). bgpd rejects any other value once a policy references // the list. @@ -129,8 +130,8 @@ TEST_F(ConfigBgpPolicyPrefixListEntryTest, DeleteEntryAndCommit) { // config. discardSession(); clearBgpSession(); - stageEntry({kEntrySeq, "base-prefix", "10.0.0.0/8"}); - stageEntry({kEntrySeq2, "base-prefix", "192.168.0.0/16"}); + stageEntry({kPrefix}); + stageEntry({kPrefix2}); commitAndGetSha(); ASSERT_TRUE(waitForBgpDaemonActive()) << "bgpd did not return active after commit; state=" @@ -141,8 +142,8 @@ TEST_F(ConfigBgpPolicyPrefixListEntryTest, DeleteEntryAndCommit) { ASSERT_NE(list, nullptr) << "setup commit did not land the prefix-list in bgpd's running " "config"; - ASSERT_NE(findEntry(*list, 10), nullptr); - ASSERT_NE(findEntry(*list, 20), nullptr); + ASSERT_NE(findEntry(*list, kPrefix), nullptr); + ASSERT_NE(findEntry(*list, kPrefix2), nullptr); } clearBgpSession(); @@ -153,15 +154,15 @@ TEST_F(ConfigBgpPolicyPrefixListEntryTest, DeleteEntryAndCommit) { "policy", "prefix-list", kList, - "entry", - kEntrySeq}); + "prefix", + kPrefix}); EXPECT_THAT( result.stdout, HasSubstr( fmt::format( - "Successfully deleted BGP prefix-list {} entry {}", + "Successfully deleted BGP prefix-list {} prefix {}", kList, - kEntrySeq))); + kPrefix))); commitAndGetSha(); ASSERT_TRUE(waitForBgpDaemonActive()) << "bgpd did not return active after delete commit; state=" @@ -170,8 +171,8 @@ TEST_F(ConfigBgpPolicyPrefixListEntryTest, DeleteEntryAndCommit) { const auto* list = findList(running, kList); ASSERT_NE(list, nullptr) << "deleting one entry must not delete the prefix-list"; - EXPECT_EQ(findEntry(*list, 10), nullptr) + EXPECT_EQ(findEntry(*list, kPrefix), nullptr) << "deleted entry still present in bgpd's running config"; - EXPECT_NE(findEntry(*list, 20), nullptr) + EXPECT_NE(findEntry(*list, kPrefix2), nullptr) << "surviving entry missing from bgpd's running config"; } diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index e909c5474c51e..b93c21e9f7bc4 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -666,10 +666,6 @@ def _prefix_list_entry_scalar_commands( is a warning, not a command. """ commands = [] - if entry.get("base_prefix"): - commands.append( - f"{prefix} base-prefix {escape_shell_arg(entry['base_prefix'])}" - ) if entry.get("description"): commands.append( f"{prefix} description {escape_shell_arg(entry['description'])}" @@ -744,24 +740,34 @@ def _prefix_list_entry_range_commands( def generate_prefix_list_entry_commands( list_name: str, entry: dict[str, Any] ) -> list[str]: - """Generate `... prefix-list entry ` commands for one entry. + """Generate `... prefix-list prefix ` commands for one entry. - The CLI keys entries by seq_num and supports a single prefix_len_range; - anything beyond that surfaces as a warning. + The CLI keys entries by base_prefix, the identity bgpd keeps (it merges + entries by prefix and rejects seq_num), and supports a single + prefix_len_range; anything beyond that surfaces as a warning. """ - if "seq_num" not in entry: + if not entry.get("base_prefix"): return [ _warning( - f"prefix-list {list_name}: entry '{entry.get('base_prefix', '')}' " - "has no seq_num and cannot be addressed by the CLI; not emitted" + f"prefix-list {list_name}: an entry has no base_prefix and cannot " + "be addressed by the CLI; not emitted" ) ] - label = f"prefix-list {list_name} entry {entry['seq_num']}" + base_prefix = entry["base_prefix"] + label = f"prefix-list {list_name} prefix {base_prefix}" prefix = ( f"config protocol bgp policy prefix-list {escape_shell_arg(list_name)} " - f"entry {escape_shell_arg(entry['seq_num'])}" + f"prefix {escape_shell_arg(base_prefix)}" ) - commands = _prefix_list_entry_scalar_commands(prefix, label, entry) + commands = [] + if "seq_num" in entry: + commands.append( + _warning( + f"{label}: seq_num {entry['seq_num']} is rejected by bgpd and " + "not part of the CLI's entry identity; dropped" + ) + ) + commands.extend(_prefix_list_entry_scalar_commands(prefix, label, entry)) commands.extend(_prefix_list_entry_range_commands(prefix, label, entry)) if entry.get("regex"): commands.append(f"{prefix} regex {escape_shell_arg(entry['regex'])}") @@ -771,7 +777,8 @@ def generate_prefix_list_entry_commands( commands.append( _warning(f"{label}: ip_version has no CLI equivalent; not emitted") ) - if not commands: + if all(c.startswith(_WARNING_PREFIX) for c in commands): + # Nothing expressible to set: still create the entry by its prefix. commands.append(prefix) return commands diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index b27fb1abc3889..b969ca74baccb 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 @@ -1330,13 +1330,12 @@ def test_policy_block_order(self) -> None: class GeneratePrefixListEntryCommandsTest(unittest.TestCase): """Tests for generate_prefix_list_entry_commands (entry grammar).""" - PREFIX = "config protocol bgp policy prefix-list PL entry 10" + PREFIX = "config protocol bgp policy prefix-list PL prefix 10.0.0.0/8" def test_full_entry(self) -> None: commands = generate_prefix_list_entry_commands( "PL", { - "seq_num": 10, "base_prefix": "10.0.0.0/8", "description": "rfc1918 a", "match_logic": 1, @@ -1349,10 +1348,9 @@ def test_full_entry(self) -> None: self.assertEqual( commands, [ - f"{self.PREFIX} base-prefix 10.0.0.0/8", f"{self.PREFIX} description 'rfc1918 a'", - "# WARNING: prefix-list PL entry 10: match_logic NOT_EQUAL is not " - "accepted by bgpd (only EQUAL); not emitted", + "# WARNING: prefix-list PL prefix 10.0.0.0/8: match_logic NOT_EQUAL " + "is not accepted by bgpd (only EQUAL); not emitted", f"{self.PREFIX} max-allowed-subnet-count 4", f"{self.PREFIX} prefix-len-range compare-operator LE", f"{self.PREFIX} prefix-len-range value 24", @@ -1364,7 +1362,7 @@ def test_full_entry(self) -> None: def test_equal_default_omitted(self) -> None: commands = generate_prefix_list_entry_commands( - "PL", {"seq_num": 10, "match_logic": 0} + "PL", {"base_prefix": "10.0.0.0/8", "match_logic": 0} ) self.assertEqual(commands, [self.PREFIX]) @@ -1372,32 +1370,46 @@ def test_range_operator_rg_warns(self) -> None: commands = generate_prefix_list_entry_commands( "PL", { - "seq_num": 10, + "base_prefix": "10.0.0.0/8", "prefix_len_ranges": [{"compare_operator": 7, "value": 24}], }, ) self.assertEqual( commands, [ - "# WARNING: prefix-list PL entry 10: prefix_len_ranges " + "# WARNING: prefix-list PL prefix 10.0.0.0/8: prefix_len_ranges " "compare_operator RG is not accepted by bgpd; the range is not " - "emitted" + "emitted", + # The entry itself still exists, so it is created by its prefix. + self.PREFIX, ], ) - def test_missing_seq_num_warns(self) -> None: - commands = generate_prefix_list_entry_commands( - "PL", {"base_prefix": "10.0.0.0/8"} - ) + def test_missing_base_prefix_warns(self) -> None: + commands = generate_prefix_list_entry_commands("PL", {"seq_num": 10}) self.assertEqual(len(commands), 1) self.assertTrue(commands[0].startswith("# WARNING:")) - self.assertIn("10.0.0.0/8", commands[0]) + self.assertIn("no base_prefix", commands[0]) + + def test_seq_num_is_dropped_with_warning(self) -> None: + """bgpd rejects seq_num; the prefix is the entry's identity.""" + commands = generate_prefix_list_entry_commands( + "PL", {"seq_num": 10, "base_prefix": "10.0.0.0/8"} + ) + self.assertEqual( + commands, + [ + "# WARNING: prefix-list PL prefix 10.0.0.0/8: seq_num 10 is " + "rejected by bgpd and not part of the CLI's entry identity; dropped", + self.PREFIX, + ], + ) def test_extra_ranges_and_ip_version_warn(self) -> None: commands = generate_prefix_list_entry_commands( "PL", { - "seq_num": 10, + "base_prefix": "10.0.0.0/8", "prefix_len_ranges": [{"value": 24}, {"value": 32}], "ip_version": 1, }, @@ -1409,18 +1421,11 @@ def test_extra_ranges_and_ip_version_warn(self) -> None: def test_entries_emitted_inside_list(self) -> None: commands = generate_prefix_list_commands( - { - "name": "PL", - "version": 4, - "prefixes": [{"seq_num": 10, "base_prefix": "10.0.0.0/8"}], - } + {"name": "PL", "version": 4, "prefixes": [{"base_prefix": "10.0.0.0/8"}]} ) self.assertEqual( commands, - [ - "config protocol bgp policy prefix-list PL ip-version v4", - f"{self.PREFIX} base-prefix 10.0.0.0/8", - ], + ["config protocol bgp policy prefix-list PL ip-version v4", self.PREFIX], ) From fb0f0fa2db065ac56fd34160559bdee8c5a279e2 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 16 Oct 2026 06:55:48 +0000 Subject: [PATCH 7/7] Use the canonical Meta copyright header in new files --- .../entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp | 2 +- .../entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h | 2 +- .../test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp | 2 +- .../integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp index c0253764224fd..2dc4fa48e4f02 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h index 1a4cf34485e5b..454bbb047fd4b 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp index 5fce629afbb74..9ba4ab64a9578 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. #include #include diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp index def8205bad0b0..13eb1a6506019 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. /** * End-to-end tests for `fboss2-dev config protocol bgp policy prefix-list