Skip to content

[Nexthop][fboss2-dev] fboss2 bgp policy as-path-list entry subcommand - #1485

Closed
hillol-nexthop wants to merge 18 commits into
meta:mainfrom
nexthop-ai:bgp-policy-aspathlist-entry
Closed

hillol-nexthop wants to merge 18 commits into
meta:mainfrom
nexthop-ai:bgp-policy-aspathlist-entry

Conversation

@hillol-nexthop

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

Copy link
Copy Markdown
Contributor

Stacked on #1401 (config/delete protocol bgp policy as-path-list). This PR's branch contains the whole stack below it, so the diff shows those commits too — please review only the bgp policy as-path-list entry commit(s) here. Merge the lower PRs first.

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

Summary

Splits the entry level of config protocol bgp policy as-path-list out of the
list-level PR into its own CLI11 subcommand, mirroring
routing-policy <name> term <seq-num>.

Before, entry <seq-num> was parsed as tokens inside BgpAsPathListConfig, so
one arg type and one queryClient carried both key levels. Now the list
command owns only list attributes, and this command owns the entry: the list
name reaches it through the ancestor-args tuple, and lookup/create is shared
through BgpAsPathListCliUtils.h (added by the base PR).

The user-visible grammar is unchanged — as-path-list AS100 entry 10 asn-regexp '^65000_' still works; it is now dispatched by CLI11 rather than by
hand-rolled token matching.

A list attribute typed alongside entry is rejected rather than silently
dropped, because only the leaf command runs.

Sample usage

$ fboss2-dev config protocol bgp policy as-path-list AS100 entry 10
Successfully created BGP as-path-list AS100 entry 10
Config saved to: ~/.fboss2/bgp_config.json

$ fboss2-dev config protocol bgp policy as-path-list AS100 entry 10 asn-regexp '^65000_'
Successfully set asn-regexp to: ^65000_ for as-path-list AS100 entry 10

$ fboss2-dev config protocol bgp policy as-path-list AS100 entry 10 description match 65000
Successfully set description to: match 65000 for as-path-list AS100 entry 10

$ fboss2-dev config protocol bgp policy as-path-list AS100 entry 10 match-logic NOT_EQUAL
Successfully set match-logic to: NOT_EQUAL for as-path-list AS100 entry 10

$ fboss2-dev config protocol bgp policy as-path-list AS100 entry 10 match-logic MAYBE
Error: Invalid match-logic value 'MAYBE'; expected EQUAL|NOT_EQUAL

$ fboss2-dev config protocol bgp policy as-path-list AS100 description mixed entry 10
Error: configure as-path-list attributes and entry in separate commands (got as-path-list attribute 'description' alongside entry 10)

Test plan

Unit — fboss2_cmd_config_test:

$ fboss2_cmd_config_test --gtest_filter=*AsPathListEntry*
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.argValidation (107 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.bareCreateEntry (81 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.setEntryAttributes (108 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.entriesAccumulateAndAreKeyed (130 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.asnRegexpAllowsSpaces (119 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.reReferenceReportsExisting (109 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.matchLogicDefaultIsEqual (79 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.invalidMatchLogicRejected (82 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.rejectedEntryOnExistingListKeepsList (92 ms)
[       OK ] CmdConfigBgpPolicyAsPathListEntryTestFixture.listAttributeMixedWithEntryRejected (67 ms)
[==========] 10 tests from 1 test suite ran. (978 ms total)
[  PASSED  ] 10 tests.

Integration — fboss2_integration_test on a lab device with bgp_pp active:

$ fboss2_integration_test --gtest_filter='ConfigBgpPolicyAsPathListEntryTest.*'
[       OK ] ConfigBgpPolicyAsPathListEntryTest.SetEntryAsnRegexpAndCommit (1274 ms)
[       OK ] ConfigBgpPolicyAsPathListEntryTest.SetEntryMatchLogicAndCommit (2294 ms)
[==========] 2 tests from 1 test suite ran.
[  PASSED  ] 2 tests.

Based-on: #1401

@meta-cla meta-cla Bot added the CLA Signed label Aug 7, 2026
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-aspathlist-entry branch from 17ea8c8 to 19fead1 Compare August 10, 2026 04:44
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-aspathlist-entry branch 2 times, most recently from 131988a to a785bed 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
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-aspathlist-entry branch 3 times, most recently from 52feeda to 40276bd Compare August 11, 2026 05:52
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-aspathlist-entry branch 6 times, most recently from 8c89bc4 to 647225d Compare August 27, 2026 12:32
static const std::map<std::string, AttrHandler<AsPathListEntry>, std::less<>>
kHandlers = {
{std::string(kAsnRegexp),
joinedStringAttr<AsPathListEntry>(kAsnRegexp, setAsnRegexp)},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command saves asn-regexp without checking whether the expression is valid. For example, it accepts the invalid expression [.
Please validate the expression before saving it. Add a test confirming that an invalid expression does not change the staged config.

// valid-attribute set and the handler table stay in sync.
constexpr std::string_view kDescription = "description";
constexpr std::string_view kAsnRegexp = "asn-regexp";
constexpr std::string_view kMatchLogic = "match-logic";

@induvsuresh induvsuresh Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of sequence_number and match-logic is unclear. These entries are combined with OR, so NOT_EQUAL does not behave like an ordered deny rule. Please confirm the intended behavior. If sequence numbers define first-match order, use permit and deny. Otherwise, document the OR behavior and use command names that describe it clearly.

}

TEST_F(CmdConfigBgpPolicyAsPathListEntryTestFixture, asnRegexpAllowsSpaces) {
// AS-path regexes separate ASNs with spaces; the pattern must survive as a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test says AS numbers are separated by spaces. Current BGP++ policy matching separates them with underscores.
^65000 65001$ will not match the current BGP++ AS-path value. Please test ^65000_65001$ through real policy matching, or implement and test conversion from spaces.

@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-aspathlist-entry branch from d0e8798 to 35df440 Compare September 7, 2026 05:41
@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

meta-codesync Bot pushed a commit that referenced this pull request Sep 11, 2026
Summary:
> **Base of the `fboss2-dev config protocol bgp` stack** (follow-up to the merged #1344). Everything above it in the list below builds on this PR, so merge this one first.
>
> **Stack (bottom → top):** **#1446 (this PR)** → #1345 → #1391 → #1395 → #1401 → #1485 → #1475 → #1486 → #1477 → #1487 → #1488 → #1489 → #1490 → #1491

Follow-up to #1344 (BGP-aware fboss2 config session): unify the agent and BGP
domains inside `ConfigSession` and decouple the `ConfigSession.h` header from
the heavy generated thrift types.

- Make `ConfigActionLevel` generic across services (review feedback below):
  `HITLESS` / `SERVICE_RESTART` / `DISRUPTIVE_SERVICE_RESTART` replace
  `HITLESS` / `AGENT_WARMBOOT` / `AGENT_COLDBOOT` (+ the bgpd-only `BGP_RESTART`).
  What a level means is decided per service from the (service, level) pair in
  `FbossServiceUtil::restartTypeName()`: agent → warmboot / coldboot, bgpd (no
  warmboot or hitless reload) → plain restart. `config session commit` labels
  the restarted services accordingly (`wedge_agent (warmboot)`, `bgpd (restart)`).
  A session staged before this change (old level names in
  `~/.fboss2/cli_metadata.json`) no longer parses and loses its recorded
  restart level (WARNING logged) -- `config session clear` it first.
- 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).

Pull Request resolved: #1446

Test Plan:
- `fboss/cli/fboss2/test/config:cmd_config_test` passes (the config-session /
  commit / diff / BGP / clear unit tests cover the refactored paths); re-run in
  full after the action-level rename (871 tests).
- `//fboss/cli/...` builds and tests pass on top of `upstream/main`.
- Verified the agent no-op commit behaviour live on test switches.

Reviewed By: AarjunC

Differential Revision: D114376130

Pulled By: joseph5wu

fbshipit-source-id: 3a00f779a3c26b280be27dba92b28549bec692ca
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-aspathlist-entry branch from 35df440 to 2f7ba1b Compare September 15, 2026 16:52
@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

hillol-nexthop and others added 18 commits September 15, 2026 17:04
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>
…p global

`router-id` stored whatever token it was given. bgpd builds an IPAddress from
it at config load (an unparseable string fails the load) and later calls
asV4() on it, so a v6 address passes construction and throws afterwards.
Parse with folly::IPAddress::tryFromString, require IPv4 and store the
normalized form.

`switch-limit-overload-protection-mode` cast any int32 straight into the enum;
bgpd switches on the value without a default and logs anything but 2 as
DROP_EXCESS_PREFIXES, so -5 or 999 silently behaved as mode 1. Accept the
enum name or its integer, reject everything else listing the valid names, and
validate before ensure() so a rejected value does not materialize
switch_limit_config. The success message keeps printing the integer so the
j2c round-trip is unchanged.

Adds CmdConfigBgpGlobalTest.cpp (the global command had no unit fixture) and
rejection/positive-commit cases to ConfigBgpGlobalTest.cpp.

Co-Authored-By: Claude Fable 5.1 <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>
Review feedback on meta#1391: the generator quoted free-form strings but emitted
numeric/boolean fields raw. The input JSON is not type-checked, so a string
in an ASN or timer field reached the generated script unquoted. Quote every
value uniformly; shlex.quote is a no-op for valid integers and booleans.
… flag unconverted j2c fields

Route limits: when neither the peer nor its group carries a RouteLimit, the
first warning-only/threshold edit seeded the thrift default (max_routes 12000,
a hard cap with session teardown) where bgpd enforced nothing. Seed
{max_routes 0, warning_only false, warning_limit 0} instead: bgpd reads 0 as
unlimited, so only the requested field changes.

Attributes: add remove-private-as, enforce-first-as, route-refresh and
ttl-security-hops (1..255, bgpd's kMin/kMaxTtlSecurityHops), the remaining
BgpPeer fields bgpd resolves peer > group. Document the fields deliberately
not exposed (enabled, router_port_id, graceful_restart_end_of_rib_seconds:
nothing in bgpd reads them).

bgp_json_to_cli.py: shared bool/int/link-bandwidth tables for the fields
BgpPeer and PeerGroup have in common; deprecated i32 ASNs map to the 4-byte
attribute; any JSON field with no CLI equivalent emits a `# WARNING` comment
line in the script and on stderr instead of being dropped silently.
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.
…UT gaps

delete protocol bgp peer-group now refuses to delete a group that a
neighbor's peer_group_name still references, naming the referencing
neighbors, instead of committing a dangling reference bgpd has no
defined behavior for.

New/extended unit tests close the coverage gaps found in review:
- timers out-delay and withdraw-unprog-delay (previously untested;
  withdraw-unprog-delay is a parity attribute tested for neighbor)
- valueValidation now asserts the half-created group is rolled back
  (state assertion, not just the error string)
- wrongValueCountRejected: zero/too-many value token counts
- overwriteSameAttributeLastWins: re-setting an attribute updates in
  place
- namedGroupsAreDistinct now asserts field preservation across
  mutations and sibling isolation
- delete: referenced-group rejected (all referencing neighbors named),
  unreferenced delete unaffected by other references, delete-then-
  recreate starts clean

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two pre-existing CI failures surfaced on every CLI-touching PR:

- ConfigBgp*Test: the SetUp gate sampled systemctl is-active once. On
  fboss-sim bgpd flaps activating->active->failed from boot, so one
  unlucky sample lands in the brief active window, runs the test, and
  the bgpd thrift RPC then times out after 90s (seen as
  SetGracefulRestartTime / SetCountConfedsInAsPathLenFalse failures;
  green runs are the ones where every sample landed on inactive and
  skipped). The gate now also requires bgpd to answer its
  getRunningConfig RPC within 10s, else the test skips like the rest
  of the flap cycle.

- ConfigVlanDefaultTest.ChangeDefaultVlanWithPortInNonDefaultVlan
  vacated only the test port before changing the default VLAN, but the
  change-default guard refuses while ANY port still uses the old
  default VLAN without an interface. On baselines generated before the
  default VLAN, so the guard always fired. Vacate every eth port from
  the old default VLAN first and restore them after.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The suite is ~60 files of ConfigSession-backed tests, each doing several
fsync-backed writeFileAtomic calls per case; with this PR's added
peer-group tests the full run takes ~530s on a dev box and timed out at
the CI job's default moderate test timeout (300s) — killed
mid-fsync in ConfigSessionTestFixture.commandTrackingMultipleCommands,
50/51 remaining tests green. size=large raises the test timeout to 900s
(same precedent as thrift_packet_stream/tests).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rammar

Signed-off-by: hillol-nexthop <hillol@nexthop.ai>
…gap, flag unconverted j2c fields

Route limits: a group with no RouteLimit imposes none on its members (bgpd
skips the check when the struct is absent), yet the first warning-only or
threshold edit created the struct with the thrift default max_routes 12000,
a hard cap with session teardown the operator never asked for. The struct is
now seeded {max_routes 0, warning_only false, warning_limit 0} (0 = unlimited
in bgpd) and only the requested field changes; a rejected value leaves no
struct behind.

Attributes: add every remaining PeerGroup field bgpd resolves peer > group:
remove-private-as, enforce-first-as, route-refresh, ttl-security-hops
(1..255), link-bandwidth, advertise-lbw, receive-lbw -- same spellings and
validation as the neighbor command. Document the fields deliberately not
exposed because bgpd never reads them from a group (local_addr, next_hop4/6,
enabled, router_port_id, graceful_restart_end_of_rib_seconds).

bgp_json_to_cli.py: the peer-group generator covers the new attributes via
the shared tables, maps the deprecated i32 ASNs to the 4-byte attribute, and
emits a `# WARNING` line (script + stderr) for any field with no CLI
equivalent instead of dropping it. test_complete_peer_group_loses_no_field
now asserts 36 commands.
…y as-path-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>
Deleting an as-path-list that a routing-policy term still names committed a
dangling as_path_list_names reference; bgpd fails the config load on it and
crash-loops. The whole-list delete now refuses and names every referencing
`policy <P> term <seq>`, the same rule the peer-group delete applies to
neighbors. findTermsReferencingAsPathList lives in BgpAsPathListCliUtils.h
because the boolean-operator setter uses the same walk.

The list gained the two fields bgpd actually matches on: `regex <regex>`
appends one boost::regex to as_paths (compiled here so a pattern bgpd would
reject at load never reaches the session; whitespace is refused since bgpd
renders the AS path `_`-joined), and `boolean-operator <AND|OR>` (NOT is not
offered: bgpd treats anything but OR as AND). The operator is written through
to every referencing term's inline copy, which bgpd requires to agree with the
list ("Conflicting boolean_operator"). `delete ... as-path-list <name> regex
<regex>` removes a single pattern and is allowed on a referenced list.

Adds boost_regex to the fboss2 config lib (BUCK: boost_regex; cmake:
Boost::regex, regex component) and the unit/integration coverage for each
path.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bgp_json_to_cli.py rebuilt global, peer-group and peer settings only; every
policy object in the input was dropped, so a replayed script could commit
peers naming policies that no longer exist. This adds
generate_as_path_list_commands (description, one `regex` line per as_paths
pattern, `boolean-operator` when not the OR default) and a
generate_policy_commands block that the nested families hook into.

`--raw` and the script wrapper now share one generate_commands sequence:
global, policy objects, then the peer-groups and peers that reference them,
so the emitted order never stages a dangling name. Fields the CLI cannot
express (as_path_list_names, the unread as_path_list entries, NOT) are
emitted as `# WARNING:` comment lines, which the script wrapper passes
through unprefixed instead of silently dropping the data.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…bcommand

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>
@hillol-nexthop
hillol-nexthop force-pushed the bgp-policy-aspathlist-entry branch from 2f7ba1b to 7f02eb3 Compare September 15, 2026 17:37
@facebook-github-tools

Copy link
Copy Markdown

@hillol-nexthop has updated the pull request. You must reimport the pull request before landing.

@hillol-nexthop
hillol-nexthop deleted the bgp-policy-aspathlist-entry branch September 16, 2026 10:42
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.

2 participants