[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy as-path-list commands - #1401
Open
hillol-nexthop wants to merge 13 commits into
Open
[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy as-path-list commands#1401hillol-nexthop wants to merge 13 commits into
hillol-nexthop wants to merge 13 commits into
Conversation
Make the fboss2 ConfigSession and `config session` lifecycle BGP-aware. This is
the base for the `config protocol bgp` commands, which stack on top.
- ConfigSession owns the BGP config as a typed bgp::thrift::BgpConfig, exactly
the way it owns cfg::AgentConfig for the agent. getBgpConfig()/saveBgpConfig()
expose the WHOLE typed config and stage it to ~/.fboss2/bgp_config.json.
ConfigSession is BGP-aware but agnostic about the config's internal structure:
it has no notion of "global" vs "peer" vs "peer-group" -- a command mutates
whichever typed fields it needs, mirroring how interface commands mutate
getAgentConfig().sw()->ports(). Because the whole config round-trips through
the typed struct, no surgical merge / preserve-list is needed.
- commit() promotes bgp_config.json to /etc/coop/bgpcpp/bgpcpp.conf, restarts
bgp_pp, and versions it in the same /etc/coop git repo as agent.conf.
- config session clear/diff/rebase/rollback are BGP-aware: clear removes a
staged BGP session, diff shows staged BGP changes, rebase 3-way-merges
bgpcpp.conf, rollback restores bgpcpp.conf + restarts bgp_pp (using metadata
history so BGP-only commits are reachable).
- FbossServiceUtil + cli_metadata gain the bgp_pp (BGP_PP) service/action; the
config lib gains the bgp_config cpp2 dependency.
- Unit tests in CmdConfigSession{,Diff}Test cover the typed round-trip, commit,
rebase, and rollback behavior.
Test Plan:
- bazel test //fboss/cli/fboss2/test/config:cmd_config_test //fboss/cli/fboss2/test:cmd_test
- fboss2_integration_test on a DUT (ConfigConcurrentSessionsTest.ConflictAndRebase,
ConfigSessionClearTest, ConfigInterfaceMtuTest, ConfigInterfaceDescriptionTest): 5/5 pass
The BGP++ daemon ships as bgpd.service (via the bgpd RPM) on devices and in the fboss-sim runtime image; bgp_pp only ever worked via a manually created alias unit. Rename the unit that ConfigSession/FbossServiceUtil restarts on commit, and the related comments and unit-test expectations.
…ged bgpcpp/ subdir `config session commit` writes the bgpd config to /etc/coop/bgpcpp/bgpcpp.conf (kBgpGitRelPath) and expects the daemon to read exactly that path, which forces a per-device systemd drop-in overriding bgpd's --config. Mirror the agent.conf scheme instead: on every BGP commit, keep the daemon-facing path /etc/coop/bgpcpp.conf a symlink into the CLI-managed bgpcpp/ subdir (exactly as agent.conf symlinks to cli/agent.conf). bgpd keeps its provisioned `--config /etc/coop/bgpcpp.conf` and follows the symlink, so no drop-in is needed and the arrangement is self-healing across re-provision. The symlink is git-tracked alongside the config so rollback restores it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hillol-nexthop
force-pushed
the
bgp-policy-aspathlist
branch
from
July 21, 2026 14:10
323d638 to
2af948f
Compare
Use auto for the SimpleJSONSerializer template-cast result in ConfigSession::saveBgpConfig() (modernize-use-auto).
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).
…or` commands Adds `fboss2-dev config protocol bgp neighbor <ip> [<attribute> <value> ...]` as a single dispatcher (mirroring the `global` command): the neighbor address is the first positional token, one- or two-token attributes are matched longest-prefix-first against a dispatch table, and handlers mutate the typed bgp::thrift::BgpPeer via ConfigSession::getBgpConfig()/saveBgpConfig(). - Bare `neighbor <ip>` creates the peer; attribute sets find-or-create it, keyed by the folly::IPAddress-normalized peer_addr (prefix notation for passive sessions is accepted). - New peers are seeded with parseable any-addresses for local_addr / next_hop4 / next_hop6: bgpd constructs folly::IPAddress from these non-optional fields at config load and aborts on the empty strings a freshly created peer would otherwise serialize (reproduced on an NH-4010-F device). - add-path send/receive merge into the single AddPath enum (RECEIVE/SEND/ BOTH); clearing the last direction unsets the field. - Rejected instead of persisting dead config: connect-mode BOTH (thrift only models is_passive), peer-port / bind-addr port (no per-peer port field), afi ipv4/ipv6-labeled-unicast (no thrift field). - Value validation: 4-byte ASN range, IP family for next-hop4/next-hop6, bind-addr address parse, thrift-enum membership for advertise-lbw / receive-lbw, non-negative timers and route limits. A rejected value never lands on disk. - Deletes the 26 per-attribute `peer` command classes plus the now-dead per-peer folly::dynamic BgpConfigSession API they used; parity attributes those commands supported (next-hop4/6, next-hop-self, peer-id, type, link-bandwidth, advertise/receive-lbw, timers withdraw-unprog-delay, pre/post warning limits) are carried over in the dispatcher. - `delete protocol bgp neighbor <ip>` removes the matching peer from the staged config via shared BgpPeerAddr.h normalize/find helpers; deleting an unknown neighbor errors without persisting anything. - bgp_json_to_cli.py peer generators emit the new grammar (shlex-escaped, including the neighbor address); explicit zero timers/limits are no longer dropped and symbolic add_path enum names are handled. - Integration tests verify committed neighbor config through the bgpd TBgpService::getRunningConfig RPC (with a brief connection retry: systemd reports the unit active before the thrift server binds its port), proving the daemon parsed and adopted the promoted config rather than only checking the file the CLI wrote.
Drop the redundant .c_str() on the TEnumTraits::findValue() argument in the neighbor enum attribute handler (readability-redundant-string-cstr).
Collapse the ~20 legacy per-attribute `config protocol bgp peer-group` command classes (on the deprecated folly::dynamic BgpConfigSession) into a single typed dispatcher, mirroring CmdConfigProtocolBgpNeighbor: the group name is the first positional, the next one or two tokens name the attribute, and the rest are its value(s); dispatch and per-attribute parsing live in the .cpp so adding a peer-group tunable is a one-entry change. Attributes write the typed bgp::thrift::PeerGroup directly via ConfigSession::getBgpConfig()/ saveBgpConfig(). Covers the 24 attributes with a PeerGroup thrift field (remote/local-asn, description, peer-tag, ingress/egress-policy, rr-client, confed-peer, redistribute-peer, enhanced-route-refresh, connect-mode, add-path send| receive, afi disable-ipv4/ipv6-afi|ipv4-over-ipv6-nh, graceful-restart restart-time|stateful-ha, max-route pre/post-filter + warning knobs, timers hold-time|keepalive|out-delay|withdraw-unprog-delay, next-hop-self). Attributes with no per-peer-group thrift field (afi ipv4/ipv6-labeled-unicast, peer-port) are not part of the dispatch table, so they are refused as unknown attributes rather than persisting dead config. connect-mode BOTH is rejected (no is_passive representation); add-path merges the RECEIVE/SEND bitmask. Adds `delete protocol bgp peer-group <name>`. Unit tests (CmdConfigBgpPeerGroupTest, CmdDeleteBgpPeerGroupTest) and integration tests (ConfigBgpPeerGroupTest) verify staging, the reject paths, and — for the commit path — that bgpd parses and adopts peer_groups via its getRunningConfig RPC.
Drop the unused parseInt using-declaration in the peer-group dispatcher (misc-unused-using-decls).
Adds `config protocol bgp policy as-path-list <name> [entry <seq-num>] [<attribute> <value> ...]` and `delete protocol bgp policy as-path-list <name>` — the first of the BGP policy object-type command families, under a new `policy` grouping node (config + delete) beneath `protocol bgp` (sibling to global/neighbor/peer-group). - as-path-list dispatcher with a two-level key (list name + entry sequence-number) writing bgp_policy.BgpPolicies.aspath_lists[] through the typed ConfigSession; asn-regexp sets the AsPathType union's inline AsPath arm (.as_path.as_path.asn_regexp), match-logic maps to MatchValueLogicOperator, plus per-list and per-entry description. - shared template attribute-handler factories (BgpCliAttrHandlers.h), reused by the upcoming community-list/prefix-list/routing-policy types. - unit tests (16) + integration (10/10) on an NH-4010-F device. The commit-path integration tests confirm bgpd accepts and adopts the .policies blob end-to-end via the getRunningConfig RPC.
Drop the unused bgp_policy_types.h include from the as-path-list delete command (misc-include-cleaner, warnings-as-errors).
hillol-nexthop
force-pushed
the
bgp-policy-aspathlist
branch
from
July 21, 2026 15:14
85ccd7f to
c5420a1
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.
Summary
Adds the first BGP policy object-type command family to
fboss2-dev:config protocol bgp policy as-path-list <name> [entry <seq-num>] [<attribute> <value> ...]delete protocol bgp policy as-path-list <name>under a new
policygrouping node beneathprotocol bgp(sibling toglobal/neighbor/peer-group).Commands
as-path-list <name>aspath_lists[](create/select)as-path-list <name> description <string>AsPathList.descriptionas-path-list <name> entry <seq-num>as_path_list[](create/select)as-path-list <name> entry <seq-num> asn-regexp <regex>AsPathListEntry.as_path.as_path.asn_regexpas-path-list <name> entry <seq-num> description <string>AsPathListEntry.descriptionas-path-list <name> entry <seq-num> match-logic <EQUAL|NOT_EQUAL>AsPathListEntry.match_logic_typeDesign
entry <seq-num>) selecting a nestedAsPathListEntry; writesbgp_policy.BgpPolicies.aspath_lists[]through the typedConfigSession(no new session plumbing —BgpConfig.policiesalready exists).asn-regexpsets theAsPathTypeunion's inlineAsPatharm; the pattern may contain spaces (AS-path regexes separate ASNs with spaces, e.g.^65000 65001$).BgpCliAttrHandlers.h—template<typename T>attribute-handler factories (stringAttr/joinedStringAttr/enumAttr) shared across policy object types, to be reused by the upcoming community-list / prefix-list / routing-policy families.Test Plan
Unit —
CmdConfigBgpPolicyAsPathListTestandCmdDeleteBgpPolicyAsPathListTest, 16/16 pass.Integration on device (real
bgpddaemon),ConfigBgpPolicyAsPathListTest: 10/10 pass. The commit-path tests assert againstbgpd's running config via thegetRunningConfigRPC, confirmingbgpdaccepts and adopts the.policiesblob end-to-end (including a bare entry whose AS-path union is unset).