Repository navigation
[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy as-path-list commands - #1401
hillol-nexthop wants to merge 5 commits into
Conversation
85ccd7f to
c5420a1
Compare
72cdd81 to
0bf546a
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
This pull request has been imported. If you are a Meta employee, you can view this in D114376328. (Because this pull request was imported automatically, there will not be any future comments.) |
0bf546a to
d88e6a7
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
40059ec to
2ad8bdc
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
2ad8bdc to
5f67dd7
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
5f67dd7 to
924c188
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
924c188 to
e071a4e
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
e071a4e to
2892023
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
2892023 to
0f5aef0
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
0f5aef0 to
95f1404
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
95f1404 to
3c14626
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
c3e47a7 to
523fed8
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
523fed8 to
3b24df8
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Summary: > **Stack (bottom → top):** **#1345 (this PR)** → #1391 → #1395 → #1401 → #1475 → #1477 → #1487 → #1488 → #1489 → #1490 → #1491 Add `fboss2-dev config protocol bgp global <attr> <value>` on top of the BGP-aware `ConfigSession`. It 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`). - Collapses the 10 per-attribute global command classes into one dispatcher; rejects `cluster-id` (no `BgpConfig` field) instead of writing dead config; bounds `switch-limit` / `max_golden_vips` so out-of-range values aren't truncated. - Integration tests: `ConfigBgpGlobalTest` (each attribute set+commit, verified in the promoted `/etc/coop/bgpcpp/bgpcpp.conf`) and `ConfigBgpSessionTest` (clear/diff/commit/rollback + an unchanged-config no-op-restart regression), sharing `ConfigBgpTestBase`. ## Sample usage ``` $ fboss2-dev config protocol bgp global router-id 10.0.0.1 Successfully set BGP router-id to: 10.0.0.1 Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp global local-asn 65000 Successfully set BGP local-asn to: 65000 Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp global hold-time 90 Successfully set BGP hold-time to: 90 seconds Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp global switch-limit-overload-protection-mode APPLY_GOLDEN_PREFIX_POLICY Successfully set switch_limit_config overload_protection_mode to: 2 Config saved to: ~/.fboss2/bgp_config.json # Rejected before anything is staged: $ fboss2-dev config protocol bgp global router-id not-an-ip Error: Invalid router-id address 'not-an-ip' $ fboss2-dev config protocol bgp global router-id 2001:db8::1 Error: router-id requires an IPv4 address, got '2001:db8::1' $ fboss2-dev config protocol bgp global switch-limit-overload-protection-mode 999 Error: switch-limit-overload-protection-mode value '999' is not a valid mode; expected one of: DROP_EXCESS_PREFIXES, APPLY_GOLDEN_PREFIX_POLICY ``` Pull Request resolved: #1345 Test Plan: **Unit** — `fboss2_cmd_config_test`: ``` $ fboss2_cmd_config_test --gtest_filter=CmdConfigBgpGlobal* [ OK ] CmdConfigBgpGlobalTestFixture.argValidation (101 ms) [ OK ] CmdConfigBgpGlobalTestFixture.routerIdAccepted (145 ms) [ OK ] CmdConfigBgpGlobalTestFixture.routerIdInvalidRejected (109 ms) [ OK ] CmdConfigBgpGlobalTestFixture.routerIdV6Rejected (90 ms) [ OK ] CmdConfigBgpGlobalTestFixture.routerIdArityRejected (97 ms) [ OK ] CmdConfigBgpGlobalTestFixture.rejectedRouterIdKeepsStagedValue (133 ms) [ OK ] CmdConfigBgpGlobalTestFixture.overloadProtectionModeIntegerAccepted (170 ms) [ OK ] CmdConfigBgpGlobalTestFixture.overloadProtectionModeNameAccepted (178 ms) [ OK ] CmdConfigBgpGlobalTestFixture.overloadProtectionModeInvalidRejected (71 ms) [ OK ] CmdConfigBgpGlobalTestFixture.rejectedOverloadProtectionModeKeepsStagedValue (100 ms) [==========] 10 tests from 1 test suite ran. (1199 ms total) [ PASSED ] 10 tests. ``` **Integration** — `fboss2_integration_test` on a lab device with `bgp_pp` active: ``` $ fboss2_integration_test --gtest_filter='ConfigBgpGlobalTest.*' [ OK ] ConfigBgpGlobalTest.SetCountConfedsInAsPathLenTrue (127 ms) [ OK ] ConfigBgpGlobalTest.SetCountConfedsInAsPathLenFalse (283 ms) [ OK ] ConfigBgpGlobalTest.SetGracefulRestartTime (2276 ms) [ OK ] ConfigBgpGlobalTest.SetRibAllocatedPathIdsTrue (2261 ms) [ OK ] ConfigBgpGlobalTest.SetRibAllocatedPathIdsFalse (2259 ms) [ OK ] ConfigBgpGlobalTest.InvalidBoolValueRejected (1052 ms) [ OK ] ConfigBgpGlobalTest.RouterIdInvalidRejected (46 ms) [ OK ] ConfigBgpGlobalTest.RouterIdV6Rejected (48 ms) [ OK ] ConfigBgpGlobalTest.OverloadProtectionModeOutOfRangeRejected (47 ms) [ OK ] ConfigBgpGlobalTest.SetOverloadProtectionModeAndCommit (106 ms) [ OK ] ConfigBgpGlobalTest.NegativeGracefulRestartTimeRejected (48 ms) [==========] 11 tests from 1 test suite ran. [ PASSED ] 11 tests. ``` Reviewed By: Anurag-Gumidelli Differential Revision: D113469141 Pulled By: hjkim417 fbshipit-source-id: 21f9cd0cea6eada213408e58a82392103af29ad8
3b24df8 to
330102e
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Summary: > **Stacked on #1345 (`config protocol bgp global`). This PR's branch contains the whole stack below it, so the diff shows those commits too — please review only the `config/delete protocol bgp neighbor` commit(s) here. Merge the lower PRs first. > > **Stack (bottom → top):** #1345 → **#1391 (this PR)** → #1395 → #1401 → #1475 → #1477 → #1487 → #1488 → #1489 → #1490 → #1491 Adds `fboss2-dev config protocol bgp neighbor <ip-address> [<attribute> <value> ...]` as a single dispatcher mirroring the `global` command: the neighbor address is the first positional token, one- or two-token attributes (`timers hold-time`, `add-path send`, ...) are matched longest-prefix-first, and handlers mutate the typed `bgp::thrift::BgpPeer` via `ConfigSession::getBgpConfig()` / `saveBgpConfig()`. Also adds `fboss2-dev delete protocol bgp neighbor <ip-address>`. The 26 old per-attribute `peer` command classes and the now-dead per-peer `folly::dynamic BgpConfigSession` API are deleted. | Attribute | `BgpPeer` field | |---|---| | `remote-asn` / `local-asn` | `remote_as_4_byte` / `local_as_4_byte` (4-byte-bounded) | | `description`, `peer-tag`, `peer-group`, `peer-id`, `type` | `description`, `peer_tag`, `peer_group_name`, `peer_id`, `type` | | `ingress-policy` / `egress-policy` | `ingress_policy_name` / `egress_policy_name` | | `rr-client`, `confed-peer`, `redistribute-peer`, `enhanced-route-refresh`, `route-refresh`, `remove-private-as`, `enforce-first-as`, `next-hop-self` | matching bools | | `passive <true\|false>` | `is_passive` (true = PASSIVE_ONLY, false = PASSIVE_ACTIVE in bgpd) | | `ttl-security-hops <1-255>` | `ttl_security_hops` (bgpd's `kMin/kMaxTtlSecurityHops`; it throws at load outside that range) | | `afi disable-ipv4-afi` / `disable-ipv6-afi` / `ipv4-over-ipv6-nh` | `disable_ipv4_afi` / `disable_ipv6_afi` / `v4_over_v6_nexthop` | | `add-path send\|receive <bool>` | merged into the `AddPath` enum (RECEIVE/SEND/BOTH) | | `bind-addr address`, `next-hop4`, `next-hop6` | `local_addr`, `next_hop4`, `next_hop6` (IP/family-validated) | | `graceful-restart restart-time` / `stateful-ha` | `bgp_peer_timers.graceful_restart_seconds` / `enable_stateful_ha` | | `timers hold-time` / `keepalive` / `out-delay` / `withdraw-unprog-delay` | `bgp_peer_timers.*` | | `max-route pre-filter` / `post-filter` / `{pre,post}-warning-threshold` / `{pre,post}-warning-only` | `pre_filter` / `post_filter` RouteLimit fields | | `link-bandwidth`, `advertise-lbw`, `receive-lbw` | `link_bandwidth_bps`, `advertise_link_bandwidth`, `receive_link_bandwidth` (enum-membership-validated) | That is every `BgpPeer` field bgpd's `Config.cpp` reads. Deliberately not exposed (documented in the dispatcher): `enabled`, `router_port_id` and `bgp_peer_timers.graceful_restart_end_of_rib_seconds` — nothing in bgpd reads them, so staging them would persist dead config. 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 and host bits are masked (`10.0.0.1/24` → `10.0.0.0/24`), matching how bgpd keys dynamic peers, so equivalent spellings update/delete the same neighbor. `delete protocol bgp neighbor <ip>` removes the matching peer via the same shared normalize/find helpers (`BgpPeerAddr.h`); deleting an unknown neighbor errors without persisting anything. ### Behavior notes - **Inherited values are preserved.** bgpd resolves `bgp_peer_timers`, `pre_filter` and `post_filter` per *struct* against the peer group, and `hold_time_seconds` / `keep_alive_seconds` are non-optional. Before the first attribute of such a struct is set on a peer, the struct is seeded from what the peer inherits today — the group's copy, else the global hold time with hold/3 keepalive — and only the requested field changes. For route limits with nothing to inherit, the seed is `{max_routes 0, warning_only false, warning_limit 0}`: bgpd applies no limit to a peer without a struct and reads `max_routes == 0` as unlimited, whereas the thrift default (12000) would have introduced a hard cap with session teardown that the operator never asked for. - **Handlers edit a copy** of the peer and commit on success, so a rejected value never leaves a half-edited peer or a seeded struct behind. - **add-path** merges into the *effective* value (the peer's, else the group's). Disabling a direction the group still enables is rejected with a message naming the group: `AddPath` has no disabled member and an unset peer field inherits the group, so it would otherwise be a no-op reported as success. Narrowing a group's `BOTH` to one direction is allowed. - **Validation matches bgpd**: hold time `0` or `3..65535`, graceful-restart `0..4095`, TTL security hops `1..255`; `link-bandwidth` accepts exactly what bgpd's `parseLinkBandwidthBps` accepts (`auto`, decimals, case-insensitive K/M/G). New peers are seeded with parseable any-addresses (`local_addr`/`next_hop4`/`next_hop6` = `0.0.0.0`/`::`) because bgpd constructs `folly::IPAddress` from these non-optional fields at load and aborts on empty strings. - **Rejected instead of persisting dead config**: `peer-port` / `bind-addr port` (no per-peer port field; the daemon uses the global `listen_port`), `afi ipv4/ipv6-labeled-unicast` (no thrift field). - **`--help`** lists every attribute with its value shape, generated from the dispatch table (`bgpNeighborAttributeHelp()` is the subcommand's CLI11 footer); tests assert every registered attribute appears and parses. Tab completion cannot see free-form attributes; registering them as CLI11 subcommands is a larger change left for a follow-up. - **CLI11 token stealing fixed**: `positionals_at_end()` on the neighbor command stops parent-chain subcommand fallthrough from reclassifying attribute tokens that match sibling command names (`neighbor <ip> peer-group X` misparsed with `X` as the neighbor address). - **`bgp_json_to_cli.py`** emits the neighbor grammar with every JSON-sourced value passed through `shlex.quote`; explicit zero timers/limits are kept; symbolic `add_path` enum names are handled; the deprecated i32 `remote_as`/`local_as` map to the 4-byte attribute; any JSON field with no CLI equivalent produces a `# WARNING: neighbor <addr>: field <f> has no CLI equivalent, not converted` comment line in the generated script (and on stderr) instead of being dropped silently. The shared bool/int/link-bandwidth/timer/route-limit tables are reused by the peer-group generator in #1395. - Integration tests verify the committed neighbor config through the daemon's `TBgpService::getRunningConfig` RPC, proving bgpd parsed and adopted the promoted config rather than only checking the file the CLI wrote. **Delete is idempotent.** Deleting an absent target is a success with a `Warning:` (nothing is saved), so a replayed script never fails on something already gone; the reference guards still return `Error:`. ## Sample usage ``` $ fboss2-dev config protocol bgp neighbor 2001:db8::99 remote-asn 65510 Successfully set remote-asn to: 65510 for neighbor 2001:db8::99 $ fboss2-dev config protocol bgp neighbor 2001:db8::99 timers hold-time 90 Successfully set timers hold-time to: 90 seconds for neighbor 2001:db8::99 $ fboss2-dev config protocol bgp neighbor 2001:db8::99 passive true Successfully enabled passive for neighbor 2001:db8::99 $ fboss2-dev config protocol bgp neighbor 2001:db8::99 ttl-security-hops 300 Error: Invalid ttl-security-hops value '300'; expected 1-255 $ fboss2-dev config session commit Config session committed successfully as d99c4cd and bgpd (restart) restarted. $ fboss2-dev delete protocol bgp neighbor 2001:db8::99 Successfully deleted BGP neighbor 2001:db8::99 ``` Idempotent delete, recorded on a lab device: ``` $ fboss2-dev delete protocol bgp neighbor 192.0.2.99 Warning: BGP neighbor 192.0.2.99 does not exist; nothing to delete ``` Pull Request resolved: #1391 Test Plan: **Unit (delete)** — `fboss2_cmd_config_test`: ``` $ fboss2_cmd_config_test --gtest_filter=*DeleteBgpNeighbor* [ OK ] CmdDeleteBgpNeighborTestFixture.argValidation (52 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteExistingNeighbor (165 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteByAnySpelling (123 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteUnknownNeighborWarns (68 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteTwiceIsIdempotent (588 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteUnknownLeavesOthersIntact (119 ms) [==========] 6 tests from 1 test suites ran. [ PASSED ] 6 tests. ``` **Unit** — `fboss2_cmd_config_test`: ``` $ fboss2_cmd_config_test --gtest_filter=*BgpNeighbor* [ OK ] CmdConfigBgpNeighborTestFixture.argValidation (95 ms) [ OK ] CmdConfigBgpNeighborTestFixture.bareCreate (110 ms) [ OK ] CmdConfigBgpNeighborTestFixture.setRemoteAsn (116 ms) [ OK ] CmdConfigBgpNeighborTestFixture.timersAccumulate (151 ms) [ OK ] CmdConfigBgpNeighborTestFixture.addPathMerge (230 ms) [ OK ] CmdConfigBgpNeighborTestFixture.passive (185 ms) [ OK ] CmdConfigBgpNeighborTestFixture.boolAndStringAttributes (1007 ms) [ OK ] CmdConfigBgpNeighborTestFixture.parityAttributes (873 ms) [ OK ] CmdConfigBgpNeighborTestFixture.valueValidation (133 ms) [ OK ] CmdConfigBgpNeighborTestFixture.routeLimits (140 ms) [ OK ] CmdConfigBgpNeighborTestFixture.normalizedAddressesShareOnePeer (114 ms) [ OK ] CmdConfigBgpNeighborTestFixture.prefixSpellingsShareOnePeer (90 ms) [ OK ] CmdConfigBgpNeighborTestFixture.timersSeededFromPeerGroup (111 ms) [ OK ] CmdConfigBgpNeighborTestFixture.timersSeededFromGlobalWithoutGroup (75 ms) [ OK ] CmdConfigBgpNeighborTestFixture.timersNotReseededOncePresent (140 ms) [ OK ] CmdConfigBgpNeighborTestFixture.routeLimitSeededFromPeerGroup (88 ms) [ OK ] CmdConfigBgpNeighborTestFixture.routeLimitSeededUnlimitedWithoutGroup (127 ms) [ OK ] CmdConfigBgpNeighborTestFixture.routeLimitNotReseededOncePresent (126 ms) [ OK ] CmdConfigBgpNeighborTestFixture.ttlSecurityHopsRange (171 ms) [ OK ] CmdConfigBgpNeighborTestFixture.addPathMergesWithPeerGroupValue (149 ms) [ OK ] CmdConfigBgpNeighborTestFixture.addPathCannotDisableGroupDirection (116 ms) [ OK ] CmdConfigBgpNeighborTestFixture.addPathNarrowsGroupBothToOneDirection (119 ms) [ OK ] CmdConfigBgpNeighborTestFixture.timerRanges (164 ms) [ OK ] CmdConfigBgpNeighborTestFixture.rejectedValueLeavesPeerUntouched (162 ms) [ OK ] CmdConfigBgpNeighborTestFixture.distinctAddressesCreateDistinctPeers (109 ms) [ OK ] CmdConfigBgpNeighborTestFixture.attributeHelpListsEveryAttribute (50 ms) [ OK ] CmdConfigBgpNeighborTestFixture.cliHelpFooterCarriesAttributeList (53 ms) [ OK ] CmdConfigBgpNeighborTestFixture.invalidValuesRejected (66 ms) [ OK ] CmdConfigBgpNeighborTestFixture.unsupportedAttributesRejected (70 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.argValidation (53 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteExistingNeighbor (106 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteByAnySpelling (160 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteByEquivalentPrefixSpelling (115 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteUnknownNeighborRejected (57 ms) [ OK ] CmdDeleteBgpNeighborTestFixture.deleteUnknownLeavesOthersIntact (180 ms) [==========] 35 tests from 2 test suites ran. (5828 ms total) [ PASSED ] 35 tests. ``` **Integration** — `fboss2_integration_test` on a lab device with `bgp_pp` active: ``` $ fboss2_integration_test --gtest_filter='ConfigBgpNeighborTest.*' [ OK ] ConfigBgpNeighborTest.SetRemoteAsnAndCommit (13722 ms) [ OK ] ConfigBgpNeighborTest.SetTimersHoldTimeAndCommit (12148 ms) [ OK ] ConfigBgpNeighborTest.GracefulRestartKeepsInheritedHoldTime (11413 ms) [ OK ] ConfigBgpNeighborTest.SessionKnobsAndCommit (11168 ms) [ OK ] ConfigBgpNeighborTest.RouteLimitWarningKeepsUnlimitedCap (12787 ms) [ OK ] ConfigBgpNeighborTest.DeleteNeighborAndCommit (20256 ms) [ PASSED ] 6 tests. ``` Based-on: #1345 Reviewed By: jaiharil Differential Revision: D114376255 Pulled By: hjkim417 fbshipit-source-id: 3fe82432503ececae3ff911da5e07cf00ee2f7e7
6dce114 to
f540a90
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
b8edbce to
bfd45a2
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Summary: > **Stacked on #1391 (`config/delete protocol bgp neighbor`). This PR's branch contains the whole stack below it, so the diff shows those commits too — please review only the `config/delete protocol bgp peer-group` commit(s) here. Merge the lower PRs first. > > **Stack (bottom → top):** #1345 → #1391 → **#1395 (this PR)** → #1401 → #1475 → #1477 → #1487 → #1488 → #1489 → #1490 → #1491 Collapses the ~20 legacy per-attribute `config protocol bgp peer-group` command classes (built on the deprecated `folly::dynamic` `BgpConfigSession`) into a single typed dispatcher, mirroring `CmdConfigProtocolBgpNeighbor` (#1391). 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` directly via `ConfigSession::getBgpConfig()` / `saveBgpConfig()`, so adding a tunable is a one-entry change in the dispatch table rather than a new command class. Also adds `delete protocol bgp peer-group <name>`. Deleting a group that a neighbor's `peer_group_name` still references is refused (the error names the referencing neighbors), so a commit can never carry a dangling group reference. Covers the 36 dispatch keys that map to a `PeerGroup` field bgpd reads from a group: `remote-asn`/`local-asn` (4-byte-bounded), `description`, `peer-tag`, `ingress-policy`/`egress-policy`, `rr-client`, `confed-peer`, `redistribute-peer`, `enhanced-route-refresh`, `route-refresh`, `remove-private-as`, `enforce-first-as`, `passive`, `ttl-security-hops`, `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` (+ the warning-threshold/warning-only knobs), `timers hold-time|keepalive|out-delay|withdraw-unprog-delay`, `next-hop-self`, `link-bandwidth`, `advertise-lbw`, `receive-lbw`. Spellings and validation are identical to the neighbor command, so the two share one attribute grammar. Deliberately not exposed (documented in the dispatcher): `local_addr`, `next_hop4`, `next_hop6` (bgpd reads them per peer only, with no group fallback), `enabled`, `router_port_id` and `bgp_peer_timers.graceful_restart_end_of_rib_seconds` (read nowhere in bgpd). ### Behavior notes - **`passive true|false`** exposes `is_passive` directly: bgpd reads true as PASSIVE_ONLY and false as PASSIVE_ACTIVE (listen and connect); the config model has no way to request active-only. `add-path send|receive` merges into the `AddPath` enum bitmask (RECEIVE/SEND/BOTH), and clearing the last direction unsets the field. - **Inherited timers are preserved.** bgpd applies a group's `bgp_peer_timers` to its members as a whole struct, and hold/keepalive are non-optional i32 (default 0 = no keepalives). The first timer attribute set on a group seeds the struct from the global hold time (keepalive = hold/3, bgpd's own fallback) and then changes the one field. - **Route limits are seeded unlimited, not 12000.** A group with no `pre_filter`/`post_filter` imposes no limit on its members (`AdjRib::capRoutesPerPeer` returns early on an absent struct), and bgpd reads `max_routes == 0` as unlimited. So the first `max-route ...-warning-threshold` / `...-warning-only` edit seeds `{max_routes 0, warning_only false, warning_limit 0}` and changes only the requested field, rather than materialising the thrift default `max_routes = 12000` — a hard cap with session teardown the operator never asked for. - **Handlers edit a copy** of the group, so a rejected value leaves nothing seeded behind (and a rejected first attribute leaves no half-created group). - **Validation matches bgpd**: hold time 0 or 3..65535 (bgpd refuses 1-2, the OPEN field is 16 bits), graceful-restart 0..4095 (RFC 4724), TTL security hops 1..255 (`kMin/kMaxTtlSecurityHops`; bgpd throws at load otherwise), `link-bandwidth` accepts what `parseLinkBandwidthBps` accepts (`auto`, decimals, case-insensitive K/M/G). - **`bgp_json_to_cli.py`** generates the peer-group grammar above from the per-shape tables shared with the neighbor generator (only the scalar field list differs), so a fully populated `PeerGroup` converts with nothing dropped: `test_complete_peer_group_loses_no_field` asserts one command per leaf field (36). The deprecated i32 `remote_as`/`local_as` map to the 4-byte attribute. Fields bgpd never reads from a group are reported as `# WARNING: peer-group <name>: field <f> has no CLI equivalent, not converted` comment lines in the generated script (and on stderr) instead of being dropped silently. - **Attributes with no per-peer-group thrift field** — `afi ipv4-labeled-unicast`, `afi ipv6-labeled-unicast`, `peer-port` — are not part of the dispatch table, so they are refused at parse time as unknown attributes; a rejected value never lands on disk. - **CLI11 token stealing fixed** (same fix as the neighbor command): `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. - Integration tests verify the committed peer-group config through the daemon's `getRunningConfig` RPC — proving `bgpd` parsed and adopted the promoted `peer_groups` rather than only checking the file the CLI wrote. **Delete is idempotent.** Deleting an absent target is a success with a `Warning:` (nothing is saved), so a replayed script never fails on something already gone; the reference guards still return `Error:`. ## Sample usage ``` $ fboss2-dev config protocol bgp peer-group SPINE remote-asn 65510 Successfully set remote-asn to: 65510 for peer-group SPINE $ fboss2-dev config protocol bgp peer-group SPINE timers hold-time 90 Successfully set timers hold-time to: 90 seconds for peer-group SPINE $ fboss2-dev config protocol bgp peer-group SPINE passive true Successfully enabled passive for peer-group SPINE $ fboss2-dev config protocol bgp peer-group SPINE max-route pre-warning-threshold 30000 Successfully set max-route pre-warning-threshold to: 30000 for peer-group SPINE $ fboss2-dev config protocol bgp peer-group SPINE timers hold-time 2 Error: timers hold-time must be 0 or between 3 and 65535 seconds, got 2 $ fboss2-dev delete protocol bgp peer-group SPINE Error: BGP peer-group SPINE is still referenced by neighbor(s): 192.0.2.1; delete or re-point those neighbors first ``` Idempotent delete, recorded on a lab device: ``` $ fboss2-dev delete protocol bgp peer-group NO-SUCH-GROUP Warning: BGP peer-group NO-SUCH-GROUP does not exist; nothing to delete ``` Pull Request resolved: #1395 Test Plan: **Unit (delete)** — `fboss2_cmd_config_test`: ``` $ fboss2_cmd_config_test --gtest_filter=*DeleteBgpPeerGroup* [ OK ] CmdDeleteBgpPeerGroupTestFixture.argValidation (91 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteExistingGroup (195 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteUnknownGroupWarns (84 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteTwiceIsIdempotent (149 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteUnknownLeavesOthersIntact (102 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteReferencedGroupRejected (92 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteUnreferencedGroupSucceeds (124 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteThenRecreateStartsClean (174 ms) [==========] 8 tests from 1 test suites ran. [ PASSED ] 8 tests. ``` **Unit** — `fboss2_cmd_config_test`: ``` $ fboss2_cmd_config_test --gtest_filter=*BgpPeerGroup* [ OK ] CmdConfigBgpPeerGroupTestFixture.argValidation (94 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.bareCreate (106 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.setRemoteAsn (79 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.timersAccumulate (187 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.addPathMerge (152 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.passive (97 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.boolAndStringAttributes (765 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.routeLimits (369 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.linkBandwidthAttributes (211 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.ttlSecurityHopsRange (108 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.valueValidation (82 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.wrongValueCountRejected (76 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.overwriteSameAttributeLastWins (137 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.namedGroupsAreDistinct (128 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.timersSeededFromGlobal (102 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.timersNotReseededOncePresent (161 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.routeLimitSeededUnlimited (122 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.routeLimitNotReseededOncePresent (115 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.timerRanges (193 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.rejectedValueLeavesGroupUntouched (114 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.invalidValuesRejected (66 ms) [ OK ] CmdConfigBgpPeerGroupTestFixture.unsupportedAttributesRejected (65 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.argValidation (69 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteExistingGroup (118 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteUnknownGroupRejected (59 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteUnknownLeavesOthersIntact (95 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteReferencedGroupRejected (103 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteUnreferencedGroupSucceeds (148 ms) [ OK ] CmdDeleteBgpPeerGroupTestFixture.deleteThenRecreateStartsClean (128 ms) [==========] 29 tests from 2 test suites ran. (4261 ms total) [ PASSED ] 29 tests. ``` **Integration** — `fboss2_integration_test` on a lab device with `bgp_pp` active: ``` $ fboss2_integration_test --gtest_filter='ConfigBgpPeerGroupTest.*' [ OK ] ConfigBgpPeerGroupTest.SetRemoteAsnAndCommit (11580 ms) [ OK ] ConfigBgpPeerGroupTest.SetTimersHoldTimeAndCommit (14385 ms) [ OK ] ConfigBgpPeerGroupTest.SetPassiveAndCommit (11728 ms) [ OK ] ConfigBgpPeerGroupTest.SessionKnobsAndCommit (12531 ms) [ OK ] ConfigBgpPeerGroupTest.RouteLimitWarningKeepsUnlimitedCap (11697 ms) [ OK ] ConfigBgpPeerGroupTest.DeleteGroupAndCommit (19856 ms) [ PASSED ] 6 tests. ``` Based-on: #1391 Reviewed By: induvsuresh Differential Revision: D114376290 Pulled By: hjkim417 fbshipit-source-id: d643ae0ecd569b12abfbb42d79a08151282349ad
…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>
Delete mirrors add: deleting an absent list or an absent regex is a success with a `Warning:` and saves nothing, so a replayed script stays idempotent. The term-reference guard still errors. bgpd matches on the flat `as_paths` string list and never reads the structured `as_path_list[]` entries, so the helpers that keyed entries by sequence number are dead; drop them and describe the flat grammar in the CLI doc. `positionals_at_end()` is restored on the leaf command now that it has no subcommand. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bfd45a2 to
c91affe
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
|
@induvsuresh merged this pull request in 558b6b2. |
Summary: > **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 `config/delete protocol bgp policy community-list` commit(s) here. Merge the lower PRs first. > > **Stack (bottom → top):** #1345 → #1391 → #1395 → #1401 → **#1475 (this PR)** → #1477 → #1487 → #1488 → #1489 → #1490 → #1491 Adds the second BGP **policy** object-type command family to `fboss2-dev`: - `config protocol bgp policy community-list <name> [<attribute> <value>]` - `delete protocol bgp policy community-list <name> [community <community>]` Community values are **flat** `community <community>` attributes, one per invocation. bgpd matches on the flat `communities` string list (a literal `<asn>:<value>` or a regex, compiled at load) and never reads the structured `members[]`, so a nested member level would write a field the daemon ignores. The same rule already shapes as-path-list (#1401): flat repetition where the daemon stores a flat string list, nested entries only where it stores objects (prefix-list, routing-policy). ### Commands | Command | Thrift target | |---|---| | `community-list <name>` | `community_lists[]` (create/select) | | `community-list <name> description <string>` | `CommunityList.description` | | `community-list <name> community <community>` | appends to `CommunityList.communities` (idempotent) | | `community-list <name> boolean-operator <AND\|OR>` | `CommunityList.boolean_operator` | | `community-list <name> exact-match <bool>` | `CommunityList.exact_match` | | `delete ... community-list <name>` | removes the `community_lists[]` entry | | `delete ... community-list <name> community <community>` | removes one value (list survives) | ### Design - Same dispatcher shape as as-path-list: value-shape factories from the shared `BgpCliAttrHandlers.h` (#1391), named setters, one registry line per attribute. Writes `bgp_policy.BgpPolicies.community_lists[]` through the typed `ConfigSession`. - `community <community>` is a hand-written handler like as-path-list's `regex`: one token, no whitespace, compiled with `boost::regex` so a value bgpd would reject at load never reaches the session; append-if-absent, `already present` on repeat. - `boolean-operator` offers only `AND|OR`: bgpd treats every operator other than OR as "all must match", so `NOT` would silently behave as AND. - Delete is idempotent: an absent list or value is a success with a `Warning:` and saves nothing. Deleting the last value resets `communities` so a later add starts from the same shape. - A rejected value never leaves a phantom list behind: the dispatcher rolls back a list it implicitly created in the same invocation. - `bgp_json_to_cli.py` emits one `community` line per value and warns on `NOT`, `members` and `community_list_names` instead of dropping them. ## Sample usage Recorded on a lab device. ``` $ fboss2-dev config protocol bgp policy community-list PROD Successfully created BGP community-list PROD Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp policy community-list PROD community 65000:100 Successfully added community 65000:100 for community-list PROD Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp policy community-list PROD community 65000:100 community 65000:100 already present for community-list PROD Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp policy community-list PROD community ^65000:.*$ Successfully added community ^65000:.*$ for community-list PROD Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp policy community-list PROD community 65000:100 65000:200 Error: community '65000:100 65000:200' must not contain whitespace; repeat the command once per community $ fboss2-dev config protocol bgp policy community-list PROD community 65000:( Error: Malformed community '65000:(': Unmatched marking parenthesis ( or \(. The error occurred while parsing the regular expression: '65000:(>>>HERE>>>'. $ fboss2-dev config protocol bgp policy community-list PROD exact-match true Successfully enabled exact-match for community-list PROD Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp policy community-list PROD boolean-operator AND Successfully set boolean-operator to: AND for community-list PROD Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev config protocol bgp policy community-list PROD boolean-operator NOT Error: Invalid boolean-operator value 'NOT'; expected AND|OR $ fboss2-dev delete protocol bgp policy community-list PROD community 65000:100 Successfully deleted BGP community-list PROD community 65000:100 Config saved to: ~/.fboss2/bgp_config.json $ fboss2-dev delete protocol bgp policy community-list PROD community 65000:100 Warning: BGP community-list PROD has no community 65000:100; nothing to delete $ fboss2-dev delete protocol bgp policy community-list NO-SUCH Warning: BGP community-list NO-SUCH does not exist; nothing to delete ``` After `config session commit`, bgpd's running config carries the value where it is matched: ``` "community_lists": [ { "boolean_operator": 2, "communities": [ "65000:100" ], "description": "", "name": "FBOSS2-TX-CL" } ], ``` Pull Request resolved: #1475 Test Plan: **Unit** — `fboss2_cmd_config_test`: ``` $ fboss2_cmd_config_test --gtest_filter=*BgpPolicyCommunityList* [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.argValidation (62 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.bareCreateList (204 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.setListDescription (105 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.setBooleanOperator (121 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.booleanOperatorNotRejected (66 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.booleanOperatorDefaultIsOr (160 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.setExactMatch (178 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.namedListsAreDistinct (249 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.reReferenceReportsExisting (194 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.invalidBooleanOperatorRejected (94 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.invalidExactMatchRejected (82 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.communityAppends (284 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.communityAcceptsRegex (149 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.communityAddAgainIsIdempotent (187 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.malformedCommunityRejected (103 ms) [ OK ] CmdConfigBgpPolicyCommunityListTestFixture.communityWithWhitespaceRejected (158 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.argValidation (74 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteExistingList (157 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteUnknownListWarns (69 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteTwiceIsIdempotent (94 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteUnknownLeavesOthersIntact (85 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteOneCommunity (226 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteUnknownCommunityWarns (118 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteCommunityTwiceIsIdempotent (115 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteCommunityFromValuelessListWarns (108 ms) [ OK ] CmdDeleteBgpPolicyCommunityListTestFixture.deleteCommunityFromUnknownListWarns (58 ms) [==========] 26 tests from 2 test suites ran. [ PASSED ] 26 tests. ``` **j2c** — `python3 -m unittest fboss.cli.fboss2.tools.tests.test_bgp_json_to_cli`: OK. **Integration** — `fboss2_integration_test` on a lab device with `bgp_pp` active: ``` $ fboss2_integration_test --gtest_filter='ConfigBgpPolicyCommunityListTest.*' [ OK ] ConfigBgpPolicyCommunityListTest.SetListAttributesAndCommit (311 ms) [ OK ] ConfigBgpPolicyCommunityListTest.SetCommunityAndOperatorAndCommit (2329 ms) [ OK ] ConfigBgpPolicyCommunityListTest.MalformedCommunityRejected (1052 ms) [ OK ] ConfigBgpPolicyCommunityListTest.DeleteListAndCommit (2372 ms) [==========] 4 tests from 1 test suite ran. [ PASSED ] 4 tests. ``` Based-on: #1401 Reviewed By: srikrishnagopu Differential Revision: D115034419 Pulled By: induvsuresh fbshipit-source-id: 2d7171534bbc37147494c020481dbaba3c72caa2
Summary
Adds the first BGP policy object-type command family to
fboss2-dev, under a newpolicygrouping node beneathprotocol bgp(sibling toglobal/neighbor/peer-group):config protocol bgp policy as-path-list <name> [description <string>]delete protocol bgp policy as-path-list <name>Patterns are flat
regex <regex>attributes (one per invocation): bgpd matches on the flatas_pathsstring list and never reads the structuredas_path_list[]entries, so the list has no nested entry level.Commands
as-path-list <name>aspath_lists[](create/select)as-path-list <name> description <string>AsPathList.descriptiondelete ... as-path-list <name>aspath_lists[]entryDesign
BgpCliAttrHandlers.h(added in [Nexthop][fboss2-dev] fboss2 config/delete protocol bgp neighbor commands #1391), named setters that do nothing but assign the thrift field, and a registry that is one line per attribute. Writesbgp_policy.BgpPolicies.aspath_lists[]through the typedConfigSession— no new session plumbing,BgpConfig.policiesalready exists.BgpAsPathListCliUtils.h, shared with the delete command.Sample usage
Recorded on a lab device.
Reference guard:
Previous hand-written sample
Test plan
Unit —
fboss2_cmd_config_test:Integration —
fboss2_integration_teston a lab device withbgp_ppactive:Based-on: #1395