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..3a1ef56a11529 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>, + {{ + "prefix", + "Configure a prefix-list entry: " + " [ ...] " + "(communities, description, match-logic, " + "max-allowed-subnet-count, " + "prefix-len-range, regex)", + commandHandler< + CmdConfigProtocolBgpPolicyPrefixListEntry>, + argRegistrar< + CmdConfigProtocolBgpPolicyPrefixListEntryTraits>, + }}, }}, }, }, @@ -824,7 +837,8 @@ const CommandTree& kConfigCommandTree() { }, { "prefix-list", - "Delete a BGP prefix-list: ", + "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 4d3b6a69bdd63..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,6 +10,7 @@ #pragma once +#include #include #include @@ -22,10 +23,11 @@ /** * 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 prefix + * subcommand (CmdConfigProtocolBgpPolicyPrefixListEntry), and the delete + * counterparts. A PrefixList is keyed by name; a PrefixListEntry (in + * prefixes[]) is keyed by base_prefix, the only identity bgpd keeps (it + * merges entries by prefix and rejects seq_num). */ namespace facebook::fboss::bgpcli { @@ -60,4 +62,39 @@ 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, + const std::string& basePrefix) { + const auto& entries = *list.prefixes(); + return std::any_of(entries.begin(), entries.end(), [&](const auto& entry) { + return *entry.base_prefix() == basePrefix; + }); +} + +// 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, + const std::string& basePrefix) { + auto& entries = *list.prefixes(); + for (auto& entry : entries) { + if (*entry.base_prefix() == basePrefix) { + return entry; + } + } + entries.emplace_back(); + auto& entry = entries.back(); + entry.base_prefix() = basePrefix; + return entry; +} + } // namespace facebook::fboss::bgpcli 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 new file mode 100644 index 0000000000000..2dc4fa48e4f02 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp @@ -0,0 +1,402 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * 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 "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" +#include "fmt/format.h" + +#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. 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"; +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). 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"; + +// 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"; + +// 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; + } + 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; + } + 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); + 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; +} + +// 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; +} + +// ---- hand-written handlers -------------------------------------------------- +// 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) { + 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 = std::string(kMatchLogicEqual); + static const std::map, std::less<>> + kHandlers = { + {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: prefix is required, optionally followed by an " + " "); + } + if (!bgpcli::isPrefixWithLength(v[0])) { + throw std::invalid_argument( + fmt::format("Error: Invalid prefix '{}'; expected ", v[0])); + } + basePrefix_ = v[0]; + if (v.size() == 1) { + return; // bare `prefix `: 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 prefix attribute '{}'. Valid " + "attributes: {}", + attr_, + validAttrList())); + } +} + +CmdConfigProtocolBgpPolicyPrefixListEntryTraits::RetType +CmdConfigProtocolBgpPolicyPrefixListEntry::queryClient( + const HostInfo& /* hostInfo */, + const BgpPrefixListConfig& listArgs, + const ObjectArgType& args) { + // 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 prefix in separate " + "commands (got prefix-list attribute '{}' alongside prefix {})", + listArgs.attr(), + 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.basePrefix()); + auto& entry = bgpcli::findOrCreatePrefixListEntry(list, args.basePrefix()); + + Result result = args.attr().empty() + ? ok(entryCreated + ? fmt::format( + "Successfully created BGP prefix-list {} prefix {}", + listArgs.listName(), + args.basePrefix()) + : fmt::format( + "BGP prefix-list {} prefix {} already exists", + listArgs.listName(), + 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()); + + if (result.ok) { + if (!args.attr().empty()) { + result.message += fmt::format( + " for prefix-list {} prefix {}", + listArgs.listName(), + args.basePrefix()); + } + 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..454bbb047fd4b --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h @@ -0,0 +1,100 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "CLI/App.hpp" +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `prefix [ ...]`, validated at +// construction. An entry (routing_policy.PrefixListEntry in +// 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: +// ... 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 prefix max-allowed-subnet-count +// (bgpd reads it only for golden-prefix policies) +// ... 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); + const std::string& basePrefix() const { + return basePrefix_; + } + const std::string& attr() const { + return attr_; + } + const std::vector& values() const { + return values_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::string basePrefix_; + 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. 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; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // 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, " [ ...]"); + } + 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/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp index ce04e7803bcef..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 @@ -18,7 +18,11 @@ #include #include #include +#include +#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" @@ -33,6 +37,11 @@ namespace facebook::fboss { namespace { +// 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 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), // labelled the way the CLI addresses the term. @@ -75,23 +84,41 @@ 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 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: `prefix` 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, + kPrefixKeyword, + "Error: delete protocol bgp policy prefix-list requires , " + "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( - fmt::format( - "Error: unexpected token '{}'. Usage: delete protocol bgp policy " - "prefix-list ", - v[1])); + selector.memberName + ? fmt::format( + "Error: unexpected token '{}' after prefix ", + v[selector.restStart]) + : fmt::format( + "Error: unexpected token '{}'. Usage: delete protocol bgp " + "policy prefix-list [prefix ]", + v[selector.restStart])); + } + listName_ = std::move(selector.listName); + if (selector.memberName) { + if (!bgpcli::isPrefixWithLength(*selector.memberName)) { + throw std::invalid_argument( + fmt::format( + "Error: Invalid prefix '{}'; expected ", + *selector.memberName)); + } + basePrefix_ = *selector.memberName; } - listName_ = v[0]; } CmdDeleteProtocolBgpPolicyPrefixListTraits::RetType @@ -118,9 +145,32 @@ CmdDeleteProtocolBgpPolicyPrefixList::queryClient( if (it == lists.end()) { return absent(); } + 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.base_prefix() == args.basePrefix(); + }); + if (entryIt == entries.end()) { + return fmt::format( + "Warning: BGP prefix-list {} has no prefix {}; nothing to delete", + args.listName(), + args.basePrefix()); + } + entries.erase(entryIt); + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP prefix-list {} prefix {}\n" + "Config saved to: {}", + args.listName(), + args.basePrefix(), + 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..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,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 [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) @@ -32,17 +34,24 @@ class BgpPrefixListRef : public utils::BaseObjectArgType { const std::string& listName() const { return listName_; } + bool hasPrefix() const { + return basePrefix_.has_value(); + } + const std::string& basePrefix() const { + return *basePrefix_; + } const static utils::ObjectArgTypeId id = utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; private: std::string listName_; + std::optional basePrefix_; }; 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, " [prefix ]"); } using ObjectArgType = BgpPrefixListRef; using RetType = std::string; 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..9ba4ab64a9578 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp @@ -0,0 +1,324 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +#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.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.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"})); + + // 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({"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.0.0.0/8", "base-prefix", "10.0.0.0/8"}), + std::invalid_argument); + EXPECT_THROW( + BgpPrefixListEntryConfig({"10.0.0.0/8", "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.0.0.0/8"}); + EXPECT_THAT( + 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); + // 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.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_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); + EXPECT_EQ(*e.max_allowed_golden_prefix_subnet_count(), 64); + EXPECT_EQ(*e.regex(), "^10\\..*"); +} + +TEST_F(CmdConfigBgpPolicyPrefixListEntryTestFixture, setPrefixLenRange) { + 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); + const auto& range = entry(0, 0).prefix_len_ranges()->front(); + EXPECT_EQ(*range.compare_operator(), ComparisonOperator::GE); + EXPECT_EQ(*range.value(), 24); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + valuesBgpdRejectsAreRefused) { + runEntry( + {"PL100"}, {"10.0.0.0/8", "prefix-len-range", "compare-operator", "GE"}); + // bgpd: toPolicyComparisonOperator() throws on 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.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.0.0.0/8", "communities", "65000:100"}); + EXPECT_THAT(result, HasSubstr("Successfully added 65000:100 to communities")); + runEntry({"PL100"}, {"10.0.0.0/8", "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.0.0.0/8", "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.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 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); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + 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, 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"); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + invalidPrefixLenRangeRejected) { + // Unknown sub-attribute. + 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.0.0.0/8", "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.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()); +} + +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.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); + EXPECT_TRUE(lists()[0].prefixes()->empty()); + EXPECT_EQ(*lists()[0].description(), "keep-me"); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + rejectedEntryKeepsExistingEntries) { + runEntry({"PL100"}, {"10.0.0.0/8"}); + ASSERT_EQ(lists()[0].prefixes()->size(), 1); + + 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).base_prefix(), "10.0.0.0/8"); +} + +TEST_F( + CmdConfigBgpPolicyPrefixListEntryTestFixture, + reReferenceReportsExisting) { + runEntry({"PL100"}, {"10.0.0.0/8"}); + EXPECT_THAT( + runEntry({"PL100"}, {"10.0.0.0/8"}), + HasSubstr("prefix 10.0.0.0/8 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.0.0.0/8"}); + EXPECT_THAT(result, HasSubstr("separate commands")); + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp index 4bb40be22a255..98b3cf1f66846 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,20 @@ 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, + const std::string& basePrefix) { + auto& session = ConfigSession::getInstance(); + auto& list = + bgpcli::findOrCreatePrefixList(session.getBgpConfig(), listName); + bgpcli::findOrCreatePrefixListEntry(list, basePrefix); + session.saveBgpConfig(); + } + std::string del(const std::vector& tokens) { CmdDeleteProtocolBgpPolicyPrefixList cmd; HostInfo hostInfo("testhost"); @@ -111,14 +126,27 @@ class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { auto listOnly = BgpPrefixListRef({"PL100"}); EXPECT_EQ(listOnly.listName(), "PL100"); + EXPECT_FALSE(listOnly.hasPrefix()); - // Invalid: empty, empty name, any second token (entries are deleted - // through the entry subcommand, not here). + 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-`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", "prefix"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListRef({"PL100", "prefix", "10.0.0.0"}), std::invalid_argument); EXPECT_THROW( - BgpPrefixListRef({"PL100", "entry", "10"}), std::invalid_argument); + BgpPrefixListRef({"PL100", "prefix", "ten"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListRef({"PL100", "prefix", "10.0.0.0/8", "extra"}), + std::invalid_argument); } // ============================================================================== @@ -126,7 +154,7 @@ TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { // ============================================================================== TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingList) { - configure({"PL100", "description", "drop"}); + configureEntry("PL100", "10.0.0.0/8"); configure({"PL200", "description", "keep"}); ASSERT_EQ(lists().size(), 2); @@ -178,6 +206,92 @@ TEST_F( EXPECT_EQ(*lists()[0].name(), "PL100"); } +// ============================================================================== +// queryClient: single entry (prefix) deletion +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingEntry) { + configureEntry("PL100", "10.0.0.0/8"); + configureEntry("PL100", "192.168.0.0/16"); + + auto result = del({"PL100", "prefix", "10.0.0.0/8"}); + EXPECT_THAT( + 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].base_prefix(), "192.168.0.0/16"); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteLastEntryKeepsList) { + configureEntry("PL100", "10.0.0.0/8"); + + 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). + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); + EXPECT_TRUE(lists()[0].prefixes()->empty()); +} + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownEntryWarns) { + 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", "prefix", "172.16.0.0/12"}); + EXPECT_THAT( + result, + HasSubstr( + "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].base_prefix(), "10.0.0.0/8"); +} + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteEntryTwiceIsIdempotent) { + 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", "prefix", "10.0.0.0/8"}); + EXPECT_THAT( + again, + HasSubstr( + "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()); +} + +TEST_F( + CmdDeleteBgpPolicyPrefixListTestFixture, + deleteEntryFromUnknownListWarns) { + auto result = del({"NO-SUCH-LIST", "prefix", "10.0.0.0/8"}); + 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 +351,21 @@ 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.0.0.0/8"); + configureEntry("PL100", "192.168.0.0/16"); + addPolicyTermMatching("RM100", 10, "PL100"); + + auto result = del({"PL100", "prefix", "10.0.0.0/8"}); + EXPECT_THAT( + 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); +} + } // 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..13eb1a6506019 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp @@ -0,0 +1,178 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy prefix-list + * 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 + * 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 "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 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 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, "prefix"}; + 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 base_prefix `prefix` inside `list`, or nullptr. + static const folly::dynamic* findEntry( + const folly::dynamic& list, + const std::string& prefix) { + if (list.count("prefixes") == 0) { + return nullptr; + } + for (const auto& entry : list["prefixes"]) { + if (entry.count("base_prefix") && + entry["base_prefix"].asString() == prefix) { + return &entry; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyPrefixListEntryTest, SetAttributesAndCommit) { + discardSession(); + clearBgpSession(); + 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=" + << 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, 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. + 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); + 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({kPrefix}); + stageEntry({kPrefix2}); + 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, kPrefix), nullptr); + ASSERT_NE(findEntry(*list, kPrefix2), nullptr); + } + + clearBgpSession(); + auto result = runCli( + {"delete", + "protocol", + "bgp", + "policy", + "prefix-list", + kList, + "prefix", + kPrefix}); + EXPECT_THAT( + result.stdout, + HasSubstr( + fmt::format( + "Successfully deleted BGP prefix-list {} prefix {}", + kList, + kPrefix))); + 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, kPrefix), nullptr) + << "deleted entry still present in bgpd's running config"; + EXPECT_NE(findEntry(*list, kPrefix2), nullptr) + << "surviving entry missing from bgpd's running config"; +} 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. * diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index a159f42b21eb7..b93c21e9f7bc4 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -645,16 +645,140 @@ 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.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, 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("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( + _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 " + 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: + 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 {escape_shell_arg(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"prefix-list {name}: {len(prefix_list['prefixes'])} prefixes " - "are not emitted until the entry subcommand lands" + f"{label}: only the first of {len(ranges)} prefix_len_ranges is " + "expressible; the rest are not emitted" ) ) - commands.extend(_prefix_list_warnings(name, prefix_list)) - if all(command.startswith("#") for command in commands): - # Warnings do not recreate the list; preserve it with a bare command. + return commands + + +def generate_prefix_list_entry_commands( + list_name: str, entry: dict[str, Any] +) -> list[str]: + """Generate `... prefix-list prefix ` commands for one entry. + + 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 not entry.get("base_prefix"): + return [ + _warning( + f"prefix-list {list_name}: an entry has no base_prefix and cannot " + "be addressed by the CLI; not emitted" + ) + ] + 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"prefix {escape_shell_arg(base_prefix)}" + ) + 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'])}") + 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 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 2947ad31597e5..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 @@ -30,6 +30,7 @@ generate_peer_group_commands, generate_policy_commands, generate_prefix_list_commands, + generate_prefix_list_entry_commands, json_to_cli, ) @@ -1244,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}), @@ -1342,6 +1327,108 @@ 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 prefix 10.0.0.0/8" + + def test_full_entry(self) -> None: + commands = generate_prefix_list_entry_commands( + "PL", + { + "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} description 'rfc1918 a'", + "# 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", + 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", {"base_prefix": "10.0.0.0/8", "match_logic": 0} + ) + self.assertEqual(commands, [self.PREFIX]) + + def test_range_operator_rg_warns(self) -> None: + commands = generate_prefix_list_entry_commands( + "PL", + { + "base_prefix": "10.0.0.0/8", + "prefix_len_ranges": [{"compare_operator": 7, "value": 24}], + }, + ) + self.assertEqual( + commands, + [ + "# 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", + # The entry itself still exists, so it is created by its prefix. + self.PREFIX, + ], + ) + + 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("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", + { + "base_prefix": "10.0.0.0/8", + "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": [{"base_prefix": "10.0.0.0/8"}]} + ) + self.assertEqual( + commands, + ["config protocol bgp policy prefix-list PL ip-version v4", self.PREFIX], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands."""