diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 144cda2ecec3a..5e40b091ce010 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -986,6 +986,13 @@ 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/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 @@ -1132,6 +1139,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..29bcae6eab0ff 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -19,6 +19,9 @@ 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/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 @@ -60,6 +63,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..43b781ff6498a 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -23,6 +23,9 @@ 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/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 8fbf66e1ed8a9..e9cc54dd31720 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1220,6 +1220,9 @@ 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/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", @@ -1292,6 +1295,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 +1378,10 @@ 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/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", @@ -1447,6 +1455,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/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 3c5f21a33e8ac..ad990ca62a599 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -48,6 +48,9 @@ #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/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" @@ -117,6 +120,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 +486,50 @@ const CommandTree& kConfigCommandTree() { CmdConfigProtocolBgpPolicyRoutingPolicy>, argRegistrar< CmdConfigProtocolBgpPolicyRoutingPolicyTraits>, + {{ + "term", + "Configure a routing-policy term: " + " [ ...] " + "(description)", + commandHandler< + 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>, + }}, + }, + { + "match", + "Configure a term match: " + "from as-path-list|origin|" + "prefix-list ", + commandHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch>, + argRegistrar< + CmdConfigProtocolBgpPolicyRoutingPolicyTermMatchTraits>, + }}, + }}, }}, }, }, @@ -862,6 +910,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..dab6686fd6377 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h @@ -0,0 +1,108 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include + +#ifndef IS_OSS +#include +#include +#else +#include +#endif + +/** + * 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..f458650858fc6 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp @@ -0,0 +1,199 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/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" + +#ifndef IS_OSS +#include +#include +#else +#include +#endif + +namespace facebook::fboss { + +namespace { + +// The attribute names, exactly as documented. Kept here so the +// valid-attribute set and the handler table stay in sync. The 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().ensure().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..215833ed51377 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h @@ -0,0 +1,93 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#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) { + // 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; + 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/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..afe84228751f3 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.cpp @@ -0,0 +1,506 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/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 +#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" +#include "fmt/format.h" + +#ifndef IS_OSS +#include +#include +#else +#include +#endif + +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 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"; + +// 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 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().ensure().bgp_policy_statements()->pop_back(); + } + } + return result.message; +} + +// ---- result handler -------------------------------------------------------- + +std::optional parseResult(const std::string& value) { + if (value == kResultAccept) { + return BgpPolicyActionType::PERMIT; + } + if (value == kResultReject) { + return BgpPolicyActionType::DENY; + } + if (value == kResultContinue) { + return BgpPolicyActionType::CONTINUE; + } + return std::nullopt; +} + +// 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( + "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())); +} + +// 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 +// 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]"); + } + 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) + .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..5f8e6260b3d58 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/action/CmdConfigProtocolBgpPolicyRoutingPolicyTermAction.h @@ -0,0 +1,149 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "CLI/App.hpp" +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/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/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..f518b8952ac6e --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.cpp @@ -0,0 +1,309 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h" + +#include "fboss/cli/fboss2/CmdHandler.cpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/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" +#include "fboss/cli/fboss2/utils/HostInfo.h" +#include "fmt/format.h" + +#ifndef IS_OSS +#include +#include +#else +#include +#endif + +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(); + // 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]); + } + } + // 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 = + 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) { + 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(), + 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().ensure().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..59d071e2f08e6 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/match/CmdConfigProtocolBgpPolicyRoutingPolicyTermMatch.h @@ -0,0 +1,89 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include "CLI/App.hpp" +#include "fboss/cli/fboss2/CmdHandler.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/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/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..8d7c3ea12b76e --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp @@ -0,0 +1,106 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#include "fboss/cli/fboss2/commands/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(); + // 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( + "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) { + return term.sequence_number().has_value() && + *term.sequence_number() == args.seqNum(); + }); + if (it == terms.end()) { + return fmt::format( + "Warning: BGP routing-policy {} has no term {}; nothing to delete", + 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..d62de1ea1ab43 --- /dev/null +++ b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h @@ -0,0 +1,71 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ + +#pragma once + +#include +#include +#include +#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..aea74e757b6e6 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -19,6 +19,9 @@ cpp_unittest( "CmdConfigBgpPolicyCommunityListTest.cpp", "CmdConfigBgpPolicyPrefixListEntryTest.cpp", "CmdConfigBgpPolicyPrefixListTest.cpp", + "CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp", + "CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp", + "CmdConfigBgpPolicyRoutingPolicyTermTest.cpp", "CmdConfigBgpPolicyRoutingPolicyTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", @@ -60,6 +63,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/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp new file mode 100644 index 0000000000000..ff47cd9aebeae --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -0,0 +1,345 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +#include +#include + +#include +#include +#include +#include +#include + +#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 { + +// 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 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"}); + 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"}); + 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 (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, + 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, + 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); + 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/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp new file mode 100644 index 0000000000000..f4e15fc2dc621 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -0,0 +1,297 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +#include +#include + +#include +#include +#include +#include +#include + +#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/routing_policy_types.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/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" +#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 { + +// 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(); + } + + // `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; + } + + // `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()); + } +}; + +// ============================================================================== +// 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) { + 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")); + 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) { + addPrefixList("PL1"); + 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. + addAsPathList("ASPL1"); + addPrefixList("PL1"); + addPrefixList("PL2"); + 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, + 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) { + 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"; +} + +// ============================================================================== +// 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/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp new file mode 100644 index 0000000000000..1608454750506 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp @@ -0,0 +1,222 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +#include +#include + +#include +#include +#include +#include + +#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 { + +// 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..ce04a84ac6095 --- /dev/null +++ b/fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp @@ -0,0 +1,193 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +#include +#include + +#include +#include +#include +#include + +#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" + +#ifndef IS_OSS +#include +#include +#else +#include +#endif + +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()); +} + +// 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( + "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); +} + +TEST_F( + CmdDeleteBgpPolicyRoutingPolicyTermTestFixture, + 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( + "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 a no-op delete"; +} + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/test/integration_test/BUCK b/fboss/cli/fboss2/test/integration_test/BUCK index 278743e7520c5..63d97bdf7c4d7 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -29,6 +29,9 @@ cpp_binary( "ConfigBgpPolicyCommunityListTest.cpp", "ConfigBgpPolicyPrefixListEntryTest.cpp", "ConfigBgpPolicyPrefixListTest.cpp", + "ConfigBgpPolicyRoutingPolicyTermActionTest.cpp", + "ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp", + "ConfigBgpPolicyRoutingPolicyTermTest.cpp", "ConfigBgpPolicyRoutingPolicyTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.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..47029431f906e --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermActionTest.cpp @@ -0,0 +1,173 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * 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 (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", "REJECT"}); + 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 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"); + 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); +} 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..34887c4650492 --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermMatchTest.cpp @@ -0,0 +1,204 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * 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 + +#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, + 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(); + // 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", + "regex", + "^65000$"}); + // 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"}); + 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"); +} 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..abc5d1504bd9a --- /dev/null +++ b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp @@ -0,0 +1,174 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * 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"; +} diff --git a/fboss/cli/fboss2/tools/bgp_json_to_cli.py b/fboss/cli/fboss2/tools/bgp_json_to_cli.py index c432ede93a7a2..1a8c6d38f86cc 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -825,12 +825,286 @@ 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. + 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 + + +_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"} +# 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: + 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: + 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 {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 + + +_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 630fdcde07a10..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 @@ -32,6 +32,9 @@ generate_prefix_list_commands, generate_prefix_list_entry_commands, generate_routing_policy_commands, + generate_routing_policy_term_action_commands, + generate_routing_policy_term_commands, + generate_routing_policy_term_match_commands, json_to_cli, ) @@ -1483,6 +1486,224 @@ 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 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: + # 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, {"policy_action_entries": [{"type": raw}]} + ) + self.assertEqual(commands, [f"{self.TERM} action result {keyword}"], raw) + + def test_term_miss_action_default_omitted(self) -> None: + self.assertEqual( + generate_routing_policy_term_action_commands( + self.TERM, {"term_miss_action": 3} + ), + [], + ) + + 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 = { + "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, + "policy_action_entries": [ + {"type": 5}, + {"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 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"]}} + ] + }, + "policy_action_entries": [{"type": 5}], + }, + ) + 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."""