[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy prefix-list commands - #1477
[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy prefix-list commands#1477hillol-nexthop wants to merge 13 commits into
Conversation
|
@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 D115057124. (Because this pull request was imported automatically, there will not be any future comments.) |
b8b80b3 to
7681ea9
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
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).
7681ea9 to
851312f
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
851312f to
2525baf
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
2525baf to
633de9f
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
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).
633de9f to
8f81d30
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Adds `config protocol bgp neighbor <ip-address> [<attribute> <value> ...]` and `delete protocol bgp neighbor <ip-address>` — the first per-peer BGP config family, beneath the `protocol bgp` grouping node. - neighbor dispatcher keyed by peer address, writing bgp_config.BgpPeer through the typed ConfigSession. Attributes cover the peer identity (remote/local ASN, description, peer-tag, peer-group), policy bindings (ingress/egress policy), session tunables (timers, route limits, next-hop, add-path, RR client, and the rest of the documented set), and the sheet-documented attributes bgpd does not support, which are rejected with an explanatory message rather than staged. - delete removes the peer by address. - ConfigBgpTestBase restores the committed BGP config after every test, so a failed run cannot leave a device carrying test config; commit-path integration tests verify each value through bgpd's own getRunningConfig RPC rather than only the staged file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ands Collapses the 18 per-attribute `config protocol bgp peer-group` command classes (built on the deprecated `folly::dynamic` BgpConfigSession) into a single typed dispatcher, mirroring CmdConfigProtocolBgpNeighbor. Also adds `delete protocol bgp peer-group <name>`. The group name is the first positional token; the next one or two tokens name the attribute, matched longest-prefix-first so `timers hold-time` wins over any `timers` prefix; the rest are its value(s). Handlers mutate the typed bgp::thrift::PeerGroup through ConfigSession::getBgpConfig() / saveBgpConfig(), so adding a tunable is a one-entry change in the dispatch table rather than a new command class. 36 command files are deleted. - Covers the 29 dispatch keys that map to a PeerGroup thrift field: remote-asn / local-asn (4-byte-bounded), description, peer-tag, ingress-policy / egress-policy, rr-client, confed-peer, redistribute-peer, enhanced-route-refresh, connect-mode, add-path send|receive, afi disable-ipv4-afi|disable-ipv6-afi|ipv4-over-ipv6-nh, graceful-restart restart-time|stateful-ha, max-route pre-filter|post-filter (plus the warning-threshold / warning-only knobs), timers hold-time|keepalive|out-delay|withdraw-unprog-delay, and next-hop-self. - Rejected rather than persisted as dead config (precedent: cluster-id in the global command, connect-mode BOTH in the neighbor command): connect-mode BOTH, since thrift only models is_passive. add-path send|receive merges into the AddPath enum bitmask, and clearing the last direction unsets the field. - Attributes with no per-peer-group thrift field (afi ipv4-labeled-unicast, afi ipv6-labeled-unicast, peer-port) are absent from the dispatch table, so they are refused at parse time as unknown attributes; a rejected value never lands on disk. - positionals_at_end() stops parent-chain subcommand fallthrough from reclassifying an attribute token that matches a sibling command name once the group name has been consumed (same fix as the neighbor command). - unit tests (16): CmdConfigBgpPeerGroupTest (12) covering arg validation, longest-prefix match, the add-path merge matrix, connect-mode, the bool/string/route-limit attributes, value validation and unknown-attribute rejection, plus CmdDeleteBgpPeerGroupTest (4). - integration tests (3): ConfigBgpPeerGroupTest asserts the committed peer_groups through bgpd's getRunningConfig RPC, proving the daemon parsed and adopted the promoted config rather than only checking the file the CLI wrote. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…list Adds the first BGP policy object-type command family, under a new `policy` grouping node beneath `protocol bgp` (sibling to global/neighbor/peer-group): config protocol bgp policy as-path-list <name> [description <string>] delete protocol bgp policy as-path-list <name> The list's `entry <seq-num>` level is a CLI11 subcommand of its own and lands in the next commit, so this one owns only the list level. - Follows the dispatcher shape the neighbor/peer-group families use: one factory per value shape from the shared BgpCliAttrHandlers.h, named setters that do nothing but assign the thrift field, and a registry that is one line per attribute. Writes bgp_policy.BgpPolicies.aspath_lists[] through the typed ConfigSession — no new session plumbing, since BgpConfig.policies already exists. - Lookup/create helpers live in BgpAsPathListCliUtils.h, including the entry-level helpers, so the delete command and the entry subcommand in the next commit share them rather than re-deriving the scan. - A rejected value leaves nothing staged: a list implicitly created for the failed command is rolled back before returning. - There is no per-entry delete; `delete ... as-path-list <name>` removes the whole list. - unit tests (10): CmdConfigBgpPolicyAsPathListTest (6) covering arg validation, bare create, the description round-trip, named lists staying distinct, re-reference reporting the existing list and unknown-attribute rejection, plus CmdDeleteBgpPolicyAsPathListTest (4). - integration tests (2): ConfigBgpPolicyAsPathListTest stages and commits, then asserts against bgpd's running config via the getRunningConfig RPC, confirming bgpd accepts and adopts the .policies blob end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Splits the `entry` level of `config protocol bgp policy as-path-list` into
its own CLI11 subcommand. The previous commit ships the list level; this one
ships everything keyed by `entry <seq-num>`:
as-path-list <name> entry <seq-num>
-> AsPathList.as_path_list[], keyed by sequence_number
as-path-list <name> entry <seq-num> asn-regexp <regex>
-> AsPathListEntry.as_path.as_path.asn_regexp
as-path-list <name> entry <seq-num> description <string>
-> AsPathListEntry.description
as-path-list <name> entry <seq-num> match-logic <EQUAL|NOT_EQUAL>
-> AsPathListEntry.match_logic_type
- `entry` is a real CLI11 subcommand rather than tokens parsed inside the
parent's arg type: the list name arrives through the ancestor-args tuple,
and lookup/create is shared with the parent through
BgpAsPathListCliUtils.h. Adding an entry attribute is a one-line registry
change, same as at the list level.
- A list attribute typed alongside `entry` is rejected rather than silently
dropped, because only the leaf command runs.
- asn-regexp sets the AsPathType union's inline AsPath arm. The pattern may
contain spaces, since AS-path regexes separate ASNs with spaces
(e.g. `^65000 65001$`), so it takes the joined-string value shape rather
than requiring a single token.
- A rejected value leaves nothing staged: a list or entry implicitly created
for the failed command is rolled back.
- unit tests (10): CmdConfigBgpPolicyAsPathListEntryTest covering arg
validation, bare entry create, attribute round-trips, seq-num keying,
asn-regexp accepting spaces, the match-logic default, and the rejection
paths.
- integration tests (2): ConfigBgpPolicyAsPathListEntryTest stages and
commits each attribute, then reads it back out of bgpd's running config
via the getRunningConfig RPC. Enums come back over SimpleJSON as
integers, so match_logic_type is asserted as MatchValueLogicOperator's
numeric value rather than its name.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y-list Adds `config protocol bgp policy community-list <name> [community <name>] [<attribute> <value> ...]` and `delete protocol bgp policy community-list <name>` — the second BGP policy object-type command family, beneath the `policy` grouping node alongside as-path-list. - community-list dispatcher with a two-level key (list name + inline community member name) writing bgp_policy.BgpPolicies.community_lists[] through the typed ConfigSession; boolean-operator maps to routing_policy.BooleanOperator, exact-match to the optional bool, and the member attributes (description/type/value) set the CommunityRefType union's inline Community arm, keyed by Community.name. - new boolAttr factory in the shared BgpCliAttrHandlers.h (generalizing the neighbor dispatcher's), reused by upcoming prefix-list. - fix a latent dangling-string_view in the shared enumAttr factory: the lambda captured a string_view over the caller's fmt::format temporary, so every invalid-enum-value rejection printed garbage after "expected". valueDesc is now taken (and captured) by value; the as-path-list and community-list rejection tests pin the full message. - unit tests (19) + commit-path integration tests mirroring ConfigBgpPolicyAsPathListTest (not yet run on a DUT). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mand
Splits the `community` (inline member) level of `config protocol bgp policy
community-list` into its own CLI11 subcommand, mirroring
`as-path-list <name> entry <seq-num>`. The previous commit ships the list
level and the whole delete command; this one ships the member level of the
config command:
community-list <name> community <name>
-> members[] inline Community, keyed by name
community-list <name> community <name> value <string>
-> Community.value
community-list <name> community <name> type <NORMAL|EXTENDED|LARGE>
-> Community.type
community-list <name> community <name> description <string>
-> Community.description
- The member is selected through the CommunityRefType union's inline arm.
The list name arrives through the ancestor-args tuple, and lookup/create
is shared with the parent through BgpCommunityListCliUtils.h.
- A list attribute typed alongside `community` is rejected rather than
silently dropped, because only the leaf command runs.
- `delete ... community-list <name> community <name>` already ships in the
previous commit and is untouched here: delete is a single dispatcher that
handles both levels through parseListMemberSelector(), so it did not need
splitting.
- A rejected value leaves nothing staged: a list or member implicitly
created for the failed command is rolled back.
- unit tests (9): CmdConfigBgpPolicyCommunityListCommunityTest covering arg
validation, bare member create, the three member attributes, named
members staying distinct, and the rejection paths.
- integration tests (2): ConfigBgpPolicyCommunityListCommunityTest stages
and commits, then verifies through bgpd's getRunningConfig RPC, including
that the list and a sibling member survive a single-member delete.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds `config protocol bgp policy prefix-list <name> [entry <seq-num>] [<attribute> <value> ...]` and `delete protocol bgp policy prefix-list <name> [entry <seq-num>]` — the third BGP policy object-type command family, beneath the `policy` grouping node alongside as-path-list and community-list. - prefix-list dispatcher with a two-level key (list name + entry seq-num) writing bgp_policy.BgpPolicies.prefix_lists[] through the typed ConfigSession. List level: boolean-operator, compare-operator (EQ|GE|LE|NE|GT|LT), description, and ip-version <v4|v6> (stored as the numeric routing_policy.PrefixList.version, the field the sheet documents). Entry level (routing_policy.PrefixListEntry keyed by seq_num in prefixes[]): base-prefix (validated as <prefix/len>, an explicit /len is required), communities (accumulates into the optional set), description, match-logic, max-allowed-subnet-count (-> max_allowed_golden_prefix_subnet_count), regex, and the prefix-len-range compare-operator|value pair targeting the single CompareNumericValue the CLI maintains at prefix_len_ranges[0] (compare-operator additionally accepts RG; value is bounded 0-128). - delete mirrors community-list's two levels: the whole list by name, or a single entry by `entry <seq-num>`. - new intAttr factory in the shared BgpCliAttrHandlers.h (bounded int32, valueDesc taken by value like enumAttr's), used by the two numeric entry attributes. - unit tests (19) + commit-path integration tests mirroring ConfigBgpPolicyCommunityListTest, run on a DUT (all 4 pass). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8f81d30 to
a0c636c
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Summary
Adds the third BGP policy object-type command family to
fboss2-dev:config protocol bgp policy prefix-list <name> [<attribute> <value>]delete protocol bgp policy prefix-list <name> [entry <seq-num>]The
entry <seq-num>level of the config command is a CLI11 subcommand of its own and ships in #1487. The delete side is whole (both levels) here, since it is one dispatcher.Commands
prefix-list <name>prefix_lists[](create/select)prefix-list <name> description <string>PrefixList.descriptionprefix-list <name> boolean-operator <AND|OR|NOT>PrefixList.boolean_operatorprefix-list <name> compare-operator <EQ|GE|LE|NE|GT|LT>PrefixList.compare_operatorprefix-list <name> ip-version <v4|v6>PrefixList.version(numeric 4/6)delete ... prefix-list <name>prefix_lists[]entrydelete ... prefix-list <name> entry <seq-num>Design
BgpCliAttrHandlers.h(added in [Nexthop][fboss2-dev] fboss2 config/delete protocol bgp neighbor commands #1391), named setters, one registry line per attribute. Writesbgp_policy.BgpPolicies.prefix_lists[]through the typedConfigSession.parseListMemberSelector()withentryas the member keyword and parses the member token as a non-negative int32, pre-empting the generic missing-member message so a bareentryreports that it needs a<seq-num>rather than a<name>.BgpPrefixListCliUtils.h, shared with the delete command and with theentrysubcommand in [Nexthop][fboss2-dev] fboss2 bgp policy prefix-list entry subcommand #1487.Test Plan
Unit —
CmdConfigBgpPolicyPrefixListTest(11) andCmdDeleteBgpPolicyPrefixListTest(8): arg validation, bare create, the four list attributes and their enum/ip-versionvalidation, named lists staying distinct, rejection messages, phantom-rollback, whole-list delete, and single-entry delete (including the seq-num parse errors). All pass.Integration on device (real
bgpddaemon),ConfigBgpPolicyPrefixListTest(2):SetListAttributesAndCommitandDeleteListAndCommit. Both stage and commit, then assert the result inbgpd's running config via thegetRunningConfigRPC.Review Findings
Pre-publication review (parallel correctness + robustness reviewers, adversarially verified). Two findings were fixed before publication: the
delete ... entrymissing-seq-num error message called the token a<name>, and one delete test claimed a non-persistence assertion it did not make. No remaining findings above the confidence threshold.