Skip to content

[Nexthop][fboss2-dev] fboss2 bgp policy community-list community subcommand - #1486

Open
hillol-nexthop wants to merge 12 commits into
facebook:mainfrom
nexthop-ai:bgp-policy-community-list-community
Open

[Nexthop][fboss2-dev] fboss2 bgp policy community-list community subcommand#1486
hillol-nexthop wants to merge 12 commits into
facebook:mainfrom
nexthop-ai:bgp-policy-community-list-community

Conversation

@hillol-nexthop

@hillol-nexthop hillol-nexthop commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Stacked on #1475 (bgp policy community-list). This PR's branch contains the whole stack below it, so the diff shows those commits too — please review only the bgp policy community-list community commit here. Merge the lower PRs first.

Stack (bottom → top): #1446#1345#1391#1395#1401#1485#1475#1486 (this PR)#1477#1487#1488#1489#1490#1491

Summary

Adds the community (inline member) level of config protocol bgp policy community-list as its own CLI11 subcommand, mirroring as-path-list <name> entry <seq-num> (#1485). #1475 ships the list level and the whole delete command; this PR ships the member level of the config command.

Commands

Command Thrift target
community-list <name> community <name> members[] inline Community (create/select)
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

delete ... community-list <name> community <name> already ships in #1475 and is untouched here.

Design

  • The member is selected through the CommunityRefType union's inline arm; the list name reaches the subcommand 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.
  • A rejected value never leaves a phantom list or member behind: objects implicitly created for the failed command are rolled back.

Test Plan

UnitCmdConfigBgpPolicyCommunityListCommunityTest (9): arg validation, bare member create, the three member attributes, named members staying distinct, and rejection paths. All pass as part of cmd_config_test.

Integration on device (real bgpd daemon), ConfigBgpPolicyCommunityListCommunityTest (2): SetAttributesAndCommit and DeleteCommunityAndCommit — each stages and commits, then verifies through bgpd's getRunningConfig RPC, including that the list and a sibling member survive a single-member delete.

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
hillol-nexthop force-pushed the bgp-policy-community-list-community branch from 60d9845 to 72e754a Compare August 10, 2026 04:44
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-community-list-community branch 2 times, most recently from c4d61f8 to 4ccab72 Compare August 10, 2026 05:43
@hillol-nexthop
hillol-nexthop marked this pull request as ready for review August 10, 2026 09:54
@hillol-nexthop
hillol-nexthop requested review from a team as code owners August 10, 2026 09:54
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).
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-community-list-community branch from 4ccab72 to 7e490d9 Compare August 10, 2026 10:12
hillol-nexthop and others added 2 commits August 10, 2026 13:55
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>
hillol-nexthop and others added 4 commits August 10, 2026 13:56
…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>
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-community-list-community branch from 7e490d9 to 796f2b2 Compare August 10, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant