Skip to content

Commit 8bd9da9

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 (15) + commit-path integration tests that exercise the CLI11 subcommand dispatch through the full parse and verify terms in bgpd's running config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent dbcee87 commit 8bd9da9

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
@@ -899,6 +899,9 @@ add_library(fboss2_config_lib
899899
fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h
900900
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp
901901
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h
902+
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h
903+
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp
904+
fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h
902905
fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.cpp
903906
fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h
904907
fboss/cli/fboss2/commands/config/ptp/transparent_clock/CmdConfigPtpTransparentClock.cpp
@@ -1007,6 +1010,8 @@ add_library(fboss2_config_lib
10071010
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h
10081011
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp
10091012
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h
1013+
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp
1014+
fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h
10101015
fboss/cli/fboss2/commands/delete/protocol/CmdDeleteProtocol.h
10111016
fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp
10121017
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
@@ -50,6 +51,7 @@ add_executable(fboss2_cmd_config_test
5051
fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyCommunityListTest.cpp
5152
fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyPrefixListTest.cpp
5253
fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTest.cpp
54+
fboss/cli/fboss2/test/config/CmdDeleteBgpPolicyRoutingPolicyTermTest.cpp
5355
fboss/cli/fboss2/test/config/CmdDeleteConfigInterfaceTest.cpp
5456
fboss/cli/fboss2/test/config/CmdDeleteDhcpTest.cpp
5557
fboss/cli/fboss2/test/config/CmdDeleteInterfaceIpv6NdpTest.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
@@ -1108,6 +1108,7 @@ cpp_library(
11081108
"commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.cpp",
11091109
"commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.cpp",
11101110
"commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.cpp",
1111+
"commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.cpp",
11111112
"commands/config/protocol/static/CmdConfigProtocolStatic.cpp",
11121113
"commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.cpp",
11131114
"commands/config/ptp/CmdConfigPtp.cpp",
@@ -1162,6 +1163,7 @@ cpp_library(
11621163
"commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.cpp",
11631164
"commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.cpp",
11641165
"commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.cpp",
1166+
"commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.cpp",
11651167
"commands/delete/protocol/static/CmdDeleteProtocolStatic.cpp",
11661168
"commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.cpp",
11671169
"commands/delete/qos/CmdDeleteQos.cpp",
@@ -1231,6 +1233,8 @@ cpp_library(
12311233
"commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h",
12321234
"commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h",
12331235
"commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h",
1236+
"commands/config/protocol/bgp/policy/routing-policy/BgpRoutingPolicyCliUtils.h",
1237+
"commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h",
12341238
"commands/config/protocol/static/CmdConfigProtocolStatic.h",
12351239
"commands/config/protocol/static/route/StaticRouteUtils.h",
12361240
"commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h",
@@ -1286,6 +1290,7 @@ cpp_library(
12861290
"commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h",
12871291
"commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h",
12881292
"commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h",
1293+
"commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h",
12891294
"commands/delete/protocol/static/CmdDeleteProtocolStatic.h",
12901295
"commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h",
12911296
"commands/delete/qos/CmdDeleteQos.h",

fboss/cli/fboss2/CmdListConfig.cpp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/CmdConfigProtocolBgpPolicyPrefixList.h"
4949
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/prefix-list/entry/CmdConfigProtocolBgpPolicyPrefixListEntry.h"
5050
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/CmdConfigProtocolBgpPolicyRoutingPolicy.h"
51+
#include "fboss/cli/fboss2/commands/config/protocol/bgp/policy/routing-policy/term/CmdConfigProtocolBgpPolicyRoutingPolicyTerm.h"
5152
#include "fboss/cli/fboss2/commands/config/protocol/static/CmdConfigProtocolStatic.h"
5253
#include "fboss/cli/fboss2/commands/config/protocol/static/route/add/CmdConfigProtocolStaticRouteAdd.h"
5354
#include "fboss/cli/fboss2/commands/config/ptp/CmdConfigPtp.h"
@@ -100,6 +101,7 @@
100101
#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/community-list/CmdDeleteProtocolBgpPolicyCommunityList.h"
101102
#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/prefix-list/CmdDeleteProtocolBgpPolicyPrefixList.h"
102103
#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/CmdDeleteProtocolBgpPolicyRoutingPolicy.h"
104+
#include "fboss/cli/fboss2/commands/delete/protocol/bgp/policy/routing-policy/term/CmdDeleteProtocolBgpPolicyRoutingPolicyTerm.h"
103105
#include "fboss/cli/fboss2/commands/delete/protocol/static/CmdDeleteProtocolStatic.h"
104106
#include "fboss/cli/fboss2/commands/delete/protocol/static/route/CmdDeleteProtocolStaticRoute.h"
105107
#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
},
@@ -801,6 +813,14 @@ const CommandTree& kConfigCommandTree() {
801813
CmdDeleteProtocolBgpPolicyRoutingPolicy>,
802814
argRegistrar<
803815
CmdDeleteProtocolBgpPolicyRoutingPolicyTraits>,
816+
{{
817+
"term",
818+
"Delete a routing-policy term: <seq-num>",
819+
commandHandler<
820+
CmdDeleteProtocolBgpPolicyRoutingPolicyTerm>,
821+
argRegistrar<
822+
CmdDeleteProtocolBgpPolicyRoutingPolicyTermTraits>,
823+
}},
804824
}},
805825
}},
806826
},
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)