From b26756f148ce7402091d3226a6e699640e910e5b Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 7 Aug 2026 04:13:13 +0000 Subject: [PATCH 01/18] [Nexthop] fboss2 bgp policy routing-policy term commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `config protocol bgp policy routing-policy term [description ]` and `delete protocol bgp policy routing-policy term ` — the seq-num-keyed term level of the routing-policy family. The term's action and match levels land as follow-up subcommands. Unlike the sibling policy families (which parse their nested level inside the parent's ObjectArgType), term is a real CLI11 subcommand with its own handler, mirroring the `config interface ... switchport` chain: the policy name arrives through the ancestor-args tuple. This required dropping positionals_at_end() from the routing-policy parent so CLI11 can classify `term` after the policy name; the trade-off (a policy-level attribute value spelling `term` is stolen by subcommand matching) is documented in the traits. The term subcommand keeps positionals_at_end() so its own value tokens are safe. A policy-level attribute mixed with a term command (`routing-policy X description y term 10`) is rejected instead of silently dropping the description, since only the leaf handler runs. - term dispatcher keyed by (policy name from parent) + seq-num, writing bgp_policy.BgpPolicyTerm in policy_entries[]. Term level: description term name from the seq-num (next_term_id references terms by name). - shared BgpRoutingPolicyCliUtils.h lookup/create helpers, used by the policy-level dispatcher, the term subcommand, and delete. - delete removes a single term by seq-num; the policy and other terms stay. - unit tests (15) + commit-path integration tests that exercise the CLI11 subcommand dispatch through the full parse and verify terms in bgpd's running config. 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 | 20 ++ .../routing-policy/BgpRoutingPolicyCliUtils.h | 103 +++++++++ ...mdConfigProtocolBgpPolicyRoutingPolicy.cpp | 37 +-- .../CmdConfigProtocolBgpPolicyRoutingPolicy.h | 9 +- ...nfigProtocolBgpPolicyRoutingPolicyTerm.cpp | 194 ++++++++++++++++ ...ConfigProtocolBgpPolicyRoutingPolicyTerm.h | 91 ++++++++ ...leteProtocolBgpPolicyRoutingPolicyTerm.cpp | 104 +++++++++ ...DeleteProtocolBgpPolicyRoutingPolicyTerm.h | 71 ++++++ fboss/cli/fboss2/test/config/BUCK | 2 + ...mdConfigBgpPolicyRoutingPolicyTermTest.cpp | 217 ++++++++++++++++++ ...mdDeleteBgpPolicyRoutingPolicyTermTest.cpp | 155 +++++++++++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + .../ConfigBgpPolicyRoutingPolicyTermTest.cpp | 174 ++++++++++++++ 17 files changed, 1155 insertions(+), 36 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp create mode 100644 fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp create mode 100644 fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 144cda2ecec3a..f66e7a45bf4dd 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -986,6 +986,9 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.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 @@ -1132,6 +1135,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.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 f0a4a4b7ce5cb..1ef54701f321d 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -19,6 +19,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp @@ -60,6 +61,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTest.cpp + fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.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 b572867cfcc09..b8e42f7a2073c 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -23,6 +23,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.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 8fbf66e1ed8a9..b5124de097bd5 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1220,6 +1220,7 @@ cpp_library( "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp", "commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp", "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp", + "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1292,6 +1293,7 @@ cpp_library( "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp", "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp", "commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp", + "commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp", "commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp", "commands/delete/qos/CmdDeleteQos.cpp", @@ -1374,6 +1376,8 @@ cpp_library( "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h", "commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h", "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h", + "commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h", + "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", @@ -1447,6 +1451,7 @@ cpp_library( "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h", "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h", "commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h", + "commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.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 3c5f21a33e8ac..efee454a7c34e 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -48,6 +48,7 @@ #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.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" @@ -117,6 +118,7 @@ #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.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" @@ -482,6 +484,16 @@ const CommandTree& kConfigCommandTree() { CmdConfigProtocolBgpPolicyRoutingPolicy>, argRegistrar< CmdConfigProtocolBgpPolicyRoutingPolicyTraits>, + {{ + "term", + "Configure a routing-policy term: " + " [ ...] " + "(description)", + commandHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTerm>, + argRegistrar< + CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits>, + }}, }}, }, }, @@ -862,6 +874,14 @@ const CommandTree& kConfigCommandTree() { CmdDeleteProtocolBgpPolicyRoutingPolicy>, argRegistrar< CmdDeleteProtocolBgpPolicyRoutingPolicyTraits>, + {{ + "term", + "Delete a routing-policy term: ", + commandHandler< + CmdDeleteProtocolBgpPolicyRoutingPolicyTerm>, + argRegistrar< + CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits>, + }}, }}, }}, }, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h new file mode 100644 index 0000000000000..c8a71531bc2ba --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#include +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" + +/** + * Lookup/create helpers for the routing-policy CLI family, shared between the + * policy-level dispatcher (CmdConfigProtocolBgpPolicyRoutingPolicy), the term + * subcommand (CmdConfigProtocolBgpPolicyRoutingPolicyTerm), and their delete + * counterparts. A routing-policy (BgpPolicyStatement) is keyed by name; a + * term (BgpPolicyTerm in policy_entries[]) is keyed by sequence_number. + */ +namespace facebook::fboss::bgpcli { + +inline bool routingPolicyExists( + const bgp::thrift::BgpConfig& cfg, + const std::string& name) { + if (!cfg.policies().has_value()) { + return false; + } + const auto& policies = *cfg.policies()->bgp_policy_statements(); + return std::any_of(policies.begin(), policies.end(), [&](const auto& p) { + return *p.name() == name; + }); +} + +// The routing-policy statement keyed by name, or nullptr. Non-creating; +// used by the delete commands so a typo'd delete can't stage a change. +inline bgp::bgp_policy::BgpPolicyStatement* findRoutingPolicy( + bgp::thrift::BgpConfig& cfg, + const std::string& name) { + if (!cfg.policies().has_value()) { + return nullptr; + } + auto& policies = *cfg.policies()->bgp_policy_statements(); + auto it = std::find_if(policies.begin(), policies.end(), [&](const auto& p) { + return *p.name() == name; + }); + return it == policies.end() ? nullptr : &*it; +} + +// Find the routing-policy statement keyed by name, creating it if absent. +// Setting an attribute on a not-yet-created policy implicitly creates it, so +// command ordering stays forgiving; a bare `routing-policy ` creates one +// explicitly. BgpPolicyStatement's only key field is `name`. +inline bgp::bgp_policy::BgpPolicyStatement& findOrCreateRoutingPolicy( + bgp::thrift::BgpConfig& cfg, + const std::string& name) { + auto& policies = *cfg.policies().ensure().bgp_policy_statements(); + for (auto& policy : policies) { + if (*policy.name() == name) { + return policy; + } + } + policies.emplace_back(); + auto& policy = policies.back(); + policy.name() = name; + return policy; +} + +inline bool routingPolicyTermExists( + const bgp::bgp_policy::BgpPolicyStatement& policy, + int64_t seqNum) { + const auto& terms = *policy.policy_entries(); + return std::any_of(terms.begin(), terms.end(), [&](const auto& t) { + return t.sequence_number().has_value() && *t.sequence_number() == seqNum; + }); +} + +// Find the term keyed by sequence_number, creating it if absent. The name is +// seeded from the seq-num so every term has a stable, unique identity — +// BgpPolicyAction.next_term_id references terms by name. +inline bgp::bgp_policy::BgpPolicyTerm& findOrCreateRoutingPolicyTerm( + bgp::bgp_policy::BgpPolicyStatement& policy, + int64_t seqNum) { + auto& terms = *policy.policy_entries(); + for (auto& term : terms) { + if (term.sequence_number().has_value() && + *term.sequence_number() == seqNum) { + return term; + } + } + terms.emplace_back(); + auto& term = terms.back(); + term.sequence_number() = seqNum; + term.name() = std::to_string(seqNum); + return term; +} + +} // namespace facebook::fboss::bgpcli diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp index bd07534aa83f1..6cc9e0a4241b8 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp @@ -22,6 +22,7 @@ #include #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" #include "fboss/cli/fboss2/utils/HostInfo.h" @@ -85,37 +86,6 @@ std::string validAttrList() { return out; } -// Find the routing-policy statement keyed by name, creating it if absent. -// Setting an attribute on a not-yet-created policy implicitly creates it, so -// command ordering stays forgiving; a bare `routing-policy ` creates one -// explicitly. BgpPolicyStatement's only key field is `name`. -BgpPolicyStatement& findOrCreatePolicy( - bgp::thrift::BgpConfig& cfg, - const std::string& name) { - auto& policies = *cfg.policies().ensure().bgp_policy_statements(); - for (auto& policy : policies) { - if (*policy.name() == name) { - return policy; - } - } - policies.emplace_back(); - auto& policy = policies.back(); - policy.name() = name; - return policy; -} - -bool policyExists(const bgp::thrift::BgpConfig& cfg, const std::string& name) { - if (!cfg.policies().has_value()) { - return false; - } - for (const auto& policy : *cfg.policies()->bgp_policy_statements()) { - if (*policy.name() == name) { - return true; - } - } - return false; -} - } // namespace // Parse + validate at construction so queryClient stays a thin dispatch. @@ -156,8 +126,9 @@ CmdConfigProtocolBgpPolicyRoutingPolicy::queryClient( const ObjectArgType& args) { auto& session = ConfigSession::getInstance(); auto& cfg = session.getBgpConfig(); - const bool policyCreated = !policyExists(cfg, args.policyName()); - auto& policy = findOrCreatePolicy(cfg, args.policyName()); + const bool policyCreated = + !bgpcli::routingPolicyExists(cfg, args.policyName()); + auto& policy = bgpcli::findOrCreateRoutingPolicy(cfg, args.policyName()); Result result = args.attr().empty() ? ok(policyCreated diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h index 1b0792c094de9..1aa32f56fea0e 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h @@ -58,9 +58,12 @@ struct CmdConfigProtocolBgpPolicyRoutingPolicyTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgpPolicy; static void addCliArg(CLI::App& cmd, std::vector& args) { - // Stops CLI11 from classifying attribute tokens as subcommands once the - // policy name is consumed. See CmdConfigProtocolBgpNeighborTraits. - cmd.positionals_at_end(); + // Unlike the sibling policy families, no positionals_at_end() here: the + // seq-num-keyed term level is a real CLI11 subcommand (`routing-policy + // term ...`), and positionals_at_end() would stop CLI11 + // from classifying `term` after the policy name. The trade-off is that a + // free-text attribute value equal to `term` is stolen by subcommand + // matching (same exposure as the `config interface` subcommand chain). cmd.add_option("args", args, " [ ...]"); } using ObjectArgType = BgpRoutingPolicyConfig; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp new file mode 100644 index 0000000000000..5017a382c429e --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp @@ -0,0 +1,194 @@ +/* + * 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/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.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 "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.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 term's action +// and match levels are their own subcommands (follow-ups), not attributes. +constexpr std::string_view kDescription = "description"; + +using BgpPolicyTerm = bgp::bgp_policy::BgpPolicyTerm; +using bgpcli::AttrHandler; +using bgpcli::joinedStringAttr; +using bgpcli::ok; +using bgpcli::Result; + +// ---- term-level setters ----------------------------------------------------- +// Each writes one already-parsed, already-validated value. Parsing and message +// text belong to the shared factories in BgpCliAttrHandlers.h. + +void setDescription(BgpPolicyTerm& term, const std::string& description) { + term.description() = description; +} + +// ---- term-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<>>& +termAttrHandlers() { + static const std::map, std::less<>> + kHandlers = { + {std::string(kDescription), + joinedStringAttr(kDescription, setDescription)}, + }; + return kHandlers; +} + +std::string validAttrList() { + std::string out; + for (const auto& [name, _] : termAttrHandlers()) { + 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 BgpRoutingPolicyConfig). +BgpRoutingPolicyTermConfig::BgpRoutingPolicyTermConfig( + std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: term is required, optionally followed by an " + " "); + } + auto seq = bgpcli::parseNonNegInt64(v[0]); + if (!seq) { + throw std::invalid_argument( + fmt::format( + "Error: invalid term seq-num '{}'; expected a non-negative " + "integer", + v[0])); + } + seqNum_ = *seq; + if (v.size() == 1) { + return; // bare `term `: create it + } + + attr_ = v[1]; + values_.assign(v.begin() + 2, v.end()); + + if (termAttrHandlers().find(attr_) == termAttrHandlers().end()) { + throw std::invalid_argument( + fmt::format( + "Error: unknown routing-policy term attribute '{}'. Valid " + "attributes: {}", + attr_, + validAttrList())); + } +} + +CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits::RetType +CmdConfigProtocolBgpPolicyRoutingPolicyTerm::queryClient( + const HostInfo& /* hostInfo */, + const BgpRoutingPolicyConfig& policyArgs, + const ObjectArgType& args) { + // The parent parse accepts `routing-policy ... term + // ...`, but only the leaf (this command) runs — silently dropping the + // policy-level attribute would look like it was staged. Reject the mix. + if (!policyArgs.attr().empty()) { + return fmt::format( + "Error: configure routing-policy attributes and term in separate " + "commands (got routing-policy attribute '{}' alongside term {})", + policyArgs.attr(), + args.seqNum()); + } + + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool policyCreated = + !bgpcli::routingPolicyExists(cfg, policyArgs.policyName()); + auto& policy = + bgpcli::findOrCreateRoutingPolicy(cfg, policyArgs.policyName()); + const bool termCreated = + !bgpcli::routingPolicyTermExists(policy, args.seqNum()); + auto& term = bgpcli::findOrCreateRoutingPolicyTerm(policy, args.seqNum()); + + Result result = args.attr().empty() + ? ok(termCreated + ? fmt::format( + "Successfully created BGP routing-policy {} term {}", + policyArgs.policyName(), + args.seqNum()) + : fmt::format( + "BGP routing-policy {} term {} already exists", + policyArgs.policyName(), + args.seqNum())) + // The attribute is guaranteed valid: BgpRoutingPolicyTermConfig's + // constructor rejects an unknown attribute before we get here. + : termAttrHandlers().find(args.attr())->second(term, args.values()); + + if (result.ok) { + if (!args.attr().empty()) { + result.message += fmt::format( + " for routing-policy {} term {}", + policyArgs.policyName(), + args.seqNum()); + } + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } else { + // A rejected value must not leave a phantom term (or a phantom policy + // implicitly created for it) visible to later lookups in this process. + if (termCreated) { + policy.policy_entries()->pop_back(); + } + if (policyCreated) { + cfg.policies()->bgp_policy_statements()->pop_back(); + } + } + return result.message; +} + +void CmdConfigProtocolBgpPolicyRoutingPolicyTerm::printOutput( + const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTerm, + CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h new file mode 100644 index 0000000000000..e2cc5a06c2015 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h @@ -0,0 +1,91 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#include "CLI/App.hpp" +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `term [ ...]`, validated at +// construction. A term (bgp_policy.BgpPolicyTerm in +// BgpPolicyStatement.policy_entries[]) is keyed by ; the policy it +// belongs to is supplied by the parent command's args. The term's action and +// match levels are their own subcommands (follow-ups), not attributes. +// +// Grammar (from the FBOSS proposed syntax): +// ... routing-policy term (create/select) +// ... routing-policy term description +class BgpRoutingPolicyTermConfig + : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpRoutingPolicyTermConfig(std::vector v); + int64_t seqNum() const { + return seqNum_; + } + const std::string& attr() const { + return attr_; + } + const std::vector& values() const { + return values_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + int64_t seqNum_{0}; + std::string attr_; // matched dispatch key ("" = bare create) + std::vector values_; +}; + +// The term level of the routing-policy family as its own CLI11 subcommand, +// mirroring the sibling policy families (`as-path-list ... entry`, +// `community-list ... community`, `prefix-list ... entry`): the parent's +// parsed args arrive through the ancestor-args tuple, as in the +// `config interface ... switchport` chain. +struct CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits + : public WriteCommandTraits { + using ParentCmd = CmdConfigProtocolBgpPolicyRoutingPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // Term has no nested subcommands; stop CLI11's parent-chain fallthrough + // from stealing a value token that spells `term` (e.g. in a description). + cmd.positionals_at_end(); + cmd.add_option("args", args, " [ ...]"); + } + using ObjectArgType = BgpRoutingPolicyTermConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyRoutingPolicyTerm + : public CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTerm, + CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits::ObjectArgType; + using RetType = CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BgpRoutingPolicyConfig& policyArgs, + const ObjectArgType& args); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp new file mode 100644 index 0000000000000..08a645db8caaf --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp @@ -0,0 +1,104 @@ +/* + * 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/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.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 { + +// Parse + validate at construction so queryClient stays a thin dispatch. +BgpRoutingPolicyTermRef::BgpRoutingPolicyTermRef(std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: delete protocol bgp policy routing-policy term " + "requires "); + } + auto seq = bgpcli::parseNonNegInt64(v[0]); + if (!seq) { + throw std::invalid_argument( + fmt::format( + "Error: invalid term seq-num '{}'; expected a non-negative " + "integer", + v[0])); + } + // Nothing may follow the seq-num: there are no attributes to delete + // through this command. + if (v.size() > 1) { + throw std::invalid_argument( + fmt::format( + "Error: unexpected token '{}'. Usage: delete protocol bgp " + "policy routing-policy term ", + v[1])); + } + seqNum_ = *seq; +} + +CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits::RetType +CmdDeleteProtocolBgpPolicyRoutingPolicyTerm::queryClient( + const HostInfo& /* hostInfo */, + const BgpRoutingPolicyRef& policyArgs, + const ObjectArgType& args) { + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + // Nothing is persisted for an unknown policy/term, so a typo'd delete + // can't stage an unrelated session change. + auto* policy = bgpcli::findRoutingPolicy(cfg, policyArgs.policyName()); + if (policy == nullptr) { + return fmt::format( + "Error: BGP routing-policy {} not found", policyArgs.policyName()); + } + auto& terms = *policy->policy_entries(); + auto it = std::find_if(terms.begin(), terms.end(), [&](const auto& term) { + return term.sequence_number().has_value() && + *term.sequence_number() == args.seqNum(); + }); + if (it == terms.end()) { + return fmt::format( + "Error: BGP routing-policy {} term {} not found", + policyArgs.policyName(), + args.seqNum()); + } + terms.erase(it); + session.saveBgpConfig(); + return fmt::format( + "Successfully deleted BGP routing-policy {} term {}\n" + "Config saved to: {}", + policyArgs.policyName(), + args.seqNum(), + session.getBgpSessionConfigPath()); +} + +void CmdDeleteProtocolBgpPolicyRoutingPolicyTerm::printOutput( + const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdDeleteProtocolBgpPolicyRoutingPolicyTerm, + CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h new file mode 100644 index 0000000000000..9b3e9d7526129 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h @@ -0,0 +1,71 @@ +/* + * Copyright (c) 2004-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#include "CLI/App.hpp" +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `delete protocol bgp policy routing-policy term `, +// validated at construction. The seq-num is the term's identity within the +// policy named by the parent command's args; the whole term is deleted. +class BgpRoutingPolicyTermRef : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpRoutingPolicyTermRef(std::vector v); + int64_t seqNum() const { + return seqNum_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + int64_t seqNum_{0}; +}; + +// The term level as its own CLI11 subcommand, mirroring the config side +// (CmdConfigProtocolBgpPolicyRoutingPolicyTerm); the parent's parsed args +// arrive through the ancestor-args tuple. +struct CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits + : public WriteCommandTraits { + using ParentCmd = CmdDeleteProtocolBgpPolicyRoutingPolicy; + static void addCliArg(CLI::App& cmd, std::vector& args) { + cmd.add_option("args", args, ""); + } + using ObjectArgType = BgpRoutingPolicyTermRef; + using RetType = std::string; +}; + +class CmdDeleteProtocolBgpPolicyRoutingPolicyTerm + : public CmdHandler< + CmdDeleteProtocolBgpPolicyRoutingPolicyTerm, + CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits> { + public: + using ObjectArgType = + CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits::ObjectArgType; + using RetType = CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BgpRoutingPolicyRef& policyArgs, + 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 75d4cac77073a..0e52fb3b1ec41 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -19,6 +19,7 @@ cpp_unittest( "CmdConfigBgpPolicyCommunityListTest.cpp", "CmdConfigBgpPolicyPrefixListEntryTest.cpp", "CmdConfigBgpPolicyPrefixListTest.cpp", + "CmdConfigBgpPolicyRoutingPolicyTermTest.cpp", "CmdConfigBgpPolicyRoutingPolicyTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", @@ -60,6 +61,7 @@ cpp_unittest( "CmdDeleteBgpPolicyAsPathListTest.cpp", "CmdDeleteBgpPolicyCommunityListTest.cpp", "CmdDeleteBgpPolicyPrefixListTest.cpp", + "CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp", "CmdDeleteBgpPolicyRoutingPolicyTest.cpp", "CmdDeleteConfigInterfaceTest.cpp", "CmdDeleteCoppQueueTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp new file mode 100644 index 0000000000000..9f36a3cafa02b --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp @@ -0,0 +1,217 @@ +// (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/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.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 { + +// The term 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 +// CmdConfigBgpPolicyRoutingPolicyTest). +class CmdConfigBgpPolicyRoutingPolicyTermTestFixture + : public CmdConfigTestBase { + public: + CmdConfigBgpPolicyRoutingPolicyTermTestFixture() + : CmdConfigTestBase("bgp_routing_policy_term_test_%%%%-%%%%-%%%%", "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + // Invoke the term handler the way the framework does: the parent's parsed + // args plus the term's own tokens. + std::string runTerm( + const std::vector& policyTokens, + const std::vector& termTokens) { + CmdConfigProtocolBgpPolicyRoutingPolicyTerm cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient( + hostInfo, + BgpRoutingPolicyConfig(policyTokens), + BgpRoutingPolicyTermConfig(termTokens)); + } + + std::string runPolicy(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyRoutingPolicy cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient(hostInfo, BgpRoutingPolicyConfig(tokens)); + } + + const std::vector& policies() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .bgp_policy_statements(); + } + + const std::vector& terms(size_t policyIdx) { + return *policies()[policyIdx].policy_entries(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpRoutingPolicyTermConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermTestFixture, argValidation) { + // Bare create term. + auto bareTerm = BgpRoutingPolicyTermConfig({"10"}); + EXPECT_EQ(bareTerm.seqNum(), 10); + EXPECT_TRUE(bareTerm.attr().empty()); + + // Term-level attribute. + auto termDesc = BgpRoutingPolicyTermConfig({"10", "description", "a", "b"}); + EXPECT_EQ(termDesc.attr(), "description"); + EXPECT_EQ(termDesc.values(), std::vector({"a", "b"})); + + // Invalid: empty, non-numeric/negative seq-num, unknown attribute. + EXPECT_THROW(BgpRoutingPolicyTermConfig({}), std::invalid_argument); + EXPECT_THROW(BgpRoutingPolicyTermConfig({"ten"}), std::invalid_argument); + EXPECT_THROW(BgpRoutingPolicyTermConfig({"-1"}), std::invalid_argument); + EXPECT_THROW( + BgpRoutingPolicyTermConfig({"10", "no-such-attr", "1"}), + std::invalid_argument); +} + +// ============================================================================== +// Term-level handlers +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermTestFixture, bareCreateTerm) { + auto result = runTerm({"RM100"}, {"10"}); + EXPECT_THAT( + result, + HasSubstr("Successfully created BGP routing-policy RM100 term 10")); + // The policy is implicitly created for its term. + ASSERT_EQ(policies().size(), 1); + EXPECT_EQ(*policies()[0].name(), "RM100"); + ASSERT_EQ(terms(0).size(), 1); + EXPECT_EQ(*terms(0)[0].sequence_number(), 10); + // The name is seeded from the seq-num (next_term_id references terms by + // name, so every term needs a stable identity). + EXPECT_EQ(*terms(0)[0].name(), "10"); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermTestFixture, setTermDescription) { + auto result = + runTerm({"RM100"}, {"10", "description", "deny", "private", "asns"}); + EXPECT_THAT(result, HasSubstr("Successfully set description")); + EXPECT_THAT(result, HasSubstr("for routing-policy RM100 term 10")); + ASSERT_EQ(terms(0).size(), 1); + // Multi-token description is re-joined. + EXPECT_EQ(*terms(0)[0].description(), "deny private asns"); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermTestFixture, termsAreKeyedBySeqNum) { + runTerm({"RM100"}, {"10", "description", "one"}); + runTerm({"RM100"}, {"20", "description", "two"}); + ASSERT_EQ(policies().size(), 1); + ASSERT_EQ(terms(0).size(), 2); + + // Re-referencing an existing term by seq-num updates it, not appends. + runTerm({"RM100"}, {"10", "description", "one-updated"}); + ASSERT_EQ(terms(0).size(), 2); + EXPECT_EQ(*terms(0)[0].description(), "one-updated"); + EXPECT_EQ(*terms(0)[1].description(), "two"); +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermTestFixture, + reReferenceReportsExisting) { + runTerm({"RM100"}, {"10"}); + // A second bare reference must not claim to have created it again. + EXPECT_THAT(runTerm({"RM100"}, {"10"}), HasSubstr("already exists")); + ASSERT_EQ(terms(0).size(), 1); +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermTestFixture, + termOnExistingPolicyKeepsPolicyAttributes) { + runPolicy({"RM100", "description", "keep-me"}); + runTerm({"RM100"}, {"10", "description", "term-desc"}); + ASSERT_EQ(policies().size(), 1); + EXPECT_EQ(*policies()[0].description(), "keep-me"); + ASSERT_EQ(terms(0).size(), 1); + EXPECT_EQ(*terms(0)[0].description(), "term-desc"); +} + +// ============================================================================== +// Reject paths — the error is surfaced and nothing is persisted +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermTestFixture, + emptyDescriptionRejected) { + auto result = runTerm({"RM100"}, {"10", "description"}); + EXPECT_THAT(result, HasSubstr("Error: description requires ")); + // The rejected value must not leave a phantom term or policy behind. + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermTestFixture, + rejectedValueOnExistingPolicyKeepsPolicy) { + runPolicy({"RM100", "description", "keep-me"}); + ASSERT_EQ(policies().size(), 1); + + auto result = runTerm({"RM100"}, {"10", "description"}); + EXPECT_THAT(result, HasSubstr("Error: description requires ")); + // The pre-existing policy survives; only the phantom term is rolled back. + ASSERT_EQ(policies().size(), 1); + EXPECT_EQ(*policies()[0].description(), "keep-me"); + EXPECT_TRUE(terms(0).empty()); +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermTestFixture, + rejectedValueOnExistingTermKeepsTerm) { + runTerm({"RM100"}, {"10", "description", "keep-me"}); + ASSERT_EQ(terms(0).size(), 1); + + auto result = runTerm({"RM100"}, {"10", "description"}); + EXPECT_THAT(result, HasSubstr("Error: description requires ")); + // The pre-existing term survives untouched. + ASSERT_EQ(terms(0).size(), 1); + EXPECT_EQ(*terms(0)[0].description(), "keep-me"); +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermTestFixture, + policyAttributeAlongsideTermRejected) { + // The CLI parse accepts `routing-policy RM100 description x term 10`, but + // only the term leaf runs — the policy attribute must be rejected, not + // silently dropped. + auto result = runTerm({"RM100", "description", "x"}, {"10"}); + EXPECT_THAT(result, HasSubstr("Error: configure routing-policy attributes")); + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp new file mode 100644 index 0000000000000..122f02f3b5a27 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp @@ -0,0 +1,155 @@ +// (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/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.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 +// CmdDeleteBgpPolicyRoutingPolicyTest). +class CmdDeleteBgpPolicyRoutingPolicyTermTestFixture + : public CmdConfigTestBase { + public: + CmdDeleteBgpPolicyRoutingPolicyTermTestFixture() + : CmdConfigTestBase( + "bgp_routing_policy_term_delete_test_%%%%-%%%%-%%%%", + "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + std::string configureTerm( + const std::string& policyName, + const std::vector& termTokens) { + CmdConfigProtocolBgpPolicyRoutingPolicyTerm cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient( + hostInfo, + BgpRoutingPolicyConfig({policyName}), + BgpRoutingPolicyTermConfig(termTokens)); + } + + std::string delTerm( + const std::string& policyName, + const std::vector& termTokens) { + CmdDeleteProtocolBgpPolicyRoutingPolicyTerm cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient( + hostInfo, + BgpRoutingPolicyRef({policyName}), + BgpRoutingPolicyTermRef(termTokens)); + } + + const std::vector& policies() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .bgp_policy_statements(); + } + + const std::vector& terms(size_t policyIdx) { + return *policies()[policyIdx].policy_entries(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpRoutingPolicyTermRef (arg) validation +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, argValidation) { + auto ref = BgpRoutingPolicyTermRef({"10"}); + EXPECT_EQ(ref.seqNum(), 10); + + // Invalid: empty, non-numeric/negative seq-num, extra tokens. + EXPECT_THROW(BgpRoutingPolicyTermRef({}), std::invalid_argument); + EXPECT_THROW(BgpRoutingPolicyTermRef({"ten"}), std::invalid_argument); + EXPECT_THROW(BgpRoutingPolicyTermRef({"-1"}), std::invalid_argument); + EXPECT_THROW(BgpRoutingPolicyTermRef({"10", "extra"}), std::invalid_argument); +} + +// ============================================================================== +// queryClient +// ============================================================================== + +TEST_F(CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, deleteExistingTerm) { + configureTerm("RM100", {"10", "description", "delete-me"}); + configureTerm("RM100", {"20", "description", "keep"}); + ASSERT_EQ(terms(0).size(), 2); + + auto result = delTerm("RM100", {"10"}); + EXPECT_THAT( + result, + HasSubstr("Successfully deleted BGP routing-policy RM100 term 10")); + // The policy and its other term survive. + ASSERT_EQ(policies().size(), 1); + ASSERT_EQ(terms(0).size(), 1); + EXPECT_EQ(*terms(0)[0].sequence_number(), 20); + EXPECT_EQ(*terms(0)[0].description(), "keep"); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F( + CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, + deleteLastTermLeavesPolicy) { + configureTerm("RM100", {"10"}); + + auto result = delTerm("RM100", {"10"}); + EXPECT_THAT(result, HasSubstr("Successfully deleted")); + // Deleting the only term must not delete the policy itself. + ASSERT_EQ(policies().size(), 1); + EXPECT_EQ(*policies()[0].name(), "RM100"); + EXPECT_TRUE(terms(0).empty()); +} + +TEST_F( + CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, + deleteUnknownTermRejected) { + configureTerm("RM100", {"10"}); + + auto result = delTerm("RM100", {"20"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP routing-policy RM100 term 20 not found")); + // The existing term is untouched. + ASSERT_EQ(terms(0).size(), 1); + EXPECT_EQ(*terms(0)[0].sequence_number(), 10); +} + +TEST_F( + CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, + deleteTermFromUnknownPolicyRejected) { + auto result = delTerm("NO-SUCH-POLICY", {"10"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP routing-policy NO-SUCH-POLICY not found")); + // Nothing was persisted for the failed delete. + 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 278743e7520c5..149e01d868289 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -29,6 +29,7 @@ cpp_binary( "ConfigBgpPolicyCommunityListTest.cpp", "ConfigBgpPolicyPrefixListEntryTest.cpp", "ConfigBgpPolicyPrefixListTest.cpp", + "ConfigBgpPolicyRoutingPolicyTermTest.cpp", "ConfigBgpPolicyRoutingPolicyTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp new file mode 100644 index 0000000000000..1668e5b8f6947 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp @@ -0,0 +1,174 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy routing-policy + * term [ ...]`. + * + * Scope: the term level (bgp_policy.BgpPolicyTerm in + * .policies.bgp_policy_statements[].policy_entries[]). The term level is a + * real CLI11 subcommand beneath `routing-policy` — unlike the sibling policy + * families' token-parsed nesting — so these tests also exercise the + * subcommand dispatch through the full CLI parse, not just the handler. + * Every test stages the change AND commits it, then asserts the value landed + * at the correct thrift field path inside bgpd's running config (via + * getRunningConfig RPC). + * + * Requirements: + * - The fboss2-dev binary under test (config subcommand tree). + * - HOME is set (the session file lives under $HOME/.fboss2). + * - bgpd is installed/active (commit restarts it). + */ + +#include +#include +#include +#include + +#include "fboss/cli/fboss2/test/integration_test/ConfigBgpTestBase.h" +#include "folly/json/dynamic.h" +#include "gmock/gmock.h" + +using namespace facebook::fboss; +using ::testing::HasSubstr; +using ::testing::Not; + +namespace { +// Test-only routing-policy name, unlikely to collide with a real policy in +// the device's running BGP config. +const std::string kPolicy = "FBOSS2-TEST-RP-TERM"; +} // namespace + +class ConfigBgpPolicyRoutingPolicyTermTest : public ConfigBgpTestBase { + protected: + // Stage `config protocol bgp policy routing-policy term + // ` WITHOUT clearing the staged session, so attributes can + // accumulate across invocations. Returns the staged session JSON. + folly::dynamic stageTerm(const std::vector& tokens) { + std::vector args = { + "config", + "protocol", + "bgp", + "policy", + "routing-policy", + kPolicy, + "term"}; + 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.bgp_policy_statements[] entry named `name`, or nullptr. + static const folly::dynamic* findPolicy( + const folly::dynamic& config, + const std::string& name) { + if (config.count("policies") == 0 || + config["policies"].count("bgp_policy_statements") == 0) { + return nullptr; + } + for (const auto& policy : config["policies"]["bgp_policy_statements"]) { + if (policy.count("name") && policy["name"].asString() == name) { + return &policy; + } + } + return nullptr; + } + + // The .policy_entries[] term with sequence_number `seqNum`, or nullptr. + static const folly::dynamic* findTerm( + const folly::dynamic& policy, + int64_t seqNum) { + if (policy.count("policy_entries") == 0) { + return nullptr; + } + for (const auto& term : policy["policy_entries"]) { + if (term.count("sequence_number") && + term["sequence_number"].asInt() == seqNum) { + return &term; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyRoutingPolicyTermTest, SetTermAttributesAndCommit) { + discardSession(); + clearBgpSession(); + stageTerm({"10", "description", "test", "term", "ten"}); + 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 term inside the + // .policies.bgp_policy_statements[].policy_entries[] blob. + auto config = readSystemBgpConfig(); + const auto* policy = findPolicy(config, kPolicy); + ASSERT_NE(policy, nullptr) + << "committed config has no routing-policy " << kPolicy; + const auto* term = findTerm(*policy, 10); + ASSERT_NE(term, nullptr) << "committed config has no term 10"; + EXPECT_EQ((*term)["description"].asString(), "test term ten"); + + auto running = readRunningBgpConfigViaRpc(); + const auto* runningPolicy = findPolicy(running, kPolicy); + ASSERT_NE(runningPolicy, nullptr) + << "bgpd's running config has no routing-policy " << kPolicy; + const auto* runningTerm = findTerm(*runningPolicy, 10); + ASSERT_NE(runningTerm, nullptr) << "bgpd's running config has no term 10"; + EXPECT_EQ((*runningTerm)["description"].asString(), "test term ten"); + EXPECT_EQ((*runningTerm)["sequence_number"].asInt(), 10); +} + +TEST_F(ConfigBgpPolicyRoutingPolicyTermTest, DeleteTermAndCommit) { + // Land a routing-policy with two terms in the system config, then delete + // one term through a second commit and verify the policy and the other + // term survive in bgpd's running config. + discardSession(); + clearBgpSession(); + stageTerm({"10", "description", "delete-me"}); + stageTerm({"20", "description", "keep-me"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + { + auto running = readRunningBgpConfigViaRpc(); + const auto* policy = findPolicy(running, kPolicy); + ASSERT_NE(policy, nullptr) + << "setup commit did not land the routing-policy in bgpd's running " + "config"; + ASSERT_NE(findTerm(*policy, 10), nullptr); + ASSERT_NE(findTerm(*policy, 20), nullptr); + } + + clearBgpSession(); + auto result = runCli( + {"delete", + "protocol", + "bgp", + "policy", + "routing-policy", + kPolicy, + "term", + "10"}); + EXPECT_THAT( + result.stdout, + HasSubstr( + fmt::format( + "Successfully deleted BGP routing-policy {} term 10", kPolicy))); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after delete commit; state=" + << bgpDaemonActiveState(); + auto running = readRunningBgpConfigViaRpc(); + const auto* policy = findPolicy(running, kPolicy); + ASSERT_NE(policy, nullptr) + << "deleting one term must not delete the routing-policy"; + EXPECT_EQ(findTerm(*policy, 10), nullptr) + << "deleted term still present in bgpd's running config"; + EXPECT_NE(findTerm(*policy, 20), nullptr) + << "surviving term missing from bgpd's running config"; +} From 5e8b8ef7309b8df2a19356c6a42a89fd7ebb8138 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:27:04 +0000 Subject: [PATCH 02/18] Emit routing-policy term commands from bgp_json_to_cli.py Each policy term becomes `... routing-policy term ` lines (description), emitted inside the routing-policy generator; the nested action and match generators hook in here. A term without sequence_number cannot be addressed by the CLI and is emitted as a `# WARNING:` line. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 27 +++++++++++++ .../tools/tests/test_bgp_json_to_cli.py | 40 +++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index c432ede93a7a2..5e7b4fb436f0d 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -825,12 +825,39 @@ def generate_routing_policy_commands(policy: dict[str, Any]) -> list[str]: "equivalent; not emitted" ) ) + for term in policy.get("policy_entries") or []: + commands.extend(generate_routing_policy_term_commands(name, term)) if not commands: # Nothing to set: still recreate the (empty) routing-policy by name. commands.append(prefix) return commands +def generate_routing_policy_term_commands( + policy_name: str, term: dict[str, Any] +) -> list[str]: + """Generate `... routing-policy term ` commands for one + term. The CLI keys terms by sequence_number.""" + if "sequence_number" not in term: + return [ + _warning( + f"routing-policy {policy_name}: term '{term.get('name', '')}' has " + "no sequence_number and cannot be addressed by the CLI; not emitted" + ) + ] + prefix = ( + f"config protocol bgp policy routing-policy {escape_shell_arg(policy_name)} " + f"term {escape_shell_arg(term['sequence_number'])}" + ) + commands = [] + if term.get("description"): + commands.append(f"{prefix} description {escape_shell_arg(term['description'])}") + # Nested term generators (action, match) hook in here. + if not commands: + commands.append(prefix) + return commands + + def generate_policy_commands(config: dict[str, Any]) -> list[str]: """Generate the `config protocol bgp policy ...` commands. diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index 630fdcde07a10..586891c8128e8 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 @@ -32,6 +32,7 @@ generate_prefix_list_commands, generate_prefix_list_entry_commands, generate_routing_policy_commands, + generate_routing_policy_term_commands, json_to_cli, ) @@ -1483,6 +1484,45 @@ def test_policy_block_order_lists_then_policies(self) -> None: ) +class GenerateRoutingPolicyTermCommandsTest(unittest.TestCase): + """Tests for generate_routing_policy_term_commands (term grammar).""" + + PREFIX = "config protocol bgp policy routing-policy RM term 10" + + def test_bare_term_is_recreated(self) -> None: + commands = generate_routing_policy_term_commands("RM", {"sequence_number": 10}) + self.assertEqual(commands, [self.PREFIX]) + + def test_description(self) -> None: + commands = generate_routing_policy_term_commands( + "RM", {"sequence_number": 10, "description": "deny bogons"} + ) + self.assertEqual(commands, [f"{self.PREFIX} description 'deny bogons'"]) + + def test_missing_sequence_number_warns(self) -> None: + commands = generate_routing_policy_term_commands("RM", {"name": "t1"}) + self.assertEqual(len(commands), 1) + self.assertTrue(commands[0].startswith("# WARNING:")) + self.assertIn("t1", commands[0]) + + def test_terms_emitted_inside_policy(self) -> None: + commands = generate_routing_policy_commands( + { + "name": "RM", + "description": "d", + "policy_entries": [{"sequence_number": 10}, {"sequence_number": 20}], + } + ) + self.assertEqual( + commands, + [ + "config protocol bgp policy routing-policy RM description d", + self.PREFIX, + "config protocol bgp policy routing-policy RM term 20", + ], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands.""" From 7241ca279a435bddddb631597fbabd9af2014b96 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 16 Sep 2026 07:46:02 +0000 Subject: [PATCH 03/18] Make BGP routing-policy term delete idempotent Delete mirrors add: deleting a term from an absent policy, or an absent term, is a success with a `Warning:` and saves nothing, so a replayed script stays idempotent. Co-Authored-By: Claude Fable 5.1 --- ...leteProtocolBgpPolicyRoutingPolicyTerm.cpp | 10 ++-- ...mdDeleteBgpPolicyRoutingPolicyTermTest.cpp | 49 ++++++++++++++++--- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp index 08a645db8caaf..9721881fd8d2e 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp @@ -63,12 +63,14 @@ CmdDeleteProtocolBgpPolicyRoutingPolicyTerm::queryClient( const ObjectArgType& args) { auto& session = ConfigSession::getInstance(); auto& cfg = session.getBgpConfig(); - // Nothing is persisted for an unknown policy/term, so a typo'd delete - // can't stage an unrelated session change. + // Delete mirrors add: an absent policy or term 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* policy = bgpcli::findRoutingPolicy(cfg, policyArgs.policyName()); if (policy == nullptr) { return fmt::format( - "Error: BGP routing-policy {} not found", policyArgs.policyName()); + "Warning: BGP routing-policy {} does not exist; nothing to delete", + policyArgs.policyName()); } auto& terms = *policy->policy_entries(); auto it = std::find_if(terms.begin(), terms.end(), [&](const auto& term) { @@ -77,7 +79,7 @@ CmdDeleteProtocolBgpPolicyRoutingPolicyTerm::queryClient( }); if (it == terms.end()) { return fmt::format( - "Error: BGP routing-policy {} term {} not found", + "Warning: BGP routing-policy {} has no term {}; nothing to delete", policyArgs.policyName(), args.seqNum()); } diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp index 122f02f3b5a27..041832fc688fd 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp @@ -128,14 +128,25 @@ TEST_F( EXPECT_TRUE(terms(0).empty()); } -TEST_F( - CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, - deleteUnknownTermRejected) { +// Delete mirrors add: an absent target is a success with a warning, never an +// error, so a replayed script stays idempotent. +TEST_F(CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, deleteUnknownTermWarns) { configureTerm("RM100", {"10"}); + // Remove the session file created by configureTerm 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 = delTerm("RM100", {"20"}); EXPECT_THAT( - result, HasSubstr("Error: BGP routing-policy RM100 term 20 not found")); + result, + HasSubstr( + "Warning: BGP routing-policy RM100 has no term 20; nothing to " + "delete")); + EXPECT_THAT(result, Not(HasSubstr("Error:"))); + EXPECT_FALSE(sessionFileExists()) + << "no-op delete must not persist a session file"; // The existing term is untouched. ASSERT_EQ(terms(0).size(), 1); EXPECT_EQ(*terms(0)[0].sequence_number(), 10); @@ -143,13 +154,35 @@ TEST_F( TEST_F( CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, - deleteTermFromUnknownPolicyRejected) { + deleteTermTwiceIsIdempotent) { + configureTerm("RM100", {"10"}); + EXPECT_THAT(delTerm("RM100", {"10"}), HasSubstr("Successfully deleted")); + EXPECT_TRUE(terms(0).empty()); + + auto again = delTerm("RM100", {"10"}); + EXPECT_THAT( + again, + HasSubstr( + "Warning: BGP routing-policy RM100 has no term 10; nothing to " + "delete")); + EXPECT_THAT(again, Not(HasSubstr("Error:"))); + ASSERT_EQ(policies().size(), 1); + EXPECT_TRUE(terms(0).empty()); +} + +TEST_F( + CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, + deleteTermFromUnknownPolicyWarns) { auto result = delTerm("NO-SUCH-POLICY", {"10"}); EXPECT_THAT( - result, HasSubstr("Error: BGP routing-policy NO-SUCH-POLICY not found")); - // Nothing was persisted for the failed delete. + result, + HasSubstr( + "Warning: BGP routing-policy NO-SUCH-POLICY 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"; } } // namespace facebook::fboss From 596cb78c9c2c31c1b6a798612ac4befe1c72c35e Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 16 Oct 2026 06:55:50 +0000 Subject: [PATCH 04/18] Use the canonical Meta copyright header in new files --- .../bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h | 2 +- .../term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp | 2 +- .../term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h | 2 +- .../term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp | 2 +- .../term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h | 2 +- .../test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp | 2 +- .../test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp | 2 +- .../integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h index c8a71531bc2ba..52aed8a2f352b 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp index 5017a382c429e..a50afc4bf9ccb 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h index e2cc5a06c2015..d3d11da9301c2 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp index 9721881fd8d2e..8d7c3ea12b76e 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h index 9b3e9d7526129..d62de1ea1ab43 100644 --- a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp index 9f36a3cafa02b..c4555f9d7c662 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. #include #include diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp index 041832fc688fd..dd75a478e093b 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. #include #include diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp index 1668e5b8f6947..abc5d1504bd9a 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. /** * End-to-end tests for `fboss2-dev config protocol bgp policy routing-policy From f87bc4d6dbf4d68b3030b691cf9454f716796fc0 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 23 Oct 2026 18:35:49 +0000 Subject: [PATCH 05/18] Mirror the Meta import amendments from the routing-policy level Use the IS_OSS include split for the bgp_config/bgp_policy thrift headers and ensure() the optional policies field in the term rollback, matching what landed for #1488. Co-Authored-By: Claude Fable 5.1 --- .../policy/routing-policy/BgpRoutingPolicyCliUtils.h | 9 +++++++-- .../CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp | 11 ++++++++--- .../CmdConfigBgpPolicyRoutingPolicyTermTest.cpp | 9 +++++++-- .../CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp | 9 +++++++-- 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h index 52aed8a2f352b..dab6686fd6377 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h @@ -10,11 +10,16 @@ #pragma once -#include #include #include #include -#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" + +#ifndef IS_OSS +#include +#include +#else +#include +#endif /** * Lookup/create helpers for the routing-policy CLI family, shared between the diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp index a50afc4bf9ccb..f458650858fc6 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp @@ -13,7 +13,6 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include -#include #include #include #include @@ -23,7 +22,6 @@ #include #include #include -#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h" @@ -32,6 +30,13 @@ #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fmt/format.h" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + namespace facebook::fboss { namespace { @@ -175,7 +180,7 @@ CmdConfigProtocolBgpPolicyRoutingPolicyTerm::queryClient( policy.policy_entries()->pop_back(); } if (policyCreated) { - cfg.policies()->bgp_policy_statements()->pop_back(); + cfg.policies().ensure().bgp_policy_statements()->pop_back(); } } return result.message; diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp index c4555f9d7c662..1608454750506 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp @@ -1,6 +1,5 @@ // Copyright (c) Meta Platforms, Inc. and affiliates. -#include #include #include @@ -9,13 +8,19 @@ #include #include -#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" #include "fboss/cli/fboss2/utils/HostInfo.h" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + using namespace ::testing; namespace facebook::fboss { diff --git a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp index dd75a478e093b..ce04a84ac6095 100644 --- a/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp @@ -1,6 +1,5 @@ // Copyright (c) Meta Platforms, Inc. and affiliates. -#include #include #include @@ -9,7 +8,6 @@ #include #include -#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h" @@ -18,6 +16,13 @@ #include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" #include "fboss/cli/fboss2/utils/HostInfo.h" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + using namespace ::testing; namespace facebook::fboss { From ef98e57030979c89a4fa2c13e0b171af0cd36fd1 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 7 Aug 2026 05:06:37 +0000 Subject: [PATCH 06/18] [Nexthop] NOS-6672..NOS-6679: fboss2 bgp policy routing-policy term action commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `config protocol bgp policy routing-policy term action result ` and `... action set ...` — the action level of a routing-policy term. `action` is a pure grouping node; `result` and `set` are its subcommands, each with its own handler (the policy and term args arrive through the ancestor-args tuple). The term traits drop positionals_at_end() so CLI11 can classify `action` (and later `match`) after the seq-num; ancestor attributes mixed with an action command are rejected since only the leaf runs. `set` MUST be a real subcommand rather than a parsed token: CLI11's _valid_subcommand walks the parent chain unconditionally, so a bare `set` arg token inside `action` was classified as the top-level `set` VERB and stolen ("The following arguments were not expected"). A local subcommand named `set` shadows the verb because _find_subcommand checks the local scope first. ACCEPT/DENY/NEXT_TERM in term_miss_action. GOTO-TERM stays deferred: no FlowControlAction arm, and the per-action-entry next_term_id target has no slot in the documented grammar. - `set ` writes one bgp_policy.BgpPolicyAction entry per action kind in policy_entries[].policy_action_entries[] (keyed by which payload field is set; re-issuing a kind updates its entry): repeat_times}; the documented must be uniform since the thrift models one ASN repeated N times single-member CommunityList + route_action COMMUNITY_LIST_ADD/SET (bgpd rejects set_self) and `peer-address` (no thrift arm) are deferred - unit tests (14) + a commit-path integration test staging every action kind and verifying each entry in bgpd's running config. Also bumps CmdArgsLists MAX_DEPTH from 5 to 6: term action is the first six-level nested command (config protocol bgp policy routing-policy term action). Co-Authored-By: Claude Fable 5 --- cmake/CliFboss2.cmake | 2 + cmake/CliFboss2TestConfig.cmake | 1 + cmake/CliFboss2TestIntegrationTest.cmake | 1 + fboss/cli/fboss2/BUCK | 2 + fboss/cli/fboss2/CmdArgsLists.h | 2 +- fboss/cli/fboss2/CmdListConfig.cpp | 25 + ...ConfigProtocolBgpPolicyRoutingPolicyTerm.h | 8 +- ...otocolBgpPolicyRoutingPolicyTermAction.cpp | 460 ++++++++++++++++++ ...ProtocolBgpPolicyRoutingPolicyTermAction.h | 149 ++++++ fboss/cli/fboss2/test/config/BUCK | 1 + ...igBgpPolicyRoutingPolicyTermActionTest.cpp | 316 ++++++++++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + ...igBgpPolicyRoutingPolicyTermActionTest.cpp | 166 +++++++ 13 files changed, 1130 insertions(+), 4 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index f66e7a45bf4dd..4a43a419f312f 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -989,6 +989,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.cpp fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index 1ef54701f321d..b4d088624560a 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -20,6 +20,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index b8e42f7a2073c..9bc34db9073e1 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -24,6 +24,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.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 b5124de097bd5..9bc047c8e61f6 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1221,6 +1221,7 @@ cpp_library( "commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp", "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp", "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp", + "commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1378,6 +1379,7 @@ cpp_library( "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h", "commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h", "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h", + "commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", diff --git a/fboss/cli/fboss2/CmdArgsLists.h b/fboss/cli/fboss2/CmdArgsLists.h index 4f7b81e475fc7..c7e49ccbe0624 100644 --- a/fboss/cli/fboss2/CmdArgsLists.h +++ b/fboss/cli/fboss2/CmdArgsLists.h @@ -71,7 +71,7 @@ class CmdArgsLists { private: // since we do a lot of work of figuring out argument types at compile time, // for now we need to know the the theoretical max depth of nested subcommands - static constexpr auto MAX_DEPTH = 5; + static constexpr auto MAX_DEPTH = 6; std::array, MAX_DEPTH> data_; }; diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index efee454a7c34e..043e8af67f82f 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -49,6 +49,7 @@ #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.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" @@ -493,6 +494,30 @@ const CommandTree& kConfigCommandTree() { CmdConfigProtocolBgpPolicyRoutingPolicyTerm>, argRegistrar< CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits>, + {{ + "action", + "Configure a term action", + {{ + "result", + "Set the term result: " + "", + commandHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult>, + argRegistrar< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits>, + }, + { + "set", + "Set a route attribute: " + "as-path prepend|community|" + "local-pref|med|next-hop|" + "origin|weight ...", + commandHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet>, + argRegistrar< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits>, + }}, + }}, }}, }}, }, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h index d3d11da9301c2..215833ed51377 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h @@ -62,9 +62,11 @@ struct CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits : public WriteCommandTraits { using ParentCmd = CmdConfigProtocolBgpPolicyRoutingPolicy; static void addCliArg(CLI::App& cmd, std::vector& args) { - // Term has no nested subcommands; stop CLI11's parent-chain fallthrough - // from stealing a value token that spells `term` (e.g. in a description). - cmd.positionals_at_end(); + // No positionals_at_end() here: the term's action (and later match) + // level is a real CLI11 subcommand, and positionals_at_end() would stop + // CLI11 from classifying it after the seq-num. The trade-off is that a + // term-level attribute value spelling a child subcommand name is stolen + // by subcommand matching (same exposure as the routing-policy parent). cmd.add_option("args", args, " [ ...]"); } using ObjectArgType = BgpRoutingPolicyTermConfig; diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp new file mode 100644 index 0000000000000..55ac019fbe5f4 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp @@ -0,0 +1,460 @@ +/* + * 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/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#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 "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.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 `set ` dispatch keys, exactly as documented. Kept here so +// the valid-attribute set and the handler table stay in sync. +constexpr std::string_view kAsPathPrepend = "as-path prepend"; +constexpr std::string_view kCommunity = "community"; +constexpr std::string_view kLocalPref = "local-pref"; +constexpr std::string_view kMed = "med"; +constexpr std::string_view kNextHop = "next-hop"; +constexpr std::string_view kOrigin = "origin"; +constexpr std::string_view kWeight = "weight"; + +// result values, mapped onto bgp_policy.FlowControlAction. GOTO-TERM (the +// sheet's fourth value) is deferred: FlowControlAction has no GOTO arm, and +// the thrift models "go to term X" as a per-action-entry next_term_id whose +// target the documented grammar does not carry. +constexpr std::string_view kResultAccept = "ACCEPT"; +constexpr std::string_view kResultReject = "REJECT"; +constexpr std::string_view kResultContinue = "CONTINUE"; + +// origin values (bgp_policy.Origin names). +constexpr std::string_view kOriginIgp = "IGP"; +constexpr std::string_view kOriginEgp = "EGP"; +constexpr std::string_view kOriginIncomplete = "INCOMPLETE"; + +// next-hop's non-address spelling. bgpd's SetNexthop validator explicitly +// rejects set_self ("Unsupported nexthop config"), so the CLI rejects it +// up front instead of staging a config the daemon aborts on. +constexpr std::string_view kNextHopSelf = "self"; +// `additive` switches community from overwrite (SET) to append (ADD). +constexpr std::string_view kCommunityAdditive = "additive"; + +constexpr int64_t kUint32Max = std::numeric_limits::max(); +constexpr int32_t kUint16Max = std::numeric_limits::max(); + +using BgpPolicyAction = bgp::bgp_policy::BgpPolicyAction; +using BgpPolicyActionType = bgp::bgp_policy::BgpPolicyActionType; +using CommunityActionType = bgp::bgp_policy::CommunityActionType; +using BgpPolicyTerm = bgp::bgp_policy::BgpPolicyTerm; +using FlowControlAction = bgp::bgp_policy::FlowControlAction; +using MedActionType = bgp::bgp_policy::MedActionType; +using Origin = bgp::bgp_policy::Origin; +using WeightActionType = bgp::bgp_policy::WeightActionType; +using bgpcli::AttrHandler; +using bgpcli::enumAttr; +using bgpcli::err; +using bgpcli::ok; +using bgpcli::Result; +using bgpcli::Tokens; +using bgpcli::uintAttr; + +// Find the action entry this attribute kind lives in, creating it if absent. +// The CLI keeps one BgpPolicyAction entry per action kind (the thrift models +// policy_action_entries as a list of single-purpose actions), keyed by the +// BgpPolicyActionType; re-issuing a kind updates its entry. The `type` field +// is marked deprecated in the thrift, but bgpd's +// createPolicyAttributesActionItem still dispatches on it and aborts on the +// unset default (0) — every entry MUST carry it. +BgpPolicyAction& findOrCreateActionEntry( + BgpPolicyTerm& term, + BgpPolicyActionType type) { + auto& actions = *term.policy_action_entries(); + for (auto& action : actions) { + if (*action.type() == type) { + return action; + } + } + actions.emplace_back(); + auto& action = actions.back(); + action.type() = type; + return action; +} + +// The shared "find-or-create the policy and term, dispatch, roll back +// phantoms on failure, persist on success" flow of both action leaves. +template +std::string runTermActionCommand( + const BgpRoutingPolicyConfig& policyArgs, + const BgpRoutingPolicyTermConfig& termArgs, + const std::string& leafName, + Handler&& handler) { + // Ancestor-level attributes mixed with an action command parse, but only + // the leaf runs — reject instead of silently dropping them. + if (!policyArgs.attr().empty() || !termArgs.attr().empty()) { + return fmt::format( + "Error: configure routing-policy/term attributes and action in " + "separate commands (got attribute '{}' alongside action {})", + !policyArgs.attr().empty() ? policyArgs.attr() : termArgs.attr(), + leafName); + } + + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool policyCreated = + !bgpcli::routingPolicyExists(cfg, policyArgs.policyName()); + auto& policy = + bgpcli::findOrCreateRoutingPolicy(cfg, policyArgs.policyName()); + const bool termCreated = + !bgpcli::routingPolicyTermExists(policy, termArgs.seqNum()); + auto& term = bgpcli::findOrCreateRoutingPolicyTerm(policy, termArgs.seqNum()); + + Result result = handler(term); + + if (result.ok) { + result.message += fmt::format( + " for routing-policy {} term {}", + policyArgs.policyName(), + termArgs.seqNum()); + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } else { + // A rejected value must not leave a phantom term (or a phantom policy + // implicitly created for it) visible to later lookups in this process. + if (termCreated) { + policy.policy_entries()->pop_back(); + } + if (policyCreated) { + cfg.policies()->bgp_policy_statements()->pop_back(); + } + } + return result.message; +} + +// ---- result handler -------------------------------------------------------- + +std::optional parseResult(const std::string& value) { + if (value == kResultAccept) { + return FlowControlAction::ACCEPT; + } + if (value == kResultReject) { + return FlowControlAction::DENY; + } + if (value == kResultContinue) { + return FlowControlAction::NEXT_TERM; + } + return std::nullopt; +} + +void setResult(BgpPolicyTerm& term, FlowControlAction result) { + term.term_miss_action() = result; +} + +const AttrHandler& resultHandler() { + static const std::string kResultValues = + fmt::format("{}|{}|{}", kResultAccept, kResultReject, kResultContinue); + static const AttrHandler kHandler = + enumAttr( + "result", kResultValues, parseResult, setResult); + return kHandler; +} + +// ---- `set ` setters --------------------------------------------- +// Each writes one already-parsed, already-validated value into the term's +// action entry for its kind. Parsing, bounds and message text belong to the +// shared factories in BgpCliAttrHandlers.h; the three below that remain full +// handlers do so because their value shape has no factory: a uniform ASN +// list, a value plus an optional trailing flag, and an address with a +// rejected spelling. + +void setLocalPref(BgpPolicyTerm& term, int64_t localPref) { + findOrCreateActionEntry(term, BgpPolicyActionType::SET_LOCAL_PREF) + .set_local_pref() + .ensure() + .local_pref() = localPref; +} + +void setMed(BgpPolicyTerm& term, int64_t med) { + auto& medAction = findOrCreateActionEntry(term, BgpPolicyActionType::MED) + .med_action() + .ensure(); + medAction.med_value() = med; + medAction.med_action_type() = MedActionType::SET; +} + +std::optional parseOrigin(const std::string& value) { + if (value == kOriginIgp) { + return Origin::IGP; + } + if (value == kOriginEgp) { + return Origin::EGP; + } + if (value == kOriginIncomplete) { + return Origin::INCOMPLETE; + } + return std::nullopt; +} + +void setOrigin(BgpPolicyTerm& term, Origin origin) { + findOrCreateActionEntry(term, BgpPolicyActionType::ORIGIN).set_origin() = + origin; +} + +void setWeight(BgpPolicyTerm& term, int64_t weight) { + auto& weightAction = + findOrCreateActionEntry(term, BgpPolicyActionType::WEIGHT) + .weight_action() + .ensure(); + weightAction.weight_value() = static_cast(weight); + weightAction.weight_action_type() = WeightActionType::SET; +} + +// SetAsPathPrepend models ONE ASN repeated N times, so the documented +// must be uniform: `65000 65000` becomes {asn=65000, +// repeat_times=2}. +Result actionAsPathPrepend(BgpPolicyTerm& term, const Tokens& values) { + if (values.empty()) { + return err("Error: as-path prepend requires [ ...]"); + } + auto first = bgpcli::parseAsn4Byte(values[0]); + for (const auto& v : values) { + auto asn = bgpcli::parseAsn4Byte(v); + if (!asn) { + return err( + fmt::format( + "Error: Invalid as-path prepend ASN '{}'; expected a 4-byte ASN", + v)); + } + if (*asn != *first) { + return err( + "Error: as-path prepend supports a single ASN repeated N times " + "(e.g. `65000 65000`); mixed ASNs are not supported"); + } + } + auto& prepend = + findOrCreateActionEntry(term, BgpPolicyActionType::AS_PATH_PREPEND) + .set_as_path_prepend() + .ensure(); + prepend.asn() = *first; + prepend.repeat_times() = static_cast(values.size()); + return ok( + fmt::format( + "Successfully set as-path prepend to: {} x{}", + *first, + values.size())); +} + +// [additive]: additive appends (ADD), the default +// overwrites (SET). bgpd's CommunityAction reads the thrift-deprecated but +// still load-bearing community_action.communities string list — the sheet's +// JSON field path agrees. +Result actionCommunity(BgpPolicyTerm& term, const Tokens& values) { + if (values.empty() || values.size() > 2 || + (values.size() == 2 && values[1] != kCommunityAdditive)) { + return err("Error: community requires [additive]"); + } + const bool additive = values.size() == 2; + auto& communityAction = + findOrCreateActionEntry(term, BgpPolicyActionType::COMMUNITY_LIST) + .community_action() + .ensure(); + communityAction.name() = values[0]; + communityAction.communities().ensure() = {values[0]}; + communityAction.action_type() = + additive ? CommunityActionType::ADD : CommunityActionType::SET; + return ok( + fmt::format( + "Successfully set community to: {}{}", + values[0], + additive ? " (additive)" : "")); +} + +// . The sheet also documents `self` and `peer-address`, but +// bgpd's SetNexthop validator rejects set_self and the thrift has no peer +// arm — both are deferred rather than staging a config the daemon aborts on. +Result actionNextHop(BgpPolicyTerm& term, const Tokens& values) { + if (values.size() != 1) { + return err("Error: next-hop requires "); + } + if (values[0] == kNextHopSelf) { + return err( + "Error: next-hop `self` is not supported by bgpd yet; expected an " + "IP address"); + } + auto addr = folly::IPAddress::tryFromString(values[0]); + if (!addr) { + return err( + fmt::format( + "Error: Invalid next-hop value '{}'; expected an IP address", + values[0])); + } + auto& nexthop = findOrCreateActionEntry(term, BgpPolicyActionType::NEXT_HOP) + .set_nexthop() + .ensure(); + nexthop.set_self() = false; + auto& hop = nexthop.next_hop().ensure(); + hop.version() = addr->isV6() ? 6 : 4; + hop.next_hop_prefix() = values[0]; + return ok(fmt::format("Successfully set next-hop to: {}", values[0])); +} + +// ---- `set ` registry -------------------------------------------- +// One line per documented attribute: its dispatch key, its value shape, and +// the setter or handler that stores it. +const std::map, std::less<>>& +setAttrHandlers() { + static const std::string kOriginValues = + fmt::format("{}|{}|{}", kOriginIgp, kOriginEgp, kOriginIncomplete); + static const std::map, std::less<>> + kHandlers = { + {std::string(kAsPathPrepend), actionAsPathPrepend}, + {std::string(kCommunity), actionCommunity}, + {std::string(kLocalPref), + uintAttr(kLocalPref, kUint32Max, setLocalPref)}, + {std::string(kMed), + uintAttr(kMed, kUint32Max, setMed)}, + {std::string(kNextHop), actionNextHop}, + {std::string(kOrigin), + enumAttr( + kOrigin, kOriginValues, parseOrigin, setOrigin)}, + {std::string(kWeight), + uintAttr(kWeight, kUint16Max, setWeight)}, + }; + return kHandlers; +} + +std::string validSetAttrList() { + std::string out; + for (const auto& [name, _] : setAttrHandlers()) { + 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 BgpRoutingPolicyTermConfig). +BgpRoutingPolicyTermActionResultConfig::BgpRoutingPolicyTermActionResultConfig( + std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + "Error: action result requires "); + } + values_ = std::move(v); +} + +BgpRoutingPolicyTermActionSetConfig::BgpRoutingPolicyTermActionSetConfig( + std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty()) { + throw std::invalid_argument( + fmt::format( + "Error: action set requires an . Valid attributes: {}", + validSetAttrList())); + } + // `as-path` only exists composed with `prepend`; anything else is a + // single-token attribute name. + size_t valueStart = 1; + if (v[0] == "as-path") { + if (v.size() < 2 || v[1] != "prepend") { + throw std::invalid_argument( + "Error: `as-path` must be followed by `prepend` " + "(usage: set as-path prepend [ ...])"); + } + attr_ = std::string(kAsPathPrepend); + valueStart = 2; + } else { + attr_ = v[0]; + } + values_.assign(v.begin() + valueStart, v.end()); + + if (setAttrHandlers().find(attr_) == setAttrHandlers().end()) { + throw std::invalid_argument( + fmt::format( + "Error: unknown action set attribute '{}'. Valid attributes: {}", + attr_, + validSetAttrList())); + } +} + +CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits::RetType +CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult::queryClient( + const HostInfo& /* hostInfo */, + const BgpRoutingPolicyConfig& policyArgs, + const BgpRoutingPolicyTermConfig& termArgs, + const ObjectArgType& args) { + return runTermActionCommand( + policyArgs, termArgs, "result", [&](BgpPolicyTerm& term) { + return resultHandler()(term, args.values()); + }); +} + +void CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult::printOutput( + const RetType& output) { + std::cout << output << std::endl; +} + +CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits::RetType +CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet::queryClient( + const HostInfo& /* hostInfo */, + const BgpRoutingPolicyConfig& policyArgs, + const BgpRoutingPolicyTermConfig& termArgs, + const ObjectArgType& args) { + return runTermActionCommand( + policyArgs, termArgs, "set", [&](BgpPolicyTerm& term) { + // The attribute is guaranteed valid: the constructor rejects an + // unknown attribute before we get here. + return setAttrHandlers().find(args.attr())->second(term, args.values()); + }); +} + +void CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet::printOutput( + const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiations +template void CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult, + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits>::run(); +template void CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet, + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h new file mode 100644 index 0000000000000..76e6daca5e80c --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h @@ -0,0 +1,149 @@ +/* + * 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/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +/** + * The action level of a routing-policy term. `action` itself is a pure + * grouping node; `result` and `set` are its subcommands, each with its own + * handler (the policy and term args arrive through the ancestor-args tuple). + * + * `set` MUST be a real subcommand rather than a parsed token: CLI11's + * _valid_subcommand walks the parent chain unconditionally, so a bare `set` + * arg token inside `action` would be classified as the top-level `set` verb + * and stolen. A local subcommand named `set` shadows the verb because + * _find_subcommand checks the local scope first. + * + * Grammar (from the FBOSS proposed syntax): + * ... term action result + * ... term action set as-path prepend [ ...] + * ... term action set community [additive] + * ... term action set local-pref + * ... term action set med + * ... term action set next-hop + * ... term action set origin + * ... term action set weight + */ +namespace facebook::fboss { + +// Parsed `action result `, validated at construction. +class BgpRoutingPolicyTermActionResultConfig + : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpRoutingPolicyTermActionResultConfig( + std::vector v); + const std::vector& values() const { + return values_; + } + const static utils::ObjectArgTypeId id = + utils::ObjectArgTypeId::OBJECT_ARG_TYPE_ID_MESSAGE; + + private: + std::vector values_; +}; + +// Parsed `action set ...`, validated at construction. +// Attribute names may be composed of more than one CLI token +// (`as-path prepend`), matched as a unit. +class BgpRoutingPolicyTermActionSetConfig + : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpRoutingPolicyTermActionSetConfig( + std::vector v); + 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 attr_; // matched dispatch key (never empty; no bare create) + std::vector values_; +}; + +struct CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits + : public WriteCommandTraits { + using ParentCmd = CmdConfigProtocolBgpPolicyRoutingPolicyTerm; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // Stop CLI11's parent-chain fallthrough from stealing a value token + // that spells an ancestor subcommand name. + cmd.positionals_at_end(); + cmd.add_option("args", args, ""); + } + using ObjectArgType = BgpRoutingPolicyTermActionResultConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult + : public CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult, + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits:: + ObjectArgType; + using RetType = + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BgpRoutingPolicyConfig& policyArgs, + const BgpRoutingPolicyTermConfig& termArgs, + const ObjectArgType& args); + + void printOutput(const RetType& output); +}; + +struct CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits + : public WriteCommandTraits { + using ParentCmd = CmdConfigProtocolBgpPolicyRoutingPolicyTerm; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // Stop CLI11's parent-chain fallthrough from stealing a value token + // that spells an ancestor subcommand name. + cmd.positionals_at_end(); + cmd.add_option("args", args, " ..."); + } + using ObjectArgType = BgpRoutingPolicyTermActionSetConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet + : public CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet, + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits::ObjectArgType; + using RetType = + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BgpRoutingPolicyConfig& policyArgs, + const BgpRoutingPolicyTermConfig& termArgs, + 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 0e52fb3b1ec41..62b7600286bdc 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -19,6 +19,7 @@ cpp_unittest( "CmdConfigBgpPolicyCommunityListTest.cpp", "CmdConfigBgpPolicyPrefixListEntryTest.cpp", "CmdConfigBgpPolicyPrefixListTest.cpp", + "CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp", "CmdConfigBgpPolicyRoutingPolicyTermTest.cpp", "CmdConfigBgpPolicyRoutingPolicyTest.cpp", "CmdConfigCoppTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp new file mode 100644 index 0000000000000..32386c9688530 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -0,0 +1,316 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.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 { + +// The action 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 +// CmdConfigBgpPolicyRoutingPolicyTermTest). +class CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture + : public CmdConfigTestBase { + public: + CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture() + : CmdConfigTestBase( + "bgp_routing_policy_term_action_test_%%%%-%%%%-%%%%", + "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + // Invoke the action leaves the way the framework does: the ancestors' + // parsed args plus the leaf's own tokens. + std::string runResult(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient( + hostInfo, + BgpRoutingPolicyConfig({"RM100"}), + BgpRoutingPolicyTermConfig({"10"}), + BgpRoutingPolicyTermActionResultConfig(tokens)); + } + + std::string runSet(const std::vector& tokens) { + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient( + hostInfo, + BgpRoutingPolicyConfig({"RM100"}), + BgpRoutingPolicyTermConfig({"10"}), + BgpRoutingPolicyTermActionSetConfig(tokens)); + } + + const std::vector& policies() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .bgp_policy_statements(); + } + + const bgp::bgp_policy::BgpPolicyTerm& term() { + return (*policies()[0].policy_entries())[0]; + } + + const std::vector& actions() { + return *term().policy_action_entries(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// Arg validation (both action leaves) +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, argValidation) { + // result. + auto result = BgpRoutingPolicyTermActionResultConfig({"ACCEPT"}); + EXPECT_EQ(result.values(), std::vector({"ACCEPT"})); + + // Single-token set attribute. + auto localPref = BgpRoutingPolicyTermActionSetConfig({"local-pref", "200"}); + EXPECT_EQ(localPref.attr(), "local-pref"); + EXPECT_EQ(localPref.values(), std::vector({"200"})); + + // Composed two-token set attribute. + auto prepend = BgpRoutingPolicyTermActionSetConfig( + {"as-path", "prepend", "65000", "65000"}); + EXPECT_EQ(prepend.attr(), "as-path prepend"); + EXPECT_EQ(prepend.values(), std::vector({"65000", "65000"})); + + // Invalid: empty, unknown set attribute, `as-path` without `prepend`. + EXPECT_THROW( + BgpRoutingPolicyTermActionResultConfig({}), std::invalid_argument); + EXPECT_THROW(BgpRoutingPolicyTermActionSetConfig({}), std::invalid_argument); + EXPECT_THROW( + BgpRoutingPolicyTermActionSetConfig({"no-such-attr", "1"}), + std::invalid_argument); + EXPECT_THROW( + BgpRoutingPolicyTermActionSetConfig({"as-path", "65000"}), + std::invalid_argument); +} + +// ============================================================================== +// result +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setResult) { + // Each CLI value maps onto its FlowControlAction arm; the policy and term + // are implicitly created. + runResult({"ACCEPT"}); + EXPECT_EQ(*term().term_miss_action(), FlowControlAction::ACCEPT); + + runResult({"REJECT"}); + EXPECT_EQ(*term().term_miss_action(), FlowControlAction::DENY); + + auto result = runResult({"CONTINUE"}); + EXPECT_THAT(result, HasSubstr("Successfully set result to: CONTINUE")); + EXPECT_THAT(result, HasSubstr("for routing-policy RM100 term 10")); + EXPECT_EQ(*term().term_miss_action(), FlowControlAction::NEXT_TERM); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, gotoTermRejected) { + // GOTO-TERM is documented but deferred (no FlowControlAction arm; the + // thrift models it per-action-entry). It must be rejected, not mismapped. + auto result = runResult({"GOTO-TERM"}); + EXPECT_THAT( + result, + HasSubstr( + "Invalid result value 'GOTO-TERM'; expected " + "ACCEPT|REJECT|CONTINUE")); + // The rejected value must not leave a phantom term or policy behind. + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +// ============================================================================== +// set — each writes its own action entry +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setAsPathPrepend) { + auto result = runSet({"as-path", "prepend", "65000", "65000", "65000"}); + EXPECT_THAT(result, HasSubstr("Successfully set as-path prepend")); + ASSERT_EQ(actions().size(), 1); + // bgpd dispatches on the (deprecated, still load-bearing) type field. + EXPECT_EQ(*actions()[0].type(), BgpPolicyActionType::AS_PATH_PREPEND); + const auto& prepend = *actions()[0].set_as_path_prepend(); + EXPECT_EQ(*prepend.asn(), 65000); + EXPECT_EQ(*prepend.repeat_times(), 3); +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, + mixedAsnPrependRejected) { + // SetAsPathPrepend models one ASN repeated N times; a mixed list has no + // thrift representation and must be rejected. + auto result = runSet({"as-path", "prepend", "65000", "65001"}); + EXPECT_THAT(result, HasSubstr("single ASN repeated")); + EXPECT_TRUE(policies().empty()); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setCommunity) { + auto result = runSet({"community", "65000:100"}); + EXPECT_THAT(result, HasSubstr("Successfully set community to: 65000:100")); + ASSERT_EQ(actions().size(), 1); + const auto& action = actions()[0]; + // bgpd dispatches on the (deprecated, still load-bearing) type field and + // reads community_action.communities. + EXPECT_EQ(*action.type(), BgpPolicyActionType::COMMUNITY_LIST); + // Overwrite semantics without `additive`. + EXPECT_EQ( + *action.community_action()->action_type(), CommunityActionType::SET); + EXPECT_EQ( + *action.community_action()->communities(), + std::vector({"65000:100"})); + + // `additive` switches to append semantics on the same entry. + runSet({"community", "65000:200", "additive"}); + ASSERT_EQ(actions().size(), 1); + EXPECT_EQ( + *actions()[0].community_action()->action_type(), + CommunityActionType::ADD); + EXPECT_EQ( + *actions()[0].community_action()->communities(), + std::vector({"65000:200"})); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setLocalPref) { + runSet({"local-pref", "4294967295"}); + ASSERT_EQ(actions().size(), 1); + EXPECT_EQ(*actions()[0].set_local_pref()->local_pref(), 4294967295); + + // Out-of-range is rejected without touching the stored value. + auto result = runSet({"local-pref", "4294967296"}); + EXPECT_THAT(result, HasSubstr("local-pref requires <0-4294967295>")); + EXPECT_EQ(*actions()[0].set_local_pref()->local_pref(), 4294967295); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setMed) { + auto result = runSet({"med", "500"}); + EXPECT_THAT(result, HasSubstr("Successfully set med to: 500")); + ASSERT_EQ(actions().size(), 1); + const auto& med = *actions()[0].med_action(); + EXPECT_EQ(*med.med_value(), 500); + EXPECT_EQ(*med.med_action_type(), MedActionType::SET); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setNextHop) { + runSet({"next-hop", "2001:db8::1"}); + ASSERT_EQ(actions().size(), 1); + const auto& nexthop = *actions()[0].set_nexthop(); + EXPECT_FALSE(*nexthop.set_self()); + EXPECT_EQ(*nexthop.next_hop()->next_hop_prefix(), "2001:db8::1"); + EXPECT_EQ(*nexthop.next_hop()->version(), 6); + + // bgpd's SetNexthop validator rejects set_self, so the CLI must reject + // `self` up front instead of staging a config the daemon aborts on. + auto selfResult = runSet({"next-hop", "self"}); + EXPECT_THAT(selfResult, HasSubstr("not supported by bgpd yet")); + EXPECT_FALSE(*actions()[0].set_nexthop()->set_self()); + + auto result = runSet({"next-hop", "not-an-ip"}); + EXPECT_THAT(result, HasSubstr("Invalid next-hop value 'not-an-ip'")); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setOrigin) { + auto result = runSet({"origin", "IGP"}); + EXPECT_THAT(result, HasSubstr("Successfully set origin to: IGP")); + ASSERT_EQ(actions().size(), 1); + EXPECT_EQ(*actions()[0].set_origin(), Origin::IGP); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setWeight) { + runSet({"weight", "65535"}); + ASSERT_EQ(actions().size(), 1); + const auto& weight = *actions()[0].weight_action(); + EXPECT_EQ(*weight.weight_value(), 65535); + EXPECT_EQ(*weight.weight_action_type(), WeightActionType::SET); + + auto result = runSet({"weight", "65536"}); + EXPECT_THAT(result, HasSubstr("weight requires <0-65535>")); +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, + actionKindsGetTheirOwnEntries) { + // Each action kind lives in its own policy_action_entries[] entry; + // re-issuing a kind updates its entry instead of appending. + runSet({"local-pref", "100"}); + runSet({"med", "50"}); + runSet({"origin", "EGP"}); + ASSERT_EQ(actions().size(), 3); + + runSet({"local-pref", "200"}); + ASSERT_EQ(actions().size(), 3); + size_t localPrefEntries = 0; + for (const auto& action : actions()) { + if (action.set_local_pref().has_value()) { + ++localPrefEntries; + EXPECT_EQ(*action.set_local_pref()->local_pref(), 200); + } + } + EXPECT_EQ(localPrefEntries, 1); +} + +// ============================================================================== +// Reject paths — the error is surfaced and nothing is persisted +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, + rejectedValueRollsBackPhantoms) { + auto result = runSet({"local-pref", "not-a-number"}); + EXPECT_THAT(result, HasSubstr("local-pref requires")); + // The rejected value must not leave a phantom term or policy behind. + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, + ancestorAttributeAlongsideActionRejected) { + // The CLI parse accepts `... term 10 description x action result ACCEPT`, + // but only the leaf runs — the term attribute must be rejected, not + // silently dropped. + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult cmd; + HostInfo hostInfo("testhost"); + auto result = cmd.queryClient( + hostInfo, + BgpRoutingPolicyConfig({"RM100"}), + BgpRoutingPolicyTermConfig({"10", "description", "x"}), + BgpRoutingPolicyTermActionResultConfig({"ACCEPT"})); + EXPECT_THAT( + result, HasSubstr("Error: configure routing-policy/term attributes")); + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 149e01d868289..4afdd454f09ad 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -29,6 +29,7 @@ cpp_binary( "ConfigBgpPolicyCommunityListTest.cpp", "ConfigBgpPolicyPrefixListEntryTest.cpp", "ConfigBgpPolicyPrefixListTest.cpp", + "ConfigBgpPolicyRoutingPolicyTermActionTest.cpp", "ConfigBgpPolicyRoutingPolicyTermTest.cpp", "ConfigBgpPolicyRoutingPolicyTest.cpp", "ConfigBgpSessionTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp new file mode 100644 index 0000000000000..d4c7acf6dfc40 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -0,0 +1,166 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy routing-policy + * term action result|set ...`. + * + * Scope: the action level of a routing-policy term (term_miss_action and + * bgp_policy.BgpPolicyAction entries in .policy_entries[] + * .policy_action_entries[]). Like `term`, `action` is a real CLI11 + * subcommand, so these tests exercise the two-deep subcommand dispatch + * through the full CLI parse. Every test stages the change AND commits it, + * then asserts the values landed at the correct thrift field paths inside + * bgpd's running config (via getRunningConfig RPC). + * + * 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 routing-policy name, unlikely to collide with a real policy in +// the device's running BGP config. +const std::string kPolicy = "FBOSS2-TEST-RP-ACTION"; +} // namespace + +class ConfigBgpPolicyRoutingPolicyTermActionTest : public ConfigBgpTestBase { + protected: + // Stage `... routing-policy term 10 action ` WITHOUT + // clearing the staged session, so actions can accumulate across + // invocations. Returns the staged session JSON. + folly::dynamic stageAction(const std::vector& tokens) { + std::vector args = { + "config", + "protocol", + "bgp", + "policy", + "routing-policy", + kPolicy, + "term", + "10", + "action"}; + 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 .policy_entries[] term with sequence_number 10 of the policy named + // kPolicy, or nullptr. + static const folly::dynamic* findTerm(const folly::dynamic& config) { + if (config.count("policies") == 0 || + config["policies"].count("bgp_policy_statements") == 0) { + return nullptr; + } + for (const auto& policy : config["policies"]["bgp_policy_statements"]) { + if (policy.count("name") && policy["name"].asString() == kPolicy && + policy.count("policy_entries")) { + for (const auto& term : policy["policy_entries"]) { + if (term.count("sequence_number") && + term["sequence_number"].asInt() == 10) { + return &term; + } + } + } + } + return nullptr; + } + + // The action entry containing `field`, or nullptr. + static const folly::dynamic* findAction( + const folly::dynamic& term, + const std::string& field) { + if (term.count("policy_action_entries") == 0) { + return nullptr; + } + for (const auto& action : term["policy_action_entries"]) { + if (action.count(field)) { + return &action; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyRoutingPolicyTermActionTest, SetActionsAndCommit) { + discardSession(); + clearBgpSession(); + stageAction({"result", "CONTINUE"}); + stageAction({"set", "as-path", "prepend", "65000", "65000"}); + stageAction({"set", "community", "65000:100", "additive"}); + stageAction({"set", "local-pref", "200"}); + stageAction({"set", "med", "50"}); + stageAction({"set", "next-hop", "2001:db8::99"}); + stageAction({"set", "origin", "IGP"}); + stageAction({"set", "weight", "100"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + // bgpd's getRunningConfig RPC proves it parsed and adopted the term's + // action entries, one entry per action kind. + auto running = readRunningBgpConfigViaRpc(); + const auto* term = findTerm(running); + ASSERT_NE(term, nullptr) << "bgpd's running config has no routing-policy " + << kPolicy << " term 10"; + + // result CONTINUE → FlowControlAction.NEXT_TERM = 3 (integer on the + // SimpleJSON wire). + EXPECT_EQ((*term)["term_miss_action"].asInt(), 3); + + const auto* prepend = findAction(*term, "set_as_path_prepend"); + ASSERT_NE(prepend, nullptr); + EXPECT_EQ((*prepend)["set_as_path_prepend"]["asn"].asInt(), 65000); + EXPECT_EQ((*prepend)["set_as_path_prepend"]["repeat_times"].asInt(), 2); + + const auto* community = findAction(*term, "community_action"); + ASSERT_NE(community, nullptr); + // additive → CommunityActionType.ADD = 1 (bgpd reads the deprecated + // community_action struct; the type field rides the wire too: + // BgpPolicyActionType.COMMUNITY_LIST = 2). + EXPECT_EQ((*community)["type"].asInt(), 2); + EXPECT_EQ((*community)["community_action"]["action_type"].asInt(), 1); + EXPECT_EQ( + (*community)["community_action"]["communities"][0].asString(), + "65000:100"); + + const auto* localPref = findAction(*term, "set_local_pref"); + ASSERT_NE(localPref, nullptr); + EXPECT_EQ((*localPref)["set_local_pref"]["local_pref"].asInt(), 200); + + const auto* med = findAction(*term, "med_action"); + ASSERT_NE(med, nullptr); + EXPECT_EQ((*med)["med_action"]["med_value"].asInt(), 50); + // MedActionType.SET = 1. + EXPECT_EQ((*med)["med_action"]["med_action_type"].asInt(), 1); + + const auto* nexthop = findAction(*term, "set_nexthop"); + ASSERT_NE(nexthop, nullptr); + EXPECT_EQ( + (*nexthop)["set_nexthop"]["next_hop"]["next_hop_prefix"].asString(), + "2001:db8::99"); + + const auto* origin = findAction(*term, "set_origin"); + ASSERT_NE(origin, nullptr); + // Origin.IGP = 1. + EXPECT_EQ((*origin)["set_origin"].asInt(), 1); + + const auto* weight = findAction(*term, "weight_action"); + ASSERT_NE(weight, nullptr); + EXPECT_EQ((*weight)["weight_action"]["weight_value"].asInt(), 100); +} From 172e74d1b8538a960e938753830e33bac97899c8 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:27:49 +0000 Subject: [PATCH 07/18] Emit term action commands from bgp_json_to_cli.py A term's term_miss_action becomes `action result ACCEPT|REJECT|CONTINUE` (omitted for the NEXT_TERM default) and each policy_action_entries item one `action set` line, dispatched by BgpPolicyActionType: as-path prepend (asn repeated repeat_times), community [additive], local-pref, med (SET only), next-hop, origin and weight. Everything else (next-hop self, MED UPDATE/IGP, multi-community actions, GOTO and the other flow actions) is emitted as a `# WARNING:` line. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 149 ++++++++++++++++++ .../tools/tests/test_bgp_json_to_cli.py | 105 ++++++++++++ 2 files changed, 254 insertions(+) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 5e7b4fb436f0d..866084bbb1ed2 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -853,11 +853,160 @@ def generate_routing_policy_term_commands( if term.get("description"): commands.append(f"{prefix} description {escape_shell_arg(term['description'])}") # Nested term generators (action, match) hook in here. + commands.extend(generate_routing_policy_term_action_commands(prefix, term)) if not commands: commands.append(prefix) return commands +_POLICY_ACTION_TYPE_NAMES = { + 1: "AS_PATH_PREPEND", + 2: "COMMUNITY_LIST", + 3: "SET_LOCAL_PREF", + 4: "ORIGIN", + 5: "PERMIT", + 6: "DENY", + 7: "CONTINUE", + 8: "NEXT_HOP", + 9: "AS_PATH", + 10: "MED", + 11: "GOTO", + 12: "LBW_EXT_COMMUNITY", + 13: "AS_PATH_TO_AS_SET", + 14: "EXT_COMMUNITY_LIST", + 15: "WEIGHT", + 16: "ADD_BACKUP_ADDR", +} +_ORIGIN_NAMES = {1: "IGP", 2: "EGP", 3: "INCOMPLETE"} +_COMMUNITY_ACTION_TYPE_NAMES = {1: "ADD", 2: "SET", 3: "REMOVE"} +_MED_ACTION_TYPE_NAMES = {1: "SET", 2: "UPDATE", 3: "IGP"} +# term_miss_action -> `action result` keyword; other flow-control values have +# no CLI spelling. +_TERM_RESULT_KEYWORDS = {"ACCEPT": "ACCEPT", "DENY": "REJECT", "NEXT_TERM": "CONTINUE"} + + +def _enum_name(raw: Any, names: dict[int, str]) -> str: + if isinstance(raw, str): + return raw + return names.get(int(raw), str(raw)) + + +def _action_as_path_prepend( + prefix: str, _label: str, action: dict[str, Any] +) -> str | None: + prepend = action.get("set_as_path_prepend") or {} + if "asn" not in prepend: + return None + repeat = max(int(prepend.get("repeat_times", 1)), 1) + asns = " ".join([escape_shell_arg(prepend["asn"])] * repeat) + return f"{prefix} as-path prepend {asns}" + + +def _action_community(prefix: str, label: str, action: dict[str, Any]) -> str | None: + community_action = action.get("community_action") or {} + communities = community_action.get("communities") or [] + action_type = _enum_name( + community_action.get("action_type", 0), _COMMUNITY_ACTION_TYPE_NAMES + ) + if len(communities) == 1 and action_type in ("ADD", "SET"): + additive = " additive" if action_type == "ADD" else "" + return f"{prefix} community {escape_shell_arg(communities[0])}{additive}" + return _warning( + f"{label}: community action ({action_type}, {len(communities)} " + "communities) is not expressible as a single `community " + "[additive]`; not emitted" + ) + + +def _action_local_pref(prefix: str, _label: str, action: dict[str, Any]) -> str | None: + local_pref = (action.get("set_local_pref") or {}).get("local_pref") + return ( + None + if local_pref is None + else f"{prefix} local-pref {escape_shell_arg(local_pref)}" + ) + + +def _action_origin(prefix: str, _label: str, action: dict[str, Any]) -> str | None: + if "set_origin" not in action: + return None + return f"{prefix} origin {escape_shell_arg(_enum_name(action['set_origin'], _ORIGIN_NAMES))}" + + +def _action_next_hop(prefix: str, label: str, action: dict[str, Any]) -> str | None: + nexthop = action.get("set_nexthop") or {} + if nexthop.get("set_self"): + return _warning(f"{label}: next-hop self is not supported by bgpd; not emitted") + address = (nexthop.get("next_hop") or {}).get("next_hop_prefix") + return None if not address else f"{prefix} next-hop {escape_shell_arg(address)}" + + +def _action_med(prefix: str, label: str, action: dict[str, Any]) -> str | None: + med = action.get("med_action") or {} + med_type = _enum_name(med.get("med_action_type", 0), _MED_ACTION_TYPE_NAMES) + if med_type == "SET" and "med_value" in med: + return f"{prefix} med {escape_shell_arg(med['med_value'])}" + return _warning(f"{label}: med action {med_type} is not expressible; not emitted") + + +def _action_weight(prefix: str, _label: str, action: dict[str, Any]) -> str | None: + weight = (action.get("weight_action") or {}).get("weight_value") + return None if weight is None else f"{prefix} weight {escape_shell_arg(weight)}" + + +# BgpPolicyActionType name -> emitter of one `action set` line (or a warning); +# None means the payload the CLI needs is absent. +_ACTION_SET_EMITTERS = { + "AS_PATH_PREPEND": _action_as_path_prepend, + "COMMUNITY_LIST": _action_community, + "SET_LOCAL_PREF": _action_local_pref, + "ORIGIN": _action_origin, + "NEXT_HOP": _action_next_hop, + "MED": _action_med, + "WEIGHT": _action_weight, +} + + +def _generate_action_set_command( + term_prefix: str, label: str, action: dict[str, Any] +) -> list[str]: + """One `action set ...` line for a BgpPolicyAction, or a warning.""" + kind = _enum_name(action.get("type", 0), _POLICY_ACTION_TYPE_NAMES) + emitter = _ACTION_SET_EMITTERS.get(kind) + line = emitter(f"{term_prefix} action set", label, action) if emitter else None + if line is None: + return [ + _warning( + f"{label}: action {kind} is not expressible by the CLI; not emitted" + ) + ] + return [line] + + +def generate_routing_policy_term_action_commands( + term_prefix: str, term: dict[str, Any] +) -> list[str]: + """Generate the `action result` / `action set` commands of one term.""" + label = term_prefix.removeprefix("config protocol bgp policy ") + commands = [] + if "term_miss_action" in term: + result = _flow_control_action_name(term["term_miss_action"]) + if result in _TERM_RESULT_KEYWORDS: + if result != "NEXT_TERM": # the thrift and CLI default + commands.append( + f"{term_prefix} action result {_TERM_RESULT_KEYWORDS[result]}" + ) + else: + commands.append( + _warning( + f"{label}: term_miss_action {result} has no CLI equivalent; not emitted" + ) + ) + for action in term.get("policy_action_entries") or []: + commands.extend(_generate_action_set_command(term_prefix, label, action)) + return commands + + def generate_policy_commands(config: dict[str, Any]) -> list[str]: """Generate the `config protocol bgp policy ...` commands. diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index 586891c8128e8..6b8926c4db5e7 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 @@ -32,6 +32,7 @@ generate_prefix_list_commands, generate_prefix_list_entry_commands, generate_routing_policy_commands, + generate_routing_policy_term_action_commands, generate_routing_policy_term_commands, json_to_cli, ) @@ -1523,6 +1524,110 @@ def test_terms_emitted_inside_policy(self) -> None: ) +class GenerateRoutingPolicyTermActionCommandsTest(unittest.TestCase): + """Tests for generate_routing_policy_term_action_commands.""" + + TERM = "config protocol bgp policy routing-policy RM term 10" + + def test_result(self) -> None: + for raw, keyword in ((1, "ACCEPT"), (2, "REJECT"), ("DENY", "REJECT")): + commands = generate_routing_policy_term_action_commands( + self.TERM, {"term_miss_action": raw} + ) + self.assertEqual(commands, [f"{self.TERM} action result {keyword}"], raw) + + def test_result_next_term_default_omitted(self) -> None: + self.assertEqual( + generate_routing_policy_term_action_commands( + self.TERM, {"term_miss_action": 3} + ), + [], + ) + + def test_result_unexpressible_warns(self) -> None: + commands = generate_routing_policy_term_action_commands( + self.TERM, {"term_miss_action": 6} + ) + self.assertEqual(len(commands), 1) + self.assertTrue(commands[0].startswith("# WARNING:")) + + def test_set_actions(self) -> None: + term = { + "policy_action_entries": [ + {"type": 1, "set_as_path_prepend": {"asn": 65000, "repeat_times": 2}}, + { + "type": 2, + "community_action": {"communities": ["65000:1"], "action_type": 1}, + }, + { + "type": "COMMUNITY_LIST", + "community_action": {"communities": ["65000:2"], "action_type": 2}, + }, + {"type": 3, "set_local_pref": {"local_pref": 200}}, + {"type": 4, "set_origin": 3}, + { + "type": 8, + "set_nexthop": {"next_hop": {"next_hop_prefix": "10.0.0.1"}}, + }, + {"type": 10, "med_action": {"med_value": 50, "med_action_type": 1}}, + { + "type": 15, + "weight_action": {"weight_value": 7, "weight_action_type": 1}, + }, + ] + } + commands = generate_routing_policy_term_action_commands(self.TERM, term) + self.assertEqual( + commands, + [ + f"{self.TERM} action set as-path prepend 65000 65000", + f"{self.TERM} action set community 65000:1 additive", + f"{self.TERM} action set community 65000:2", + f"{self.TERM} action set local-pref 200", + f"{self.TERM} action set origin INCOMPLETE", + f"{self.TERM} action set next-hop 10.0.0.1", + f"{self.TERM} action set med 50", + f"{self.TERM} action set weight 7", + ], + ) + + def test_unexpressible_actions_warn(self) -> None: + term = { + "policy_action_entries": [ + {"type": 8, "set_nexthop": {"set_self": True}}, + {"type": 10, "med_action": {"med_value": 1, "med_action_type": 2}}, + {"type": 11}, + { + "type": 2, + "community_action": {"communities": ["a", "b"], "action_type": 2}, + }, + ] + } + commands = generate_routing_policy_term_action_commands(self.TERM, term) + self.assertEqual(len(commands), 4) + for c in commands: + self.assertTrue(c.startswith("# WARNING:"), c) + + def test_actions_emitted_inside_term(self) -> None: + commands = generate_routing_policy_term_commands( + "RM", + { + "sequence_number": 10, + "term_miss_action": 1, + "policy_action_entries": [ + {"type": 3, "set_local_pref": {"local_pref": 5}} + ], + }, + ) + self.assertEqual( + commands, + [ + f"{self.TERM} action result ACCEPT", + f"{self.TERM} action set local-pref 5", + ], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands.""" From 5fc814357c3f119e9bf14b9af20d344ddc342dad Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 16 Oct 2026 06:55:51 +0000 Subject: [PATCH 08/18] Use the canonical Meta copyright header in new files --- .../CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp | 2 +- .../action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h | 2 +- .../config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp | 2 +- .../ConfigBgpPolicyRoutingPolicyTermActionTest.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp index 55ac019fbe5f4..790c061f9276a 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h index 76e6daca5e80c..5f8e6260b3d58 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp index 32386c9688530..f0ec9971139d3 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. #include #include diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp index d4c7acf6dfc40..a2d6fc22f9f23 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. /** * End-to-end tests for `fboss2-dev config protocol bgp policy routing-policy From d1431fb9cedc4535d146643669cc7282fc5c0cc7 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 20 Oct 2026 23:17:02 +0000 Subject: [PATCH 09/18] Map action result to the match action and validate communities `action result` wrote term_miss_action, which bgpd applies when the term does NOT match. The action on a match is the term's single flow-control entry in policy_action_entries (PERMIT/DENY/CONTINUE/GOTO), defaulting to permit. As a result, ACCEPT failed bgpd's config load (the FlowControlAction miss path has no ACCEPT case), REJECT accepted matching routes and denied everything else, and CONTINUE was a no-op. `result` now replaces the flow-control entry with PERMIT/DENY/CONTINUE (bgpd rejects more than one per term) and leaves term_miss_action alone. bgp_json_to_cli.py emits `result` from that entry and warns on a non-default term_miss_action. `action set community` now rejects anything bgpd's parseCommunityStr would not accept (two uint16 parts separated by one ':'). bgpd treats any other value as a regex, which it only allows for REMOVE, so a typo used to fail the daemon's config load. --- ...otocolBgpPolicyRoutingPolicyTermAction.cpp | 65 +++++++++++++++---- ...igBgpPolicyRoutingPolicyTermActionTest.cpp | 36 ++++++++-- ...igBgpPolicyRoutingPolicyTermActionTest.cpp | 25 ++++--- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 24 +++---- .../tools/tests/test_bgp_json_to_cli.py | 31 +++++---- 5 files changed, 132 insertions(+), 49 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp index 790c061f9276a..c5a09227d1ced 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp @@ -13,8 +13,11 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include +#include #include +#include #include +#include #include #include #include @@ -49,10 +52,10 @@ constexpr std::string_view kNextHop = "next-hop"; constexpr std::string_view kOrigin = "origin"; constexpr std::string_view kWeight = "weight"; -// result values, mapped onto bgp_policy.FlowControlAction. GOTO-TERM (the -// sheet's fourth value) is deferred: FlowControlAction has no GOTO arm, and -// the thrift models "go to term X" as a per-action-entry next_term_id whose -// target the documented grammar does not carry. +// result values, mapped onto the flow-control BgpPolicyActionType of the +// term's match action (PERMIT/DENY/CONTINUE). GOTO-TERM (the sheet's fourth +// value) is deferred: bgpd's GOTO entry needs a next_term_id target that the +// documented grammar does not carry. constexpr std::string_view kResultAccept = "ACCEPT"; constexpr std::string_view kResultReject = "REJECT"; constexpr std::string_view kResultContinue = "CONTINUE"; @@ -76,7 +79,6 @@ using BgpPolicyAction = bgp::bgp_policy::BgpPolicyAction; using BgpPolicyActionType = bgp::bgp_policy::BgpPolicyActionType; using CommunityActionType = bgp::bgp_policy::CommunityActionType; using BgpPolicyTerm = bgp::bgp_policy::BgpPolicyTerm; -using FlowControlAction = bgp::bgp_policy::FlowControlAction; using MedActionType = bgp::bgp_policy::MedActionType; using Origin = bgp::bgp_policy::Origin; using WeightActionType = bgp::bgp_policy::WeightActionType; @@ -163,28 +165,49 @@ std::string runTermActionCommand( // ---- result handler -------------------------------------------------------- -std::optional parseResult(const std::string& value) { +std::optional parseResult(const std::string& value) { if (value == kResultAccept) { - return FlowControlAction::ACCEPT; + return BgpPolicyActionType::PERMIT; } if (value == kResultReject) { - return FlowControlAction::DENY; + return BgpPolicyActionType::DENY; } if (value == kResultContinue) { - return FlowControlAction::NEXT_TERM; + return BgpPolicyActionType::CONTINUE; } return std::nullopt; } -void setResult(BgpPolicyTerm& term, FlowControlAction result) { - term.term_miss_action() = result; +// The action bgpd's PolicyTerm takes when the term MATCHES is the single +// flow-control entry (PERMIT/DENY/CONTINUE/GOTO) in policy_action_entries; it +// defaults to permit when absent. term_miss_action is the action on a +// non-match and is left alone. bgpd rejects a term carrying more than one +// flow-control entry, so the result replaces any existing one. +bool isFlowControlEntry(const BgpPolicyAction& action) { + switch (*action.type()) { + case BgpPolicyActionType::PERMIT: + case BgpPolicyActionType::DENY: + case BgpPolicyActionType::CONTINUE: + case BgpPolicyActionType::GOTO: + return true; + default: + return false; + } +} + +void setResult(BgpPolicyTerm& term, BgpPolicyActionType result) { + auto& actions = *term.policy_action_entries(); + actions.erase( + std::remove_if(actions.begin(), actions.end(), isFlowControlEntry), + actions.end()); + actions.emplace_back().type() = result; } const AttrHandler& resultHandler() { static const std::string kResultValues = fmt::format("{}|{}|{}", kResultAccept, kResultReject, kResultContinue); static const AttrHandler kHandler = - enumAttr( + enumAttr( "result", kResultValues, parseResult, setResult); return kHandler; } @@ -274,6 +297,18 @@ Result actionAsPathPrepend(BgpPolicyTerm& term, const Tokens& values) { values.size())); } +// Mirrors bgpd's parseCommunityStr: exactly two uint16 parts separated by a +// single ':'. bgpd treats anything else as a regex, which it only allows for +// REMOVE, so an invalid value here would fail the daemon's config load. +bool isValidCommunity(const std::string& value) { + std::vector parts; + folly::split(':', value, parts); + return parts.size() == 2 && + std::all_of(parts.begin(), parts.end(), [](std::string_view part) { + return folly::tryTo(part).hasValue(); + }); +} + // [additive]: additive appends (ADD), the default // overwrites (SET). bgpd's CommunityAction reads the thrift-deprecated but // still load-bearing community_action.communities string list — the sheet's @@ -283,6 +318,12 @@ Result actionCommunity(BgpPolicyTerm& term, const Tokens& values) { (values.size() == 2 && values[1] != kCommunityAdditive)) { return err("Error: community requires [additive]"); } + if (!isValidCommunity(values[0])) { + return err( + fmt::format( + "Error: Invalid community '{}'; expected <0-65535>:<0-65535>", + values[0])); + } const bool additive = values.size() == 2; auto& communityAction = findOrCreateActionEntry(term, BgpPolicyActionType::COMMUNITY_LIST) diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp index f0ec9971139d3..9999ce91abebc 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -119,24 +119,34 @@ TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, argValidation) { // ============================================================================== TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setResult) { - // Each CLI value maps onto its FlowControlAction arm; the policy and term - // are implicitly created. + // Each CLI value maps onto the term's single flow-control action entry + // (the action bgpd takes on a match); the policy and term are implicitly + // created, and term_miss_action (the non-match action) is untouched. runResult({"ACCEPT"}); - EXPECT_EQ(*term().term_miss_action(), FlowControlAction::ACCEPT); + ASSERT_EQ(actions().size(), 1); + EXPECT_EQ(*actions()[0].type(), BgpPolicyActionType::PERMIT); + // Re-issuing replaces the entry: bgpd rejects more than one per term. runResult({"REJECT"}); - EXPECT_EQ(*term().term_miss_action(), FlowControlAction::DENY); + ASSERT_EQ(actions().size(), 1); + EXPECT_EQ(*actions()[0].type(), BgpPolicyActionType::DENY); + // Attribute actions are kept alongside the flow-control entry. + runSet({"local-pref", "200"}); auto result = runResult({"CONTINUE"}); EXPECT_THAT(result, HasSubstr("Successfully set result to: CONTINUE")); EXPECT_THAT(result, HasSubstr("for routing-policy RM100 term 10")); + ASSERT_EQ(actions().size(), 2); + EXPECT_EQ(*actions()[0].type(), BgpPolicyActionType::SET_LOCAL_PREF); + EXPECT_EQ(*actions()[1].type(), BgpPolicyActionType::CONTINUE); EXPECT_EQ(*term().term_miss_action(), FlowControlAction::NEXT_TERM); EXPECT_TRUE(sessionFileExists()); } TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, gotoTermRejected) { - // GOTO-TERM is documented but deferred (no FlowControlAction arm; the - // thrift models it per-action-entry). It must be rejected, not mismapped. + // GOTO-TERM is documented but deferred (bgpd's GOTO entry needs a + // next_term_id the grammar does not carry). It must be rejected, not + // mismapped. auto result = runResult({"GOTO-TERM"}); EXPECT_THAT( result, @@ -200,6 +210,20 @@ TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setCommunity) { std::vector({"65000:200"})); } +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, + invalidCommunityRejected) { + // bgpd treats a non-: value as a regex, which it rejects + // for SET/ADD at config load, so the CLI must refuse it up front. + for (const auto& bad : + {"65000:1OO", "65000", "65536:1", "1:2:3", "65000:", ":100"}) { + auto result = runSet({"community", bad}); + EXPECT_THAT(result, HasSubstr("Invalid community")) << bad; + } + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()); +} + TEST_F(CmdConfigBgpPolicyRoutingPolicyTermActionTestFixture, setLocalPref) { runSet({"local-pref", "4294967295"}); ASSERT_EQ(actions().size(), 1); diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp index a2d6fc22f9f23..47029431f906e 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -4,12 +4,11 @@ * End-to-end tests for `fboss2-dev config protocol bgp policy routing-policy * term action result|set ...`. * - * Scope: the action level of a routing-policy term (term_miss_action and - * bgp_policy.BgpPolicyAction entries in .policy_entries[] - * .policy_action_entries[]). Like `term`, `action` is a real CLI11 - * subcommand, so these tests exercise the two-deep subcommand dispatch - * through the full CLI parse. Every test stages the change AND commits it, - * then asserts the values landed at the correct thrift field paths inside + * Scope: the action level of a routing-policy term (bgp_policy.BgpPolicyAction + * entries in .policy_entries[].policy_action_entries[]). Like `term`, `action` + * is a real CLI11 subcommand, so these tests exercise the two-deep subcommand + * dispatch through the full CLI parse. Every test stages the change AND commits + * it, then asserts the values landed at the correct thrift field paths inside * bgpd's running config (via getRunningConfig RPC). * * Requirements: @@ -100,7 +99,7 @@ class ConfigBgpPolicyRoutingPolicyTermActionTest : public ConfigBgpTestBase { TEST_F(ConfigBgpPolicyRoutingPolicyTermActionTest, SetActionsAndCommit) { discardSession(); clearBgpSession(); - stageAction({"result", "CONTINUE"}); + stageAction({"result", "REJECT"}); stageAction({"set", "as-path", "prepend", "65000", "65000"}); stageAction({"set", "community", "65000:100", "additive"}); stageAction({"set", "local-pref", "200"}); @@ -119,8 +118,16 @@ TEST_F(ConfigBgpPolicyRoutingPolicyTermActionTest, SetActionsAndCommit) { ASSERT_NE(term, nullptr) << "bgpd's running config has no routing-policy " << kPolicy << " term 10"; - // result CONTINUE → FlowControlAction.NEXT_TERM = 3 (integer on the - // SimpleJSON wire). + // result REJECT → a flow-control entry of BgpPolicyActionType.DENY = 6 + // (integer on the SimpleJSON wire), the action bgpd takes on a match; + // term_miss_action keeps its NEXT_TERM = 3 default. + int denyEntries = 0; + for (const auto& action : (*term)["policy_action_entries"]) { + if (action["type"].asInt() == 6) { + ++denyEntries; + } + } + EXPECT_EQ(denyEntries, 1); EXPECT_EQ((*term)["term_miss_action"].asInt(), 3); const auto* prepend = findAction(*term, "set_as_path_prepend"); diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 866084bbb1ed2..19ea7d61f6999 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -880,9 +880,9 @@ def generate_routing_policy_term_commands( _ORIGIN_NAMES = {1: "IGP", 2: "EGP", 3: "INCOMPLETE"} _COMMUNITY_ACTION_TYPE_NAMES = {1: "ADD", 2: "SET", 3: "REMOVE"} _MED_ACTION_TYPE_NAMES = {1: "SET", 2: "UPDATE", 3: "IGP"} -# term_miss_action -> `action result` keyword; other flow-control values have -# no CLI spelling. -_TERM_RESULT_KEYWORDS = {"ACCEPT": "ACCEPT", "DENY": "REJECT", "NEXT_TERM": "CONTINUE"} +# Flow-control BgpPolicyActionType (the action bgpd takes when the term +# matches) -> `action result` keyword; GOTO has no CLI spelling. +_TERM_RESULT_KEYWORDS = {"PERMIT": "ACCEPT", "DENY": "REJECT", "CONTINUE": "CONTINUE"} def _enum_name(raw: Any, names: dict[int, str]) -> str: @@ -990,19 +990,21 @@ def generate_routing_policy_term_action_commands( label = term_prefix.removeprefix("config protocol bgp policy ") commands = [] if "term_miss_action" in term: - result = _flow_control_action_name(term["term_miss_action"]) - if result in _TERM_RESULT_KEYWORDS: - if result != "NEXT_TERM": # the thrift and CLI default - commands.append( - f"{term_prefix} action result {_TERM_RESULT_KEYWORDS[result]}" - ) - else: + miss_action = _flow_control_action_name(term["term_miss_action"]) + if miss_action != "NEXT_TERM": # the thrift default commands.append( _warning( - f"{label}: term_miss_action {result} has no CLI equivalent; not emitted" + f"{label}: term_miss_action {miss_action} has no CLI " + "equivalent; not emitted" ) ) for action in term.get("policy_action_entries") or []: + kind = _enum_name(action.get("type", 0), _POLICY_ACTION_TYPE_NAMES) + if kind in _TERM_RESULT_KEYWORDS: + commands.append( + f"{term_prefix} action result {_TERM_RESULT_KEYWORDS[kind]}" + ) + continue commands.extend(_generate_action_set_command(term_prefix, label, action)) 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 6b8926c4db5e7..1af7f165590a5 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 @@ -1530,13 +1530,19 @@ class GenerateRoutingPolicyTermActionCommandsTest(unittest.TestCase): TERM = "config protocol bgp policy routing-policy RM term 10" def test_result(self) -> None: - for raw, keyword in ((1, "ACCEPT"), (2, "REJECT"), ("DENY", "REJECT")): + # The result is the flow-control action entry (the action on a match). + for raw, keyword in ( + (5, "ACCEPT"), + (6, "REJECT"), + (7, "CONTINUE"), + ("DENY", "REJECT"), + ): commands = generate_routing_policy_term_action_commands( - self.TERM, {"term_miss_action": raw} + self.TERM, {"policy_action_entries": [{"type": raw}]} ) self.assertEqual(commands, [f"{self.TERM} action result {keyword}"], raw) - def test_result_next_term_default_omitted(self) -> None: + def test_term_miss_action_default_omitted(self) -> None: self.assertEqual( generate_routing_policy_term_action_commands( self.TERM, {"term_miss_action": 3} @@ -1544,12 +1550,15 @@ def test_result_next_term_default_omitted(self) -> None: [], ) - def test_result_unexpressible_warns(self) -> None: - commands = generate_routing_policy_term_action_commands( - self.TERM, {"term_miss_action": 6} - ) - self.assertEqual(len(commands), 1) - self.assertTrue(commands[0].startswith("# WARNING:")) + def test_term_miss_action_non_default_warns(self) -> None: + # term_miss_action is the action on a non-match; the CLI has no + # spelling for it. + for raw in (1, 2, 6): + commands = generate_routing_policy_term_action_commands( + self.TERM, {"term_miss_action": raw} + ) + self.assertEqual(len(commands), 1, raw) + self.assertTrue(commands[0].startswith("# WARNING:"), raw) def test_set_actions(self) -> None: term = { @@ -1613,9 +1622,9 @@ def test_actions_emitted_inside_term(self) -> None: "RM", { "sequence_number": 10, - "term_miss_action": 1, "policy_action_entries": [ - {"type": 3, "set_local_pref": {"local_pref": 5}} + {"type": 5}, + {"type": 3, "set_local_pref": {"local_pref": 5}}, ], }, ) From 74a0b147a59e9b13e6505bb975c50b8e2b857d51 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 23 Oct 2026 18:36:06 +0000 Subject: [PATCH 10/18] Mirror the Meta import amendments from the routing-policy level Same IS_OSS include split and ensure() on the optional policies field as landed for #1488, applied to the term action files. Co-Authored-By: Claude Fable 5.1 --- ...ConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp | 11 ++++++++--- .../CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp | 9 +++++++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp index c5a09227d1ced..afe84228751f3 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp @@ -16,7 +16,6 @@ #include #include #include -#include #include #include #include @@ -29,7 +28,6 @@ #include #include #include -#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h" @@ -38,6 +36,13 @@ #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fmt/format.h" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + namespace facebook::fboss { namespace { @@ -157,7 +162,7 @@ std::string runTermActionCommand( policy.policy_entries()->pop_back(); } if (policyCreated) { - cfg.policies()->bgp_policy_statements()->pop_back(); + cfg.policies().ensure().bgp_policy_statements()->pop_back(); } } return result.message; diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp index 9999ce91abebc..ff47cd9aebeae 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -1,6 +1,5 @@ // Copyright (c) Meta Platforms, Inc. and affiliates. -#include #include #include @@ -10,13 +9,19 @@ #include #include -#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" #include "fboss/cli/fboss2/utils/HostInfo.h" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + using namespace ::testing; namespace facebook::fboss { From 981922acb402d11a21fb6768d84fa798ccfcf782 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 7 Aug 2026 05:09:11 +0000 Subject: [PATCH 11/18] [Nexthop] fboss2 bgp policy routing-policy term match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `config protocol bgp policy routing-policy term match from ` — the match level of a routing-policy term, as a CLI11 subcommand with its own handler alongside `action`; the policy and term args arrive through the ancestor-args tuple, and ancestor attributes mixed with a match command are rejected since only the leaf runs. Scoped to the three match kinds bgpd actually implements. Reading private-BGP's PolicyTerm.cpp turned up two things the sheet's Thrift reference column does not convey, but its JSON field path column does: 1. bgpd reads BgpPolicyTerm.policy_match_entries — a single BgpPolicyMatch, marked @thrift.Deprecated. Its match loop is guarded by `if (term.policy_match_entries())`, and NOTHING in private-BGP reads the newer `policy_matches` list, so matches written there are silently ignored: the config round-trips through getRunningConfig while bgpd applies no match at all. Same for the atomic payloads — bgpd reads the deprecated as_path_filters/communities_filter/prefix_filters, not the newer as_path_list/community_list/prefix_list. 2. createPolicyAttributeMatchItem has no LOCAL_PREFERENCE, MED or NEXT_HOP case, so those atomic types hit `default:` and throw "BgpPolicyAtomicMatch Config input error for type", crash-looping the daemon. `from local-pref`, `from med` and `from next-hop` offered; they need a private-BGP change first, and a unit test pins that they stay rejected. CommunityMatch's constructor throws "The attribute \"communities\" is empty" unless the inline communities list is non-empty, and CommunityMatch::PopulateReferences -- the only thing that resolves community_list_names -- runs later, from PolicyManager, once every Policy is already constructed. The by-name reference path is therefore unreachable, so only inlined values would work, which is a snapshot rather than a reference. Same unit test pins it as rejected. - `from ` writes one BgpPolicyAtomicMatch per match kind (keyed by BgpPolicyAtomicMatchType; re-issuing a kind updates its entry) into policy_match_entries, composing under its default AND — which is also the only operator bgpd accepts for more than one entry: The two list-typed matches take a full inline object with no name-only union arm, and bgpd treats that object's `name` as the reference: the match class keeps the name, then PolicyManager::PopulateReferences swaps in the real list from its by-name map. A name-only object is exactly how a by-name match is expressed. - setters are named functions, so the registry reads as a table: one line per attribute giving its dispatch key, value shape, and setter. - unit tests (8) + a commit-path integration test. The integration assertions are load-bearing here in a way staged-config ones are not: bgpd constructs a Policy from every statement, so a wrong encoding crash-loops it and fails the test rather than sitting inert. Co-Authored-By: Claude Fable 5 --- cmake/CliFboss2.cmake | 2 + cmake/CliFboss2TestConfig.cmake | 1 + cmake/CliFboss2TestIntegrationTest.cmake | 1 + fboss/cli/fboss2/BUCK | 2 + fboss/cli/fboss2/CmdListConfig.cpp | 57 ++-- ...rotocolBgpPolicyRoutingPolicyTermMatch.cpp | 273 ++++++++++++++++++ ...gProtocolBgpPolicyRoutingPolicyTermMatch.h | 89 ++++++ fboss/cli/fboss2/test/config/BUCK | 1 + ...figBgpPolicyRoutingPolicyTermMatchTest.cpp | 221 ++++++++++++++ fboss/cli/fboss2/test/integration_test/BUCK | 1 + ...figBgpPolicyRoutingPolicyTermMatchTest.cpp | 185 ++++++++++++ 11 files changed, 810 insertions(+), 23 deletions(-) create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp create mode 100644 fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h create mode 100644 fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp create mode 100644 fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 4a43a419f312f..5e40b091ce010 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -991,6 +991,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.cpp fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index b4d088624560a..29bcae6eab0ff 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -21,6 +21,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index 9bc34db9073e1..43b781ff6498a 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -25,6 +25,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.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 9bc047c8e61f6..e9cc54dd31720 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1222,6 +1222,7 @@ cpp_library( "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp", "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp", "commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp", + "commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1380,6 +1381,7 @@ cpp_library( "commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h", "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h", "commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h", + "commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 043e8af67f82f..ad990ca62a599 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -50,6 +50,7 @@ #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.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" @@ -495,29 +496,39 @@ const CommandTree& kConfigCommandTree() { argRegistrar< CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits>, {{ - "action", - "Configure a term action", - {{ - "result", - "Set the term result: " - "", - commandHandler< - CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult>, - argRegistrar< - CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits>, - }, - { - "set", - "Set a route attribute: " - "as-path prepend|community|" - "local-pref|med|next-hop|" - "origin|weight ...", - commandHandler< - CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet>, - argRegistrar< - CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits>, - }}, - }}, + "action", + "Configure a term action", + {{ + "result", + "Set the term result: " + "", + commandHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResult>, + argRegistrar< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionResultTraits>, + }, + { + "set", + "Set a route attribute: " + "as-path prepend|community|" + "local-pref|med|next-hop|" + "origin|weight ...", + commandHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSet>, + argRegistrar< + CmdConfigProtocolBgpPolicyRoutingPolicyTermActionSetTraits>, + }}, + }, + { + "match", + "Configure a term match: " + "from as-path-list|origin|" + "prefix-list ", + commandHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch>, + argRegistrar< + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatchTraits>, + }}, }}, }}, }, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp new file mode 100644 index 0000000000000..f2bd59a1c94d6 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp @@ -0,0 +1,273 @@ +/* + * 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/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.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/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.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 composed `from ` dispatch keys, exactly as documented. Kept +// here so the valid-attribute set and the handler table stay in sync. +// +// Four of the seven documented `match from` attributes are NOT offered, +// because bgpd cannot apply them (each was proven by crash-looping bgpd on a +// device, then confirmed in private-BGP): +// +// - local-pref, med, next-hop (NOS-6683/6684/6685): +// createPolicyAttributeMatchItem has no LOCAL_PREFERENCE, MED or NEXT_HOP +// case, so those atomic types hit `default:` and throw +// "BgpPolicyAtomicMatch Config input error for type". +// - community-list (NOS-6682): CommunityMatch's constructor throws +// "The attribute \"communities\" is empty" unless the inline communities +// list is non-empty, and CommunityMatch::PopulateReferences — the only +// thing that resolves community_list_names — runs later, from +// PolicyManager, once every Policy is already constructed. The by-name +// reference path is therefore unreachable; only inlined community values +// work, which is a snapshot rather than a reference. +// +// All four need a private-BGP change before the CLI can accept them. +constexpr std::string_view kFromKeyword = "from"; +constexpr std::string_view kFromAsPathList = "from as-path-list"; +constexpr std::string_view kFromOrigin = "from origin"; +constexpr std::string_view kFromPrefixList = "from prefix-list"; + +// origin values (bgp_policy.Origin names). +constexpr std::string_view kOriginIgp = "IGP"; +constexpr std::string_view kOriginEgp = "EGP"; +constexpr std::string_view kOriginIncomplete = "INCOMPLETE"; + +using BgpPolicyAtomicMatch = bgp::bgp_policy::BgpPolicyAtomicMatch; +using BgpPolicyAtomicMatchType = bgp::bgp_policy::BgpPolicyAtomicMatchType; +using BgpPolicyTerm = bgp::bgp_policy::BgpPolicyTerm; +using Origin = bgp::bgp_policy::Origin; +using bgpcli::AttrHandler; +using bgpcli::enumAttr; +using bgpcli::Result; +using bgpcli::stringAttr; + +// Find the atomic match of `type`, creating it (and the term's match object) +// if absent. Atomic entries are keyed by BgpPolicyAtomicMatchType, so +// re-issuing a match kind updates its entry rather than appending. +// +// The container is BgpPolicyTerm.policy_match_entries — a single +// BgpPolicyMatch, marked @thrift.Deprecated but the ONLY one bgpd reads: +// PolicyTerm.cpp guards its match loop on `if (term.policy_match_entries())`, +// and nothing in private-BGP reads the newer `policy_matches` list at all, so +// matches written there are silently ignored. Entries compose under +// BgpPolicyMatch's default AND, which is also the only operator bgpd accepts +// for more than one entry. +BgpPolicyAtomicMatch& findOrCreateAtomicMatch( + BgpPolicyTerm& term, + BgpPolicyAtomicMatchType type) { + auto& entries = *term.policy_match_entries().ensure().match_entries(); + for (auto& entry : entries) { + if (*entry.type() == type) { + return entry; + } + } + entries.emplace_back(); + auto& entry = entries.back(); + entry.type() = type; + return entry; +} + +// ---- match setters --------------------------------------------------------- +// Each writes one already-parsed, already-validated value into the term's +// atomic match of the matching type. Parsing and message text belong to the +// shared factories in BgpCliAttrHandlers.h. +// +// The list-typed matches take a full inline object rather than a name-only +// union arm, and a reference is expressed by its *_list_names field — NOT by +// the object's `name`, which is only a label. bgpd's match class keeps those +// names, then PolicyManager::PopulateReferences resolves each against its +// by-name map and merges in the referenced list's contents. Writing `name` +// instead leaves the match with nothing to compare against, so it silently +// matches nothing rather than failing loudly. + +void setAsPathList(BgpPolicyTerm& term, const std::string& name) { + findOrCreateAtomicMatch(term, BgpPolicyAtomicMatchType::AS_PATH) + .as_path_filters() + .ensure() + .as_path_list_names() + .ensure() = {name}; +} + +void setOrigin(BgpPolicyTerm& term, Origin origin) { + findOrCreateAtomicMatch(term, BgpPolicyAtomicMatchType::ORIGIN).origin() = + origin; +} + +void setPrefixList(BgpPolicyTerm& term, const std::string& name) { + findOrCreateAtomicMatch(term, BgpPolicyAtomicMatchType::PREFIX_LIST) + .prefix_filters() + .ensure() + .prefix_list_names() = {name}; +} + +std::optional parseOrigin(const std::string& value) { + if (value == kOriginIgp) { + return Origin::IGP; + } + if (value == kOriginEgp) { + return Origin::EGP; + } + if (value == kOriginIncomplete) { + return Origin::INCOMPLETE; + } + return std::nullopt; +} + +// ---- match attribute registry ---------------------------------------------- +// One line per supported attribute: its dispatch key, its value shape, and +// the setter that stores it. +const std::map, std::less<>>& +matchAttrHandlers() { + static const std::string kOriginValues = + fmt::format("{}|{}|{}", kOriginIgp, kOriginEgp, kOriginIncomplete); + static const std::map, std::less<>> + kHandlers = { + {std::string(kFromAsPathList), + stringAttr("as-path-list", "name", setAsPathList)}, + {std::string(kFromOrigin), + enumAttr( + "origin", kOriginValues, parseOrigin, setOrigin)}, + {std::string(kFromPrefixList), + stringAttr("prefix-list", "name", setPrefixList)}, + }; + return kHandlers; +} + +std::string validAttrList() { + std::string out; + for (const auto& [name, _] : matchAttrHandlers()) { + 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 BgpRoutingPolicyTermActionSetConfig). +BgpRoutingPolicyTermMatchConfig::BgpRoutingPolicyTermMatchConfig( + std::vector v) + : utils::BaseObjectArgType(v) { + if (v.empty() || v[0] != kFromKeyword) { + throw std::invalid_argument( + "Error: match requires `from `"); + } + if (v.size() < 2) { + throw std::invalid_argument( + fmt::format( + "Error: `from` requires an . Valid attributes: {}", + validAttrList())); + } + attr_ = fmt::format("{} {}", kFromKeyword, v[1]); + values_.assign(v.begin() + 2, v.end()); + + if (matchAttrHandlers().find(attr_) == matchAttrHandlers().end()) { + throw std::invalid_argument( + fmt::format( + "Error: unknown match attribute '{}'. Valid attributes: {}", + attr_, + validAttrList())); + } +} + +CmdConfigProtocolBgpPolicyRoutingPolicyTermMatchTraits::RetType +CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch::queryClient( + const HostInfo& /* hostInfo */, + const BgpRoutingPolicyConfig& policyArgs, + const BgpRoutingPolicyTermConfig& termArgs, + const ObjectArgType& args) { + // Ancestor-level attributes mixed with a match command parse, but only + // the leaf (this command) runs — reject instead of silently dropping them. + if (!policyArgs.attr().empty() || !termArgs.attr().empty()) { + return fmt::format( + "Error: configure routing-policy/term attributes and match in " + "separate commands (got attribute '{}' alongside match)", + !policyArgs.attr().empty() ? policyArgs.attr() : termArgs.attr()); + } + + auto& session = ConfigSession::getInstance(); + auto& cfg = session.getBgpConfig(); + const bool policyCreated = + !bgpcli::routingPolicyExists(cfg, policyArgs.policyName()); + auto& policy = + bgpcli::findOrCreateRoutingPolicy(cfg, policyArgs.policyName()); + const bool termCreated = + !bgpcli::routingPolicyTermExists(policy, termArgs.seqNum()); + auto& term = bgpcli::findOrCreateRoutingPolicyTerm(policy, termArgs.seqNum()); + + // The attribute is guaranteed valid: BgpRoutingPolicyTermMatchConfig's + // constructor rejects an unknown attribute before we get here. + Result result = + matchAttrHandlers().find(args.attr())->second(term, args.values()); + + if (result.ok) { + result.message += fmt::format( + " for routing-policy {} term {} match", + policyArgs.policyName(), + termArgs.seqNum()); + session.saveBgpConfig(); + result.message += + fmt::format("\nConfig saved to: {}", session.getBgpSessionConfigPath()); + } else { + // A rejected value must not leave a phantom term (or a phantom policy + // implicitly created for it) visible to later lookups in this process. + if (termCreated) { + policy.policy_entries()->pop_back(); + } + if (policyCreated) { + cfg.policies()->bgp_policy_statements()->pop_back(); + } + } + return result.message; +} + +void CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch::printOutput( + const RetType& output) { + std::cout << output << std::endl; +} + +// Explicit template instantiation +template void CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch, + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatchTraits>::run(); + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h new file mode 100644 index 0000000000000..32b9e0987b3e9 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h @@ -0,0 +1,89 @@ +/* + * 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/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" +#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" +#include "fboss/cli/fboss2/utils/HostInfo.h" + +namespace facebook::fboss { + +// Parsed `match from ...`, validated at construction. +// The policy and term this match belongs to are supplied by the ancestor +// commands' args. +// +// Grammar (the subset of the FBOSS proposed syntax bgpd supports): +// ... term match from as-path-list +// ... term match from origin +// ... term match from prefix-list +// +// `from community-list`, `from local-pref`, `from med` and `from next-hop` +// are documented but not offered — bgpd either has no match case for the +// atomic type or cannot resolve the reference. See the .cpp for the detail. +class BgpRoutingPolicyTermMatchConfig + : public utils::BaseObjectArgType { + public: + // NOLINTNEXTLINE(google-explicit-constructor) + /* implicit */ BgpRoutingPolicyTermMatchConfig(std::vector v); + 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 attr_; // matched dispatch key (never empty; no bare create) + std::vector values_; +}; + +// The match level of a routing-policy term as its own CLI11 subcommand, +// like `action`; the policy and term args arrive through the ancestor-args +// tuple. +struct CmdConfigProtocolBgpPolicyRoutingPolicyTermMatchTraits + : public WriteCommandTraits { + using ParentCmd = CmdConfigProtocolBgpPolicyRoutingPolicyTerm; + static void addCliArg(CLI::App& cmd, std::vector& args) { + // Match has no nested subcommands; stop CLI11's parent-chain + // fallthrough from stealing a value token that spells `match`/`action`. + cmd.positionals_at_end(); + cmd.add_option("args", args, "from ..."); + } + using ObjectArgType = BgpRoutingPolicyTermMatchConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch + : public CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch, + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatchTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatchTraits::ObjectArgType; + using RetType = + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatchTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BgpRoutingPolicyConfig& policyArgs, + const BgpRoutingPolicyTermConfig& termArgs, + 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 62b7600286bdc..aea74e757b6e6 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -20,6 +20,7 @@ cpp_unittest( "CmdConfigBgpPolicyPrefixListEntryTest.cpp", "CmdConfigBgpPolicyPrefixListTest.cpp", "CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp", + "CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp", "CmdConfigBgpPolicyRoutingPolicyTermTest.cpp", "CmdConfigBgpPolicyRoutingPolicyTest.cpp", "CmdConfigCoppTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp new file mode 100644 index 0000000000000..d749e3bc8f11c --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -0,0 +1,221 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.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 { + +// The match 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 +// CmdConfigBgpPolicyRoutingPolicyTermActionTest). +class CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture + : public CmdConfigTestBase { + public: + CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture() + : CmdConfigTestBase( + "bgp_routing_policy_term_match_test_%%%%-%%%%-%%%%", + "") {} + + void SetUp() override { + CmdConfigTestBase::SetUp(); + setupTestableConfigSession(); + } + + // Invoke the match handler the way the framework does: the ancestors' + // parsed args plus the match's own tokens. + std::string runMatch(const std::vector& matchTokens) { + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch cmd; + HostInfo hostInfo("testhost"); + return cmd.queryClient( + hostInfo, + BgpRoutingPolicyConfig({"RM100"}), + BgpRoutingPolicyTermConfig({"10"}), + BgpRoutingPolicyTermMatchConfig(matchTokens)); + } + + const std::vector& policies() { + return *ConfigSession::getInstance() + .getBgpConfig() + .policies() + .ensure() + .bgp_policy_statements(); + } + + const bgp::bgp_policy::BgpPolicyTerm& term() { + return (*policies()[0].policy_entries())[0]; + } + + // The atomic match entries of the term's match object. bgpd only reads the + // (thrift-deprecated) policy_match_entries container, so that is where the + // CLI must write and what these tests assert on. + const std::vector& atomics() { + return *term().policy_match_entries()->match_entries(); + } + + bool sessionFileExists() { + return std::filesystem::exists( + ConfigSession::getInstance().getBgpSessionConfigPath()); + } +}; + +// ============================================================================== +// BgpRoutingPolicyTermMatchConfig (arg) validation +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, argValidation) { + auto prefixList = + BgpRoutingPolicyTermMatchConfig({"from", "prefix-list", "PL1"}); + EXPECT_EQ(prefixList.attr(), "from prefix-list"); + EXPECT_EQ(prefixList.values(), std::vector({"PL1"})); + + // Invalid: empty, missing `from`, bare `from`, unknown attribute. + EXPECT_THROW(BgpRoutingPolicyTermMatchConfig({}), std::invalid_argument); + EXPECT_THROW( + BgpRoutingPolicyTermMatchConfig({"prefix-list", "PL1"}), + std::invalid_argument); + EXPECT_THROW( + BgpRoutingPolicyTermMatchConfig({"from"}), std::invalid_argument); + EXPECT_THROW( + BgpRoutingPolicyTermMatchConfig({"from", "no-such-attr", "1"}), + std::invalid_argument); +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, + unsupportedMatchesRejected) { + // These four are documented in the sheet but bgpd cannot apply them: + // local-pref/med/next-hop have no match case at all, and community-list's + // match validates its inline communities before anything resolves the + // by-name reference. A term carrying any of them aborts the daemon, so they + // must be rejected at parse time rather than staged. + for (const auto& attr : {"community-list", "local-pref", "med", "next-hop"}) { + EXPECT_THROW( + BgpRoutingPolicyTermMatchConfig({"from", attr, "1"}), + std::invalid_argument) + << "expected `from " << attr << "` to be rejected"; + } +} + +// ============================================================================== +// from — atomic matches keyed by type +// ============================================================================== + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, fromAsPathList) { + auto result = runMatch({"from", "as-path-list", "ASPL1"}); + EXPECT_THAT(result, HasSubstr("Successfully set as-path-list to: ASPL1")); + EXPECT_THAT(result, HasSubstr("for routing-policy RM100 term 10 match")); + ASSERT_EQ(atomics().size(), 1); + EXPECT_EQ(*atomics()[0].type(), BgpPolicyAtomicMatchType::AS_PATH); + // The reference lives in as_path_list_names, NOT in `name` (a label bgpd + // never resolves): AsPathMatch keeps these names and + // PolicyManager::PopulateReferences merges in asPathListMap_'s entry. + EXPECT_EQ( + *atomics()[0].as_path_filters()->as_path_list_names(), + std::vector({"ASPL1"})); + EXPECT_TRUE(sessionFileExists()); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, fromOrigin) { + auto result = runMatch({"from", "origin", "EGP"}); + EXPECT_THAT(result, HasSubstr("Successfully set origin to: EGP")); + ASSERT_EQ(atomics().size(), 1); + EXPECT_EQ(*atomics()[0].type(), BgpPolicyAtomicMatchType::ORIGIN); + EXPECT_EQ(*atomics()[0].origin(), Origin::EGP); + + auto invalid = runMatch({"from", "origin", "BGP"}); + EXPECT_THAT( + invalid, + HasSubstr("Invalid origin value 'BGP'; expected IGP|EGP|INCOMPLETE")); + EXPECT_EQ(*atomics()[0].origin(), Origin::EGP); +} + +TEST_F(CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, fromPrefixList) { + runMatch({"from", "prefix-list", "PL1"}); + ASSERT_EQ(atomics().size(), 1); + EXPECT_EQ(*atomics()[0].type(), BgpPolicyAtomicMatchType::PREFIX_LIST); + EXPECT_EQ( + *atomics()[0].prefix_filters()->prefix_list_names(), + std::vector({"PL1"})); +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, + matchKindsAreKeyedByType) { + // Each match kind gets one atomic entry; re-issuing a kind updates it. The + // entries compose under the match object's default AND, which is the only + // operator bgpd accepts for more than one entry. + runMatch({"from", "prefix-list", "PL1"}); + runMatch({"from", "as-path-list", "ASPL1"}); + runMatch({"from", "origin", "IGP"}); + ASSERT_EQ(atomics().size(), 3); + EXPECT_EQ( + *term().policy_match_entries()->match_logic_type(), + bgp::routing_policy::BooleanOperator::AND); + + runMatch({"from", "prefix-list", "PL2"}); + ASSERT_EQ(atomics().size(), 3); + size_t prefixListEntries = 0; + for (const auto& atomic : atomics()) { + if (*atomic.type() == BgpPolicyAtomicMatchType::PREFIX_LIST) { + ++prefixListEntries; + EXPECT_EQ( + *atomic.prefix_filters()->prefix_list_names(), + std::vector({"PL2"})); + } + } + EXPECT_EQ(prefixListEntries, 1); +} + +// ============================================================================== +// Reject paths — the error is surfaced and nothing is persisted +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, + rejectedValueRollsBackPhantoms) { + auto result = runMatch({"from", "prefix-list"}); + EXPECT_THAT(result, HasSubstr("Error: prefix-list requires ")); + // The rejected value must not leave a phantom term or policy behind. + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, + ancestorAttributeAlongsideMatchRejected) { + // The CLI parse accepts `... term 10 description x match from origin IGP`, + // but only the leaf runs — the term attribute must be rejected, not + // silently dropped. + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch cmd; + HostInfo hostInfo("testhost"); + auto result = cmd.queryClient( + hostInfo, + BgpRoutingPolicyConfig({"RM100"}), + BgpRoutingPolicyTermConfig({"10", "description", "x"}), + BgpRoutingPolicyTermMatchConfig({"from", "origin", "IGP"})); + EXPECT_THAT( + result, HasSubstr("Error: configure routing-policy/term attributes")); + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 4afdd454f09ad..63d97bdf7c4d7 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -30,6 +30,7 @@ cpp_binary( "ConfigBgpPolicyPrefixListEntryTest.cpp", "ConfigBgpPolicyPrefixListTest.cpp", "ConfigBgpPolicyRoutingPolicyTermActionTest.cpp", + "ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp", "ConfigBgpPolicyRoutingPolicyTermTest.cpp", "ConfigBgpPolicyRoutingPolicyTest.cpp", "ConfigBgpSessionTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp new file mode 100644 index 0000000000000..de33abec734a2 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -0,0 +1,185 @@ +// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. + +/** + * End-to-end tests for `fboss2-dev config protocol bgp policy routing-policy + * term match from `. + * + * Scope: the match level of a routing-policy term + * (bgp_policy.BgpPolicyAtomicMatch entries under the term's + * .policy_entries[].policy_match_entries). Like `action`, `match` is a real + * CLI11 subcommand, so these tests exercise the two-deep subcommand dispatch + * through the full CLI parse. Every test stages the change AND commits it, + * then asserts the values landed at the correct thrift field paths inside + * bgpd's running config (via getRunningConfig RPC). + * + * These assertions are load-bearing in a way the staged-config ones are not: + * bgpd CONSTRUCTS a Policy from every statement in its config, rejecting an + * atomic match whose type it does not support, whose payload field is unset, + * or whose by-name reference does not resolve — so a wrong encoding + * crash-loops the daemon and fails the test here rather than sitting inert in + * the config. That is also why this test creates the as-path-list and + * prefix-list it matches on. + * + * 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 routing-policy name, unlikely to collide with a real policy in +// the device's running BGP config. +const std::string kPolicy = "FBOSS2-TEST-RP-MATCH"; +} // namespace + +class ConfigBgpPolicyRoutingPolicyTermMatchTest : public ConfigBgpTestBase { + protected: + // Stage `... routing-policy term 10 match ` WITHOUT + // clearing the staged session, so matches can accumulate across + // invocations. Returns the staged session JSON. + folly::dynamic stageMatch(const std::vector& tokens) { + std::vector args = { + "config", + "protocol", + "bgp", + "policy", + "routing-policy", + kPolicy, + "term", + "10", + "match"}; + 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(); + } + + // Stage any `config ...` command, failing the test on a non-zero exit. + void stageCli(const std::vector& args) { + auto result = runCli(args); + EXPECT_EQ(result.exitCode, 0) + << "stdout=" << result.stdout << " stderr=" << result.stderr; + EXPECT_THAT(result.stdout, Not(HasSubstr("Error:"))); + } + + // The .policy_entries[] term with sequence_number 10 of the policy named + // kPolicy, or nullptr. + static const folly::dynamic* findTerm(const folly::dynamic& config) { + if (config.count("policies") == 0 || + config["policies"].count("bgp_policy_statements") == 0) { + return nullptr; + } + for (const auto& policy : config["policies"]["bgp_policy_statements"]) { + if (policy.count("name") && policy["name"].asString() == kPolicy && + policy.count("policy_entries")) { + for (const auto& term : policy["policy_entries"]) { + if (term.count("sequence_number") && + term["sequence_number"].asInt() == 10) { + return &term; + } + } + } + } + return nullptr; + } + + // The atomic match of `type` in the term's policy_match_entries — the only + // match container bgpd reads. BgpPolicyAtomicMatchType rides the SimpleJSON + // wire as an int. + static const folly::dynamic* findAtomic( + const folly::dynamic& term, + int64_t type) { + if (term.count("policy_match_entries") == 0) { + return nullptr; + } + const auto& match = term["policy_match_entries"]; + if (match.count("match_entries") == 0) { + return nullptr; + } + for (const auto& atomic : match["match_entries"]) { + if (atomic.count("type") && atomic["type"].asInt() == type) { + return &atomic; + } + } + return nullptr; + } +}; + +TEST_F(ConfigBgpPolicyRoutingPolicyTermMatchTest, SetMatchesAndCommit) { + discardSession(); + clearBgpSession(); + // A match reference must resolve: PolicyManager::PopulateReferences throws + // "Could not find reference" for a name absent from its by-name map, + // which crash-loops bgpd. So create the lists this term will reference + // before referencing them. + stageCli( + {"config", + "protocol", + "bgp", + "policy", + "as-path-list", + "FBOSS2-TEST-ASPL", + "entry", + "10", + "asn-regexp", + "^65000$"}); + // Deliberately entry-less. bgpd's + // PrefixTreeMatch::validateAndCreatePrefixTree throws "Unsupported Prefix + // configuration: seq_num" for any prefix entry carrying a seq_num — which is + // exactly how `prefix-list entry ` keys its entries — so a + // referenced prefix-list with entries crash-loops the daemon today. An empty + // prefixes[] skips that validation loop, which still proves this term's + // reference resolves and is encoded correctly. + stageCli( + {"config", "protocol", "bgp", "policy", "prefix-list", "FBOSS2-TEST-PL"}); + stageMatch({"from", "as-path-list", "FBOSS2-TEST-ASPL"}); + stageMatch({"from", "origin", "IGP"}); + stageMatch({"from", "prefix-list", "FBOSS2-TEST-PL"}); + commitAndGetSha(); + ASSERT_TRUE(waitForBgpDaemonActive()) + << "bgpd did not return active after commit; state=" + << bgpDaemonActiveState(); + // bgpd's getRunningConfig RPC proves it parsed, adopted AND constructed the + // term's atomic matches: an unsupported type or an unset payload field + // throws during Policy construction and crash-loops the daemon, so simply + // reaching these assertions is itself a check on the encoding. + // BgpPolicyAtomicMatchType wire values: AS_PATH=2, ORIGIN=4, + // PREFIX_LIST=5. + auto running = readRunningBgpConfigViaRpc(); + const auto* term = findTerm(running); + ASSERT_NE(term, nullptr) << "bgpd's running config has no routing-policy " + << kPolicy << " term 10"; + + // A reference is carried in *_list_names, which + // PolicyManager::PopulateReferences resolves against its by-name maps. + const auto* asPath = findAtomic(*term, 2); + ASSERT_NE(asPath, nullptr); + EXPECT_EQ( + (*asPath)["as_path_filters"]["as_path_list_names"][0].asString(), + "FBOSS2-TEST-ASPL"); + + const auto* origin = findAtomic(*term, 4); + ASSERT_NE(origin, nullptr); + // Origin.IGP = 1. + EXPECT_EQ((*origin)["origin"].asInt(), 1); + + const auto* prefixList = findAtomic(*term, 5); + ASSERT_NE(prefixList, nullptr); + EXPECT_EQ( + (*prefixList)["prefix_filters"]["prefix_list_names"][0].asString(), + "FBOSS2-TEST-PL"); +} From e6155c56a50097dedf7722069baf89acdaaf78d0 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:28:13 +0000 Subject: [PATCH 12/18] Emit term match commands from bgp_json_to_cli.py A term's policy_match_entries (the field bgpd reads; policy_matches is not) become `match from as-path-list|origin|prefix-list ` lines. Match types the CLI cannot express, a non-AND match_logic_type, extra list names and policy_matches are emitted as `# WARNING:` lines. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/bgp_json_to_cli.py | 96 +++++++++++++++++++ .../tools/tests/test_bgp_json_to_cli.py | 67 +++++++++++++ 2 files changed, 163 insertions(+) diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index 19ea7d61f6999..1a8c6d38f86cc 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -854,6 +854,7 @@ def generate_routing_policy_term_commands( commands.append(f"{prefix} description {escape_shell_arg(term['description'])}") # Nested term generators (action, match) hook in here. commands.extend(generate_routing_policy_term_action_commands(prefix, term)) + commands.extend(generate_routing_policy_term_match_commands(prefix, term)) if not commands: commands.append(prefix) return commands @@ -1009,6 +1010,101 @@ def generate_routing_policy_term_action_commands( return commands +_ATOMIC_MATCH_TYPE_NAMES = { + 1: "AS_PATH_LEN", + 2: "AS_PATH", + 3: "COMMUNITY_LIST", + 4: "ORIGIN", + 5: "PREFIX_LIST", + 6: "NEXT_HOP", + 7: "NEIGHBOR_LIST", + 8: "ROUTE_TYPE", + 9: "COMMUNITY_COUNT", + 10: "INTERFACE", + 11: "LOCAL_PREFERENCE", + 12: "METRIC", + 13: "TAG_LIST", + 14: "MED", + 15: "FAMILY", + 16: "LEVEL", + 17: "ROUTE_FILTER", + 18: "PROTOCOL", + 19: "AS_PATH_LEN_WITH_CONFED", + 20: "ALWAYS", + 21: "WEIGHT", +} + + +def _generate_list_reference_match( + term_prefix: str, label: str, keyword: str, names: list[str] +) -> list[str]: + """`match from ` for the single by-name reference the CLI + stores; extra names surface as a warning.""" + if not names: + return [_warning(f"{label}: {keyword} match names no list; not emitted")] + commands = [f"{term_prefix} match from {keyword} {escape_shell_arg(names[0])}"] + if len(names) > 1: + commands.append( + _warning( + f"{label}: {keyword} match names {len(names)} lists but the CLI " + "stores one; only the first is emitted" + ) + ) + return commands + + +def generate_routing_policy_term_match_commands( + term_prefix: str, term: dict[str, Any] +) -> list[str]: + """Generate the `match from ...` commands of one term from + policy_match_entries (the field bgpd reads; policy_matches is not).""" + label = term_prefix.removeprefix("config protocol bgp policy ") + commands = [] + if term.get("policy_matches"): + commands.append( + _warning(f"{label}: policy_matches is not read by bgpd; not emitted") + ) + match = term.get("policy_match_entries") + if not match: + return commands + if "match_logic_type" in match: + logic = _boolean_operator_name(match["match_logic_type"]) + if logic != "AND": + commands.append( + _warning( + f"{label}: match_logic_type {logic} has no CLI equivalent " + "(matches compose under AND); not emitted" + ) + ) + for entry in match.get("match_entries") or []: + kind = _enum_name(entry.get("type", 0), _ATOMIC_MATCH_TYPE_NAMES) + if kind == "AS_PATH": + names = (entry.get("as_path_filters") or {}).get("as_path_list_names") or [] + commands.extend( + _generate_list_reference_match( + term_prefix, label, "as-path-list", names + ) + ) + elif kind == "ORIGIN": + if "origin" in entry: + origin = _enum_name(entry["origin"], _ORIGIN_NAMES) + commands.append( + f"{term_prefix} match from origin {escape_shell_arg(origin)}" + ) + elif kind == "PREFIX_LIST": + names = (entry.get("prefix_filters") or {}).get("prefix_list_names") or [] + commands.extend( + _generate_list_reference_match(term_prefix, label, "prefix-list", names) + ) + else: + commands.append( + _warning( + f"{label}: match type {kind} is not expressible by the CLI; not emitted" + ) + ) + return commands + + def generate_policy_commands(config: dict[str, Any]) -> list[str]: """Generate the `config protocol bgp policy ...` commands. diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index 1af7f165590a5..6939ca42a194c 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 @@ -34,6 +34,7 @@ generate_routing_policy_commands, generate_routing_policy_term_action_commands, generate_routing_policy_term_commands, + generate_routing_policy_term_match_commands, json_to_cli, ) @@ -1637,6 +1638,72 @@ def test_actions_emitted_inside_term(self) -> None: ) +class GenerateRoutingPolicyTermMatchCommandsTest(unittest.TestCase): + """Tests for generate_routing_policy_term_match_commands.""" + + TERM = "config protocol bgp policy routing-policy RM term 10" + + def test_supported_matches(self) -> None: + term = { + "policy_match_entries": { + "match_entries": [ + {"type": 2, "as_path_filters": {"as_path_list_names": ["ASPL"]}}, + {"type": "ORIGIN", "origin": 1}, + {"type": 5, "prefix_filters": {"prefix_list_names": ["PL"]}}, + ] + } + } + commands = generate_routing_policy_term_match_commands(self.TERM, term) + self.assertEqual( + commands, + [ + f"{self.TERM} match from as-path-list ASPL", + f"{self.TERM} match from origin IGP", + f"{self.TERM} match from prefix-list PL", + ], + ) + + def test_no_match_entries(self) -> None: + self.assertEqual(generate_routing_policy_term_match_commands(self.TERM, {}), []) + + def test_unexpressible_matches_warn(self) -> None: + term = { + "policy_matches": [{}], + "policy_match_entries": { + "match_logic_type": 2, + "match_entries": [ + {"type": 3}, + {"type": 2, "as_path_filters": {"as_path_list_names": ["A", "B"]}}, + ], + }, + } + commands = generate_routing_policy_term_match_commands(self.TERM, term) + self.assertEqual(commands[3], f"{self.TERM} match from as-path-list A") + warnings = [c for c in commands if c.startswith("# WARNING:")] + self.assertEqual(len(warnings), 4) + + def test_matches_emitted_inside_term(self) -> None: + commands = generate_routing_policy_term_commands( + "RM", + { + "sequence_number": 10, + "policy_match_entries": { + "match_entries": [ + {"type": 5, "prefix_filters": {"prefix_list_names": ["PL"]}} + ] + }, + "term_miss_action": 1, + }, + ) + self.assertEqual( + commands, + [ + f"{self.TERM} action result ACCEPT", + f"{self.TERM} match from prefix-list PL", + ], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands.""" From e995141fe5ff4920f9b875db53e3654e3d67899f Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Tue, 15 Sep 2026 12:28:14 +0000 Subject: [PATCH 13/18] Require `from as-path-list` to name an existing list and carry its operator `match from as-path-list ` accepted any name, so a misspelled or forward reference committed a dangling as_path_list_names entry; bgpd fails the config load on it and crash-loops. This is the mirror of the as-path-list delete guard: the match now refuses a name that does not exist, before the policy or term is created, and copies the list's boolean_operator into the term's inline copy, which bgpd requires to agree with the list ("Conflicting boolean_operator"). The unit fixture seeds the list the way the as-path-list command stores it; the integration test already created its list before matching on it. Co-Authored-By: Claude Fable 5.1 --- ...rotocolBgpPolicyRoutingPolicyTermMatch.cpp | 21 ++++++++ ...figBgpPolicyRoutingPolicyTermMatchTest.cpp | 48 +++++++++++++++++++ ...figBgpPolicyRoutingPolicyTermMatchTest.cpp | 24 ++++++++++ 3 files changed, 93 insertions(+) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp index f2bd59a1c94d6..08708fde3a287 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp @@ -27,6 +27,7 @@ #include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" @@ -226,6 +227,19 @@ CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch::queryClient( auto& session = ConfigSession::getInstance(); auto& cfg = session.getBgpConfig(); + // bgpd resolves as_path_list_names at config load and fails the load on a + // dangling one (the mirror of the as-path-list delete guard), so a + // misspelled or forward reference is refused before anything is created. + const bgp::bgp_policy::AsPathList* referencedAsPathList = nullptr; + if (args.attr() == kFromAsPathList && args.values().size() == 1) { + referencedAsPathList = bgpcli::findAsPathList(cfg, args.values()[0]); + if (referencedAsPathList == nullptr) { + return fmt::format( + "Error: BGP as-path-list {} not found; create it before referencing " + "it", + args.values()[0]); + } + } const bool policyCreated = !bgpcli::routingPolicyExists(cfg, policyArgs.policyName()); auto& policy = @@ -240,6 +254,13 @@ CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch::queryClient( matchAttrHandlers().find(args.attr())->second(term, args.values()); if (result.ok) { + if (referencedAsPathList != nullptr) { + // bgpd requires the term's inline copy of the list to carry the same + // boolean_operator as the list ("Conflicting boolean_operator"). + findOrCreateAtomicMatch(term, BgpPolicyAtomicMatchType::AS_PATH) + .as_path_filters() + ->boolean_operator() = *referencedAsPathList->boolean_operator(); + } result.message += fmt::format( " for routing-policy {} term {} match", policyArgs.policyName(), diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp index d749e3bc8f11c..1e432bebde4aa 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -11,6 +11,8 @@ #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/as-path-list/BgpAsPathListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h" #include "fboss/cli/fboss2/session/ConfigSession.h" @@ -69,6 +71,20 @@ class CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture return *term().policy_match_entries()->match_entries(); } + // `from as-path-list` refuses a name that does not exist (the as-path-list + // CLI lives below this level); seed the list the way that command stores + // it. + bgp::bgp_policy::AsPathList& addAsPathList( + const std::string& name, + bgp::routing_policy::BooleanOperator op = + bgp::routing_policy::BooleanOperator::OR) { + auto& list = bgpcli::findOrCreateAsPathList( + ConfigSession::getInstance().getBgpConfig(), name); + list.boolean_operator() = op; + ConfigSession::getInstance().saveBgpConfig(); + return list; + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -118,6 +134,7 @@ TEST_F( // ============================================================================== TEST_F(CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, fromAsPathList) { + addAsPathList("ASPL1"); auto result = runMatch({"from", "as-path-list", "ASPL1"}); EXPECT_THAT(result, HasSubstr("Successfully set as-path-list to: ASPL1")); EXPECT_THAT(result, HasSubstr("for routing-policy RM100 term 10 match")); @@ -161,6 +178,7 @@ TEST_F( // Each match kind gets one atomic entry; re-issuing a kind updates it. The // entries compose under the match object's default AND, which is the only // operator bgpd accepts for more than one entry. + addAsPathList("ASPL1"); runMatch({"from", "prefix-list", "PL1"}); runMatch({"from", "as-path-list", "ASPL1"}); runMatch({"from", "origin", "IGP"}); @@ -218,4 +236,34 @@ TEST_F( << "session file should not exist after rejected input"; } +// ============================================================================== +// from as-path-list — the reference must resolve, and carries the list's +// operator +// ============================================================================== + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, + fromUnknownAsPathListRejected) { + auto result = runMatch({"from", "as-path-list", "NO-SUCH-LIST"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP as-path-list NO-SUCH-LIST not found")); + // Refused before anything is created: no phantom policy, nothing on disk. + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, + fromAsPathListCopiesListOperator) { + addAsPathList("ASPL1", bgp::routing_policy::BooleanOperator::AND); + runMatch({"from", "as-path-list", "ASPL1"}); + ASSERT_EQ(atomics().size(), 1); + // bgpd rejects a term whose inline copy disagrees with the list + // ("Conflicting boolean_operator"), so the match carries the list's value. + EXPECT_EQ( + *atomics()[0].as_path_filters()->boolean_operator(), + bgp::routing_policy::BooleanOperator::AND); +} + } // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp index de33abec734a2..4bfe0f30d75f2 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -27,6 +27,7 @@ */ #include +#include #include #include @@ -119,6 +120,29 @@ class ConfigBgpPolicyRoutingPolicyTermMatchTest : public ConfigBgpTestBase { } }; +TEST_F( + ConfigBgpPolicyRoutingPolicyTermMatchTest, + FromUnknownAsPathListRejected) { + clearBgpSession(); + auto result = runCli( + {"config", + "protocol", + "bgp", + "policy", + "routing-policy", + "FBOSS2-TEST-RM-UNKNOWN", + "term", + "10", + "match", + "from", + "as-path-list", + "FBOSS2-NO-SUCH-LIST"}); + // Refused at staging: bgpd would fail the load on the dangling name. + EXPECT_THAT(result.stdout, HasSubstr("not found")); + EXPECT_FALSE(std::filesystem::exists(bgpSessionPath())) + << "session file should not exist after rejected input"; +} + TEST_F(ConfigBgpPolicyRoutingPolicyTermMatchTest, SetMatchesAndCommit) { discardSession(); clearBgpSession(); From f8d74e13f7dc555c1e61b7fd42a8fbba7dfb50a2 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Wed, 16 Sep 2026 07:46:03 +0000 Subject: [PATCH 14/18] Seed the term-match test's as-path-list with the flat regex attribute `regex ` writes the as_paths field bgpd actually matches on; there is no nested entry subcommand. Co-Authored-By: Claude Fable 5.1 --- .../ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp index 4bfe0f30d75f2..b7755e56cd6ae 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -157,9 +157,7 @@ TEST_F(ConfigBgpPolicyRoutingPolicyTermMatchTest, SetMatchesAndCommit) { "policy", "as-path-list", "FBOSS2-TEST-ASPL", - "entry", - "10", - "asn-regexp", + "regex", "^65000$"}); // Deliberately entry-less. bgpd's // PrefixTreeMatch::validateAndCreatePrefixTree throws "Unsupported Prefix From 56aac342b5e9ac7b38bdf048d1ed36941c2e4380 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Thu, 8 Oct 2026 11:25:57 +0000 Subject: [PATCH 15/18] Require `from prefix-list` to name an existing list PrefixTreeMatch::PopulateReferences resolves prefix_list_names by name at config load and throws "Could not find PrefixList reference" on a dangling one, failing the whole load. `from as-path-list` already refuses an unknown name for the same reason; do the same for prefix-lists. No operator sync is needed: bgpd compares none for a prefix-list reference. Co-Authored-By: Claude Fable 5.1 --- ...rotocolBgpPolicyRoutingPolicyTermMatch.cpp | 10 ++++++++ ...figBgpPolicyRoutingPolicyTermMatchTest.cpp | 23 +++++++++++++++++++ ...figBgpPolicyRoutingPolicyTermMatchTest.cpp | 11 ++++----- 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp index 08708fde3a287..054c6c67d2453 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp @@ -28,6 +28,7 @@ #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/as-path-list/BgpAsPathListCliUtils.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h" #include "fboss/cli/fboss2/session/ConfigSession.h" #include "fboss/cli/fboss2/utils/CmdUtilsCommon.h" @@ -240,6 +241,15 @@ CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch::queryClient( args.values()[0]); } } + // Same for prefix_list_names: PrefixTreeMatch::PopulateReferences throws + // "Could not find PrefixList reference" at load. No operator sync is + // needed here; bgpd compares none for prefix-lists. + if (args.attr() == kFromPrefixList && args.values().size() == 1 && + !bgpcli::prefixListExists(cfg, args.values()[0])) { + return fmt::format( + "Error: BGP prefix-list {} not found; create it before referencing it", + args.values()[0]); + } const bool policyCreated = !bgpcli::routingPolicyExists(cfg, policyArgs.policyName()); auto& policy = diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp index 1e432bebde4aa..6f7263b6ff392 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -13,6 +13,7 @@ #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/as-path-list/BgpAsPathListCliUtils.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h" #include "fboss/cli/fboss2/session/ConfigSession.h" @@ -85,6 +86,13 @@ class CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture return list; } + // `from prefix-list` likewise refuses a name that does not exist. + void addPrefixList(const std::string& name) { + bgpcli::findOrCreatePrefixList( + ConfigSession::getInstance().getBgpConfig(), name); + ConfigSession::getInstance().saveBgpConfig(); + } + bool sessionFileExists() { return std::filesystem::exists( ConfigSession::getInstance().getBgpSessionConfigPath()); @@ -164,6 +172,7 @@ TEST_F(CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, fromOrigin) { } TEST_F(CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, fromPrefixList) { + addPrefixList("PL1"); runMatch({"from", "prefix-list", "PL1"}); ASSERT_EQ(atomics().size(), 1); EXPECT_EQ(*atomics()[0].type(), BgpPolicyAtomicMatchType::PREFIX_LIST); @@ -179,6 +188,8 @@ TEST_F( // entries compose under the match object's default AND, which is the only // operator bgpd accepts for more than one entry. addAsPathList("ASPL1"); + addPrefixList("PL1"); + addPrefixList("PL2"); runMatch({"from", "prefix-list", "PL1"}); runMatch({"from", "as-path-list", "ASPL1"}); runMatch({"from", "origin", "IGP"}); @@ -205,6 +216,18 @@ TEST_F( // Reject paths — the error is surfaced and nothing is persisted // ============================================================================== +TEST_F( + CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, + fromUnknownPrefixListRejected) { + // bgpd: "Could not find PrefixList reference" at config load. + auto result = runMatch({"from", "prefix-list", "NO-SUCH-LIST"}); + EXPECT_THAT( + result, HasSubstr("Error: BGP prefix-list NO-SUCH-LIST not found")); + EXPECT_TRUE(policies().empty()); + EXPECT_FALSE(sessionFileExists()) + << "session file should not exist after rejected input"; +} + TEST_F( CmdConfigBgpPolicyRoutingPolicyTermMatchTestFixture, rejectedValueRollsBackPhantoms) { diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp index b7755e56cd6ae..ac42205ddcf57 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -159,13 +159,10 @@ TEST_F(ConfigBgpPolicyRoutingPolicyTermMatchTest, SetMatchesAndCommit) { "FBOSS2-TEST-ASPL", "regex", "^65000$"}); - // Deliberately entry-less. bgpd's - // PrefixTreeMatch::validateAndCreatePrefixTree throws "Unsupported Prefix - // configuration: seq_num" for any prefix entry carrying a seq_num — which is - // exactly how `prefix-list entry ` keys its entries — so a - // referenced prefix-list with entries crash-loops the daemon today. An empty - // prefixes[] skips that validation loop, which still proves this term's - // reference resolves and is encoded correctly. + // Deliberately entry-less: this test proves the term's reference resolves + // and is encoded correctly, independent of the entry-level validation in + // PrefixTreeMatch::validateAndCreatePrefixTree (entries are keyed by prefix + // and covered by ConfigBgpPolicyPrefixListEntryTest). stageCli( {"config", "protocol", "bgp", "policy", "prefix-list", "FBOSS2-TEST-PL"}); stageMatch({"from", "as-path-list", "FBOSS2-TEST-ASPL"}); From b2a865d9efdae1952b1473806f169cb120d73de5 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 16 Oct 2026 06:55:51 +0000 Subject: [PATCH 16/18] Use the canonical Meta copyright header in new files --- .../match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp | 2 +- .../match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h | 2 +- .../config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp | 2 +- .../ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp index 054c6c67d2453..0bd69cf636f76 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h index 32b9e0987b3e9..59d071e2f08e6 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-present, Facebook, Inc. + * Copyright (c) Meta Platforms, Inc. and affiliates. * All rights reserved. * * This source code is licensed under the BSD-style license found in the diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp index 6f7263b6ff392..30812c0062402 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. #include #include diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp index ac42205ddcf57..34887c4650492 100644 --- a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -1,4 +1,4 @@ -// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. +// Copyright (c) Meta Platforms, Inc. and affiliates. /** * End-to-end tests for `fboss2-dev config protocol bgp policy routing-policy From be7a00b4c4ce9881ed25ab4a0bfa9f6b0da5dc4b Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 23 Oct 2026 18:36:12 +0000 Subject: [PATCH 17/18] Mirror the Meta import amendments from the routing-policy level Same IS_OSS include split and ensure() on the optional policies field as landed for #1488, applied to the term match files. Co-Authored-By: Claude Fable 5.1 --- ...dConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp | 11 ++++++++--- .../CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp | 9 +++++++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp index 0bd69cf636f76..f518b8952ac6e 100644 --- a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp @@ -13,7 +13,6 @@ #include "fboss/cli/fboss2/CmdHandler.cpp" #include -#include #include #include #include @@ -23,7 +22,6 @@ #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/BgpCliValueParsers.h" @@ -35,6 +33,13 @@ #include "fboss/cli/fboss2/utils/HostInfo.h" #include "fmt/format.h" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + namespace facebook::fboss { namespace { @@ -285,7 +290,7 @@ CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch::queryClient( policy.policy_entries()->pop_back(); } if (policyCreated) { - cfg.policies()->bgp_policy_statements()->pop_back(); + cfg.policies().ensure().bgp_policy_statements()->pop_back(); } } return result.message; diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp index 30812c0062402..f4e15fc2dc621 100644 --- a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -1,6 +1,5 @@ // Copyright (c) Meta Platforms, Inc. and affiliates. -#include #include #include @@ -10,7 +9,6 @@ #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/as-path-list/BgpAsPathListCliUtils.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/BgpPrefixListCliUtils.h" @@ -20,6 +18,13 @@ #include "fboss/cli/fboss2/test/config/CmdConfigTestBase.h" #include "fboss/cli/fboss2/utils/HostInfo.h" +#ifndef IS_OSS +#include +#include +#else +#include +#endif + using namespace ::testing; namespace facebook::fboss { From 134d9a9ba5a42a97a7d5fce540d9bc0fc507a6b9 Mon Sep 17 00:00:00 2001 From: Hillol Chakraborty Date: Fri, 23 Oct 2026 18:36:56 +0000 Subject: [PATCH 18/18] Seed the term-match emitter test's result via a PERMIT action entry The action level now maps `action result` to policy_action_entries, so term_miss_action no longer produces that command. Co-Authored-By: Claude Fable 5.1 --- fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6939ca42a194c..3bc20bc429942 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 @@ -1692,7 +1692,7 @@ def test_matches_emitted_inside_term(self) -> None: {"type": 5, "prefix_filters": {"prefix_list_names": ["PL"]}} ] }, - "term_miss_action": 1, + "policy_action_entries": [{"type": 5}], }, ) self.assertEqual(