[Nexthop][fboss2-dev] fboss2 bgp policy routing-policy term match commands - #1491
Open
hillol-nexthop wants to merge 21 commits into
Open
[Nexthop][fboss2-dev] fboss2 bgp policy routing-policy term match commands#1491hillol-nexthop wants to merge 21 commits into
hillol-nexthop wants to merge 21 commits into
Conversation
Follow-up cleanups to the BGP-aware config session infra (facebook#1344): - Collapse the BGP_RESTART action level into AGENT_WARMBOOT (bgpd has no hitless reload; its restart already runs the agent-warmboot code path). - Introduce a single ConfigDomain descriptor + configDomains() and shared per-domain helpers so commit(), rollback() and `config session diff` handle the agent and BGP domains uniformly (private DiffDomain removed). - Make the agent skip-when-unchanged like BGP: a commit whose staged config equals what is already promoted is a true no-op (no git revision, no symlink churn, no reloadConfig()/bgpd restart). Change detection is semantic (compare the deserialized thrift structs), so formatting-only diffs don't count. - Consolidate `config session clear` onto a static stagedSessionFilePaths() and reuse ConfigSession::readStagedContent() in diff. - Make ConfigSession::saveConfig(service, level) generic over the service and reduce saveBgpConfig() to a thin wrapper. - Keep the heavy generated thrift headers out of ConfigSession.h: use the *_types_fwd.h forward-declaration headers, hold agentConfig_/bgpConfig_ by std::unique_ptr, and drop the configLoaded_/bgpConfigLoaded_ bools (null == not loaded). - clang-tidy: use auto for the SimpleJSONSerializer template-cast results. Built fboss2-dev + the config unit tests; config-session/commit/diff/BGP/clear tests pass. Verified the agent no-op behaviour live on test switches.
Removed comments about the destructor definition in ConfigSession.h.
…thInterface call main renamed findFirstEthInterface() to getRandomInterfacePortName() (virtual-management-port fix); convert the branch-added no-op-commit test to the new helper. Drop the three includes ConfigSession.cpp no longer uses directly (misc-include-cleaner runs as errors in CI).
hillol-nexthop
force-pushed
the
bgp-policy-routing-policy-term-match
branch
from
August 10, 2026 04:44
2fc0379 to
e1f4a4e
Compare
This was referenced Aug 10, 2026
hillol-nexthop
force-pushed
the
bgp-policy-routing-policy-term-match
branch
3 times, most recently
from
August 10, 2026 05:52
5b81d36 to
d4d0b9b
Compare
hillol-nexthop
marked this pull request as ready for review
August 10, 2026 09:54
hillol-nexthop
force-pushed
the
bgp-policy-routing-policy-term-match
branch
2 times, most recently
from
August 10, 2026 13:56
9d08501 to
89bbec3
Compare
A first `config protocol bgp ...` edit on a freshly imaged box discards the running BGP config, leaving bgpd to crash-loop on an unset router_id. The bgp++ RPM installs /etc/coop/bgpcpp.conf as a plain file (router_id set), and the unit starts bgpd with --config /etc/coop/bgpcpp.conf. ConfigSession, however, treated the *promoted* /etc/coop/bgpcpp/bgpcpp.conf as the live read for BGP — a file that does not exist until the first commit. So loadBgpConfig() fell through to schema defaults, and the commit (which also replaces /etc/coop/bgpcpp.conf with a symlink into bgpcpp/) promoted that near-empty config over the running one. This is what fails as ConfigBgpGlobalTest.SetCountConfedsInAsPathLenTrue on a pristine box: bgpd never binds its thrift port and the test throws Connection refused; the next BGP test passes because teardown restarts bgpd. The agent domain never had this bug: its systemPath is the symlink the agent actually reads, so seeding works whether that path is still a plain file or already the promoted symlink. BGP pointed it at the promoted file instead. That inconsistency is the bug. Change: - ConfigDomain.systemPath for BGP -> getBgpSystemConfigLinkPath(). Safe because nothing writes via systemPath: writes use promotedPath, symlink creation uses symlinkPath, and systemPath is read only by 'config session diff'. - loadBgpConfig() seeds from staged edits -> the daemon's config path -> the promoted path as a backstop for a missing symlink. - initializeGit() populates the promoted path from the daemon's config path before the baseline commit, so the first revision carries a BGP snapshot and a rollback to it cannot delete the running bgpcpp.conf. (The companion ConfigBgpTestBase::systemBgpConfigPath() change is omitted here: that fixture does not exist on this branch yet and the hunk rides the BGP test PRs stacked above.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add `fboss2-dev config protocol bgp global <attr> <value>` on top of the BGP-aware ConfigSession (base PR). Edits the typed bgp::thrift::BgpConfig via ConfigSession::getBgpConfig()/saveBgpConfig() -- the whole-config, scope-agnostic typed API (no global/peer/peer-group special-casing in ConfigSession). - Collapse the 10 per-attribute global command classes into one dispatcher; reject cluster-id (no BgpConfig field) instead of writing dead config; bound switch-limit / max_golden_vips so out-of-range values aren't truncated. - Integration tests: ConfigBgpGlobalTest (each attr set+commit, verified in the promoted /etc/coop/bgpcpp/bgpcpp.conf) and ConfigBgpSessionTest (clear/diff/commit/rollback + a no-op-restart regression), sharing ConfigBgpTestBase. Test Plan: - bazel test //fboss/cli/fboss2/test/config:cmd_config_test - bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test - fboss2_integration_test on a DUT with bgp_pp active: ConfigBgpSessionTest 6/6 pass (clear, diff, commit-restarts-bgp_pp, rollback-restores-config, and the unchanged-config does-not-restart case); agent-session regression (ConfigInterfaceMtuTest) passes.
- Extract the value parsers (parseBool/parseInt/parseNonNegInt32, handler Result) into a shared BgpCliValueParsers.h so sibling BGP dispatchers can reuse them, and add a bounded parseAsn4Byte: local-asn/confed-asn accepted any uint64 and silently persisted out-of-range ASNs (>= 2^32 wrap the i64 field negative). - positionals_at_end() on the global command: CLI11's parent-chain subcommand fallthrough steals value tokens that match a sibling command name (e.g. a policy named "peer-group") and misparses the command. - Integration test base: probe the bgpd unit, and pass -c safe.directory=/etc/coop on the raw git invocations (gitHead / bgpTrackedAtRevision), mirroring the CLI's Git class -- /etc/coop is owned by another user (e.g. coop) on provisioned devices, which git otherwise rejects as dubious ownership and the helpers silently return empty results.
The commit-path global-attribute tests asserted on the promoted /etc/coop/bgpcpp/bgpcpp.conf, which only proves what the CLI wrote to disk. Route ConfigBgpTestBase::setAndCommit() through a new readRunningBgpConfigViaRpc() -- TBgpService::getRunningConfig against the local bgpd -- so every positive ConfigBgpGlobalTest asserts its attribute in the daemon's own view of its config, proving bgpd parsed and adopted the promoted file after the commit-triggered restart. The helper retries briefly on connection errors: systemd reports bgpd active as soon as the process starts, but its thrift server binds the port a few seconds later, so an RPC issued right after a restart races that window. Test Plan: - bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test - fboss2_integration_test on a DUT with bgpd active: ConfigBgpGlobalTest 7/7 pass (each attribute set+commit verified in bgpd's getRunningConfig view, plus the invalid-bool and negative-graceful-restart-time reject paths).
Companion to "fboss2 config session seeds BGP config from the daemon's config path": ConfigBgpTestBase::systemBgpConfigPath() reads the stable /etc/coop/bgpcpp.conf daemon path instead of the promoted file, so the fixture no longer requires a prior commit to have happened and BGP tests can run first-thing on a freshly imaged box. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds `config protocol bgp neighbor <ip-address> [<attribute> <value> ...]` and `delete protocol bgp neighbor <ip-address>` — the first per-peer BGP config family, beneath the `protocol bgp` grouping node. - neighbor dispatcher keyed by peer address, writing bgp_config.BgpPeer through the typed ConfigSession. Attributes cover the peer identity (remote/local ASN, description, peer-tag, peer-group), policy bindings (ingress/egress policy), session tunables (timers, route limits, next-hop, add-path, RR client, and the rest of the documented set), and the sheet-documented attributes bgpd does not support, which are rejected with an explanatory message rather than staged. - delete removes the peer by address. - ConfigBgpTestBase restores the committed BGP config after every test, so a failed run cannot leave a device carrying test config; commit-path integration tests verify each value through bgpd's own getRunningConfig RPC rather than only the staged file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ands Collapses the 18 per-attribute `config protocol bgp peer-group` command classes (built on the deprecated `folly::dynamic` BgpConfigSession) into a single typed dispatcher, mirroring CmdConfigProtocolBgpNeighbor. Also adds `delete protocol bgp peer-group <name>`. The group name is the first positional token; the next one or two tokens name the attribute, matched longest-prefix-first so `timers hold-time` wins over any `timers` prefix; the rest are its value(s). Handlers mutate the typed bgp::thrift::PeerGroup through ConfigSession::getBgpConfig() / saveBgpConfig(), so adding a tunable is a one-entry change in the dispatch table rather than a new command class. 36 command files are deleted. - Covers the 29 dispatch keys that map to a PeerGroup thrift field: remote-asn / local-asn (4-byte-bounded), description, peer-tag, ingress-policy / egress-policy, rr-client, confed-peer, redistribute-peer, enhanced-route-refresh, connect-mode, add-path send|receive, afi disable-ipv4-afi|disable-ipv6-afi|ipv4-over-ipv6-nh, graceful-restart restart-time|stateful-ha, max-route pre-filter|post-filter (plus the warning-threshold / warning-only knobs), timers hold-time|keepalive|out-delay|withdraw-unprog-delay, and next-hop-self. - Rejected rather than persisted as dead config (precedent: cluster-id in the global command, connect-mode BOTH in the neighbor command): connect-mode BOTH, since thrift only models is_passive. add-path send|receive merges into the AddPath enum bitmask, and clearing the last direction unsets the field. - Attributes with no per-peer-group thrift field (afi ipv4-labeled-unicast, afi ipv6-labeled-unicast, peer-port) are absent from the dispatch table, so they are refused at parse time as unknown attributes; a rejected value never lands on disk. - positionals_at_end() stops parent-chain subcommand fallthrough from reclassifying an attribute token that matches a sibling command name once the group name has been consumed (same fix as the neighbor command). - unit tests (16): CmdConfigBgpPeerGroupTest (12) covering arg validation, longest-prefix match, the add-path merge matrix, connect-mode, the bool/string/route-limit attributes, value validation and unknown-attribute rejection, plus CmdDeleteBgpPeerGroupTest (4). - integration tests (3): ConfigBgpPeerGroupTest asserts the committed peer_groups through bgpd's getRunningConfig RPC, proving the daemon parsed and adopted the promoted config rather than only checking the file the CLI wrote. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…list Adds the first BGP policy object-type command family, under a new `policy` grouping node beneath `protocol bgp` (sibling to global/neighbor/peer-group): config protocol bgp policy as-path-list <name> [description <string>] delete protocol bgp policy as-path-list <name> The list's `entry <seq-num>` level is a CLI11 subcommand of its own and lands in the next commit, so this one owns only the list level. - Follows the dispatcher shape the neighbor/peer-group families use: one factory per value shape from the shared BgpCliAttrHandlers.h, named setters that do nothing but assign the thrift field, and a registry that is one line per attribute. Writes bgp_policy.BgpPolicies.aspath_lists[] through the typed ConfigSession — no new session plumbing, since BgpConfig.policies already exists. - Lookup/create helpers live in BgpAsPathListCliUtils.h, including the entry-level helpers, so the delete command and the entry subcommand in the next commit share them rather than re-deriving the scan. - A rejected value leaves nothing staged: a list implicitly created for the failed command is rolled back before returning. - There is no per-entry delete; `delete ... as-path-list <name>` removes the whole list. - unit tests (10): CmdConfigBgpPolicyAsPathListTest (6) covering arg validation, bare create, the description round-trip, named lists staying distinct, re-reference reporting the existing list and unknown-attribute rejection, plus CmdDeleteBgpPolicyAsPathListTest (4). - integration tests (2): ConfigBgpPolicyAsPathListTest stages and commits, then asserts against bgpd's running config via the getRunningConfig RPC, confirming bgpd accepts and adopts the .policies blob end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Splits the `entry` level of `config protocol bgp policy as-path-list` into
its own CLI11 subcommand. The previous commit ships the list level; this one
ships everything keyed by `entry <seq-num>`:
as-path-list <name> entry <seq-num>
-> AsPathList.as_path_list[], keyed by sequence_number
as-path-list <name> entry <seq-num> asn-regexp <regex>
-> AsPathListEntry.as_path.as_path.asn_regexp
as-path-list <name> entry <seq-num> description <string>
-> AsPathListEntry.description
as-path-list <name> entry <seq-num> match-logic <EQUAL|NOT_EQUAL>
-> AsPathListEntry.match_logic_type
- `entry` is a real CLI11 subcommand rather than tokens parsed inside the
parent's arg type: the list name arrives through the ancestor-args tuple,
and lookup/create is shared with the parent through
BgpAsPathListCliUtils.h. Adding an entry attribute is a one-line registry
change, same as at the list level.
- A list attribute typed alongside `entry` is rejected rather than silently
dropped, because only the leaf command runs.
- asn-regexp sets the AsPathType union's inline AsPath arm. The pattern may
contain spaces, since AS-path regexes separate ASNs with spaces
(e.g. `^65000 65001$`), so it takes the joined-string value shape rather
than requiring a single token.
- A rejected value leaves nothing staged: a list or entry implicitly created
for the failed command is rolled back.
- unit tests (10): CmdConfigBgpPolicyAsPathListEntryTest covering arg
validation, bare entry create, attribute round-trips, seq-num keying,
asn-regexp accepting spaces, the match-logic default, and the rejection
paths.
- integration tests (2): ConfigBgpPolicyAsPathListEntryTest stages and
commits each attribute, then reads it back out of bgpd's running config
via the getRunningConfig RPC. Enums come back over SimpleJSON as
integers, so match_logic_type is asserted as MatchValueLogicOperator's
numeric value rather than its name.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y-list Adds `config protocol bgp policy community-list <name> [community <name>] [<attribute> <value> ...]` and `delete protocol bgp policy community-list <name>` — the second BGP policy object-type command family, beneath the `policy` grouping node alongside as-path-list. - community-list dispatcher with a two-level key (list name + inline community member name) writing bgp_policy.BgpPolicies.community_lists[] through the typed ConfigSession; boolean-operator maps to routing_policy.BooleanOperator, exact-match to the optional bool, and the member attributes (description/type/value) set the CommunityRefType union's inline Community arm, keyed by Community.name. - new boolAttr factory in the shared BgpCliAttrHandlers.h (generalizing the neighbor dispatcher's), reused by upcoming prefix-list. - fix a latent dangling-string_view in the shared enumAttr factory: the lambda captured a string_view over the caller's fmt::format temporary, so every invalid-enum-value rejection printed garbage after "expected". valueDesc is now taken (and captured) by value; the as-path-list and community-list rejection tests pin the full message. - unit tests (19) + commit-path integration tests mirroring ConfigBgpPolicyAsPathListTest (not yet run on a DUT). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mand
Splits the `community` (inline member) level of `config protocol bgp policy
community-list` into its own CLI11 subcommand, mirroring
`as-path-list <name> entry <seq-num>`. The previous commit ships the list
level and the whole delete command; this one ships the member level of the
config command:
community-list <name> community <name>
-> members[] inline Community, keyed by name
community-list <name> community <name> value <string>
-> Community.value
community-list <name> community <name> type <NORMAL|EXTENDED|LARGE>
-> Community.type
community-list <name> community <name> description <string>
-> Community.description
- The member is selected through the CommunityRefType union's inline arm.
The list name arrives through the ancestor-args tuple, and lookup/create
is shared with the parent through BgpCommunityListCliUtils.h.
- A list attribute typed alongside `community` is rejected rather than
silently dropped, because only the leaf command runs.
- `delete ... community-list <name> community <name>` already ships in the
previous commit and is untouched here: delete is a single dispatcher that
handles both levels through parseListMemberSelector(), so it did not need
splitting.
- A rejected value leaves nothing staged: a list or member implicitly
created for the failed command is rolled back.
- unit tests (9): CmdConfigBgpPolicyCommunityListCommunityTest covering arg
validation, bare member create, the three member attributes, named
members staying distinct, and the rejection paths.
- integration tests (2): ConfigBgpPolicyCommunityListCommunityTest stages
and commits, then verifies through bgpd's getRunningConfig RPC, including
that the list and a sibling member survive a single-member delete.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds `config protocol bgp policy prefix-list <name> [entry <seq-num>] [<attribute> <value> ...]` and `delete protocol bgp policy prefix-list <name> [entry <seq-num>]` — the third BGP policy object-type command family, beneath the `policy` grouping node alongside as-path-list and community-list. - prefix-list dispatcher with a two-level key (list name + entry seq-num) writing bgp_policy.BgpPolicies.prefix_lists[] through the typed ConfigSession. List level: boolean-operator, compare-operator (EQ|GE|LE|NE|GT|LT), description, and ip-version <v4|v6> (stored as the numeric routing_policy.PrefixList.version, the field the sheet documents). Entry level (routing_policy.PrefixListEntry keyed by seq_num in prefixes[]): base-prefix (validated as <prefix/len>, an explicit /len is required), communities (accumulates into the optional set), description, match-logic, max-allowed-subnet-count (-> max_allowed_golden_prefix_subnet_count), regex, and the prefix-len-range compare-operator|value pair targeting the single CompareNumericValue the CLI maintains at prefix_len_ranges[0] (compare-operator additionally accepts RG; value is bounded 0-128). - delete mirrors community-list's two levels: the whole list by name, or a single entry by `entry <seq-num>`. - new intAttr factory in the shared BgpCliAttrHandlers.h (bounded int32, valueDesc taken by value like enumAttr's), used by the two numeric entry attributes. - unit tests (19) + commit-path integration tests mirroring ConfigBgpPolicyCommunityListTest, run on a DUT (all 4 pass). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Splits the `entry` level of `config protocol bgp policy prefix-list` into
its own CLI11 subcommand, mirroring `as-path-list <name> entry <seq-num>`.
The previous commit ships the list level and the whole delete command; this
one ships the entry level of the config command:
prefix-list <name> entry <seq-num>
-> prefixes[], keyed by seq_num
... entry <seq> base-prefix <prefix/len> -> PrefixListEntry.base_prefix
... entry <seq> match-logic <EQUAL|NOT_EQUAL>
-> PrefixListEntry.match_logic
... entry <seq> prefix-len-range compare-operator <EQ|GE|LE|NE|GT|LT|RG>
-> prefix_len_ranges[0].compare_operator
... entry <seq> prefix-len-range value <0-128>
-> prefix_len_ranges[0].value
... entry <seq> communities <community-string>
-> PrefixListEntry.communities
... entry <seq> max-allowed-subnet-count <value>
-> PrefixListEntry.max_allowed_golden_prefix_subnet_count
... entry <seq> regex <string> -> PrefixListEntry.regex
... entry <seq> description <string> -> PrefixListEntry.description
- The list name arrives through the ancestor-args tuple, and lookup/create
is shared with the parent through BgpPrefixListCliUtils.h. A list
attribute typed alongside `entry` is rejected rather than silently
dropped, because only the leaf command runs.
- base-prefix is validated with folly::IPAddress::tryCreateNetwork and
additionally requires an explicit /len, because folly silently fills in an
address-family default mask for a bare address. The string is stored as
typed, not normalized.
- The thrift models a list of accepted length ranges (prefix_len_ranges),
but the documented CLI exposes a single range, so prefix-len-range
compare-operator|value both target the one CompareNumericValue the CLI
maintains at prefix_len_ranges[0], created on first use and rolled back if
the value is rejected.
- communities is a thrift set<string>: repeated invocations accumulate
members, and re-adding an existing member reports it without duplicating.
- Four value shapes have no factory in BgpCliAttrHandlers.h (base-prefix,
communities, prefix-len-range, regex) and get hand-written handlers; the
rest go through the shared factories. The composed
`prefix-len-range <sub-attr>` display names get static storage so the
factories' string_view name capture cannot dangle.
- `delete ... prefix-list <name> entry <seq-num>` already ships in the
previous commit and is untouched here.
- unit tests (14): CmdConfigBgpPolicyPrefixListEntryTest covering arg
validation, bare entry create, seq-num keying, the entry-level attribute
round-trips, communities accumulation, the composed prefix-len-range
sub-attributes, the rejection messages (explicit-/len requirement,
out-of-range mask, second slash, v6 accept, the 0-128 range bound), and
phantom-rollback for entry and range.
- integration tests (2): ConfigBgpPolicyPrefixListEntryTest stages and
commits, then verifies through bgpd's getRunningConfig RPC including
prefix_len_ranges[0] and communities, with the sibling entry surviving the
single-entry delete.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…policy Adds `config protocol bgp policy routing-policy <name> [<attribute> <value> ...]` and `delete protocol bgp policy routing-policy <name>` — the fourth BGP policy object-type command family, beneath the `policy` grouping node alongside as-path-list, community-list, and prefix-list. - routing-policy dispatcher keyed by policy name writing bgp_policy.BgpPolicies.bgp_policy_statements[] (BgpPolicyStatement) through the typed ConfigSession. Policy level: description (joined multi-token string, NOS-6671); bare create (NOS-6689). The seq-num-keyed `term <seq-num>` level and its match/action attributes land as follow-ups. - delete removes the whole policy statement by name; extra tokens are rejected. - unit tests (11) + commit-path integration tests mirroring ConfigBgpPolicyCommunityListTest, verifying the description lands in bgpd's running config and that bgpd adopts a policy statement with no terms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
…ands
Adds `config protocol bgp policy routing-policy <name> term <seq-num>
action result <ACCEPT|REJECT|CONTINUE>` and `... action set
<attribute> <value> ...` — the action level of a routing-policy term.
`action` is a pure grouping node; `result` and `set` are its subcommands,
each with its own handler (the policy and term args arrive through the
ancestor-args tuple). The term traits drop positionals_at_end() so CLI11
can classify `action` (and later `match`) after the seq-num; ancestor
attributes mixed with an action command are rejected since only the leaf
runs.
`set` MUST be a real subcommand rather than a parsed token: CLI11's
_valid_subcommand walks the parent chain unconditionally, so a bare `set`
arg token inside `action` was classified as the top-level `set` VERB and
stolen ("The following arguments were not expected"). A local subcommand
named `set` shadows the verb because _find_subcommand checks the local
scope first.
- `result` (NOS-6672) maps ACCEPT/REJECT/CONTINUE onto FlowControlAction
ACCEPT/DENY/NEXT_TERM in term_miss_action. GOTO-TERM stays deferred: no
FlowControlAction arm, and the per-action-entry next_term_id target has
no slot in the documented grammar.
- `set <attr>` writes one bgp_policy.BgpPolicyAction entry per action kind
in policy_entries[].policy_action_entries[] (keyed by which payload field
is set; re-issuing a kind updates its entry):
- as-path prepend <asn> [<asn> ...] (NOS-6673) -> SetAsPathPrepend{asn,
repeat_times}; the documented <asn-list> must be uniform since the
thrift models one ASN repeated N times
- community <community-string> [additive] (NOS-6674) -> inline
single-member CommunityList + route_action COMMUNITY_LIST_ADD/SET
- local-pref <0-4294967295> (NOS-6675) -> LocalPreference.local_pref
- med <0-4294967295> (NOS-6676) -> MedAction{med_value, SET}
- next-hop <ip-address> (NOS-6677) -> SetNextHop; the sheet's `self`
(bgpd rejects set_self) and `peer-address` (no thrift arm) are deferred
- origin <IGP|EGP|INCOMPLETE> (NOS-6678) -> set_origin
- weight <0-65535> (NOS-6679) -> WeightAction{weight_value, SET}
- unit tests (14) + a commit-path integration test staging every action
kind and verifying each entry in bgpd's running config.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…atch Adds `config protocol bgp policy routing-policy <name> term <seq-num> match from <attribute> <value>` — the match level of a routing-policy term, as a CLI11 subcommand with its own handler alongside `action`; the policy and term args arrive through the ancestor-args tuple, and ancestor attributes mixed with a match command are rejected since only the leaf runs. Scoped to the three match kinds bgpd actually implements. Reading private-BGP's PolicyTerm.cpp turned up two things the sheet's Thrift reference column does not convey, but its JSON field path column does: 1. bgpd reads BgpPolicyTerm.policy_match_entries — a single BgpPolicyMatch, marked @thrift.Deprecated. Its match loop is guarded by `if (term.policy_match_entries())`, and NOTHING in private-BGP reads the newer `policy_matches` list, so matches written there are silently ignored: the config round-trips through getRunningConfig while bgpd applies no match at all. Same for the atomic payloads — bgpd reads the deprecated as_path_filters/communities_filter/prefix_filters, not the newer as_path_list/community_list/prefix_list. 2. createPolicyAttributeMatchItem has no LOCAL_PREFERENCE, MED or NEXT_HOP case, so those atomic types hit `default:` and throw "BgpPolicyAtomicMatch Config input error for type", crash-looping the daemon. `from local-pref`, `from med` and `from next-hop` (NOS-6683/6684/6685) are therefore rejected at parse time rather than offered; they need a private-BGP change first, and a unit test pins that they stay rejected. 3. `from community-list` (NOS-6682) is rejected for a different reason: CommunityMatch's constructor throws "The attribute \"communities\" is empty" unless the inline communities list is non-empty, and CommunityMatch::PopulateReferences -- the only thing that resolves community_list_names -- runs later, from PolicyManager, once every Policy is already constructed. The by-name reference path is therefore unreachable, so only inlined values would work, which is a snapshot rather than a reference. Same unit test pins it as rejected. - `from <attr>` writes one BgpPolicyAtomicMatch per match kind (keyed by BgpPolicyAtomicMatchType; re-issuing a kind updates its entry) into policy_match_entries, composing under its default AND — which is also the only operator bgpd accepts for more than one entry: - as-path-list <name> (NOS-6681) -> AS_PATH, as_path_filters - origin <IGP|EGP|INCOMPLETE> (NOS-6686) -> ORIGIN, origin - prefix-list <name> (NOS-6687) -> PREFIX_LIST, prefix_filters The two list-typed matches take a full inline object with no name-only union arm, and bgpd treats that object's `name` as the reference: the match class keeps the name, then PolicyManager::PopulateReferences swaps in the real list from its by-name map. A name-only object is exactly how a by-name match is expressed. - setters are named functions, so the registry reads as a table: one line per attribute giving its dispatch key, value shape, and setter. - unit tests (8) + a commit-path integration test. The integration assertions are load-bearing here in a way staged-config ones are not: bgpd constructs a Policy from every statement, so a wrong encoding crash-loops it and fails the test rather than sitting inert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The routing-policy term match/action commands nest six levels deep (config protocol bgp policy routing-policy term match), which trips static_assert(N <= M) in utils::arrayToTuple via CmdArgsLists::getTypedArgs. Raise CmdArgsLists::MAX_DEPTH from 5 to 6 to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hillol-nexthop
force-pushed
the
bgp-policy-routing-policy-term-match
branch
from
August 11, 2026 05:52
8420b52 to
4dd273c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the match level of a routing-policy term:
Scoped to these three deliberately. The match conditions are written to
policy_match_entries(the deprecated singular container) with the referenced list named in*_list_names, because that is what bgpd reads: the newerpolicy_matchesfield is not consumed anywhere in the daemon, so writing it would stage config that is silently ignored. The remaining documented match attributes (med, next-hop, local-preference) have no support in bgpd and are left out rather than staged as dead config.Test
Built
//fboss/cli/fboss2:fboss2-devplus the config and integration test targets;cmd_config_testpasses, includingCmdConfigBgpPolicyRoutingPolicyTermMatchTest. The integration tests create the referenced as-path-list / prefix-list, commit, and verify the match lands in bgpd's running config via thegetRunningConfigRPC.🤖 Generated with Claude Code