diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 379e474011f36..e142efcb39e51 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -978,6 +978,9 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.cpp fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp @@ -1120,6 +1123,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h fboss/cli/fboss2/commands/delete/protocol/CmdDeleteProtocol.h fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index f9226bda5a1be..ad3bfebb2abfa 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -16,6 +16,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp @@ -55,6 +56,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp + fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp fboss/cli/fboss2/test/config/CmdDeleteCoppQueueTest.cpp fboss/cli/fboss2/test/config/CmdDeleteCoppReasonTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index 7bfff504c46a8..3c586c6cd55f2 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -20,6 +20,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp fboss/cli/fboss2/test/integration_test/ConfigConcurrentSessionsTest.cpp fboss/cli/fboss2/test/integration_test/ConfigHostnameTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 384df2a3ef923..d3708160cde2c 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1212,6 +1212,7 @@ cpp_library( "commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.cpp", "commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp", "commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp", + "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1282,6 +1283,7 @@ cpp_library( "commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp", "commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp", "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp", + "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp", "commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp", "commands/delete/qos/CmdDeleteQos.cpp", @@ -1361,6 +1363,8 @@ cpp_library( "commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h", "commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h", "commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h", + "commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h", + "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", @@ -1432,6 +1436,7 @@ cpp_library( "commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h", "commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h", "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h", + "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h", "commands/delete/protocol/static/CmdDeleteProtocolStatic.h", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h", "commands/delete/qos/CmdDeleteQos.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 5d3fcd0a64bd2..94d83960a4920 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -45,6 +45,7 @@ #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" #include "fboss/cli/fboss2/commands/config/protocol/static/CmdConfigProtocolStatic.h" #include "fboss/cli/fboss2/commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h" #include "fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h" @@ -112,6 +113,7 @@ #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h" #include "fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h" #include "fboss/cli/fboss2/commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h" #include "fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h" @@ -444,6 +446,17 @@ const CommandTree& kConfigCommandTree() { CmdConfigProtocolBgpPolicyCommunityList>, argRegistrar< CmdConfigProtocolBgpPolicyCommunityListTraits>, + }, + { + "prefix-list", + "Configure BGP prefix-list: " + "[ ...] " + "(boolean-operator, description, " + "ip-version)", + commandHandler< + CmdConfigProtocolBgpPolicyPrefixList>, + argRegistrar< + CmdConfigProtocolBgpPolicyPrefixListTraits>, }}, }, }, @@ -808,6 +821,13 @@ const CommandTree& kConfigCommandTree() { CmdDeleteProtocolBgpPolicyCommunityList>, argRegistrar< CmdDeleteProtocolBgpPolicyCommunityListTraits>, + }, + { + "prefix-list", + "Delete a BGP prefix-list: ", + commandHandler, + argRegistrar< + CmdDeleteProtocolBgpPolicyPrefixListTraits>, }}, }}, }, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h index 6d0115fb5aa0f..df4e340b6a1ad 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h @@ -20,7 +20,7 @@ namespace facebook::fboss { // Grouping node for `config protocol bgp policy ...`. Holds no // state of its own; the per-object-type dispatchers (as-path-list, -// community-list, and later prefix-list/routing-policy) are its subcommands. +// community-list, prefix-list, and later routing-policy) are its subcommands. // Mirrors CmdConfigProtocolBgp. struct CmdConfigProtocolBgpPolicyTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgp; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h index 6bea30b09ffa0..b7bcdd6f9622d 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h @@ -10,8 +10,10 @@ #pragma once +#include #include #include +#include #ifndef IS_OSS #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" @@ -60,4 +62,67 @@ inline bgp::bgp_policy::CommunityList& findOrCreateCommunityList( return list; } +// A routing-policy term that names a community-list. A match reference +// (`filter` set) is a COMMUNITY match whose +// communities_filter.community_list_names carries the name; an action +// reference (`filter` null, `isAction`) is a community action whose +// community_action_list_names carries it. `label` identifies the term the +// way the CLI addresses it (`policy

term `). +struct CommunityListReference { + bgp::bgp_policy::CommunityList* filter; + std::string label; + bool isAction; +}; + +// Every reference to `listName`. bgpd resolves both kinds by name at config +// load (CommunityMatch / CommunityAction::PopulateReferences) and fails the +// load on a dangling one, so the delete command refuses while this is +// non-empty. A match's inline copy must also carry the same boolean_operator +// as the list ("Conflicting boolean_operator from the reference") and bgpd +// reads exact_match only from that inline copy, so the config command writes +// both through to every match reference. +inline std::vector findTermsReferencingCommunityList( + bgp::thrift::BgpConfig& cfg, + const std::string& listName) { + std::vector refs; + if (!cfg.policies().has_value()) { + return refs; + } + auto names = [&](const auto& list) { + return std::find(list.begin(), list.end(), listName) != list.end(); + }; + for (auto& policy : *cfg.policies()->bgp_policy_statements()) { + for (auto& term : *policy.policy_entries()) { + auto label = term.sequence_number().has_value() + ? fmt::format( + "policy {} term {}", *policy.name(), *term.sequence_number()) + : fmt::format("policy {} term {}", *policy.name(), *term.name()); + if (term.policy_match_entries().has_value()) { + for (auto& match : *term.policy_match_entries()->match_entries()) { + if (!match.communities_filter().has_value() || + !match.communities_filter()->community_list_names().has_value()) { + continue; + } + auto& filter = *match.communities_filter(); + if (names(*filter.community_list_names())) { + refs.push_back({&filter, label, false}); + } + } + } + for (auto& action : *term.policy_action_entries()) { + if (!action.community_action().has_value() || + !action.community_action() + ->community_action_list_names() + .has_value()) { + continue; + } + if (names(*action.community_action()->community_action_list_names())) { + refs.push_back({nullptr, label, true}); + } + } + } + } + return refs; +} + } // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp index fb0eaf1dd6248..79cdcdb57f12d 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp @@ -237,6 +237,22 @@ CmdConfigProtocolBgpPolicyCommunityList::queryClient( if (!args.attr().empty()) { result.message += fmt::format(" for community-list {}", args.listName()); } + if (args.attr() == kBooleanOperator || args.attr() == kExactMatch) { + // bgpd requires a term's inline copy of the list to carry the same + // boolean_operator ("Conflicting boolean_operator from the reference") + // and reads exact_match only from that copy, so keep every match + // reference in step with the named list. + for (auto& ref : + bgpcli::findTermsReferencingCommunityList(cfg, args.listName())) { + if (ref.filter == nullptr) { + continue; + } + ref.filter->boolean_operator() = *list.boolean_operator(); + if (list.exact_match().has_value()) { + ref.filter->exact_match() = *list.exact_match(); + } + } + } session.saveBgpConfig(); result.message += fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h new file mode 100644 index 0000000000000..acbe11d08af78 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h @@ -0,0 +1,63 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include + +#ifndef IS_OSS +#include +#include +#else +#include +#endif + +/** + * 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). + */ +namespace facebook::fboss::bgpcli { + +inline bool prefixListExists( + const bgp::thrift::BgpConfig& cfg, + const std::string& name) { + if (!cfg.policies().has_value()) { + return false; + } + const auto& lists = *cfg.policies()->prefix_lists(); + return std::any_of(lists.begin(), lists.end(), [&](const auto& list) { + return *list.name() == name; + }); +} + +// Find the prefix-list keyed by name, creating it if absent. Setting an +// attribute on a not-yet-created list implicitly creates it, so command +// ordering stays forgiving; a bare `prefix-list ` creates one +// explicitly. PrefixList's only key field is `name`. +inline bgp::routing_policy::PrefixList& findOrCreatePrefixList( + bgp::thrift::BgpConfig& cfg, + const std::string& name) { + auto& lists = *cfg.policies().ensure().prefix_lists(); + for (auto& list : lists) { + if (*list.name() == name) { + return list; + } + } + lists.emplace_back(); + auto& list = lists.back(); + list.name() = name; + return list; +} + +} // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp new file mode 100644 index 0000000000000..f1d55bd4aea7c --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp @@ -0,0 +1,226 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "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. The list's prefixes +// are their own subcommand, not attributes. +constexpr std::string_view kBooleanOperator = "boolean-operator"; +constexpr std::string_view kDescription = "description"; +constexpr std::string_view kIpVersion = "ip-version"; + +// boolean-operator values (routing_policy.BooleanOperator names). Only OR is +// offered: bgpd's PrefixTreeMatch rejects any other value ("PrefixList +// BooleanOperator can only be OR") once a policy references the list, and +// it rejects a list-level compare_operator outright, so that attribute is +// not offered at all. +constexpr std::string_view kBooleanOperatorOr = "OR"; + +// ip-version values, stored as the numeric IP version in PrefixList.version +// (the documented thrift field for this command). +constexpr std::string_view kIpVersionV4 = "v4"; +constexpr std::string_view kIpVersionV6 = "v6"; +constexpr int32_t kIpVersionV4Value = 4; +constexpr int32_t kIpVersionV6Value = 6; + +using BooleanOperator = bgp::routing_policy::BooleanOperator; +using PrefixList = bgp::routing_policy::PrefixList; +using bgpcli::AttrHandler; +using bgpcli::enumAttr; +using bgpcli::joinedStringAttr; +using bgpcli::ok; +using bgpcli::Result; + +// ---- value lookups ---------------------------------------------------------- + +std::optional lookupBooleanOperator(const std::string& s) { + if (s == kBooleanOperatorOr) { + return BooleanOperator::OR; + } + return std::nullopt; +} + +std::optional lookupIpVersion(const std::string& s) { + if (s == kIpVersionV4) { + return kIpVersionV4Value; + } + if (s == kIpVersionV6) { + return kIpVersionV6Value; + } + return std::nullopt; +} + +// ---- list-level setters ----------------------------------------------------- +// Each writes one already-parsed, already-validated value. Parsing and message +// text belong to the shared factories in BgpCliAttrHandlers.h. + +void setListBooleanOperator(PrefixList& list, BooleanOperator op) { + list.boolean_operator() = op; +} + +void setListDescription(PrefixList& list, const std::string& description) { + list.description() = description; +} + +// ip-version is stored as the numeric PrefixList.version, the field the +// command is documented against. +void setListIpVersion(PrefixList& list, int32_t version) { + list.version() = version; +} + +// ---- list-level attribute registry ------------------------------------------ +// One line per documented attribute: its dispatch key, its value shape, and +// the setter that stores it. +const std::map, std::less<>>& +listAttrHandlers() { + static const std::string kBooleanOperatorValues = + std::string(kBooleanOperatorOr); + static const std::string kIpVersionValues = + fmt::format("{}|{}", kIpVersionV4, kIpVersionV6); + static const std::map, std::less<>> + kHandlers = { + {std::string(kBooleanOperator), + enumAttr( + kBooleanOperator, + kBooleanOperatorValues, + lookupBooleanOperator, + setListBooleanOperator)}, + {std::string(kDescription), + joinedStringAttr(kDescription, setListDescription)}, + {std::string(kIpVersion), + enumAttr( + kIpVersion, + kIpVersionValues, + lookupIpVersion, + setListIpVersion)}, + }; + return kHandlers; +} + +std::string validAttrList() { + std::string out; + for (const auto& [name, _] : listAttrHandlers()) { + if (!out.empty()) { + out += ", "; + } + out += name; + } + return out; +} + +} // namespace + +// Parse + validate at construction so queryClient stays a thin dispatch. +// Throwing std::invalid_argument is how the framework surfaces arg parse +// errors (same mechanism as BgpAsPathListConfig). +BgpPrefixListConfig::BgpPrefixListConfig(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: prefix-list is required, optionally followed by an " + " "); + } + if (v[0].empty()) { + throw std::invalid_argument("Error: prefix-list name must not be empty"); + } + listName_ = v[0]; + + if (v.size() == 1) { + return; // bare `prefix-list `: create the list + } + + attr_ = v[1]; + values_.assign(v.begin() + 2, v.end()); + + if (listAttrHandlers().find(attr_) == listAttrHandlers().end()) { + throw std::invalid_argument( + fmt::format( + "Error: unknown prefix-list attribute '{}'. Valid attributes: {}", + attr_, + validAttrList())); + } +} + +CmdConfigProtocolBgpPolicyPrefixListTraits::RetType +CmdConfigProtocolBgpPolicyPrefixList::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool listCreated = !bgpcli::prefixListExists(cfg, args.listName()); + auto& list = bgpcli::findOrCreatePrefixList(cfg, args.listName()); + auto& lists = *cfg.policies().ensure().prefix_lists(); + + Result result = args.attr().empty() + ? ok(listCreated + ? fmt::format( + "Successfully created BGP prefix-list {}", args.listName()) + : fmt::format( + "BGP prefix-list {} already exists", args.listName())) + // The attribute is guaranteed valid: BgpPrefixListConfig's constructor + // rejects an unknown attribute before we get here. + : listAttrHandlers().find(args.attr())->second(list, args.values()); + + if (result.ok) { + if (!args.attr().empty()) { + result.message += fmt::format(" for prefix-list {}", args.listName()); + } + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } else if (listCreated && !lists.empty()) { + // Drop the phantom list so a rejected value is not visible to later + // lookups in the same process. + lists.pop_back(); + } + return result.message; +} + +void CmdConfigProtocolBgpPolicyPrefixList::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdConfigProtocolBgpPolicyPrefixList, + CmdConfigProtocolBgpPolicyPrefixListTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h new file mode 100644 index 0000000000000..d6c87608c2ac4 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "CLI/App.hpp" +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `prefix-list [ ...]`, validated at +// construction. A PrefixList is keyed by . Its prefixes[] entries are +// their own subcommand (CmdConfigProtocolBgpPolicyPrefixListEntry), not +// attributes. Mirrors BgpAsPathListConfig. +// +// Grammar (from the FBOSS proposed syntax): +// prefix-list (create/select list) +// prefix-list boolean-operator OR (list attribute; bgpd accepts +// only OR, and no list-level +// compare-operator at all) +// prefix-list description (list attribute) +// prefix-list ip-version (list attribute) +class BgpPrefixListConfig : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpPrefixListConfig(std::vector v); + const std::string& listName() const { + return listName_; + } + const std::string& attr() const { + return attr_; + } + const std::vector& values() const { + return values_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::string listName_; + std::string attr_; // matched dispatch key ("" = bare create) + std::vector values_; +}; + +// Single handler for the whole `config protocol bgp policy prefix-list` +// family; dispatch and per-attribute parsing live in the .cpp so a new tunable +// is a one-entry change. Mirrors CmdConfigProtocolBgpPolicyAsPathList. +struct CmdConfigProtocolBgpPolicyPrefixListTraits : public WriteCommandTraits { + using ParentCmd = CmdConfigProtocolBgpPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // No positionals_at_end() here: CLI11 must stay free to classify the + // `entry` token as this command's subcommand rather than swallowing it + // into args. See CmdConfigProtocolBgpPolicyAsPathListTraits. + cmd.add_option("args", args, " [ ...]"); + } + using ObjectArgType = BgpPrefixListConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyPrefixList + : public CmdHandler< + CmdConfigProtocolBgpPolicyPrefixList, + CmdConfigProtocolBgpPolicyPrefixListTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyPrefixListTraits::ObjectArgType; + using RetType = CmdConfigProtocolBgpPolicyPrefixListTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& args); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp index 6caa2350a22a6..26f6df1eebde2 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp @@ -13,12 +13,14 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include +#include #include #include #include #include #include #include +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" @@ -92,6 +94,7 @@ CmdDeleteProtocolBgpPolicyCommunityList::queryClient( if (it == lists.end()) { return absent; } + auto refs = bgpcli::findTermsReferencingCommunityList(cfg, args.listName()); if (args.hasCommunity()) { // Remove one value from communities; the list itself stays. auto& list = *it; @@ -100,6 +103,26 @@ CmdDeleteProtocolBgpPolicyCommunityList::queryClient( auto valueIt = std::find(communities.begin(), communities.end(), args.community()); if (valueIt != communities.end()) { + if (communities.size() == 1) { + // A community action resolves the list's values at daemon load and + // fails on an empty one ("Missing communities in CommunityList"); + // a match reference tolerates it. + std::vector actions; + for (const auto& ref : refs) { + if (ref.isAction) { + actions.push_back(ref.label); + } + } + if (!actions.empty()) { + return fmt::format( + "Error: {} is the last community of BGP community-list {}, " + "which is still used by the community action of {}; remove " + "those actions first", + args.community(), + args.listName(), + folly::join(", ", actions)); + } + } communities.erase(valueIt); if (communities.empty()) { // Leave the list looking like one that never had values, so a @@ -121,6 +144,21 @@ CmdDeleteProtocolBgpPolicyCommunityList::queryClient( args.listName(), args.community()); } + // A term's community_list_names / community_action_list_names resolve + // against this list by name at daemon load; erasing the list while a term + // still names it would commit a dangling reference and fail the load. + // Refuse and name the terms instead. + if (!refs.empty()) { + std::vector referrers; + for (const auto& ref : refs) { + referrers.push_back(ref.label); + } + return fmt::format( + "Error: BGP community-list {} is still referenced by {}; remove those " + "matches and actions first", + args.listName(), + folly::join(", ", referrers)); + } lists.erase(it); session.saveBgpConfig(); return fmt::format( diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp new file mode 100644 index 0000000000000..23bb9bcbbc32c --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp @@ -0,0 +1,146 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" +#include "fmt/format.h" + +#ifndef IS_OSS +#include +#else +#include +#endif + +namespace facebook::fboss { + +namespace { +// Every routing-policy term whose PREFIX_LIST match names `listName` in +// prefix_filters.prefix_list_names (the field bgpd resolves at config load), +// labelled the way the CLI addresses the term. +std::vector findTermsReferencingPrefixList( + const bgp::thrift::BgpConfig& cfg, + const std::string& listName) { + std::vector referrers; + if (!cfg.policies().has_value()) { + return referrers; + } + for (const auto& policy : *cfg.policies()->bgp_policy_statements()) { + for (const auto& term : *policy.policy_entries()) { + if (!term.policy_match_entries().has_value()) { + continue; + } + for (const auto& match : *term.policy_match_entries()->match_entries()) { + if (!match.prefix_filters().has_value()) { + continue; + } + const auto& names = *match.prefix_filters()->prefix_list_names(); + if (std::find(names.begin(), names.end(), listName) == names.end()) { + continue; + } + referrers.push_back( + term.sequence_number().has_value() + ? fmt::format( + "policy {} term {}", + *policy.name(), + *term.sequence_number()) + : fmt::format( + "policy {} term {}", *policy.name(), *term.name())); + } + } + } + return referrers; +} +} // namespace + +// Parse + validate at construction so queryClient stays a thin dispatch. +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"); + } + // Nothing may follow the name: there are no attributes to delete through + // this command, and entries are deleted through the entry subcommand. + if (v.size() > 1) { + throw std::invalid_argument( + fmt::format( + "Error: unexpected token '{}'. Usage: delete protocol bgp policy " + "prefix-list ", + v[1])); + } + listName_ = v[0]; +} + +CmdDeleteProtocolBgpPolicyPrefixListTraits::RetType +CmdDeleteProtocolBgpPolicyPrefixList::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + // Delete mirrors add: an absent target is already the requested end state, + // so this is a success with a warning. Nothing is saved, so a typo'd delete + // can't stage an unrelated session change. + auto absent = fmt::format( + "Warning: BGP prefix-list {} does not exist; nothing to delete", + args.listName()); + if (!cfg.policies().has_value()) { + return absent; + } + auto& lists = *cfg.policies()->prefix_lists(); + auto it = std::find_if(lists.begin(), lists.end(), [&](const auto& list) { + return *list.name() == args.listName(); + }); + if (it == lists.end()) { + return absent; + } + // 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. + auto referrers = findTermsReferencingPrefixList(cfg, args.listName()); + if (!referrers.empty()) { + return fmt::format( + "Error: BGP prefix-list {} is still referenced by {}; remove those " + "matches first", + args.listName(), + folly::join(", ", referrers)); + } + lists.erase(it); + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP prefix-list {}\nConfig saved to: {}", + args.listName(), + session.getBgpSessionConfigPath()); +} + +void CmdDeleteProtocolBgpPolicyPrefixList::printOutput(const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdDeleteProtocolBgpPolicyPrefixList, + CmdDeleteProtocolBgpPolicyPrefixListTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h new file mode 100644 index 0000000000000..06193e1604112 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "CLI/App.hpp" +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `delete protocol bgp policy prefix-list `, validated at +// construction. The list name is the only identity the config command +// stores at this level; deleting a single entry arrives with the entry +// subcommand (CmdConfigProtocolBgpPolicyPrefixListEntry), where the entry's +// identity is decided. +class BgpPrefixListRef : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpPrefixListRef(std::vector v); + const std::string& listName() const { + return listName_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::string listName_; +}; + +struct CmdDeleteProtocolBgpPolicyPrefixListTraits : public WriteCommandTraits { + using ParentCmd = CmdDeleteProtocolBgpPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("args", args, ""); + } + using ObjectArgType = BgpPrefixListRef; + using RetType = std::string; +}; + +class CmdDeleteProtocolBgpPolicyPrefixList + : public CmdHandler< + CmdDeleteProtocolBgpPolicyPrefixList, + CmdDeleteProtocolBgpPolicyPrefixListTraits> { + public: + using ObjectArgType = + CmdDeleteProtocolBgpPolicyPrefixListTraits::ObjectArgType; + using RetType = CmdDeleteProtocolBgpPolicyPrefixListTraits::RetType; + + RetType queryClient(const HostInfo& hostInfo, const ObjectArgType& args); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/BUCK b/fboss/cli/fboss2/test/config/BUCK index 3267f3eed9a45..c74372c510f28 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -17,6 +17,7 @@ cpp_unittest( "CmdConfigBgpPeerGroupTest.cpp", "CmdConfigBgpPolicyAsPathListTest.cpp", "CmdConfigBgpPolicyCommunityListTest.cpp", + "CmdConfigBgpPolicyPrefixListTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", "CmdConfigDhcpTest.cpp", @@ -56,6 +57,7 @@ cpp_unittest( "CmdDeleteBgpPeerGroupTest.cpp", "CmdDeleteBgpPolicyAsPathListTest.cpp", "CmdDeleteBgpPolicyCommunityListTest.cpp", + "CmdDeleteBgpPolicyPrefixListTest.cpp", "CmdDeleteConfigInterfaceTest.cpp", "CmdDeleteCoppQueueTest.cpp", "CmdDeleteCoppReasonTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp index 6bc17b32ffc9d..80c45b1ac9370 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp @@ -3,6 +3,8 @@ #include #include +#include +#include #include #include #include @@ -54,6 +56,44 @@ class CmdConfigBgpPolicyCommunityListTestFixture : public CmdConfigTestBase { .community_lists(); } + // Seed a routing-policy term whose COMMUNITY match names `listName`, the + // way bgpd reads the reference. Read the inline filter back via filterOf(): + // the vectors may reallocate on the next seed. + void addPolicyTermMatching( + const std::string& policy, + int64_t seq, + const std::string& listName) { + auto& cfg = ConfigSession::getInstance().getBgpConfig(); + auto& policies = *cfg.policies().ensure().bgp_policy_statements(); + policies.emplace_back(); + policies.back().name() = policy; + auto& terms = *policies.back().policy_entries(); + terms.emplace_back(); + terms.back().sequence_number() = seq; + auto& matches = + *terms.back().policy_match_entries().ensure().match_entries(); + matches.emplace_back(); + matches.back().type() = + bgp::bgp_policy::BgpPolicyAtomicMatchType::COMMUNITY_LIST; + matches.back().communities_filter().ensure().community_list_names() = { + listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + + const bgp::bgp_policy::CommunityList& filterOf(const std::string& policy) { + auto& policies = *ConfigSession::getInstance() + .getBgpConfig() + .policies() + ->bgp_policy_statements(); + auto it = std::find_if(policies.begin(), policies.end(), [&](auto& p) { + return *p.name() == policy; + }); + return *(*(*it->policy_entries())[0] + .policy_match_entries() + ->match_entries())[0] + .communities_filter(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -263,4 +303,41 @@ TEST_F( HasSubstr("requires ")); } +// ============================================================================== +// Reference sync — bgpd reads the operator and exact_match from the term's +// inline copy of the list +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyCommunityListTestFixture, + setBooleanOperatorSyncsReferencingFilters) { + run({"CL100"}); + run({"CL200"}); + addPolicyTermMatching("RM100", 10, "CL100"); + addPolicyTermMatching("RM200", 20, "CL200"); + + run({"CL100", "boolean-operator", "AND"}); + EXPECT_EQ(*filterOf("RM100").boolean_operator(), BooleanOperator::AND); + // A filter naming another list is untouched. + EXPECT_EQ(*filterOf("RM200").boolean_operator(), BooleanOperator::OR); + + run({"CL100", "boolean-operator", "OR"}); + EXPECT_EQ(*filterOf("RM100").boolean_operator(), BooleanOperator::OR); +} + +TEST_F( + CmdConfigBgpPolicyCommunityListTestFixture, + setExactMatchSyncsReferencingFilters) { + run({"CL100"}); + addPolicyTermMatching("RM100", 10, "CL100"); + EXPECT_FALSE(filterOf("RM100").exact_match().has_value()); + + run({"CL100", "exact-match", "true"}); + ASSERT_TRUE(filterOf("RM100").exact_match().has_value()); + EXPECT_TRUE(*filterOf("RM100").exact_match()); + + run({"CL100", "exact-match", "false"}); + EXPECT_FALSE(*filterOf("RM100").exact_match()); +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp new file mode 100644 index 0000000000000..66e3a64f4f717 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp @@ -0,0 +1,204 @@ +// (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/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +using namespace ::testing; +using facebook::bgp::routing_policy::BooleanOperator; + +namespace facebook::fboss { + +// The prefix-list dispatcher only touches the BGP side of ConfigSession, +// which seeds from thrift schema defaults when neither a staged session nor a +// system bgpcpp.conf exists — so no seed agent config is needed (mirrors +// CmdConfigBgpPolicyCommunityListTest). +class CmdConfigBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { + public: + CmdConfigBgpPolicyPrefixListTestFixture() + : CmdConfigTestBase("bgp_prefix_list_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string run(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyPrefixList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPrefixListConfig(tokens)); + } + + const std::vector& lists() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .prefix_lists(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpPrefixListConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, argValidation) { + // Bare create list. + auto bareList = BgpPrefixListConfig({"PL100"}); + EXPECT_EQ(bareList.listName(), "PL100"); + EXPECT_TRUE(bareList.attr().empty()); + + // List-level attribute. + auto listAttr = BgpPrefixListConfig({"PL100", "description", "my", "list"}); + EXPECT_EQ(listAttr.attr(), "description"); + EXPECT_EQ(listAttr.values(), std::vector({"my", "list"})); + + // Invalid: empty, empty name, unknown attribute. + EXPECT_THROW(BgpPrefixListConfig({}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListConfig({""}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListConfig({"PL100", "no-such-attr", "1"}), + std::invalid_argument); + // base-prefix is an entry attribute, not a list attribute. + EXPECT_THROW( + BgpPrefixListConfig({"PL100", "base-prefix", "10.0.0.0/8"}), + std::invalid_argument); + // compare-operator is not offered: bgpd rejects a list-level + // compare_operator ("Unsupported PrefixList configuration"). + EXPECT_THROW( + BgpPrefixListConfig({"PL100", "compare-operator", "GE"}), + std::invalid_argument); +} + +// ============================================================================== +// List-level handlers +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, bareCreateList) { + auto result = run({"PL100"}); + EXPECT_THAT(result, HasSubstr("Successfully created BGP prefix-list PL100")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); + EXPECT_TRUE(lists()[0].prefixes()->empty()); + EXPECT_TRUE(sessionFileExists()); + + run({"PL200"}); + ASSERT_EQ(lists().size(), 2); + EXPECT_EQ(*lists()[1].name(), "PL200"); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setListDescription) { + auto result = run({"PL100", "description", "spine", "prefixes"}); + EXPECT_THAT(result, HasSubstr("Successfully set description")); + ASSERT_EQ(lists().size(), 1); + // Multi-token description is re-joined. + EXPECT_EQ(*lists()[0].description(), "spine prefixes"); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setBooleanOperator) { + auto result = run({"PL100", "boolean-operator", "OR"}); + EXPECT_THAT(result, HasSubstr("Successfully set boolean-operator")); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); + + // bgpd: "PrefixList BooleanOperator can only be OR". AND and NOT are + // refused and leave the stored value untouched. + for (const auto* op : {"AND", "NOT"}) { + auto rejected = run({"PL100", "boolean-operator", op}); + EXPECT_THAT( + rejected, + HasSubstr( + fmt::format( + "Invalid boolean-operator value '{}'; expected OR", op))); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); + } +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, booleanOperatorDefaultIsOr) { + run({"PL100"}); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, compareOperatorNeverSet) { + // Nothing at the list level writes compare_operator; bgpd rejects the + // field whenever it is present. + run({"PL100", "description", "x"}); + run({"PL100", "boolean-operator", "OR"}); + run({"PL100", "ip-version", "v4"}); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].compare_operator().has_value()); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, setIpVersion) { + auto result = run({"PL100", "ip-version", "v4"}); + EXPECT_THAT(result, HasSubstr("Successfully set ip-version")); + ASSERT_TRUE(lists()[0].version().has_value()); + EXPECT_EQ(*lists()[0].version(), 4); + + run({"PL100", "ip-version", "v6"}); + EXPECT_EQ(*lists()[0].version(), 6); +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, namedListsAreDistinct) { + run({"PL100", "description", "one"}); + run({"PL200", "description", "two"}); + ASSERT_EQ(lists().size(), 2); + // Re-referencing an existing list by name updates it, not appends. + run({"PL100", "description", "one-updated"}); + ASSERT_EQ(lists().size(), 2); + EXPECT_EQ(*lists()[0].description(), "one-updated"); +} + +// ============================================================================== +// Entry-level handlers +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, reReferenceReportsExisting) { + run({"PL100"}); + // A second bare reference must not claim to have created it again. + EXPECT_THAT(run({"PL100"}), HasSubstr("already exists")); + ASSERT_EQ(lists().size(), 1); +} + +// ============================================================================== +// Reject paths — the error is surfaced and nothing is persisted +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyPrefixListTestFixture, + invalidBooleanOperatorRejected) { + auto result = run({"PL100", "boolean-operator", "XOR"}); + EXPECT_THAT( + result, HasSubstr("Invalid boolean-operator value 'XOR'; expected OR")); + // The rejected value must not leave a phantom list behind. + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpPolicyPrefixListTestFixture, invalidIpVersionRejected) { + auto result = run({"PL100", "ip-version", "v5"}); + EXPECT_THAT( + result, HasSubstr("Invalid ip-version value 'v5'; expected v4|v6")); + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp index 05cce49b00cea..3e3c39b63f56c 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp @@ -3,7 +3,10 @@ #include #include +#include +#include #include +#include #include #include #include @@ -60,6 +63,56 @@ class CmdDeleteBgpPolicyCommunityListTestFixture : public CmdConfigTestBase { .community_lists(); } + // Seed a routing-policy term whose COMMUNITY match names `listName` (the + // term/match CLIs live in higher PRs), the way bgpd reads the reference: + // communities_filter.community_list_names. + void addPolicyTermMatching( + const std::string& policy, + int64_t seq, + const std::string& listName) { + auto& term = addTerm(policy, seq); + auto& matches = *term.policy_match_entries().ensure().match_entries(); + matches.emplace_back(); + matches.back().type() = + bgp::bgp_policy::BgpPolicyAtomicMatchType::COMMUNITY_LIST; + matches.back().communities_filter().ensure().community_list_names() = { + listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + + // Seed a term whose community action names `listName` + // (community_action.community_action_list_names). + void addPolicyTermAction( + const std::string& policy, + int64_t seq, + const std::string& listName) { + auto& term = addTerm(policy, seq); + auto& actions = *term.policy_action_entries(); + actions.emplace_back(); + actions.back().community_action().ensure().community_action_list_names() = { + listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + + bgp::bgp_policy::BgpPolicyTerm& addTerm( + const std::string& policy, + int64_t seq) { + auto& cfg = ConfigSession::getInstance().getBgpConfig(); + auto& policies = *cfg.policies().ensure().bgp_policy_statements(); + auto it = std::find_if(policies.begin(), policies.end(), [&](auto& p) { + return *p.name() == policy; + }); + if (it == policies.end()) { + policies.emplace_back(); + policies.back().name() = policy; + it = std::prev(policies.end()); + } + auto& terms = *it->policy_entries(); + terms.emplace_back(); + terms.back().sequence_number() = seq; + return terms.back(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -251,4 +304,78 @@ TEST_F( << "session file should not exist after a no-op delete"; } +// ============================================================================== +// Reference guard — a list a routing-policy term still names is not deletable +// ============================================================================== + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteReferencedListRejected) { + configure({"CL100", "description", "in-use"}); + addPolicyTermMatching("RM100", 10, "CL100"); + + auto result = del({"CL100"}); + EXPECT_THAT(result, HasSubstr("still referenced")); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].description(), "in-use"); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteActionReferencedListRejected) { + configure({"CL100", "community", "65000:100"}); + addPolicyTermAction("RM100", 10, "CL100"); + + auto result = del({"CL100"}); + EXPECT_THAT(result, HasSubstr("still referenced")); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + ASSERT_EQ(lists().size(), 1); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteLastCommunityOnActionReferencedListRejected) { + // bgpd: "Missing communities in CommunityList" for an action reference + // to a valueless list. + configure({"CL100", "community", "65000:100"}); + configure({"CL100", "community", "65000:200"}); + addPolicyTermAction("RM100", 10, "CL100"); + + EXPECT_THAT(del({"CL100", "community", "65000:100"}), HasSubstr("Success")); + auto result = del({"CL100", "community", "65000:200"}); + EXPECT_THAT(result, HasSubstr("is the last community")); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].communities(), std::vector({"65000:200"})); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteLastCommunityOnMatchReferencedListAllowed) { + // A match reference tolerates a valueless list. + configure({"CL100", "community", "65000:100"}); + addPolicyTermMatching("RM100", 10, "CL100"); + + auto result = del({"CL100", "community", "65000:100"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted")); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].communities().has_value()); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + referenceToOtherListDoesNotBlockDelete) { + configure({"CL100"}); + configure({"CL200"}); + addPolicyTermMatching("RM100", 10, "CL100"); + addPolicyTermAction("RM100", 20, "CL100"); + + auto result = del({"CL200"}); + EXPECT_THAT( + result, HasSubstr("Successfully deleted BGP community-list CL200")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "CL100"); +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp new file mode 100644 index 0000000000000..d1040069b4352 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp @@ -0,0 +1,214 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#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/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +using namespace ::testing; + +namespace facebook::fboss { + +// Deleting only touches the BGP side of ConfigSession, which seeds from thrift +// schema defaults when neither a staged session nor a system bgpcpp.conf +// exists — so no seed agent config is needed (mirrors +// CmdDeleteBgpPolicyCommunityListTest). +class CmdDeleteBgpPolicyPrefixListTestFixture : public CmdConfigTestBase { + public: + CmdDeleteBgpPolicyPrefixListTestFixture() + : CmdConfigTestBase("bgp_prefix_list_delete_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string configure(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyPrefixList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPrefixListConfig(tokens)); + } + + std::string del(const std::vector& tokens) { + CmdDeleteProtocolBgpPolicyPrefixList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpPrefixListRef(tokens)); + } + + const std::vector& lists() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .prefix_lists(); + } + + // Seed a routing-policy term whose PREFIX_LIST match names `listName` (the + // term/match CLIs live in higher PRs), the way bgpd reads the reference: + // prefix_filters.prefix_list_names. + void addPolicyTermMatching( + const std::string& policy, + int64_t seq, + const std::string& listName) { + auto& cfg = ConfigSession::getInstance().getBgpConfig(); + auto& policies = *cfg.policies().ensure().bgp_policy_statements(); + auto it = std::find_if(policies.begin(), policies.end(), [&](auto& p) { + return *p.name() == policy; + }); + if (it == policies.end()) { + policies.emplace_back(); + policies.back().name() = policy; + it = std::prev(policies.end()); + } + auto& terms = *it->policy_entries(); + terms.emplace_back(); + terms.back().sequence_number() = seq; + auto& matches = + *terms.back().policy_match_entries().ensure().match_entries(); + matches.emplace_back(); + matches.back().type() = + bgp::bgp_policy::BgpPolicyAtomicMatchType::PREFIX_LIST; + matches.back().prefix_filters().ensure().prefix_list_names() = {listName}; + ConfigSession::getInstance().saveBgpConfig(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpPrefixListRef (arg) validation +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, argValidation) { + auto listOnly = BgpPrefixListRef({"PL100"}); + EXPECT_EQ(listOnly.listName(), "PL100"); + + // Invalid: empty, empty name, any second token (entries are deleted + // through the entry subcommand, not here). + EXPECT_THROW(BgpPrefixListRef({}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListRef({""}), std::invalid_argument); + EXPECT_THROW(BgpPrefixListRef({"PL100", "PL200"}), std::invalid_argument); + EXPECT_THROW( + BgpPrefixListRef({"PL100", "entry", "10"}), std::invalid_argument); +} + +// ============================================================================== +// queryClient: whole-list deletion +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteExistingList) { + configure({"PL100", "description", "drop"}); + configure({"PL200", "description", "keep"}); + ASSERT_EQ(lists().size(), 2); + + auto result = del({"PL100"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted BGP prefix-list PL100")); + // The other list survives untouched. + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL200"); + EXPECT_EQ(*lists()[0].description(), "keep"); + EXPECT_TRUE(sessionFileExists()); +} + +// Delete mirrors add: an absent target is a success with a warning, never an +// error, so a replayed script stays idempotent. +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteUnknownListWarns) { + auto result = del({"NO-SUCH-LIST"}); + EXPECT_THAT( + result, + HasSubstr( + "Warning: BGP prefix-list NO-SUCH-LIST does not exist; nothing to " + "delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); + // Nothing changed, so nothing is staged. + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after a no-op delete"; +} + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteTwiceIsIdempotent) { + configure({"PL100", "description", "delete-me"}); + EXPECT_THAT(del({"PL100"}), HasSubstr("Successfully deleted")); + EXPECT_TRUE(lists().empty()); + + auto again = del({"PL100"}); + EXPECT_THAT( + again, + HasSubstr( + "Warning: BGP prefix-list PL100 does not exist; nothing to delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); + EXPECT_TRUE(lists().empty()); +} + +TEST_F( + CmdDeleteBgpPolicyPrefixListTestFixture, + deleteUnknownLeavesOthersIntact) { + configure({"PL100", "description", "one"}); + auto result = del({"PL200"}); + EXPECT_THAT(result, HasSubstr("does not exist")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); +} + +// ============================================================================== +// Reference guard — a list a routing-policy term still names is not deletable +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyPrefixListTestFixture, deleteReferencedListRejected) { + configure({"PL100", "description", "in-use"}); + addPolicyTermMatching("RM100", 10, "PL100"); + + auto result = del({"PL100"}); + EXPECT_THAT(result, HasSubstr("still referenced")); + // The refusal names the policy and term so the user can act on it. + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + EXPECT_THAT(result, HasSubstr("remove those matches first")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); + EXPECT_EQ(*lists()[0].description(), "in-use"); +} + +TEST_F( + CmdDeleteBgpPolicyPrefixListTestFixture, + deleteReferencedListNamesEveryReferrer) { + configure({"PL100"}); + addPolicyTermMatching("RM100", 10, "PL100"); + addPolicyTermMatching("RM200", 20, "PL100"); + + auto result = del({"PL100"}); + EXPECT_THAT(result, HasSubstr("policy RM100 term 10")); + EXPECT_THAT(result, HasSubstr("policy RM200 term 20")); + ASSERT_EQ(lists().size(), 1); +} + +TEST_F( + CmdDeleteBgpPolicyPrefixListTestFixture, + referenceToOtherListDoesNotBlockDelete) { + configure({"PL100"}); + configure({"PL200"}); + // A term referencing PL100 must not block deleting PL200. + addPolicyTermMatching("RM100", 10, "PL100"); + + auto result = del({"PL200"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted BGP prefix-list PL200")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "PL100"); +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 358dedfb6fc65..01a74ad36e009 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -27,6 +27,7 @@ cpp_binary( "ConfigBgpPeerGroupTest.cpp", "ConfigBgpPolicyAsPathListTest.cpp", "ConfigBgpPolicyCommunityListTest.cpp", + "ConfigBgpPolicyPrefixListTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", "ConfigCoppTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp new file mode 100644 index 0000000000000..aefb8a9845807 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp @@ -0,0 +1,127 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy prefix-list + * [ ...]`. + * + * Scope: the prefix-list level (its entries are covered by + * ConfigBgpPolicyPrefixListEntryTest in the entry subcommand's PR). Every test + * stages the change AND commits it, then asserts the value landed at the + * correct thrift field path inside the matching .policies.prefix_lists[] entry + * of bgpd's running config (via getRunningConfig RPC) — which also confirms + * bgpd accepts and adopts a `.policies` blob at all. Stage-only behavior + * (attribute parsing, validation, rejection) is covered by the unit tests; an + * integration test that never commits exercises no daemon. + * + * Requirements: + * - The fboss2-dev binary under test (config subcommand tree). + * - HOME is set (the session file lives under $HOME/.fboss2). + * - bgpd is installed/active (commit restarts it). + */ + +#include +#include +#include + +#include "fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h" +#include "folly/json/dynamic.h" +#include "gmock/gmock.h" + +using namespace facebook::fboss; +using ::testing::HasSubstr; +using ::testing::Not; + +namespace { +// Test-only prefix-list name, unlikely to collide with a real list in the +// device's running BGP config. +const std::string kList = "FBOSS2-TEST-PREFIXES"; +} // namespace + +class ConfigBgpPolicyPrefixListTest : public ConfigBgpTestBase { + protected: + // Stage `config protocol bgp policy prefix-list ` WITHOUT + // clearing the staged session, so attributes can accumulate across + // invocations. Returns the staged session JSON. + folly::dynamic stagePrefixList(const std::vector& tokens) { + std::vector args = { + "config", "protocol", "bgp", "policy", "prefix-list"}; + args.insert(args.end(), tokens.begin(), tokens.end()); + auto result = runCli(args); + EXPECT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + EXPECT_THAT(result.stdout, Not(HasSubstr("Error:"))); + return readBgpSessionConfig(); + } + + // The .policies.prefix_lists[] entry named `name`, or nullptr. + static const folly::dynamic* findList( + const folly::dynamic& config, + const std::string& name) { + if (config.count("policies") == 0 || + config["policies"].count("prefix_lists") == 0) { + return nullptr; + } + for (const auto& list : config["policies"]["prefix_lists"]) { + if (list.count("name") && list["name"].asString() == name) { + return &list; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyPrefixListTest, SetListAttributesAndCommit) { + discardSession(); + clearBgpSession(); + stagePrefixList({kList, "description", "test", "spine", "prefixes"}); + stagePrefixList({kList, "boolean-operator", "OR"}); + stagePrefixList({kList, "ip-version", "v4"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + // Two layers: the promoted file proves what the commit wrote; the daemon's + // getRunningConfig RPC proves bgpd parsed and adopted the .policies blob. + auto config = readSystemBgpConfig(); + const auto* list = findList(config, kList); + ASSERT_NE(list, nullptr) << "committed config has no prefix-list " << kList; + EXPECT_EQ((*list)["description"].asString(), "test spine prefixes"); + + auto running = readRunningBgpConfigViaRpc(); + const auto* runningList = findList(running, kList); + ASSERT_NE(runningList, nullptr) + << "bgpd's running config has no prefix-list " << kList; + EXPECT_EQ((*runningList)["description"].asString(), "test spine prefixes"); + // Enums ride the SimpleJSON wire format as integers: + // routing_policy.BooleanOperator.OR = 2. bgpd only validates list + // operators from PrefixTreeMatch::PopulateReferences, i.e. once a + // routing-policy term references the list, so an end-to-end rejection + // test belongs with the term match command. + EXPECT_EQ((*runningList)["boolean_operator"].asInt(), 2); + EXPECT_EQ((*runningList)["version"].asInt(), 4); +} + +TEST_F(ConfigBgpPolicyPrefixListTest, DeleteListAndCommit) { + // Land a prefix-list in the system config, then delete it through a second + // commit and verify it is gone from bgpd's running config. + discardSession(); + clearBgpSession(); + stagePrefixList({kList, "description", "delete-me"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + ASSERT_NE(findList(readRunningBgpConfigViaRpc(), kList), nullptr) + << "setup commit did not land the prefix-list in bgpd's running config"; + + clearBgpSession(); + auto result = + runCli({"delete", "protocol", "bgp", "policy", "prefix-list", kList}); + EXPECT_THAT(result.stdout, HasSubstr("Successfully deleted")); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after delete commit; state=" + << bgpDaemonActiveState(); + EXPECT_EQ(findList(readRunningBgpConfigViaRpc(), kList), nullptr) + << "deleted prefix-list still present in bgpd's running config"; +} diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 30fc6e9c0a472..a23d5d24b0b72 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -424,17 +424,26 @@ def generate_peer_group_commands(peer_group: dict[str, Any]) -> list[str]: def _warning(text: str) -> str: - return f"{_WARNING_PREFIX} {text}" + # Warning text interpolates JSON-sourced names and values; a control + # character in one would end the `#` comment and become shell input. + return f"{_WARNING_PREFIX} {_printable(text)}" _BOOLEAN_OPERATOR_NAMES = {1: "AND", 2: "OR", 3: "NOT"} def _boolean_operator_name(raw: Any) -> str: - """routing_policy.BooleanOperator as its name, from the int or the name.""" + """routing_policy.BooleanOperator as its name, from the int or the name. + + Not a validator: an unrecognised value is returned as-is so the CLI + rejects it on replay with its own message. + """ if isinstance(raw, str): return raw - return _BOOLEAN_OPERATOR_NAMES.get(int(raw), str(raw)) + try: + return _BOOLEAN_OPERATOR_NAMES.get(int(raw), str(raw)) + except (TypeError, ValueError): + return str(raw) def generate_as_path_list_commands(as_path_list: dict[str, Any]) -> list[str]: @@ -543,6 +552,110 @@ def generate_community_list_commands(community_list: dict[str, Any]) -> list[str return commands +_COMPARISON_OPERATOR_NAMES = { + 1: "EQ", + 2: "GE", + 3: "LE", + 4: "NE", + 5: "GT", + 6: "LT", + 7: "RG", +} +_IP_VERSION_KEYWORDS = {4: "v4", 6: "v6"} + + +def _comparison_operator_name(raw: Any) -> str: + """routing_policy.ComparisonOperator as its name; see _boolean_operator_name.""" + if isinstance(raw, str): + return raw + try: + return _COMPARISON_OPERATOR_NAMES.get(int(raw), str(raw)) + except (TypeError, ValueError): + return str(raw) + + +def _prefix_list_warnings(name: str, prefix_list: dict[str, Any]) -> list[str]: + """Warnings for PrefixList fields that have no CLI spelling.""" + warnings = [] + if prefix_list.get("prefix_list_names"): + warnings.append( + _warning( + f"prefix-list {name}: prefix_list_names has no CLI equivalent; " + "not emitted" + ) + ) + if "ip_version" in prefix_list: + warnings.append( + _warning( + f"prefix-list {name}: ip_version has no CLI equivalent (the CLI " + "writes `version`); not emitted" + ) + ) + return warnings + + +def generate_prefix_list_commands(prefix_list: dict[str, Any]) -> list[str]: + """Generate `config protocol bgp policy prefix-list` commands for one list. + + `ip-version` is the CLI spelling of the `version` field (4/6). bgpd + accepts only `boolean_operator` OR and no list-level `compare_operator` + (PrefixTreeMatch::validateAndCreatePrefixTree), and the CLI offers nothing + else, so other values surface as warnings rather than commands. The + `prefix_list_names` references, the `ip_version` enum field and (until the + entry subcommand lands) `prefixes` have no CLI spelling and also warn. + """ + name = prefix_list.get("name", "") + if not name: + return [] + + prefix = f"config protocol bgp policy prefix-list {escape_shell_arg(name)}" + commands = [] + if prefix_list.get("description"): + commands.append( + f"{prefix} description {escape_shell_arg(prefix_list['description'])}" + ) + if "boolean_operator" in prefix_list: + operator = _boolean_operator_name(prefix_list["boolean_operator"]) + if operator != "OR": + commands.append( + _warning( + f"prefix-list {name}: boolean_operator {operator} is not " + "accepted by bgpd (only OR); not emitted" + ) + ) + if "compare_operator" in prefix_list: + operator = _comparison_operator_name(prefix_list["compare_operator"]) + commands.append( + _warning( + f"prefix-list {name}: list-level compare_operator {operator} is " + "not accepted by bgpd; not emitted" + ) + ) + if "version" in prefix_list: + keyword = _IP_VERSION_KEYWORDS.get(prefix_list["version"]) + if keyword is None: + commands.append( + _warning( + f"prefix-list {name}: version {prefix_list['version']} is " + "neither 4 nor 6; not emitted" + ) + ) + else: + commands.append(f"{prefix} ip-version {keyword}") + if prefix_list.get("prefixes"): + commands.append( + _warning( + f"prefix-list {name}: {len(prefix_list['prefixes'])} prefixes " + "are not emitted until the entry subcommand lands" + ) + ) + commands.extend(_prefix_list_warnings(name, prefix_list)) + if not commands: + # Nothing to set: still recreate the (empty) prefix-list by name. + commands.append(prefix) + return commands + + def generate_policy_commands(config: dict[str, Any]) -> list[str]: """Generate the `config protocol bgp policy ...` commands. @@ -556,6 +669,8 @@ def generate_policy_commands(config: dict[str, Any]) -> list[str]: commands.extend(generate_as_path_list_commands(as_path_list)) for community_list in policies.get("community_lists", []): commands.extend(generate_community_list_commands(community_list)) + for prefix_list in policies.get("prefix_lists", []): + commands.extend(generate_prefix_list_commands(prefix_list)) return commands diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index c1c240e7f9e6b..d0f091594e29b 100644 --- a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py @@ -29,6 +29,7 @@ generate_peer_commands, generate_peer_group_commands, generate_policy_commands, + generate_prefix_list_commands, json_to_cli, ) @@ -1046,6 +1047,16 @@ def test_boolean_operator_not_warns(self) -> None: self.assertTrue(commands[0].startswith("# WARNING:"), commands[0]) self.assertIn("NOT", commands[0]) + def test_malformed_boolean_operator_passes_through(self) -> None: + """j2c does not validate enum values; the CLI rejects them on replay.""" + for raw, rendered in ((None, "None"), ([1], "'[1]'")): + commands = generate_as_path_list_commands( + {"name": "ASPL", "boolean_operator": raw} + ) + self.assertEqual( + commands, [f"{self.PREFIX} boolean-operator {rendered}"], raw + ) + def test_dead_fields_warn(self) -> None: """Fields bgpd never reads surface as warnings, not silently dropped.""" commands = generate_as_path_list_commands( @@ -1184,6 +1195,133 @@ def test_policy_block_lists_after_as_path_lists(self) -> None: ) +class GeneratePrefixListCommandsTest(unittest.TestCase): + """Tests for generate_prefix_list_commands (prefix-list grammar).""" + + PREFIX = "config protocol bgp policy prefix-list PL" + + def test_empty_name_returns_empty(self) -> None: + self.assertEqual(generate_prefix_list_commands({"version": 4}), []) + + def test_bare_list_is_recreated(self) -> None: + self.assertEqual(generate_prefix_list_commands({"name": "PL"}), [self.PREFIX]) + + def test_scalar_attributes(self) -> None: + commands = generate_prefix_list_commands( + { + "name": "PL", + "description": "loopbacks", + "boolean_operator": "AND", + "compare_operator": 2, + "version": 6, + } + ) + self.assertEqual( + commands, + [ + f"{self.PREFIX} description loopbacks", + "# WARNING: prefix-list PL: boolean_operator AND is not accepted " + "by bgpd (only OR); not emitted", + "# WARNING: prefix-list PL: list-level compare_operator GE is " + "not accepted by bgpd; not emitted", + f"{self.PREFIX} ip-version v6", + ], + ) + + def test_boolean_operator_not_or_warns(self) -> None: + """bgpd: "PrefixList BooleanOperator can only be OR".""" + for raw, rendered in ((1, "AND"), (3, "NOT"), ("NOT", "NOT")): + commands = generate_prefix_list_commands( + {"name": "PL", "boolean_operator": raw} + ) + self.assertEqual( + commands, + [ + f"# WARNING: prefix-list PL: boolean_operator {rendered} is " + "not accepted by bgpd (only OR); not emitted" + ], + raw, + ) + + def test_prefixes_warn_until_entry_subcommand(self) -> None: + commands = generate_prefix_list_commands( + { + "name": "PL", + "prefixes": [{"base_prefix": "10.0.0.0/8"}, {"base_prefix": "::/0"}], + } + ) + self.assertEqual( + commands, + [ + "# WARNING: prefix-list PL: 2 prefixes are not emitted until " + "the entry subcommand lands" + ], + ) + + def test_or_default_omitted(self) -> None: + self.assertEqual( + generate_prefix_list_commands({"name": "PL", "boolean_operator": 2}), + [self.PREFIX], + ) + + def test_unexpressible_values_warn(self) -> None: + commands = generate_prefix_list_commands( + { + "name": "PL", + "compare_operator": 7, + "version": 5, + "prefix_list_names": ["X"], + "ip_version": 1, + } + ) + self.assertEqual(len(commands), 4) + for c in commands: + self.assertTrue(c.startswith("# WARNING:"), c) + + def test_any_compare_operator_warns(self) -> None: + """bgpd rejects a list-level compare_operator whatever its value.""" + for raw, rendered in ((2, "GE"), (7, "RG"), (None, "None"), ([2], "[2]")): + commands = generate_prefix_list_commands( + {"name": "PL", "compare_operator": raw} + ) + self.assertEqual( + commands, + [ + f"# WARNING: prefix-list PL: list-level compare_operator " + f"{rendered} is not accepted by bgpd; not emitted" + ], + raw, + ) + + def test_warning_text_is_single_comment_line(self) -> None: + """A control character in a JSON name must not end the # comment.""" + commands = generate_prefix_list_commands( + {"name": "PL\nrm -rf /", "prefix_list_names": ["X"]} + ) + self.assertEqual(len(commands), 1) + self.assertTrue(commands[0].startswith("# WARNING:"), commands[0]) + self.assertNotIn("\n", commands[0]) + self.assertIn("PL?rm -rf /", commands[0]) + + def test_policy_block_order(self) -> None: + config = { + "policies": { + "aspath_lists": [{"name": "A"}], + "community_lists": [{"name": "CL"}], + "prefix_lists": [{"name": "PL"}], + } + } + commands = generate_policy_commands(config) + self.assertEqual( + commands, + [ + "config protocol bgp policy as-path-list A", + "config protocol bgp policy community-list CL", + "config protocol bgp policy prefix-list PL", + ], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands."""