From 9a086cb1cd102681e5b53d7db41b3286b892f4f4 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 28 Jul 2026 06:26:32 +0000 Subject: [PATCH 1/4] [Nexthop] NOS-6649..NOS-6656: fboss2 config/delete protocol bgp policy community-list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `config protocol bgp policy community-list [community ] [ ...]` and `delete protocol bgp policy community-list ` — the second BGP policy object-type command family, beneath the `policy` grouping node alongside as-path-list. - community-list dispatcher with a two-level key (list name + inline community member name) writing bgp_policy.BgpPolicies.community_lists[] through the typed ConfigSession; boolean-operator maps to routing_policy.BooleanOperator, exact-match to the optional bool, and the member attributes (description/type/value) set the CommunityRefType union's inline Community arm, keyed by Community.name. - new boolAttr factory in the shared BgpCliAttrHandlers.h (generalizing the neighbor dispatcher's), reused by upcoming prefix-list. - fix a latent dangling-string_view in the shared enumAttr factory: the lambda captured a string_view over the caller's fmt::format temporary, so every invalid-enum-value rejection printed garbage after "expected". valueDesc is now taken (and captured) by value; the as-path-list and community-list rejection tests pin the full message. - unit tests (19) + commit-path integration tests mirroring ConfigBgpPolicyAsPathListTest (not yet run on a DUT). Co-Authored-By: Claude Fable 5 --- cmake/CliFboss2.cmake | 5 + cmake/CliFboss2TestConfig.cmake | 2 + cmake/CliFboss2TestIntegrationTest.cmake | 1 + fboss/cli/fboss2/BUCK | 5 + fboss/cli/fboss2/CmdListConfig.cpp | 52 ++-- .../config/protocol/bgp/BgpCliValueParsers.h | 52 ++++ .../bgp/policy/CmdConfigProtocolBgpPolicy.h | 9 +- .../community-list/BgpCommunityListCliUtils.h | 90 +++++++ ...mdConfigProtocolBgpPolicyCommunityList.cpp | 204 ++++++++++++++++ .../CmdConfigProtocolBgpPolicyCommunityList.h | 86 +++++++ ...mdDeleteProtocolBgpPolicyCommunityList.cpp | 138 +++++++++++ .../CmdDeleteProtocolBgpPolicyCommunityList.h | 74 ++++++ fboss/cli/fboss2/test/config/BUCK | 2 + .../CmdConfigBgpPolicyCommunityListTest.cpp | 181 ++++++++++++++ .../CmdDeleteBgpPolicyCommunityListTest.cpp | 223 ++++++++++++++++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + .../ConfigBgpPolicyCommunityListTest.cpp | 126 ++++++++++ 17 files changed, 1232 insertions(+), 19 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp create mode 100644 fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index b7dcfabcd1219..7550836221954 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -974,6 +974,9 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h + fboss/cli/fboss2/commands/config/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/ptp/CmdConfigPtp.cpp fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp @@ -1114,6 +1117,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.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 375d53ecc1c93..f9226bda5a1be 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -15,6 +15,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigBgpNeighborTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPeerGroupTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyAsPathListTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp @@ -53,6 +54,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdDeleteBgpNeighborTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPeerGroupTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyAsPathListTest.cpp + fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.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 b011edd5a7230..7bfff504c46a8 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -19,6 +19,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigBgpNeighborTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPeerGroupTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyAsPathListTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.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 6d5b26c79fa69..6bf9e425317a9 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1184,6 +1184,7 @@ cpp_library( "commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.cpp", "commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.cpp", "commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.cpp", + "commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1253,6 +1254,7 @@ cpp_library( "commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.cpp", "commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.cpp", "commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.cpp", + "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp", "commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp", "commands/delete/qos/CmdDeleteQos.cpp", @@ -1330,6 +1332,8 @@ cpp_library( "commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h", "commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h", "commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h", + "commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h", + "commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", @@ -1400,6 +1404,7 @@ cpp_library( "commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h", "commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h", "commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h", + "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.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 efe8dc24e2feb..0728bc61c9f67 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -44,6 +44,7 @@ #include "fboss/cli/fboss2/commands/config/protocol/bgp/peer-group/CmdConfigProtocolBgpPeerGroup.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/CmdConfigProtocolBgpPolicyAsPathList.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.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" @@ -110,6 +111,7 @@ #include "fboss/cli/fboss2/commands/delete/protocol/bgp/peer-group/CmdDeleteProtocolBgpPeerGroup.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/CmdDeleteProtocolBgpPolicy.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/as-path-list/CmdDeleteProtocolBgpPolicyAsPathList.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.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" @@ -423,15 +425,25 @@ const CommandTree& kConfigCommandTree() { commandHandler, argRegistrar, {{ - "as-path-list", - "Configure BGP AS-path list: " - "[ ...] (description, " - "regex, boolean-operator)", - commandHandler< - CmdConfigProtocolBgpPolicyAsPathList>, - argRegistrar< - CmdConfigProtocolBgpPolicyAsPathListTraits>, - }}, + "as-path-list", + "Configure BGP AS-path list: " + "[ ...] (description, " + "regex, boolean-operator)", + commandHandler< + CmdConfigProtocolBgpPolicyAsPathList>, + argRegistrar< + CmdConfigProtocolBgpPolicyAsPathListTraits>, + }, + { + "community-list", + "Configure BGP community-list: " + "[ ...] " + "(boolean-operator, description, exact-match)", + commandHandler< + CmdConfigProtocolBgpPolicyCommunityList>, + argRegistrar< + CmdConfigProtocolBgpPolicyCommunityListTraits>, + }}, }, }, }, @@ -781,13 +793,21 @@ const CommandTree& kConfigCommandTree() { commandHandler, argTypeHandler, {{ - "as-path-list", - "Delete a BGP AS-path list, or one of its regexes: " - " [regex ]", - commandHandler, - argRegistrar< - CmdDeleteProtocolBgpPolicyAsPathListTraits>, - }}, + "as-path-list", + "Delete a BGP AS-path list, or one of its regexes: " + " [regex ]", + commandHandler, + argRegistrar< + CmdDeleteProtocolBgpPolicyAsPathListTraits>, + }, + { + "community-list", + "Delete a BGP community-list: ", + commandHandler< + CmdDeleteProtocolBgpPolicyCommunityList>, + argRegistrar< + CmdDeleteProtocolBgpPolicyCommunityListTraits>, + }}, }}, }, { diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h index 6e80deec368b4..5d610e328cae5 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h @@ -10,13 +10,18 @@ #pragma once +#include #include +#include #include #include #include #include +#include #include +#include #include +#include /** * Value parsing helpers shared by the BGP config dispatchers @@ -93,4 +98,51 @@ inline std::optional parseAsn4Byte(const std::string& value) { return static_cast(*parsed); } +// ---- constructor-time token parsing ---------------------------------------- +// Unlike the value parsers above, this runs while constructing a command's +// ObjectArgType, where throwing std::invalid_argument IS the framework's +// error channel — the message is surfaced to the user as the parse error. + +// The ` [ ]` prefix shared by the policy +// list commands (config and delete): the list name, the optional named +// member selected by `keyword`, and where any remaining tokens begin. +struct ListMemberSelector { + std::string listName; + std::optional memberName; + // Index of the first token after the parsed prefix (== tokens.size() when + // nothing follows). The config dispatcher reads an ... + // tail from here; the delete dispatcher rejects any tail. + size_t restStart; +}; + +// Parse the shared prefix. `objectName` is the list flavor for messages +// (e.g. "community-list"), `memberKeyword` selects the nested member (e.g. +// `community`), and `usage` is the whole-command usage line thrown when no +// tokens were given. A second token other than `memberKeyword` is left to +// the caller (restStart == 1): the config grammar treats it as a list-level +// attribute, delete as an unexpected token. +inline ListMemberSelector parseListMemberSelector( + const std::vector& tokens, + std::string_view objectName, + std::string_view memberKeyword, + std::string_view usage) { + if (tokens.empty()) { + throw std::invalid_argument(std::string(usage)); + } + if (tokens[0].empty()) { + throw std::invalid_argument( + fmt::format("Error: {} name must not be empty", objectName)); + } + ListMemberSelector selector{tokens[0], std::nullopt, 1}; + if (tokens.size() > 1 && tokens[1] == memberKeyword) { + if (tokens.size() < 3 || tokens[2].empty()) { + throw std::invalid_argument( + fmt::format("Error: `{}` requires a ", memberKeyword)); + } + selector.memberName = tokens[2]; + selector.restStart = 3; + } + return selector; +} + } // namespace facebook::fboss::bgpcli 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 4830fe77f5545..6d0115fb5aa0f 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/CmdConfigProtocolBgpPolicy.h @@ -10,15 +10,18 @@ #pragma once +#include +#include #include "fboss/cli/fboss2/CmdHandler.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/CmdConfigProtocolBgp.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" namespace facebook::fboss { // Grouping node for `config protocol bgp policy ...`. Holds no -// state of its own; the per-object-type dispatchers (as-path-list, and later -// community-list/prefix-list/routing-policy) are its subcommands. Mirrors -// CmdConfigProtocolBgp. +// state of its own; the per-object-type dispatchers (as-path-list, +// community-list, and later prefix-list/routing-policy) are its subcommands. +// Mirrors CmdConfigProtocolBgp. struct CmdConfigProtocolBgpPolicyTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgp; using ObjectArgType = std::monostate; 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 new file mode 100644 index 0000000000000..5b6f5c60394d4 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" + +/** + * Lookup/create helpers for the community-list CLI family, shared between the + * list-level dispatcher (CmdConfigProtocolBgpPolicyCommunityList), the + * community subcommand (CmdConfigProtocolBgpPolicyCommunityListCommunity), and + * the delete counterparts. A CommunityList is keyed by name; the inline + * Community members it holds are keyed by their own name. + */ +namespace facebook::fboss::bgpcli { + +inline bool communityListExists( + const bgp::thrift::BgpConfig& cfg, + const std::string& name) { + if (!cfg.policies().has_value()) { + return false; + } + const auto& lists = *cfg.policies()->community_lists(); + return std::any_of(lists.begin(), lists.end(), [&](const auto& list) { + return *list.name() == name; + }); +} + +// Find the community-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 `community-list ` creates one +// explicitly. CommunityList's only key field is `name`. +inline bgp::bgp_policy::CommunityList& findOrCreateCommunityList( + bgp::thrift::BgpConfig& cfg, + const std::string& name) { + auto& lists = *cfg.policies().ensure().community_lists(); + for (auto& list : lists) { + if (*list.name() == name) { + return list; + } + } + lists.emplace_back(); + auto& list = lists.back(); + list.name() = name; + return list; +} + +inline bool communityMemberExists( + const bgp::bgp_policy::CommunityList& list, + const std::string& name) { + if (!list.members().has_value()) { + return false; + } + const auto& members = *list.members(); + return std::any_of(members.begin(), members.end(), [&](const auto& member) { + return member.community_ref().has_value() && + *member.community_ref()->name() == name; + }); +} + +// Find the inline Community member keyed by name within a list, creating it +// if absent. The CLI always defines members inline (the CommunityRefType +// union's `community` arm); the member's name is its identity. +inline bgp::bgp_policy::Community& findOrCreateCommunityMember( + bgp::bgp_policy::CommunityList& list, + const std::string& name) { + auto& members = list.members().ensure(); + for (auto& member : members) { + if (member.community_ref().has_value() && + *member.community_ref()->name() == name) { + return *member.community_ref(); + } + } + members.emplace_back(); + auto& community = members.back().set_community(); + community.name() = name; + return community; +} + +} // 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 new file mode 100644 index 0000000000000..fcf7f8dd73725 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.cpp @@ -0,0 +1,204 @@ +/* + * 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/community-list/CmdConfigProtocolBgpPolicyCommunityList.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/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" +#include "fmt/format.h" + +namespace facebook::fboss { + +namespace { + +// The attribute names, exactly as documented. Kept here so the +// valid-attribute set and the handler table stay in sync. The list's inline +// Community members are their own subcommand, not attributes. +constexpr std::string_view kBooleanOperator = "boolean-operator"; +constexpr std::string_view kDescription = "description"; +constexpr std::string_view kExactMatch = "exact-match"; + +// boolean-operator values (routing_policy.BooleanOperator names). +constexpr std::string_view kBooleanOperatorAnd = "AND"; +constexpr std::string_view kBooleanOperatorOr = "OR"; +constexpr std::string_view kBooleanOperatorNot = "NOT"; + +using BooleanOperator = bgp::routing_policy::BooleanOperator; +using CommunityList = bgp::bgp_policy::CommunityList; +using bgpcli::AttrHandler; +using bgpcli::boolAttr; +using bgpcli::enumAttr; +using bgpcli::joinedStringAttr; +using bgpcli::ok; +using bgpcli::Result; + +// ---- value lookups ---------------------------------------------------------- + +std::optional lookupBooleanOperator(const std::string& s) { + if (s == kBooleanOperatorAnd) { + return BooleanOperator::AND; + } + if (s == kBooleanOperatorOr) { + return BooleanOperator::OR; + } + if (s == kBooleanOperatorNot) { + return BooleanOperator::NOT; + } + return std::nullopt; +} + +// ---- 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(CommunityList& list, BooleanOperator op) { + list.boolean_operator() = op; +} + +void setListDescription(CommunityList& list, const std::string& description) { + list.description() = description; +} + +void setListExactMatch(CommunityList& list, bool exactMatch) { + list.exact_match() = exactMatch; +} + +// ---- list-level attribute registry ------------------------------------------ +// One line per documented attribute: its dispatch key, its value shape, and +// the setter that stores it. +const std::map, std::less<>>& +listAttrHandlers() { + static const std::string kBooleanOperatorValues = fmt::format( + "{}|{}|{}", kBooleanOperatorAnd, kBooleanOperatorOr, kBooleanOperatorNot); + static const std::map, std::less<>> + kHandlers = { + {std::string(kBooleanOperator), + enumAttr( + kBooleanOperator, + kBooleanOperatorValues, + lookupBooleanOperator, + setListBooleanOperator)}, + {std::string(kDescription), + joinedStringAttr(kDescription, setListDescription)}, + {std::string(kExactMatch), + boolAttr(kExactMatch, setListExactMatch)}, + }; + 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). +BgpCommunityListConfig::BgpCommunityListConfig(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: community-list is required, optionally followed by an " + " "); + } + if (v[0].empty()) { + throw std::invalid_argument("Error: community-list name must not be empty"); + } + listName_ = v[0]; + + if (v.size() == 1) { + return; // bare `community-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 community-list attribute '{}'. Valid attributes: " + "{}", + attr_, + validAttrList())); + } +} + +CmdConfigProtocolBgpPolicyCommunityListTraits::RetType +CmdConfigProtocolBgpPolicyCommunityList::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool listCreated = !bgpcli::communityListExists(cfg, args.listName()); + auto& list = bgpcli::findOrCreateCommunityList(cfg, args.listName()); + + Result result = args.attr().empty() + ? ok(listCreated + ? fmt::format( + "Successfully created BGP community-list {}", + args.listName()) + : fmt::format( + "BGP community-list {} already exists", args.listName())) + // The attribute is guaranteed valid: BgpCommunityListConfig'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 community-list {}", args.listName()); + } + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } else if (listCreated) { + // Drop the phantom list so a rejected value is not visible to later + // lookups in the same process. + cfg.policies()->community_lists()->pop_back(); + } + return result.message; +} + +void CmdConfigProtocolBgpPolicyCommunityList::printOutput( + const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdConfigProtocolBgpPolicyCommunityList, + CmdConfigProtocolBgpPolicyCommunityListTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h new file mode 100644 index 0000000000000..e8113a09cbe6d --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.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 `community-list [ ...]`, validated at +// construction. A CommunityList is keyed by . Its inline Community +// members are their own subcommand +// (CmdConfigProtocolBgpPolicyCommunityListCommunity), not attributes. +// Mirrors BgpAsPathListConfig. +// +// Grammar (from the FBOSS proposed syntax): +// community-list (create/select list) +// community-list boolean-operator (list attribute) +// community-list description (list attribute) +// community-list exact-match (list attribute) +class BgpCommunityListConfig : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpCommunityListConfig(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 community-list` +// family; dispatch and per-attribute parsing live in the .cpp so a new tunable +// is a one-entry change. Mirrors CmdConfigProtocolBgpPolicyAsPathList. +struct CmdConfigProtocolBgpPolicyCommunityListTraits + : 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 + // `community` token as this command's subcommand rather than swallowing it + // into args. See CmdConfigProtocolBgpPolicyAsPathListTraits. + cmd.add_option("args", args, " [ ...]"); + } + using ObjectArgType = BgpCommunityListConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyCommunityList + : public CmdHandler< + CmdConfigProtocolBgpPolicyCommunityList, + CmdConfigProtocolBgpPolicyCommunityListTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyCommunityListTraits::ObjectArgType; + using RetType = CmdConfigProtocolBgpPolicyCommunityListTraits::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 new file mode 100644 index 0000000000000..7788aa590c96f --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp @@ -0,0 +1,138 @@ +/* + * 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/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/session/ConfigSession.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" +#include "fmt/format.h" + +namespace facebook::fboss { + +namespace { +// CLI keyword selecting the nested community member, matching the config +// command's grammar. +constexpr std::string_view kObjectName = "community-list"; +constexpr std::string_view kCommunityKeyword = "community"; +} // namespace + +// Parse + validate at construction so queryClient stays a thin dispatch. +BgpCommunityListRef::BgpCommunityListRef(std::vector v) + : utils::BaseObjectArgType(v) { + auto selector = bgpcli::parseListMemberSelector( + v, + kObjectName, + kCommunityKeyword, + "Error: delete protocol bgp policy community-list requires , " + "optionally followed by `community `"); + // Unlike the config grammar, nothing may follow the parsed prefix: there + // are no attributes to delete through this command. + if (selector.restStart < v.size()) { + throw std::invalid_argument( + selector.memberName + ? fmt::format( + "Error: unexpected token '{}' after community ", + v[selector.restStart]) + : fmt::format( + "Error: unexpected token '{}'. Usage: delete protocol bgp " + "policy community-list [community ]", + v[selector.restStart])); + } + listName_ = std::move(selector.listName); + if (selector.memberName) { + hasCommunity_ = true; + communityName_ = std::move(*selector.memberName); + } +} + +CmdDeleteProtocolBgpPolicyCommunityListTraits::RetType +CmdDeleteProtocolBgpPolicyCommunityList::queryClient( + const HostInfo& /* hostInfo */, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + if (!cfg.policies().has_value()) { + // Nothing is persisted for an unknown list, so a typo'd delete can't stage + // an unrelated session change. + return fmt::format( + "Error: BGP community-list {} not found", args.listName()); + } + auto& lists = *cfg.policies()->community_lists(); + auto it = std::find_if(lists.begin(), lists.end(), [&](const auto& list) { + return *list.name() == args.listName(); + }); + if (it == lists.end()) { + return fmt::format( + "Error: BGP community-list {} not found", args.listName()); + } + if (args.hasCommunity()) { + // Delete a single inline member; the list itself stays. + auto& list = *it; + if (list.members().has_value()) { + auto& members = *list.members(); + auto memberIt = + std::find_if(members.begin(), members.end(), [&](const auto& member) { + return member.community_ref().has_value() && + *member.community_ref()->name() == args.communityName(); + }); + if (memberIt != members.end()) { + members.erase(memberIt); + if (members.empty()) { + // Leave the list looking like one that never had members, so a + // later `community ` add starts from the same shape. + list.members().reset(); + } + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP community-list {} community {}\n" + "Config saved to: {}", + args.listName(), + args.communityName(), + session.getBgpSessionConfigPath()); + } + } + return fmt::format( + "Error: BGP community-list {} community {} not found", + args.listName(), + args.communityName()); + } + lists.erase(it); + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP community-list {}\nConfig saved to: {}", + args.listName(), + session.getBgpSessionConfigPath()); +} + +void CmdDeleteProtocolBgpPolicyCommunityList::printOutput( + const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdDeleteProtocolBgpPolicyCommunityList, + CmdDeleteProtocolBgpPolicyCommunityListTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h new file mode 100644 index 0000000000000..127ea9d6e8e74 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h @@ -0,0 +1,74 @@ +/* + * 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 community-list +// [community ]`, validated at construction. The names are the list's +// and member's identity, matched exactly the same way the config command +// stores them. Without the `community` selector the whole list is deleted; +// with it, only that inline member. Mirrors BgpAsPathListRef. +class BgpCommunityListRef : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpCommunityListRef(std::vector v); + const std::string& listName() const { + return listName_; + } + bool hasCommunity() const { + return hasCommunity_; + } + const std::string& communityName() const { + return communityName_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::string listName_; + bool hasCommunity_{false}; + std::string communityName_; +}; + +struct CmdDeleteProtocolBgpPolicyCommunityListTraits + : public WriteCommandTraits { + using ParentCmd = CmdDeleteProtocolBgpPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("args", args, " [community ]"); + } + using ObjectArgType = BgpCommunityListRef; + using RetType = std::string; +}; + +class CmdDeleteProtocolBgpPolicyCommunityList + : public CmdHandler< + CmdDeleteProtocolBgpPolicyCommunityList, + CmdDeleteProtocolBgpPolicyCommunityListTraits> { + public: + using ObjectArgType = + CmdDeleteProtocolBgpPolicyCommunityListTraits::ObjectArgType; + using RetType = CmdDeleteProtocolBgpPolicyCommunityListTraits::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 f41a1e3e4bb4a..2299fdcd28117 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -16,6 +16,7 @@ cpp_unittest( "CmdConfigBgpNeighborTest.cpp", "CmdConfigBgpPeerGroupTest.cpp", "CmdConfigBgpPolicyAsPathListTest.cpp", + "CmdConfigBgpPolicyCommunityListTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", "CmdConfigDhcpTest.cpp", @@ -54,6 +55,7 @@ cpp_unittest( "CmdDeleteBgpNeighborTest.cpp", "CmdDeleteBgpPeerGroupTest.cpp", "CmdDeleteBgpPolicyAsPathListTest.cpp", + "CmdDeleteBgpPolicyCommunityListTest.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 new file mode 100644 index 0000000000000..186bce550c7ec --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp @@ -0,0 +1,181 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include + +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.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 community-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 +// CmdConfigBgpPolicyAsPathListTest). +class CmdConfigBgpPolicyCommunityListTestFixture : public CmdConfigTestBase { + public: + CmdConfigBgpPolicyCommunityListTestFixture() + : CmdConfigTestBase("bgp_community_list_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string run(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyCommunityList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpCommunityListConfig(tokens)); + } + + const std::vector& lists() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .community_lists(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpCommunityListConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, argValidation) { + // Bare create list. + auto bareList = BgpCommunityListConfig({"CL100"}); + EXPECT_EQ(bareList.listName(), "CL100"); + EXPECT_TRUE(bareList.attr().empty()); + + // List-level attribute. + auto listAttr = + BgpCommunityListConfig({"CL100", "description", "my", "list"}); + EXPECT_EQ(listAttr.attr(), "description"); + EXPECT_EQ(listAttr.values(), std::vector({"my", "list"})); + + // Invalid: empty, empty name, unknown attribute. + EXPECT_THROW(BgpCommunityListConfig({}), std::invalid_argument); + EXPECT_THROW(BgpCommunityListConfig({""}), std::invalid_argument); + EXPECT_THROW( + BgpCommunityListConfig({"CL100", "no-such-attr", "1"}), + std::invalid_argument); + // value is a community attribute, not a list attribute. + EXPECT_THROW( + BgpCommunityListConfig({"CL100", "value", "65000:100"}), + std::invalid_argument); +} + +// ============================================================================== +// List-level handlers +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, bareCreateList) { + auto result = run({"CL100"}); + EXPECT_THAT( + result, HasSubstr("Successfully created BGP community-list CL100")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "CL100"); + EXPECT_FALSE(lists()[0].members().has_value()); + EXPECT_TRUE(sessionFileExists()); + + run({"CL200"}); + ASSERT_EQ(lists().size(), 2); + EXPECT_EQ(*lists()[1].name(), "CL200"); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, setListDescription) { + auto result = run({"CL100", "description", "spine", "communities"}); + EXPECT_THAT(result, HasSubstr("Successfully set description")); + ASSERT_EQ(lists().size(), 1); + // Multi-token description is re-joined. + EXPECT_EQ(*lists()[0].description(), "spine communities"); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, setBooleanOperator) { + auto result = run({"CL100", "boolean-operator", "AND"}); + EXPECT_THAT(result, HasSubstr("Successfully set boolean-operator")); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::AND); + + run({"CL100", "boolean-operator", "NOT"}); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::NOT); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, booleanOperatorDefaultIsOr) { + run({"CL100"}); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, setExactMatch) { + auto result = run({"CL100", "exact-match", "true"}); + EXPECT_THAT(result, HasSubstr("Successfully enabled exact-match")); + ASSERT_TRUE(lists()[0].exact_match().has_value()); + EXPECT_TRUE(*lists()[0].exact_match()); + + auto disabled = run({"CL100", "exact-match", "false"}); + EXPECT_THAT(disabled, HasSubstr("Successfully disabled exact-match")); + EXPECT_FALSE(*lists()[0].exact_match()); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, namedListsAreDistinct) { + run({"CL100", "description", "one"}); + run({"CL200", "description", "two"}); + ASSERT_EQ(lists().size(), 2); + // Re-referencing an existing list by name updates it, not appends. + run({"CL100", "description", "one-updated"}); + ASSERT_EQ(lists().size(), 2); + EXPECT_EQ(*lists()[0].description(), "one-updated"); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, reReferenceReportsExisting) { + run({"CL100"}); + // A second bare reference must not claim to have created it again. + EXPECT_THAT(run({"CL100"}), HasSubstr("already exists")); + ASSERT_EQ(lists().size(), 1); +} + +// ============================================================================== +// Reject paths — the error is surfaced and nothing is persisted +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyCommunityListTestFixture, + invalidBooleanOperatorRejected) { + auto result = run({"CL100", "boolean-operator", "XOR"}); + // The full message matters: the accepted-values suffix once printed garbage + // (a string_view capture dangling over a fmt::format temporary). + EXPECT_THAT( + result, + HasSubstr("Invalid boolean-operator value 'XOR'; expected AND|OR|NOT")); + // The rejected value must not leave a phantom list behind. + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, invalidExactMatchRejected) { + auto result = run({"CL100", "exact-match", "maybe"}); + EXPECT_THAT(result, HasSubstr("Invalid")); + 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 new file mode 100644 index 0000000000000..23daff00d388f --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp @@ -0,0 +1,223 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include + +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.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 +// CmdDeleteBgpPolicyAsPathListTest). +class CmdDeleteBgpPolicyCommunityListTestFixture : public CmdConfigTestBase { + public: + CmdDeleteBgpPolicyCommunityListTestFixture() + : CmdConfigTestBase("bgp_community_list_delete_test_%%%%-%%%%-%%%%", "") { + } + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string configure(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyCommunityList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpCommunityListConfig(tokens)); + } + + // Seed an inline Community member. The member level is its own subcommand + // (CmdConfigProtocolBgpPolicyCommunityListCommunity), which lands above this + // command in the stack — so stage it through the shared helpers rather than + // depending on that handler. + void configureCommunity( + const std::string& listName, + const std::string& communityName, + const std::string& value) { + auto& session = ConfigSession::getInstance(); + auto& list = + bgpcli::findOrCreateCommunityList(session.getBgpConfig(), listName); + bgpcli::findOrCreateCommunityMember(list, communityName).value() = value; + session.saveBgpConfig(); + } + + std::string del(const std::vector& tokens) { + CmdDeleteProtocolBgpPolicyCommunityList cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpCommunityListRef(tokens)); + } + + const std::vector& lists() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .community_lists(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpCommunityListRef (arg) validation +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, argValidation) { + auto listOnly = BgpCommunityListRef({"CL100"}); + EXPECT_EQ(listOnly.listName(), "CL100"); + EXPECT_FALSE(listOnly.hasCommunity()); + + auto withMember = BgpCommunityListRef({"CL100", "community", "CM1"}); + EXPECT_EQ(withMember.listName(), "CL100"); + EXPECT_TRUE(withMember.hasCommunity()); + EXPECT_EQ(withMember.communityName(), "CM1"); + + // Invalid: empty, empty names, non-`community` second token, missing + // community name, extra tokens. + EXPECT_THROW(BgpCommunityListRef({}), std::invalid_argument); + EXPECT_THROW(BgpCommunityListRef({""}), std::invalid_argument); + EXPECT_THROW(BgpCommunityListRef({"CL100", "CL200"}), std::invalid_argument); + EXPECT_THROW( + BgpCommunityListRef({"CL100", "community"}), std::invalid_argument); + EXPECT_THROW( + BgpCommunityListRef({"CL100", "community", ""}), std::invalid_argument); + EXPECT_THROW( + BgpCommunityListRef({"CL100", "community", "CM1", "extra"}), + std::invalid_argument); +} + +// ============================================================================== +// queryClient +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteExistingList) { + configureCommunity("CL100", "CM1", "65000:100"); + configure({"CL200", "description", "keep"}); + ASSERT_EQ(lists().size(), 2); + + auto result = del({"CL100"}); + EXPECT_THAT( + result, HasSubstr("Successfully deleted BGP community-list CL100")); + // The other list survives untouched. + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "CL200"); + EXPECT_EQ(*lists()[0].description(), "keep"); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteUnknownListRejected) { + auto result = del({"NO-SUCH-LIST"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP community-list NO-SUCH-LIST not found")); + // Nothing was persisted for the failed delete. + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected delete"; +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteUnknownLeavesOthersIntact) { + configure({"CL100", "description", "one"}); + auto result = del({"CL200"}); + EXPECT_THAT(result, HasSubstr("not found")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "CL100"); +} + +// ============================================================================== +// queryClient: single community member deletion +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteExistingCommunity) { + configureCommunity("CL100", "CM1", "65000:100"); + configureCommunity("CL100", "CM2", "65000:200"); + + auto result = del({"CL100", "community", "CM1"}); + EXPECT_THAT( + result, + HasSubstr("Successfully deleted BGP community-list CL100 community CM1")); + // The list and its other member survive. + ASSERT_EQ(lists().size(), 1); + ASSERT_TRUE(lists()[0].members().has_value()); + ASSERT_EQ(lists()[0].members()->size(), 1); + EXPECT_EQ(*(*lists()[0].members())[0].community_ref()->name(), "CM2"); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteLastCommunityUnsetsMembers) { + configureCommunity("CL100", "CM1", "65000:100"); + + auto result = del({"CL100", "community", "CM1"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted")); + // The list stays, shaped like one that never had members. + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "CL100"); + EXPECT_FALSE(lists()[0].members().has_value()); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteUnknownCommunityRejected) { + configureCommunity("CL100", "CM1", "65000:100"); + // Consume the session file created by configure so we can assert the + // rejected delete does not persist anything new. + ASSERT_TRUE(sessionFileExists()); + + auto result = del({"CL100", "community", "NO-SUCH-COMMUNITY"}); + EXPECT_THAT( + result, + HasSubstr( + "Error: BGP community-list CL100 community NO-SUCH-COMMUNITY not " + "found")); + // The existing member is untouched. + ASSERT_EQ(lists().size(), 1); + ASSERT_TRUE(lists()[0].members().has_value()); + ASSERT_EQ(lists()[0].members()->size(), 1); + EXPECT_EQ(*(*lists()[0].members())[0].community_ref()->name(), "CM1"); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteCommunityFromMemberlessListRejected) { + configure({"CL100", "description", "no", "members"}); + + auto result = del({"CL100", "community", "CM1"}); + EXPECT_THAT( + result, + HasSubstr("Error: BGP community-list CL100 community CM1 not found")); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].members().has_value()); +} + +TEST_F( + CmdDeleteBgpPolicyCommunityListTestFixture, + deleteCommunityFromUnknownListRejected) { + auto result = del({"NO-SUCH-LIST", "community", "CM1"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP community-list NO-SUCH-LIST not found")); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected delete"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 474b742608a43..a588c56107a77 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -26,6 +26,7 @@ cpp_binary( "ConfigBgpNeighborTest.cpp", "ConfigBgpPeerGroupTest.cpp", "ConfigBgpPolicyAsPathListTest.cpp", + "ConfigBgpPolicyCommunityListTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", "ConfigCoppTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp new file mode 100644 index 0000000000000..7a30d7425dcc1 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp @@ -0,0 +1,126 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy community-list + * [ ...]`. + * + * Scope: the community-list level (its inline community members are covered by + * ConfigBgpPolicyCommunityListCommunityTest). Every test + * stages the change AND commits it, then asserts the value landed at the + * correct thrift field path inside the matching .policies.community_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 community-list name, unlikely to collide with a real list in the +// device's running BGP config. +const std::string kList = "FBOSS2-TEST-COMMUNITY"; +} // namespace + +class ConfigBgpPolicyCommunityListTest : public ConfigBgpTestBase { + protected: + // Stage `config protocol bgp policy community-list ` WITHOUT + // clearing the staged session, so attributes can accumulate across + // invocations. Returns the staged session JSON. + folly::dynamic stageCommunityList(const std::vector& tokens) { + std::vector args = { + "config", "protocol", "bgp", "policy", "community-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.community_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("community_lists") == 0) { + return nullptr; + } + for (const auto& list : config["policies"]["community_lists"]) { + if (list.count("name") && list["name"].asString() == name) { + return &list; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyCommunityListTest, SetListAttributesAndCommit) { + discardSession(); + clearBgpSession(); + stageCommunityList({kList, "description", "test", "spine", "communities"}); + stageCommunityList({kList, "boolean-operator", "AND"}); + stageCommunityList({kList, "exact-match", "true"}); + 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 community-list " + << kList; + EXPECT_EQ((*list)["description"].asString(), "test spine communities"); + + auto running = readRunningBgpConfigViaRpc(); + const auto* runningList = findList(running, kList); + ASSERT_NE(runningList, nullptr) + << "bgpd's running config has no community-list " << kList; + EXPECT_EQ((*runningList)["description"].asString(), "test spine communities"); + // Enums ride the SimpleJSON wire format as integers: + // routing_policy.BooleanOperator.AND = 1. + EXPECT_EQ((*runningList)["boolean_operator"].asInt(), 1); + EXPECT_TRUE((*runningList)["exact_match"].asBool()); +} + +TEST_F(ConfigBgpPolicyCommunityListTest, DeleteListAndCommit) { + // Land a community-list in the system config, then delete it through a + // second commit and verify it is gone from bgpd's running config. + discardSession(); + clearBgpSession(); + stageCommunityList({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 community-list in bgpd's running " + "config"; + + clearBgpSession(); + auto result = + runCli({"delete", "protocol", "bgp", "policy", "community-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 community-list still present in bgpd's running config"; +} From f50b2787de3d420098ecc1de9ee1d63927483c11 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:18:05 +0000 Subject: [PATCH 2/4] Emit community-list commands from bgp_json_to_cli.py Adds generate_community_list_commands (description, boolean-operator when not the OR default, exact-match) and hooks it into the policy block after the as-path-lists. The deprecated inline `communities` and `community_list_names` have no CLI spelling and are emitted as `# WARNING:` lines. Members follow in the community subcommand change. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 46 ++++++++++++ .../tools/tests/test_bgp_json_to_cli.py | 73 +++++++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 71ed0f5d8f607..007e5b697c29e 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -488,6 +488,50 @@ def generate_as_path_list_commands(as_path_list: dict[str, Any]) -> list[str]: return commands +def generate_community_list_commands(community_list: dict[str, Any]) -> list[str]: + """Generate `config protocol bgp policy community-list` commands for one list. + + The deprecated inline `communities` and the `community_list_names` + references have no CLI spelling; they surface as warnings. + """ + name = community_list.get("name", "") + if not name: + return [] + + prefix = f"config protocol bgp policy community-list {escape_shell_arg(name)}" + commands = [] + if community_list.get("description"): + commands.append( + f"{prefix} description {escape_shell_arg(community_list['description'])}" + ) + if "boolean_operator" in community_list: + operator = _boolean_operator_name(community_list["boolean_operator"]) + if operator != "OR": + commands.append(f"{prefix} boolean-operator {escape_shell_arg(operator)}") + if "exact_match" in community_list: + commands.append( + f"{prefix} exact-match {_shell_bool(community_list['exact_match'])}" + ) + if community_list.get("communities"): + commands.append( + _warning( + f"community-list {name}: inline `communities` has no CLI " + "equivalent (use members); not emitted" + ) + ) + if community_list.get("community_list_names"): + commands.append( + _warning( + f"community-list {name}: community_list_names has no CLI " + "equivalent; not emitted" + ) + ) + if not commands: + # Nothing to set: still recreate the (empty) community-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. @@ -499,6 +543,8 @@ def generate_policy_commands(config: dict[str, Any]) -> list[str]: commands = [] for as_path_list in policies.get("aspath_lists", []): 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)) 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 0ced31be80c67..c3d9eb9449d2d 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 @@ -23,6 +23,7 @@ format_bandwidth, generate_as_path_list_commands, generate_commands, + generate_community_list_commands, generate_exec_commands, generate_global_commands, generate_peer_commands, @@ -1081,6 +1082,78 @@ def test_no_policies_key(self) -> None: self.assertEqual(generate_policy_commands({}), []) +class GenerateCommunityListCommandsTest(unittest.TestCase): + """Tests for generate_community_list_commands (community-list grammar).""" + + PREFIX = "config protocol bgp policy community-list CL" + + def test_empty_name_returns_empty(self) -> None: + self.assertEqual(generate_community_list_commands({"exact_match": True}), []) + + def test_bare_list_is_recreated(self) -> None: + self.assertEqual( + generate_community_list_commands({"name": "CL"}), [self.PREFIX] + ) + + def test_scalar_attributes(self) -> None: + commands = generate_community_list_commands( + { + "name": "CL", + "description": "transit communities", + "boolean_operator": 1, + "exact_match": True, + } + ) + self.assertEqual( + commands, + [ + f"{self.PREFIX} description 'transit communities'", + f"{self.PREFIX} boolean-operator AND", + f"{self.PREFIX} exact-match true", + ], + ) + + def test_boolean_operator_or_omitted_not_kept(self) -> None: + self.assertEqual( + generate_community_list_commands({"name": "CL", "boolean_operator": "OR"}), + [self.PREFIX], + ) + self.assertEqual( + generate_community_list_commands({"name": "CL", "boolean_operator": 3}), + [f"{self.PREFIX} boolean-operator NOT"], + ) + + def test_exact_match_false_preserved(self) -> None: + self.assertEqual( + generate_community_list_commands({"name": "CL", "exact_match": False}), + [f"{self.PREFIX} exact-match false"], + ) + + def test_dead_fields_warn(self) -> None: + commands = generate_community_list_commands( + {"name": "CL", "communities": ["65000:1"], "community_list_names": ["X"]} + ) + self.assertEqual(len(commands), 2) + for c in commands: + self.assertTrue(c.startswith("# WARNING:"), c) + + def test_policy_block_lists_after_as_path_lists(self) -> None: + config = { + "policies": { + "aspath_lists": [{"name": "A"}], + "community_lists": [{"name": "CL"}], + } + } + commands = generate_policy_commands(config) + self.assertEqual( + commands, + [ + "config protocol bgp policy as-path-list A", + "config protocol bgp policy community-list CL", + ], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands.""" From beb31f1f9f7f509a5b438f4e35ea2d0ee9e3dbb1 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 16 Sep 2026 07:44:29 +0000 Subject: [PATCH 3/4] Flatten community-list onto the field bgpd reads; make its delete idempotent bgpd matches on the flat `communities` string list and never reads the structured `members[]`, so a community configured through a nested member would be inert. Add a flat `community ` attribute that appends to `communities` (append-if-absent, one value per invocation), and let `delete ... community-list community ` remove one by value. bgpd compiles a non-literal value as a regex at load, so a value is compiled here too and a malformed one never reaches the session. The member-keyed helpers are dead; drop them. Drop `boolean-operator NOT`: bgpd treats every operator other than OR as "all must match", so NOT silently meant AND. Offer AND|OR only. Delete mirrors add: deleting an absent list or an absent value is a success with a `Warning:` and saves nothing, so a replayed script stays idempotent. j2c emits one `community` line per value, warns on NOT and on the dead `members`/`community_list_names` fields instead of dropping them. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/CmdListConfig.cpp | 3 +- .../config/protocol/bgp/BgpCliValueParsers.h | 25 ++- .../community-list/BgpCommunityListCliUtils.h | 40 +---- ...mdConfigProtocolBgpPolicyCommunityList.cpp | 64 ++++++- .../CmdConfigProtocolBgpPolicyCommunityList.h | 24 +-- ...mdDeleteProtocolBgpPolicyCommunityList.cpp | 98 +++++------ .../CmdDeleteProtocolBgpPolicyCommunityList.h | 16 +- .../CmdConfigBgpPolicyCommunityListTest.cpp | 92 +++++++++- .../CmdDeleteBgpPolicyCommunityListTest.cpp | 162 ++++++++++-------- .../ConfigBgpPolicyCommunityListTest.cpp | 48 +++++- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 29 +++- .../tools/tests/test_bgp_json_to_cli.py | 40 ++++- 12 files changed, 429 insertions(+), 212 deletions(-) diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 0728bc61c9f67..5d3fcd0a64bd2 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -438,7 +438,8 @@ const CommandTree& kConfigCommandTree() { "community-list", "Configure BGP community-list: " "[ ...] " - "(boolean-operator, description, exact-match)", + "(boolean-operator, community, description, " + "exact-match)", commandHandler< CmdConfigProtocolBgpPolicyCommunityList>, argRegistrar< diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h index 5d610e328cae5..081c770e970ac 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h @@ -103,24 +103,31 @@ inline std::optional parseAsn4Byte(const std::string& value) { // ObjectArgType, where throwing std::invalid_argument IS the framework's // error channel — the message is surfaced to the user as the parse error. -// The ` [ ]` prefix shared by the policy -// list commands (config and delete): the list name, the optional named -// member selected by `keyword`, and where any remaining tokens begin. +// The ` [ ]` prefix, for the *delete* +// command of a policy list whose members are keyed objects (prefix-list +// entries): the list name, the optional member selected by `keyword`, and +// where any remaining tokens begin. +// +// Only delete parses this shape by hand. The config side reaches the same +// grammar through CLI11 subcommand nesting instead — the member keyword is a +// registered subcommand there, so each level gets its own already-split token +// vector and never calls this helper. Lists whose members are a flat string +// list in the daemon (as-path-list `regex`, community-list `community`) have +// no member level at all: their optional ` ` selector is a +// plain attribute-style pair, parsed inline by their own delete command. struct ListMemberSelector { std::string listName; std::optional memberName; // Index of the first token after the parsed prefix (== tokens.size() when - // nothing follows). The config dispatcher reads an ... - // tail from here; the delete dispatcher rejects any tail. + // nothing follows). The delete dispatcher rejects any tail. size_t restStart; }; -// Parse the shared prefix. `objectName` is the list flavor for messages -// (e.g. "community-list"), `memberKeyword` selects the nested member (e.g. +// Parse the prefix. `objectName` is the list flavor for messages (e.g. +// "community-list"), `memberKeyword` selects the nested member (e.g. // `community`), and `usage` is the whole-command usage line thrown when no // tokens were given. A second token other than `memberKeyword` is left to -// the caller (restStart == 1): the config grammar treats it as a list-level -// attribute, delete as an unexpected token. +// the caller (restStart == 1), which delete rejects as an unexpected token. inline ListMemberSelector parseListMemberSelector( const std::vector& tokens, std::string_view objectName, 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 5b6f5c60394d4..b7299a901beed 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 @@ -17,10 +17,10 @@ /** * Lookup/create helpers for the community-list CLI family, shared between the - * list-level dispatcher (CmdConfigProtocolBgpPolicyCommunityList), the - * community subcommand (CmdConfigProtocolBgpPolicyCommunityListCommunity), and - * the delete counterparts. A CommunityList is keyed by name; the inline - * Community members it holds are keyed by their own name. + * list-level dispatcher (CmdConfigProtocolBgpPolicyCommunityList) and its + * delete counterpart. A CommunityList is keyed by name. Its values live in the + * flat `communities` string list, which is the field bgpd matches against; the + * structured members[] are never read by bgpd and have no CLI. */ namespace facebook::fboss::bgpcli { @@ -55,36 +55,4 @@ inline bgp::bgp_policy::CommunityList& findOrCreateCommunityList( return list; } -inline bool communityMemberExists( - const bgp::bgp_policy::CommunityList& list, - const std::string& name) { - if (!list.members().has_value()) { - return false; - } - const auto& members = *list.members(); - return std::any_of(members.begin(), members.end(), [&](const auto& member) { - return member.community_ref().has_value() && - *member.community_ref()->name() == name; - }); -} - -// Find the inline Community member keyed by name within a list, creating it -// if absent. The CLI always defines members inline (the CommunityRefType -// union's `community` arm); the member's name is its identity. -inline bgp::bgp_policy::Community& findOrCreateCommunityMember( - bgp::bgp_policy::CommunityList& list, - const std::string& name) { - auto& members = list.members().ensure(); - for (auto& member : members) { - if (member.community_ref().has_value() && - *member.community_ref()->name() == name) { - return *member.community_ref(); - } - } - members.emplace_back(); - auto& community = members.back().set_community(); - community.name() = name; - return community; -} - } // 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 fcf7f8dd73725..ceba3c4c5dcb2 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 @@ -12,8 +12,11 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" +#include // NOLINT(misc-include-cleaner) #include #include +#include +#include #include #include #include @@ -37,28 +40,34 @@ 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 inline -// Community members are their own subcommand, not attributes. +// valid-attribute set and the handler table stay in sync. Values are flat +// `community` attributes because bgpd stores them as a flat string list; +// nested entries are reserved for families where the daemon stores objects +// (prefix-list, routing-policy). constexpr std::string_view kBooleanOperator = "boolean-operator"; +constexpr std::string_view kCommunity = "community"; constexpr std::string_view kDescription = "description"; constexpr std::string_view kExactMatch = "exact-match"; // boolean-operator values (routing_policy.BooleanOperator names). constexpr std::string_view kBooleanOperatorAnd = "AND"; constexpr std::string_view kBooleanOperatorOr = "OR"; -constexpr std::string_view kBooleanOperatorNot = "NOT"; using BooleanOperator = bgp::routing_policy::BooleanOperator; using CommunityList = bgp::bgp_policy::CommunityList; using bgpcli::AttrHandler; using bgpcli::boolAttr; using bgpcli::enumAttr; +using bgpcli::err; using bgpcli::joinedStringAttr; using bgpcli::ok; using bgpcli::Result; +using bgpcli::Tokens; // ---- value lookups ---------------------------------------------------------- +// NOT is deliberately not offered: bgpd treats every operator other than OR +// as "all must match", so NOT would silently behave as AND. std::optional lookupBooleanOperator(const std::string& s) { if (s == kBooleanOperatorAnd) { return BooleanOperator::AND; @@ -66,9 +75,6 @@ std::optional lookupBooleanOperator(const std::string& s) { if (s == kBooleanOperatorOr) { return BooleanOperator::OR; } - if (s == kBooleanOperatorNot) { - return BooleanOperator::NOT; - } return std::nullopt; } @@ -88,13 +94,54 @@ void setListExactMatch(CommunityList& list, bool exactMatch) { list.exact_match() = exactMatch; } +// Hand-written because no factory covers an accumulating, validated set: +// each `community` appends one value to communities (the field bgpd matches +// against). bgpd accepts either a literal `:` or a regex, and +// compiles anything that is not a literal as a regex at load, so compile it +// here too and keep a pattern bgpd would reject out of the session. +// Idempotent on repeats. +Result community(CommunityList& list, const Tokens& values) { + if (values.size() != 1 || values[0].empty()) { + return err( + fmt::format( + "Error: {} requires (one token; a literal such as " + "65000:100 or a regex)", + kCommunity)); + } + const auto& value = values[0]; + if (std::any_of(value.begin(), value.end(), [](unsigned char c) { + return std::isspace(c) != 0; + })) { + return err( + fmt::format( + "Error: {} '{}' must not contain whitespace; repeat the command " + "once per community", + kCommunity, + value)); + } + try { + boost::regex compiled(value); + } catch (const boost::regex_error& e) { + return err( + fmt::format( + "Error: Malformed {} '{}': {}", kCommunity, value, e.what())); + } + auto& communities = list.communities().ensure(); + if (std::find(communities.begin(), communities.end(), value) != + communities.end()) { + return ok(fmt::format("{} {} already present", kCommunity, value)); + } + communities.push_back(value); + return ok(fmt::format("Successfully added {} {}", kCommunity, value)); +} + // ---- list-level attribute registry ------------------------------------------ // One line per documented attribute: its dispatch key, its value shape, and // the setter that stores it. const std::map, std::less<>>& listAttrHandlers() { - static const std::string kBooleanOperatorValues = fmt::format( - "{}|{}|{}", kBooleanOperatorAnd, kBooleanOperatorOr, kBooleanOperatorNot); + static const std::string kBooleanOperatorValues = + fmt::format("{}|{}", kBooleanOperatorAnd, kBooleanOperatorOr); static const std::map, std::less<>> kHandlers = { {std::string(kBooleanOperator), @@ -103,6 +150,7 @@ listAttrHandlers() { kBooleanOperatorValues, lookupBooleanOperator, setListBooleanOperator)}, + {std::string(kCommunity), community}, {std::string(kDescription), joinedStringAttr(kDescription, setListDescription)}, {std::string(kExactMatch), diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h index e8113a09cbe6d..4a61d2e195d25 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h @@ -21,16 +21,18 @@ namespace facebook::fboss { // Parsed `community-list [ ...]`, validated at -// construction. A CommunityList is keyed by . Its inline Community -// members are their own subcommand -// (CmdConfigProtocolBgpPolicyCommunityListCommunity), not attributes. -// Mirrors BgpAsPathListConfig. +// construction. A CommunityList is keyed by . Its values are a flat +// string list in the daemon, so they are flat `community` attributes here +// (one per invocation), not nested entries. Mirrors BgpAsPathListConfig. // // Grammar (from the FBOSS proposed syntax): -// community-list (create/select list) -// community-list boolean-operator (list attribute) -// community-list description (list attribute) -// community-list exact-match (list attribute) +// community-list (create/select list) +// community-list boolean-operator (list attribute) +// community-list community (append to communities, +// the field bgpd matches +// on) +// community-list description (list attribute) +// community-list exact-match (list attribute) class BgpCommunityListConfig : public utils::BaseObjectArgType { public: // NOLINTNEXTLINE(google-explicit-constructor) @@ -60,9 +62,9 @@ struct CmdConfigProtocolBgpPolicyCommunityListTraits : 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 - // `community` token as this command's subcommand rather than swallowing it - // into args. See CmdConfigProtocolBgpPolicyAsPathListTraits. + // Stops CLI11 from classifying attribute tokens as subcommands once the + // list name is consumed. See CmdConfigProtocolBgpNeighborTraits. + cmd.positionals_at_end(); cmd.add_option("args", args, " [ ...]"); } using ObjectArgType = BgpCommunityListConfig; 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 7788aa590c96f..92322498ceec4 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 @@ -20,9 +20,7 @@ #include #include #include -#include #include -#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" @@ -31,39 +29,41 @@ namespace facebook::fboss { namespace { -// CLI keyword selecting the nested community member, matching the config -// command's grammar. -constexpr std::string_view kObjectName = "community-list"; constexpr std::string_view kCommunityKeyword = "community"; } // namespace // Parse + validate at construction so queryClient stays a thin dispatch. BgpCommunityListRef::BgpCommunityListRef(std::vector v) : utils::BaseObjectArgType(v) { - auto selector = bgpcli::parseListMemberSelector( - v, - kObjectName, - kCommunityKeyword, - "Error: delete protocol bgp policy community-list requires , " - "optionally followed by `community `"); - // Unlike the config grammar, nothing may follow the parsed prefix: there - // are no attributes to delete through this command. - if (selector.restStart < v.size()) { + if (v.empty()) { throw std::invalid_argument( - selector.memberName - ? fmt::format( - "Error: unexpected token '{}' after community ", - v[selector.restStart]) - : fmt::format( - "Error: unexpected token '{}'. Usage: delete protocol bgp " - "policy community-list [community ]", - v[selector.restStart])); + "Error: delete protocol bgp policy community-list requires , " + "optionally followed by `community `"); } - listName_ = std::move(selector.listName); - if (selector.memberName) { - hasCommunity_ = true; - communityName_ = std::move(*selector.memberName); + if (v[0].empty()) { + throw std::invalid_argument("Error: community-list name must not be empty"); } + listName_ = v[0]; + if (v.size() == 1) { + return; + } + if (v[1] != kCommunityKeyword) { + throw std::invalid_argument( + fmt::format( + "Error: unexpected token '{}'. Usage: delete protocol bgp policy " + "community-list [community ]", + v[1])); + } + if (v.size() < 3 || v[2].empty()) { + throw std::invalid_argument("Error: `community` requires a "); + } + if (v.size() > 3) { + throw std::invalid_argument( + fmt::format( + "Error: unexpected token '{}' after community ", v[3])); + } + hasCommunity_ = true; + community_ = v[2]; } CmdDeleteProtocolBgpPolicyCommunityListTraits::RetType @@ -72,50 +72,50 @@ CmdDeleteProtocolBgpPolicyCommunityList::queryClient( const ObjectArgType& args) { auto& session = ConfigSession::getInstance(); auto& cfg = session.getBgpConfig(); + // Delete mirrors add: an absent target is already the requested end state, + // so this is a success with a warning. Nothing is saved, so a typo'd delete + // can't stage an unrelated session change. + auto absent = fmt::format( + "Warning: BGP community-list {} does not exist; nothing to delete", + args.listName()); if (!cfg.policies().has_value()) { - // Nothing is persisted for an unknown list, so a typo'd delete can't stage - // an unrelated session change. - return fmt::format( - "Error: BGP community-list {} not found", args.listName()); + return absent; } auto& lists = *cfg.policies()->community_lists(); auto it = std::find_if(lists.begin(), lists.end(), [&](const auto& list) { return *list.name() == args.listName(); }); if (it == lists.end()) { - return fmt::format( - "Error: BGP community-list {} not found", args.listName()); + return absent; } if (args.hasCommunity()) { - // Delete a single inline member; the list itself stays. + // Remove one value from communities; the list itself stays. auto& list = *it; - if (list.members().has_value()) { - auto& members = *list.members(); - auto memberIt = - std::find_if(members.begin(), members.end(), [&](const auto& member) { - return member.community_ref().has_value() && - *member.community_ref()->name() == args.communityName(); - }); - if (memberIt != members.end()) { - members.erase(memberIt); - if (members.empty()) { - // Leave the list looking like one that never had members, so a - // later `community ` add starts from the same shape. - list.members().reset(); + if (list.communities().has_value()) { + auto& communities = *list.communities(); + auto valueIt = + std::find(communities.begin(), communities.end(), args.community()); + if (valueIt != communities.end()) { + communities.erase(valueIt); + if (communities.empty()) { + // Leave the list looking like one that never had values, so a + // later `community ` add starts from the same shape. + list.communities().reset(); } session.saveBgpConfig(); return fmt::format( "Successfully deleted BGP community-list {} community {}\n" "Config saved to: {}", args.listName(), - args.communityName(), + args.community(), session.getBgpSessionConfigPath()); } } return fmt::format( - "Error: BGP community-list {} community {} not found", + "Warning: BGP community-list {} has no community {}; nothing to " + "delete", args.listName(), - args.communityName()); + args.community()); } lists.erase(it); session.saveBgpConfig(); diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h index 127ea9d6e8e74..bf95887a04a25 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h @@ -21,10 +21,10 @@ namespace facebook::fboss { // Parsed `delete protocol bgp policy community-list -// [community ]`, validated at construction. The names are the list's -// and member's identity, matched exactly the same way the config command -// stores them. Without the `community` selector the whole list is deleted; -// with it, only that inline member. Mirrors BgpAsPathListRef. +// [community ]`, validated at construction. The name is the +// list's identity, matched exactly the same way the config command stores +// it; the optional `community` selector removes one communities value +// instead of the whole list. Mirrors BgpAsPathListRef. class BgpCommunityListRef : public utils::BaseObjectArgType { public: // NOLINTNEXTLINE(google-explicit-constructor) @@ -35,8 +35,8 @@ class BgpCommunityListRef : public utils::BaseObjectArgType { bool hasCommunity() const { return hasCommunity_; } - const std::string& communityName() const { - return communityName_; + const std::string& community() const { + return community_; } const static utils::ObjectArgTypeId id = utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; @@ -44,14 +44,14 @@ class BgpCommunityListRef : public utils::BaseObjectArgType { private: std::string listName_; bool hasCommunity_{false}; - std::string communityName_; + std::string community_; }; struct CmdDeleteProtocolBgpPolicyCommunityListTraits : public WriteCommandTraits { using ParentCmd = CmdDeleteProtocolBgpPolicy; static void addCliArg(CLI::App& cmd, std::vector& args) { - cmd.add_option("args", args, " [community ]"); + cmd.add_option("args", args, " [community ]"); } using ObjectArgType = BgpCommunityListRef; using RetType = std::string; diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp index 186bce550c7ec..4991950c51d1e 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp @@ -77,10 +77,19 @@ TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, argValidation) { EXPECT_THROW( BgpCommunityListConfig({"CL100", "no-such-attr", "1"}), std::invalid_argument); - // value is a community attribute, not a list attribute. + // value was the removed community subcommand's attribute; bgpd never read + // the members it wrote, so it must not be accepted anywhere. EXPECT_THROW( BgpCommunityListConfig({"CL100", "value", "65000:100"}), std::invalid_argument); + + // List-level community and boolean-operator are recognized. + EXPECT_EQ( + BgpCommunityListConfig({"CL100", "community", "65000:100"}).attr(), + "community"); + EXPECT_EQ( + BgpCommunityListConfig({"CL100", "boolean-operator", "AND"}).attr(), + "boolean-operator"); } // ============================================================================== @@ -93,7 +102,7 @@ TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, bareCreateList) { result, HasSubstr("Successfully created BGP community-list CL100")); ASSERT_EQ(lists().size(), 1); EXPECT_EQ(*lists()[0].name(), "CL100"); - EXPECT_FALSE(lists()[0].members().has_value()); + EXPECT_FALSE(lists()[0].communities().has_value()); EXPECT_TRUE(sessionFileExists()); run({"CL200"}); @@ -111,11 +120,19 @@ TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, setListDescription) { TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, setBooleanOperator) { auto result = run({"CL100", "boolean-operator", "AND"}); - EXPECT_THAT(result, HasSubstr("Successfully set boolean-operator")); + EXPECT_THAT(result, HasSubstr("Successfully set boolean-operator to: AND")); EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::AND); - run({"CL100", "boolean-operator", "NOT"}); - EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::NOT); + run({"CL100", "boolean-operator", "OR"}); + EXPECT_EQ(*lists()[0].boolean_operator(), BooleanOperator::OR); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, booleanOperatorNotRejected) { + auto result = run({"CL100", "boolean-operator", "NOT"}); + EXPECT_THAT(result, HasSubstr("Invalid boolean-operator value 'NOT'")); + EXPECT_THAT(result, HasSubstr("AND|OR")); + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()); } TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, booleanOperatorDefaultIsOr) { @@ -163,7 +180,7 @@ TEST_F( // (a string_view capture dangling over a fmt::format temporary). EXPECT_THAT( result, - HasSubstr("Invalid boolean-operator value 'XOR'; expected AND|OR|NOT")); + HasSubstr("Invalid boolean-operator value 'XOR'; expected AND|OR")); // The rejected value must not leave a phantom list behind. EXPECT_TRUE(lists().empty()); EXPECT_FALSE(sessionFileExists()) @@ -178,4 +195,67 @@ TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, invalidExactMatchRejected) { << "session file should not exist after rejected input"; } +// ============================================================================== +// community — communities, the field bgpd matches against +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, communityAppends) { + auto result = run({"CL100", "community", "65000:100"}); + EXPECT_THAT(result, HasSubstr("Successfully added community 65000:100")); + EXPECT_THAT(result, HasSubstr("for community-list CL100")); + run({"CL100", "community", "65000:200"}); + ASSERT_EQ(lists().size(), 1); + // Regression guard for the inert-config bug: the value must land in the + // flat `communities` list bgpd reads, never in the dead `members`. + ASSERT_TRUE(lists()[0].communities().has_value()); + EXPECT_EQ( + *lists()[0].communities(), + std::vector({"65000:100", "65000:200"})); + EXPECT_FALSE(lists()[0].members().has_value()); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, communityAcceptsRegex) { + // bgpd compiles a non-literal value as a regex, so one is accepted here. + auto result = run({"CL100", "community", "^65000:.*$"}); + EXPECT_THAT(result, HasSubstr("Successfully added community ^65000:.*$")); + EXPECT_EQ( + *lists()[0].communities(), std::vector({"^65000:.*$"})); +} + +TEST_F( + CmdConfigBgpPolicyCommunityListTestFixture, + communityAddAgainIsIdempotent) { + run({"CL100", "community", "65000:100"}); + auto result = run({"CL100", "community", "65000:100"}); + EXPECT_THAT(result, HasSubstr("already present")); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(lists()[0].communities()->size(), 1); +} + +TEST_F(CmdConfigBgpPolicyCommunityListTestFixture, malformedCommunityRejected) { + // Neither a literal nor a compilable regex: bgpd would refuse it at load. + auto result = run({"CL100", "community", "65000:("}); + EXPECT_THAT(result, HasSubstr("Error: Malformed community")); + // A rejected first attribute leaves no phantom list and nothing on disk. + EXPECT_TRUE(lists().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F( + CmdConfigBgpPolicyCommunityListTestFixture, + communityWithWhitespaceRejected) { + run({"CL100"}); + auto result = run({"CL100", "community", "65000:100 65000:200"}); + EXPECT_THAT(result, HasSubstr("must not contain whitespace")); + ASSERT_EQ(lists().size(), 1); + EXPECT_FALSE(lists()[0].communities().has_value()); + + EXPECT_THAT(run({"CL100", "community"}), HasSubstr("requires ")); + EXPECT_THAT( + run({"CL100", "community", "65000:100", "65000:200"}), + HasSubstr("requires ")); +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp index 23daff00d388f..a627453c5a230 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp @@ -10,7 +10,6 @@ #include #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" -#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/BgpCommunityListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/community-list/CmdConfigProtocolBgpPolicyCommunityList.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h" #include "fboss/cli/fboss2/session/ConfigSession.h" @@ -42,21 +41,6 @@ class CmdDeleteBgpPolicyCommunityListTestFixture : public CmdConfigTestBase { return cmd.queryClient(hostInfo, BgpCommunityListConfig(tokens)); } - // Seed an inline Community member. The member level is its own subcommand - // (CmdConfigProtocolBgpPolicyCommunityListCommunity), which lands above this - // command in the stack — so stage it through the shared helpers rather than - // depending on that handler. - void configureCommunity( - const std::string& listName, - const std::string& communityName, - const std::string& value) { - auto& session = ConfigSession::getInstance(); - auto& list = - bgpcli::findOrCreateCommunityList(session.getBgpConfig(), listName); - bgpcli::findOrCreateCommunityMember(list, communityName).value() = value; - session.saveBgpConfig(); - } - std::string del(const std::vector& tokens) { CmdDeleteProtocolBgpPolicyCommunityList cmd; HostInfo hostInfo("testhost"); @@ -86,13 +70,13 @@ TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, argValidation) { EXPECT_EQ(listOnly.listName(), "CL100"); EXPECT_FALSE(listOnly.hasCommunity()); - auto withMember = BgpCommunityListRef({"CL100", "community", "CM1"}); - EXPECT_EQ(withMember.listName(), "CL100"); - EXPECT_TRUE(withMember.hasCommunity()); - EXPECT_EQ(withMember.communityName(), "CM1"); + // Optional `community ` selector. + auto withValue = BgpCommunityListRef({"CL100", "community", "65000:100"}); + EXPECT_EQ(withValue.listName(), "CL100"); + EXPECT_TRUE(withValue.hasCommunity()); + EXPECT_EQ(withValue.community(), "65000:100"); - // Invalid: empty, empty names, non-`community` second token, missing - // community name, extra tokens. + // Invalid: empty, empty name, extra tokens, incomplete or trailing selector. EXPECT_THROW(BgpCommunityListRef({}), std::invalid_argument); EXPECT_THROW(BgpCommunityListRef({""}), std::invalid_argument); EXPECT_THROW(BgpCommunityListRef({"CL100", "CL200"}), std::invalid_argument); @@ -101,7 +85,7 @@ TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, argValidation) { EXPECT_THROW( BgpCommunityListRef({"CL100", "community", ""}), std::invalid_argument); EXPECT_THROW( - BgpCommunityListRef({"CL100", "community", "CM1", "extra"}), + BgpCommunityListRef({"CL100", "community", "65000:100", "extra"}), std::invalid_argument); } @@ -110,7 +94,7 @@ TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, argValidation) { // ============================================================================== TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteExistingList) { - configureCommunity("CL100", "CM1", "65000:100"); + configure({"CL100", "community", "65000:100"}); configure({"CL200", "description", "keep"}); ASSERT_EQ(lists().size(), 2); @@ -124,13 +108,34 @@ TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteExistingList) { EXPECT_TRUE(sessionFileExists()); } -TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteUnknownListRejected) { +// Delete mirrors add: an absent target is a success with a warning, never an +// error, so a replayed script stays idempotent. +TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteUnknownListWarns) { auto result = del({"NO-SUCH-LIST"}); EXPECT_THAT( - result, HasSubstr("Error: BGP community-list NO-SUCH-LIST not found")); - // Nothing was persisted for the failed delete. + result, + HasSubstr( + "Warning: BGP community-list NO-SUCH-LIST does not exist; nothing " + "to delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); + // Nothing changed, so nothing is staged. EXPECT_FALSE(sessionFileExists()) - << "session file should not exist after rejected delete"; + << "session file should not exist after a no-op delete"; +} + +TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteTwiceIsIdempotent) { + configure({"CL100", "description", "delete-me"}); + EXPECT_THAT(del({"CL100"}), HasSubstr("Successfully deleted")); + EXPECT_TRUE(lists().empty()); + + auto again = del({"CL100"}); + EXPECT_THAT( + again, + HasSubstr( + "Warning: BGP community-list CL100 does not exist; nothing to " + "delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); + EXPECT_TRUE(lists().empty()); } TEST_F( @@ -138,86 +143,107 @@ TEST_F( deleteUnknownLeavesOthersIntact) { configure({"CL100", "description", "one"}); auto result = del({"CL200"}); - EXPECT_THAT(result, HasSubstr("not found")); + EXPECT_THAT(result, HasSubstr("does not exist")); ASSERT_EQ(lists().size(), 1); EXPECT_EQ(*lists()[0].name(), "CL100"); } // ============================================================================== -// queryClient: single community member deletion +// community selector — remove one value, keep the list // ============================================================================== -TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteExistingCommunity) { - configureCommunity("CL100", "CM1", "65000:100"); - configureCommunity("CL100", "CM2", "65000:200"); +TEST_F(CmdDeleteBgpPolicyCommunityListTestFixture, deleteOneCommunity) { + configure({"CL100", "community", "65000:100"}); + configure({"CL100", "community", "65000:200"}); - auto result = del({"CL100", "community", "CM1"}); + auto result = del({"CL100", "community", "65000:100"}); EXPECT_THAT( result, - HasSubstr("Successfully deleted BGP community-list CL100 community CM1")); - // The list and its other member survive. + HasSubstr( + "Successfully deleted BGP community-list CL100 community " + "65000:100")); + // The list and its other value survive. ASSERT_EQ(lists().size(), 1); - ASSERT_TRUE(lists()[0].members().has_value()); - ASSERT_EQ(lists()[0].members()->size(), 1); - EXPECT_EQ(*(*lists()[0].members())[0].community_ref()->name(), "CM2"); + EXPECT_EQ(*lists()[0].communities(), std::vector({"65000:200"})); EXPECT_TRUE(sessionFileExists()); + + // Deleting the last value clears communities but keeps the list. + del({"CL100", "community", "65000:200"}); + ASSERT_EQ(lists().size(), 1); + EXPECT_EQ(*lists()[0].name(), "CL100"); + EXPECT_FALSE(lists()[0].communities().has_value()); } TEST_F( CmdDeleteBgpPolicyCommunityListTestFixture, - deleteLastCommunityUnsetsMembers) { - configureCommunity("CL100", "CM1", "65000:100"); + deleteUnknownCommunityWarns) { + configure({"CL100", "community", "65000:100"}); + // 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({"CL100", "community", "CM1"}); - EXPECT_THAT(result, HasSubstr("Successfully deleted")); - // The list stays, shaped like one that never had members. + auto result = del({"CL100", "community", "65000:999"}); + EXPECT_THAT( + result, + HasSubstr( + "Warning: BGP community-list CL100 has no community 65000:999; " + "nothing to delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); + EXPECT_FALSE(sessionFileExists()) + << "no-op delete must not persist a session file"; + // The existing value is untouched. ASSERT_EQ(lists().size(), 1); - EXPECT_EQ(*lists()[0].name(), "CL100"); - EXPECT_FALSE(lists()[0].members().has_value()); + EXPECT_EQ(*lists()[0].communities(), std::vector({"65000:100"})); } TEST_F( CmdDeleteBgpPolicyCommunityListTestFixture, - deleteUnknownCommunityRejected) { - configureCommunity("CL100", "CM1", "65000:100"); - // Consume the session file created by configure so we can assert the - // rejected delete does not persist anything new. - ASSERT_TRUE(sessionFileExists()); + deleteCommunityTwiceIsIdempotent) { + configure({"CL100", "community", "65000:100"}); + EXPECT_THAT( + del({"CL100", "community", "65000:100"}), HasSubstr("Successfully")); + EXPECT_FALSE(lists()[0].communities().has_value()); - auto result = del({"CL100", "community", "NO-SUCH-COMMUNITY"}); + auto again = del({"CL100", "community", "65000:100"}); EXPECT_THAT( - result, + again, HasSubstr( - "Error: BGP community-list CL100 community NO-SUCH-COMMUNITY not " - "found")); - // The existing member is untouched. + "Warning: BGP community-list CL100 has no community 65000:100; " + "nothing to delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); ASSERT_EQ(lists().size(), 1); - ASSERT_TRUE(lists()[0].members().has_value()); - ASSERT_EQ(lists()[0].members()->size(), 1); - EXPECT_EQ(*(*lists()[0].members())[0].community_ref()->name(), "CM1"); + EXPECT_FALSE(lists()[0].communities().has_value()); } TEST_F( CmdDeleteBgpPolicyCommunityListTestFixture, - deleteCommunityFromMemberlessListRejected) { - configure({"CL100", "description", "no", "members"}); + deleteCommunityFromValuelessListWarns) { + configure({"CL100", "description", "no", "values"}); - auto result = del({"CL100", "community", "CM1"}); + auto result = del({"CL100", "community", "65000:100"}); EXPECT_THAT( result, - HasSubstr("Error: BGP community-list CL100 community CM1 not found")); + HasSubstr( + "Warning: BGP community-list CL100 has no community 65000:100; " + "nothing to delete")); ASSERT_EQ(lists().size(), 1); - EXPECT_FALSE(lists()[0].members().has_value()); + EXPECT_FALSE(lists()[0].communities().has_value()); } TEST_F( CmdDeleteBgpPolicyCommunityListTestFixture, - deleteCommunityFromUnknownListRejected) { - auto result = del({"NO-SUCH-LIST", "community", "CM1"}); + deleteCommunityFromUnknownListWarns) { + auto result = del({"NO-SUCH-LIST", "community", "65000:100"}); EXPECT_THAT( - result, HasSubstr("Error: BGP community-list NO-SUCH-LIST not found")); + result, + HasSubstr( + "Warning: BGP community-list NO-SUCH-LIST does not exist; nothing " + "to delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); EXPECT_FALSE(sessionFileExists()) - << "session file should not exist after rejected delete"; + << "session file should not exist after a no-op delete"; } } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp index 7a30d7425dcc1..e9d863dc7d2d9 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp @@ -4,8 +4,8 @@ * End-to-end tests for `fboss2-dev config protocol bgp policy community-list * [ ...]`. * - * Scope: the community-list level (its inline community members are covered by - * ConfigBgpPolicyCommunityListCommunityTest). Every test + * Scope: the whole community-list, including its flat `community` values (the + * communities field bgpd matches on). Every test * stages the change AND commits it, then asserts the value landed at the * correct thrift field path inside the matching .policies.community_lists[] * entry of bgpd's running config (via getRunningConfig RPC) — which also @@ -20,6 +20,7 @@ */ #include +#include #include #include @@ -99,6 +100,49 @@ TEST_F(ConfigBgpPolicyCommunityListTest, SetListAttributesAndCommit) { EXPECT_TRUE((*runningList)["exact_match"].asBool()); } +TEST_F(ConfigBgpPolicyCommunityListTest, SetCommunityAndOperatorAndCommit) { + discardSession(); + clearBgpSession(); + stageCommunityList({kList, "community", "65000:100"}); + stageCommunityList({kList, "community", "65000:200"}); + stageCommunityList({kList, "boolean-operator", "AND"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + // bgpd matches on the flat `communities` list + boolean_operator (never on + // members[]); the running config proves it parsed and adopted both. + auto running = readRunningBgpConfigViaRpc(); + const auto* list = findList(running, kList); + ASSERT_NE(list, nullptr) << "bgpd's running config has no community-list " + << kList; + ASSERT_TRUE(list->count("communities")); + std::vector communities; + for (const auto& c : (*list)["communities"]) { + communities.push_back(c.asString()); + } + EXPECT_EQ(communities, std::vector({"65000:100", "65000:200"})); + ASSERT_TRUE(list->count("boolean_operator")); + // routing_policy.BooleanOperator.AND == 1 + EXPECT_EQ((*list)["boolean_operator"].asInt(), 1); +} + +TEST_F(ConfigBgpPolicyCommunityListTest, MalformedCommunityRejected) { + clearBgpSession(); + auto result = runCli( + {"config", + "protocol", + "bgp", + "policy", + "community-list", + kList, + "community", + "65000:("}); + EXPECT_THAT(result.stdout, HasSubstr("Malformed community")); + EXPECT_FALSE(std::filesystem::exists(bgpSessionPath())) + << "session file should not exist after rejected input"; +} + TEST_F(ConfigBgpPolicyCommunityListTest, DeleteListAndCommit) { // Land a community-list in the system config, then delete it through a // second commit and verify it is gone from bgpd's running config. diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 007e5b697c29e..30fc6e9c0a472 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -491,8 +491,10 @@ def generate_as_path_list_commands(as_path_list: dict[str, Any]) -> list[str]: def generate_community_list_commands(community_list: dict[str, Any]) -> list[str]: """Generate `config protocol bgp policy community-list` commands for one list. - The deprecated inline `communities` and the `community_list_names` - references have no CLI spelling; they surface as warnings. + bgpd matches on `communities` (one `community` line each), `boolean_operator` + (emitted only when it differs from the OR default) and `exact_match`. + `community_list_names` and the `members` entries are never read by bgpd and + have no CLI spelling here, so they surface as warnings rather than vanishing. """ name = community_list.get("name", "") if not name: @@ -504,26 +506,35 @@ def generate_community_list_commands(community_list: dict[str, Any]) -> list[str commands.append( f"{prefix} description {escape_shell_arg(community_list['description'])}" ) + for community in community_list.get("communities") or []: + commands.append(f"{prefix} community {escape_shell_arg(community)}") if "boolean_operator" in community_list: operator = _boolean_operator_name(community_list["boolean_operator"]) - if operator != "OR": + if operator == "NOT": + commands.append( + _warning( + f"community-list {name}: boolean_operator NOT is not " + "accepted by the CLI (bgpd treats it as AND); not emitted" + ) + ) + elif operator != "OR": commands.append(f"{prefix} boolean-operator {escape_shell_arg(operator)}") if "exact_match" in community_list: commands.append( f"{prefix} exact-match {_shell_bool(community_list['exact_match'])}" ) - if community_list.get("communities"): + if community_list.get("community_list_names"): commands.append( _warning( - f"community-list {name}: inline `communities` has no CLI " - "equivalent (use members); not emitted" + f"community-list {name}: community_list_names is not read by " + "bgpd and has no CLI equivalent; not emitted" ) ) - if community_list.get("community_list_names"): + if community_list.get("members"): commands.append( _warning( - f"community-list {name}: community_list_names has no CLI " - "equivalent; not emitted" + f"community-list {name}: members entries are not read by bgpd " + "(bgpd matches communities); not emitted" ) ) if not 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 c3d9eb9449d2d..c1c240e7f9e6b 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 @@ -1113,15 +1113,38 @@ def test_scalar_attributes(self) -> None: ], ) - def test_boolean_operator_or_omitted_not_kept(self) -> None: + def test_community_per_value(self) -> None: + """Each communities value is one quoted `community` line, in order.""" + commands = generate_community_list_commands( + {"name": "CL", "communities": ["65000:100", "^65000:.*$"]} + ) + self.assertEqual( + commands, + [ + f"{self.PREFIX} community {escape_shell_arg('65000:100')}", + f"{self.PREFIX} community {escape_shell_arg('^65000:.*$')}", + ], + ) + + def test_injection_in_community_neutralized(self) -> None: + commands = generate_community_list_commands( + {"name": "CL", "communities": ["$(reboot)"]} + ) + self.assertEqual( + commands, [f"{self.PREFIX} community {escape_shell_arg('$(reboot)')}"] + ) + + def test_boolean_operator_or_omitted_not_warns(self) -> None: self.assertEqual( generate_community_list_commands({"name": "CL", "boolean_operator": "OR"}), [self.PREFIX], ) - self.assertEqual( - generate_community_list_commands({"name": "CL", "boolean_operator": 3}), - [f"{self.PREFIX} boolean-operator NOT"], + commands = generate_community_list_commands( + {"name": "CL", "boolean_operator": 3} ) + self.assertEqual(len(commands), 1) + self.assertTrue(commands[0].startswith("# WARNING:"), commands[0]) + self.assertIn("NOT", commands[0]) def test_exact_match_false_preserved(self) -> None: self.assertEqual( @@ -1130,10 +1153,17 @@ def test_exact_match_false_preserved(self) -> None: ) def test_dead_fields_warn(self) -> None: + """Fields bgpd never reads surface as warnings, not silently dropped.""" commands = generate_community_list_commands( - {"name": "CL", "communities": ["65000:1"], "community_list_names": ["X"]} + { + "name": "CL", + "community_list_names": ["X"], + "members": [{"community": {"name": "CM1", "value": "65000:1"}}], + } ) self.assertEqual(len(commands), 2) + self.assertIn("community_list_names", commands[0]) + self.assertIn("members entries", commands[1]) for c in commands: self.assertTrue(c.startswith("# WARNING:"), c) From cc85d1eb8376f8b951e6a022ce8ff57874a77e70 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 22 Sep 2026 05:25:54 +0000 Subject: [PATCH 4/4] Trigger CI after bot restack (empty commit)