Skip to content

Commit 98a0313

Browse files
NOS-6680, NOS-6688: fboss2 bgp policy routing-policy term commands
Adds `config protocol bgp policy routing-policy <name> term <seq-num> [description <string>]` and `delete protocol bgp policy routing-policy <name> term <seq-num>` — the seq-num-keyed term level of the routing-policy family. The term's action and match levels land as follow-up subcommands. Unlike the sibling policy families (which parse their nested level inside the parent's ObjectArgType), term is a real CLI11 subcommand with its own handler, mirroring the `config interface ... switchport` chain: the policy name arrives through the ancestor-args tuple. This required dropping positionals_at_end() from the routing-policy parent so CLI11 can classify `term` after the policy name; the trade-off (a policy-level attribute value spelling `term` is stolen by subcommand matching) is documented in the traits. The term subcommand keeps positionals_at_end() so its own value tokens are safe. A policy-level attribute mixed with a term command (`routing-policy X description y term 10`) is rejected instead of silently dropping the description, since only the leaf handler runs. - term dispatcher keyed by (policy name from parent) + seq-num, writing bgp_policy.BgpPolicyTerm in policy_entries[]. Term level: description (joined multi-token string, NOS-6680). Bare create (NOS-6688) seeds the term name from the seq-num (next_term_id references terms by name). - shared BgpRoutingPolicyCliUtils.h lookup/create helpers, used by the policy-level dispatcher, the term subcommand, and delete. - delete removes a single term by seq-num; the policy and other terms stay. - unit tests (14) + commit-path integration tests that exercise the CLI11 subcommand dispatch through the full parse and verify terms in bgpd's running config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 9971f88 commit 98a0313

17 files changed

Lines changed: 1155 additions & 36 deletions

cmake/CliFboss2.cmake

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -901,6 +901,9 @@ add_library(fboss2_config_lib
901901
fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h
902902
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp
903903
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h
904+
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h
905+
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp
906+
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h
904907
fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.cpp
905908
fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h
906909
fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp
@@ -997,6 +1000,8 @@ add_library(fboss2_config_lib
9971000
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h
9981001
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp
9991002
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h
1003+
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp
1004+
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h
10001005
fboss/cli/fboss2/commands/delete/protocol/CmdDeleteProtocol.h
10011006
fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp
10021007
fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h

cmake/CliFboss2TestConfig.cmake

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ add_executable(fboss2_cmd_config_test
1515
fboss/cli/fboss2/test/config/CmdConfigBgpPolicyCommunityListTest.cpp
1616
fboss/cli/fboss2/test/config/CmdConfigBgpPolicyPrefixListTest.cpp
1717
fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTest.cpp
18+
fboss/cli/fboss2/test/config/CmdConfigBgpPolicyRoutingPolicyTermTest.cpp
1819
fboss/cli/fboss2/test/config/CmdConfigCoppTest.cpp
1920
fboss/cli/fboss2/test/config/CmdConfigDhcpTest.cpp
2021
fboss/cli/fboss2/test/config/CmdConfigHostnameTest.cpp
@@ -48,6 +49,7 @@ add_executable(fboss2_cmd_config_test
4849
fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp
4950
fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp
5051
fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTest.cpp
52+
fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp
5153
fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp
5254
fboss/cli/fboss2/test/config/CmdDeleteInterfaceIpv6NdpTest.cpp
5355
fboss/cli/fboss2/test/config/CmdDeleteInterfaceTest.cpp

cmake/CliFboss2TestIntegrationTest.cmake

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ add_executable(fboss2_integration_test
1717
fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyCommunityListTest.cpp
1818
fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyPrefixListTest.cpp
1919
fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTest.cpp
20+
fboss/cli/fboss2/test/integration_test/ConfigBgpPolicyRoutingPolicyTermTest.cpp
2021
fboss/cli/fboss2/test/integration_test/ConfigBgpSessionTest.cpp
2122
fboss/cli/fboss2/test/integration_test/ConfigConcurrentSessionsTest.cpp
2223
fboss/cli/fboss2/test/integration_test/ConfigHostnameTest.cpp

fboss/cli/fboss2/BUCK

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,6 +1109,7 @@ cpp_library(
11091109
"commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp",
11101110
"commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp",
11111111
"commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp",
1112+
"commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp",
11121113
"commands/config/protocol/static/CmdConfigProtocolStatic.cpp",
11131114
"commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp",
11141115
"commands/config/ptp/CmdConfigPtp.cpp",
@@ -1158,6 +1159,7 @@ cpp_library(
11581159
"commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp",
11591160
"commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp",
11601161
"commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp",
1162+
"commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp",
11611163
"commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp",
11621164
"commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp",
11631165
"commands/delete/qos/CmdDeleteQos.cpp",
@@ -1225,6 +1227,8 @@ cpp_library(
12251227
"commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h",
12261228
"commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h",
12271229
"commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h",
1230+
"commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h",
1231+
"commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h",
12281232
"commands/config/protocol/static/CmdConfigProtocolStatic.h",
12291233
"commands/config/protocol/static/route/StaticRouteUtils.h",
12301234
"commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h",
@@ -1275,6 +1279,7 @@ cpp_library(
12751279
"commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h",
12761280
"commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h",
12771281
"commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h",
1282+
"commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h",
12781283
"commands/delete/protocol/static/CmdDeleteProtocolStatic.h",
12791284
"commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h",
12801285
"commands/delete/qos/CmdDeleteQos.h",

fboss/cli/fboss2/CmdListConfig.cpp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h"
5050
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h"
5151
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h"
52+
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h"
5253
#include "fboss/cli/fboss2/commands/config/protocol/static/CmdConfigProtocolStatic.h"
5354
#include "fboss/cli/fboss2/commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h"
5455
#include "fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h"
@@ -97,6 +98,7 @@
9798
#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h"
9899
#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h"
99100
#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h"
101+
#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h"
100102
#include "fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h"
101103
#include "fboss/cli/fboss2/commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h"
102104
#include "fboss/cli/fboss2/commands/delete/qos/CmdDeleteQos.h"
@@ -467,6 +469,16 @@ const CommandTree& kConfigCommandTree() {
467469
CmdConfigProtocolBgpPolicyRoutingPolicy>,
468470
argRegistrar<
469471
CmdConfigProtocolBgpPolicyRoutingPolicyTraits>,
472+
{{
473+
"term",
474+
"Configure a routing-policy term: "
475+
"<seq-num> [<attribute> <value> ...] "
476+
"(description)",
477+
commandHandler<
478+
CmdConfigProtocolBgpPolicyRoutingPolicyTerm>,
479+
argRegistrar<
480+
CmdConfigProtocolBgpPolicyRoutingPolicyTermTraits>,
481+
}},
470482
}},
471483
},
472484
},
@@ -783,6 +795,14 @@ const CommandTree& kConfigCommandTree() {
783795
CmdDeleteProtocolBgpPolicyRoutingPolicy>,
784796
argRegistrar<
785797
CmdDeleteProtocolBgpPolicyRoutingPolicyTraits>,
798+
{{
799+
"term",
800+
"Delete a routing-policy term: <seq-num>",
801+
commandHandler<
802+
CmdDeleteProtocolBgpPolicyRoutingPolicyTerm>,
803+
argRegistrar<
804+
CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits>,
805+
}},
786806
}},
787807
}},
788808
},
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
/*
2+
* Copyright (c) 2004-present, Facebook, Inc.
3+
* All rights reserved.
4+
*
5+
* This source code is licensed under the BSD-style license found in the
6+
* LICENSE file in the root directory of this source tree. An additional grant
7+
* of patent rights can be found in the PATENTS file in the same directory.
8+
*
9+
*/
10+
11+
#pragma once
12+
13+
#include <neteng/fboss/bgp/public_tld/configerator/structs/neteng/fboss/bgp/gen-cpp2/bgp_config_types.h>
14+
#include <algorithm>
15+
#include <cstdint>
16+
#include <string>
17+
#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h"
18+
19+
/**
20+
* Lookup/create helpers for the routing-policy CLI family, shared between the
21+
* policy-level dispatcher (CmdConfigProtocolBgpPolicyRoutingPolicy), the term
22+
* subcommand (CmdConfigProtocolBgpPolicyRoutingPolicyTerm), and their delete
23+
* counterparts. A routing-policy (BgpPolicyStatement) is keyed by name; a
24+
* term (BgpPolicyTerm in policy_entries[]) is keyed by sequence_number.
25+
*/
26+
namespace facebook::fboss::bgpcli {
27+
28+
inline bool routingPolicyExists(
29+
const bgp::thrift::BgpConfig& cfg,
30+
const std::string& name) {
31+
if (!cfg.policies().has_value()) {
32+
return false;
33+
}
34+
const auto& policies = *cfg.policies()->bgp_policy_statements();
35+
return std::any_of(policies.begin(), policies.end(), [&](const auto& p) {
36+
return *p.name() == name;
37+
});
38+
}
39+
40+
// The routing-policy statement keyed by name, or nullptr. Non-creating;
41+
// used by the delete commands so a typo'd delete can't stage a change.
42+
inline bgp::bgp_policy::BgpPolicyStatement* findRoutingPolicy(
43+
bgp::thrift::BgpConfig& cfg,
44+
const std::string& name) {
45+
if (!cfg.policies().has_value()) {
46+
return nullptr;
47+
}
48+
auto& policies = *cfg.policies()->bgp_policy_statements();
49+
auto it = std::find_if(policies.begin(), policies.end(), [&](const auto& p) {
50+
return *p.name() == name;
51+
});
52+
return it == policies.end() ? nullptr : &*it;
53+
}
54+
55+
// Find the routing-policy statement keyed by name, creating it if absent.
56+
// Setting an attribute on a not-yet-created policy implicitly creates it, so
57+
// command ordering stays forgiving; a bare `routing-policy <name>` creates one
58+
// explicitly. BgpPolicyStatement's only key field is `name`.
59+
inline bgp::bgp_policy::BgpPolicyStatement& findOrCreateRoutingPolicy(
60+
bgp::thrift::BgpConfig& cfg,
61+
const std::string& name) {
62+
auto& policies = *cfg.policies().ensure().bgp_policy_statements();
63+
for (auto& policy : policies) {
64+
if (*policy.name() == name) {
65+
return policy;
66+
}
67+
}
68+
policies.emplace_back();
69+
auto& policy = policies.back();
70+
policy.name() = name;
71+
return policy;
72+
}
73+
74+
inline bool routingPolicyTermExists(
75+
const bgp::bgp_policy::BgpPolicyStatement& policy,
76+
int64_t seqNum) {
77+
const auto& terms = *policy.policy_entries();
78+
return std::any_of(terms.begin(), terms.end(), [&](const auto& t) {
79+
return t.sequence_number().has_value() && *t.sequence_number() == seqNum;
80+
});
81+
}
82+
83+
// Find the term keyed by sequence_number, creating it if absent. The name is
84+
// seeded from the seq-num so every term has a stable, unique identity —
85+
// BgpPolicyAction.next_term_id references terms by name.
86+
inline bgp::bgp_policy::BgpPolicyTerm& findOrCreateRoutingPolicyTerm(
87+
bgp::bgp_policy::BgpPolicyStatement& policy,
88+
int64_t seqNum) {
89+
auto& terms = *policy.policy_entries();
90+
for (auto& term : terms) {
91+
if (term.sequence_number().has_value() &&
92+
*term.sequence_number() == seqNum) {
93+
return term;
94+
}
95+
}
96+
terms.emplace_back();
97+
auto& term = terms.back();
98+
term.sequence_number() = seqNum;
99+
term.name() = std::to_string(seqNum);
100+
return term;
101+
}
102+
103+
} // namespace facebook::fboss::bgpcli

fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp

Lines changed: 4 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "configerator/structs/neteng/bgp_policy/thrift/gen-cpp2/bgp_policy_types.h"
2626
#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliAttrHandlers.h"
2727
#include "fboss/cli/fboss2/commands/config/protocol/bgp/BgpCliValueParsers.h"
28+
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h"
2829
#include "fboss/cli/fboss2/session/ConfigSession.h"
2930
#include "fboss/cli/fboss2/utils/CmdUtilsCommon.h"
3031
#include "fboss/cli/fboss2/utils/HostInfo.h"
@@ -81,37 +82,6 @@ std::string validAttrList() {
8182
return out;
8283
}
8384

84-
// Find the routing-policy statement keyed by name, creating it if absent.
85-
// Setting an attribute on a not-yet-created policy implicitly creates it, so
86-
// command ordering stays forgiving; a bare `routing-policy <name>` creates one
87-
// explicitly. BgpPolicyStatement's only key field is `name`.
88-
BgpPolicyStatement& findOrCreatePolicy(
89-
bgp::thrift::BgpConfig& cfg,
90-
const std::string& name) {
91-
auto& policies = *cfg.policies().ensure().bgp_policy_statements();
92-
for (auto& policy : policies) {
93-
if (*policy.name() == name) {
94-
return policy;
95-
}
96-
}
97-
policies.emplace_back();
98-
auto& policy = policies.back();
99-
policy.name() = name;
100-
return policy;
101-
}
102-
103-
bool policyExists(const bgp::thrift::BgpConfig& cfg, const std::string& name) {
104-
if (!cfg.policies().has_value()) {
105-
return false;
106-
}
107-
for (const auto& policy : *cfg.policies()->bgp_policy_statements()) {
108-
if (*policy.name() == name) {
109-
return true;
110-
}
111-
}
112-
return false;
113-
}
114-
11585
} // namespace
11686

11787
// Parse + validate at construction so queryClient stays a thin dispatch.
@@ -152,8 +122,9 @@ CmdConfigProtocolBgpPolicyRoutingPolicy::queryClient(
152122
const ObjectArgType& args) {
153123
auto& session = ConfigSession::getInstance();
154124
auto& cfg = session.getBgpConfig();
155-
const bool policyCreated = !policyExists(cfg, args.policyName());
156-
auto& policy = findOrCreatePolicy(cfg, args.policyName());
125+
const bool policyCreated =
126+
!bgpcli::routingPolicyExists(cfg, args.policyName());
127+
auto& policy = bgpcli::findOrCreateRoutingPolicy(cfg, args.policyName());
157128

158129
Result result = args.attr().empty()
159130
? ok(policyCreated

fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,9 +58,12 @@ struct CmdConfigProtocolBgpPolicyRoutingPolicyTraits
5858
: public WriteCommandTraits {
5959
using ParentCmd = CmdConfigProtocolBgpPolicy;
6060
static void addCliArg(CLI::App& cmd, std::vector<std::string>& args) {
61-
// Stops CLI11 from classifying attribute tokens as subcommands once the
62-
// policy name is consumed. See CmdConfigProtocolBgpNeighborTraits.
63-
cmd.positionals_at_end();
61+
// Unlike the sibling policy families, no positionals_at_end() here: the
62+
// seq-num-keyed term level is a real CLI11 subcommand (`routing-policy
63+
// <name> term <seq-num> ...`), and positionals_at_end() would stop CLI11
64+
// from classifying `term` after the policy name. The trade-off is that a
65+
// free-text attribute value equal to `term` is stolen by subcommand
66+
// matching (same exposure as the `config interface` subcommand chain).
6467
cmd.add_option("args", args, "<name> [<attribute> <value> ...]");
6568
}
6669
using ObjectArgType = BgpRoutingPolicyConfig;

0 commit comments

Comments
 (0)