diff --git a/cmake/CliFboss2.cmake b/cmake/CliFboss2.cmake index 144cda2ecec3a..f66e7a45bf4dd 100644 --- a/cmake/CliFboss2.cmake +++ b/cmake/CliFboss2.cmake @@ -986,6 +986,9 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp + fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.cpp fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp @@ -1132,6 +1135,8 @@ add_library(fboss2_config_lib fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp + fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h fboss/cli/fboss2/commands/delete/protocol/CmdDeleteProtocol.h fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h diff --git a/cmake/CliFboss2TestConfig.cmake b/cmake/CliFboss2TestConfig.cmake index f0a4a4b7ce5cb..1ef54701f321d 100644 --- a/cmake/CliFboss2TestConfig.cmake +++ b/cmake/CliFboss2TestConfig.cmake @@ -19,6 +19,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListEntryTest.cpp fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTest.cpp + fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp fboss/cli/fboss2/test/config/CmdConfigDataPlaneTrafficPolicyTest.cpp fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp @@ -60,6 +61,7 @@ add_executable(fboss2_cmd_config_test fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTest.cpp + fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp fboss/cli/fboss2/test/config/CmdDeleteCoppQueueTest.cpp fboss/cli/fboss2/test/config/CmdDeleteCoppReasonTest.cpp diff --git a/cmake/CliFboss2TestIntegrationTest.cmake b/cmake/CliFboss2TestIntegrationTest.cmake index b572867cfcc09..b8e42f7a2073c 100644 --- a/cmake/CliFboss2TestIntegrationTest.cmake +++ b/cmake/CliFboss2TestIntegrationTest.cmake @@ -23,6 +23,7 @@ add_executable(fboss2_integration_test fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListEntryTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTest.cpp + fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp fboss/cli/fboss2/test/integration_test/ConfigConcurrentSessionsTest.cpp fboss/cli/fboss2/test/integration_test/ConfigHostnameTest.cpp diff --git a/fboss/cli/fboss2/BUCK b/fboss/cli/fboss2/BUCK index 8fbf66e1ed8a9..b5124de097bd5 100644 --- a/fboss/cli/fboss2/BUCK +++ b/fboss/cli/fboss2/BUCK @@ -1220,6 +1220,7 @@ cpp_library( "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp", "commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp", "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp", + "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp", "commands/config/protocol/static/CmdConfigProtocolStatic.cpp", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp", "commands/config/ptp/CmdConfigPtp.cpp", @@ -1292,6 +1293,7 @@ cpp_library( "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp", "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp", "commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp", + "commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp", "commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp", "commands/delete/qos/CmdDeleteQos.cpp", @@ -1374,6 +1376,8 @@ cpp_library( "commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h", "commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h", "commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h", + "commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h", + "commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h", "commands/config/protocol/static/CmdConfigProtocolStatic.h", "commands/config/protocol/static/route/StaticRouteUtils.h", "commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h", @@ -1447,6 +1451,7 @@ cpp_library( "commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h", "commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h", "commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h", + "commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h", "commands/delete/protocol/static/CmdDeleteProtocolStatic.h", "commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h", "commands/delete/qos/CmdDeleteQos.h", diff --git a/fboss/cli/fboss2/CmdListConfig.cpp b/fboss/cli/fboss2/CmdListConfig.cpp index 3c5f21a33e8ac..efee454a7c34e 100644 --- a/fboss/cli/fboss2/CmdListConfig.cpp +++ b/fboss/cli/fboss2/CmdListConfig.cpp @@ -48,6 +48,7 @@ #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h" #include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h" #include "fboss/cli/fboss2/commands/config/protocol/static/CmdConfigProtocolStatic.h" #include "fboss/cli/fboss2/commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h" #include "fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h" @@ -117,6 +118,7 @@ #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h" #include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h" +#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h" #include "fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h" #include "fboss/cli/fboss2/commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h" #include "fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h" @@ -482,6 +484,16 @@ const CommandTree& kConfigCommandTree() { CmdConfigProtocolBgpPolicyRoutingPolicy>, argRegistrar< CmdConfigProtocolBgpPolicyRoutingPolicyTraits>, + {{ + "term", + "Configure a routing-policy term: " + " [ ...] " + "(description)", + commandHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTerm>, + argRegistrar< + CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits>, + }}, }}, }, }, @@ -862,6 +874,14 @@ const CommandTree& kConfigCommandTree() { CmdDeleteProtocolBgpPolicyRoutingPolicy>, argRegistrar< CmdDeleteProtocolBgpPolicyRoutingPolicyTraits>, + {{ + "term", + "Delete a routing-policy term: ", + commandHandler< + CmdDeleteProtocolBgpPolicyRoutingPolicyTerm>, + argRegistrar< + CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits>, + }}, }}, }}, }, diff --git a/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h new file mode 100644 index 0000000000000..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..d3d11da9301c2 --- /dev/null +++ b/fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h @@ -0,0 +1,91 @@ +/* + * 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) { + // Term has no nested subcommands; stop CLI11's parent-chain fallthrough + // from stealing a value token that spells `term` (e.g. in a description). + cmd.positionals_at_end(); + cmd.add_option("args", args, " [ ...]"); + } + using ObjectArgType = BgpRoutingPolicyTermConfig; + using RetType = std::string; +}; + +class CmdConfigProtocolBgpPolicyRoutingPolicyTerm + : public CmdHandler< + CmdConfigProtocolBgpPolicyRoutingPolicyTerm, + CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits> { + public: + using ObjectArgType = + CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits::ObjectArgType; + using RetType = CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits::RetType; + + RetType queryClient( + const HostInfo& hostInfo, + const BgpRoutingPolicyConfig& policyArgs, + const ObjectArgType& args); + + void printOutput(const RetType& output); +}; + +} // namespace facebook::fboss diff --git a/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp b/fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp new file mode 100644 index 0000000000000..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..0e52fb3b1ec41 100644 --- a/fboss/cli/fboss2/test/config/BUCK +++ b/fboss/cli/fboss2/test/config/BUCK @@ -19,6 +19,7 @@ cpp_unittest( "CmdConfigBgpPolicyCommunityListTest.cpp", "CmdConfigBgpPolicyPrefixListEntryTest.cpp", "CmdConfigBgpPolicyPrefixListTest.cpp", + "CmdConfigBgpPolicyRoutingPolicyTermTest.cpp", "CmdConfigBgpPolicyRoutingPolicyTest.cpp", "CmdConfigCoppTest.cpp", "CmdConfigDataPlaneTrafficPolicyTest.cpp", @@ -60,6 +61,7 @@ cpp_unittest( "CmdDeleteBgpPolicyAsPathListTest.cpp", "CmdDeleteBgpPolicyCommunityListTest.cpp", "CmdDeleteBgpPolicyPrefixListTest.cpp", + "CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp", "CmdDeleteBgpPolicyRoutingPolicyTest.cpp", "CmdDeleteConfigInterfaceTest.cpp", "CmdDeleteCoppQueueTest.cpp", diff --git a/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp new file mode 100644 index 0000000000000..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..149e01d868289 100644 --- a/fboss/cli/fboss2/test/integration_test/BUCK +++ b/fboss/cli/fboss2/test/integration_test/BUCK @@ -29,6 +29,7 @@ cpp_binary( "ConfigBgpPolicyCommunityListTest.cpp", "ConfigBgpPolicyPrefixListEntryTest.cpp", "ConfigBgpPolicyPrefixListTest.cpp", + "ConfigBgpPolicyRoutingPolicyTermTest.cpp", "ConfigBgpPolicyRoutingPolicyTest.cpp", "ConfigBgpSessionTest.cpp", "ConfigConcurrentSessionsTest.cpp", diff --git a/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp b/fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp new file mode 100644 index 0000000000000..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..5e7b4fb436f0d 100644 --- a/fboss/cli/fboss2/tools/bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/bgp_json_to_cli.py @@ -825,12 +825,39 @@ def generate_routing_policy_commands(policy: dict[str, Any]) -> list[str]: "equivalent; not emitted" ) ) + for term in policy.get("policy_entries") or []: + commands.extend(generate_routing_policy_term_commands(name, term)) if not commands: # Nothing to set: still recreate the (empty) routing-policy by name. commands.append(prefix) return commands +def generate_routing_policy_term_commands( + policy_name: str, term: dict[str, Any] +) -> list[str]: + """Generate `... routing-policy term ` commands for one + term. The CLI keys terms by sequence_number.""" + if "sequence_number" not in term: + return [ + _warning( + f"routing-policy {policy_name}: term '{term.get('name', '')}' has " + "no sequence_number and cannot be addressed by the CLI; not emitted" + ) + ] + prefix = ( + f"config protocol bgp policy routing-policy {escape_shell_arg(policy_name)} " + f"term {escape_shell_arg(term['sequence_number'])}" + ) + commands = [] + if term.get("description"): + commands.append(f"{prefix} description {escape_shell_arg(term['description'])}") + # Nested term generators (action, match) hook in here. + if not commands: + commands.append(prefix) + return commands + + def generate_policy_commands(config: dict[str, Any]) -> list[str]: """Generate the `config protocol bgp policy ...` commands. diff --git a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py index 630fdcde07a10..586891c8128e8 100644 --- a/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py +++ b/fboss/cli/fboss2/tools/tests/test_bgp_json_to_cli.py @@ -32,6 +32,7 @@ generate_prefix_list_commands, generate_prefix_list_entry_commands, generate_routing_policy_commands, + generate_routing_policy_term_commands, json_to_cli, ) @@ -1483,6 +1484,45 @@ def test_policy_block_order_lists_then_policies(self) -> None: ) +class GenerateRoutingPolicyTermCommandsTest(unittest.TestCase): + """Tests for generate_routing_policy_term_commands (term grammar).""" + + PREFIX = "config protocol bgp policy routing-policy RM term 10" + + def test_bare_term_is_recreated(self) -> None: + commands = generate_routing_policy_term_commands("RM", {"sequence_number": 10}) + self.assertEqual(commands, [self.PREFIX]) + + def test_description(self) -> None: + commands = generate_routing_policy_term_commands( + "RM", {"sequence_number": 10, "description": "deny bogons"} + ) + self.assertEqual(commands, [f"{self.PREFIX} description 'deny bogons'"]) + + def test_missing_sequence_number_warns(self) -> None: + commands = generate_routing_policy_term_commands("RM", {"name": "t1"}) + self.assertEqual(len(commands), 1) + self.assertTrue(commands[0].startswith("# WARNING:")) + self.assertIn("t1", commands[0]) + + def test_terms_emitted_inside_policy(self) -> None: + commands = generate_routing_policy_commands( + { + "name": "RM", + "description": "d", + "policy_entries": [{"sequence_number": 10}, {"sequence_number": 20}], + } + ) + self.assertEqual( + commands, + [ + "config protocol bgp policy routing-policy RM description d", + self.PREFIX, + "config protocol bgp policy routing-policy RM term 20", + ], + ) + + class GenerateScriptCommandsTest(unittest.TestCase): """Tests for generate_exec_commands."""