[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy prefix-list commands - #1477
Draft
hillol-nexthop wants to merge 13 commits into
Draft
[Nexthop][fboss2-dev] fboss2 config/delete protocol bgp policy prefix-list commands#1477hillol-nexthop wants to merge 13 commits into
hillol-nexthop wants to merge 13 commits into
Conversation
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).
Add `fboss2-dev config protocol bgp global <attr> <value>` on top of the BGP-aware ConfigSession (base PR). Edits the typed bgp::thrift::BgpConfig via ConfigSession::getBgpConfig()/saveBgpConfig() -- the whole-config, scope-agnostic typed API (no global/peer/peer-group special-casing in ConfigSession). - Collapse the 10 per-attribute global command classes into one dispatcher; reject cluster-id (no BgpConfig field) instead of writing dead config; bound switch-limit / max_golden_vips so out-of-range values aren't truncated. - Integration tests: ConfigBgpGlobalTest (each attr set+commit, verified in the promoted /etc/coop/bgpcpp/bgpcpp.conf) and ConfigBgpSessionTest (clear/diff/commit/rollback + a no-op-restart regression), sharing ConfigBgpTestBase. Test Plan: - bazel test //fboss/cli/fboss2/test/config:cmd_config_test - bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test - fboss2_integration_test on a DUT with bgp_pp active: ConfigBgpSessionTest 6/6 pass (clear, diff, commit-restarts-bgp_pp, rollback-restores-config, and the unchanged-config does-not-restart case); agent-session regression (ConfigInterfaceMtuTest) passes.
- Extract the value parsers (parseBool/parseInt/parseNonNegInt32, handler Result) into a shared BgpCliValueParsers.h so sibling BGP dispatchers can reuse them, and add a bounded parseAsn4Byte: local-asn/confed-asn accepted any uint64 and silently persisted out-of-range ASNs (>= 2^32 wrap the i64 field negative). - positionals_at_end() on the global command: CLI11's parent-chain subcommand fallthrough steals value tokens that match a sibling command name (e.g. a policy named "peer-group") and misparses the command. - Integration test base: probe the bgpd unit, and pass -c safe.directory=/etc/coop on the raw git invocations (gitHead / bgpTrackedAtRevision), mirroring the CLI's Git class -- /etc/coop is owned by another user (e.g. coop) on provisioned devices, which git otherwise rejects as dubious ownership and the helpers silently return empty results.
The commit-path global-attribute tests asserted on the promoted /etc/coop/bgpcpp/bgpcpp.conf, which only proves what the CLI wrote to disk. Route ConfigBgpTestBase::setAndCommit() through a new readRunningBgpConfigViaRpc() -- TBgpService::getRunningConfig against the local bgpd -- so every positive ConfigBgpGlobalTest asserts its attribute in the daemon's own view of its config, proving bgpd parsed and adopted the promoted file after the commit-triggered restart. The helper retries briefly on connection errors: systemd reports bgpd active as soon as the process starts, but its thrift server binds the port a few seconds later, so an RPC issued right after a restart races that window. Test Plan: - bazel build //fboss/cli/fboss2/test/integration_test:fboss2_integration_test - fboss2_integration_test on a DUT with bgpd active: ConfigBgpGlobalTest 7/7 pass (each attribute set+commit verified in bgpd's getRunningConfig view, plus the invalid-bool and negative-graceful-restart-time reject paths).
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
Contributor
|
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.) |
hillol-nexthop
marked this pull request as draft
August 7, 2026 18:15
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>
…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 (20) + commit-path integration tests mirroring ConfigBgpPolicyAsPathListTest (not yet run on a DUT). 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 (30) + commit-path integration tests mirroring ConfigBgpPolicyCommunityListTest, run on a DUT (all 4 pass). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hillol-nexthop
force-pushed
the
bgp-policy-prefix-list
branch
from
August 7, 2026 19:29
b8b80b3 to
7681ea9
Compare
|
@hillol-nexthop has updated the pull request. You must reimport the pull request before landing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the third BGP policy object-type command family to
fboss2-dev:config protocol bgp policy prefix-list <name> [entry <seq-num>] [<attribute> <value> ...]delete protocol bgp policy prefix-list <name> [entry <seq-num>]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)prefix-list <name> entry <seq-num>prefixes[]keyed byseq_num(create/select)... entry <seq> base-prefix <prefix/len>PrefixListEntry.base_prefix... entry <seq> match-logic <EQUAL|NOT_EQUAL>PrefixListEntry.match_logic... entry <seq> prefix-len-range compare-operator <EQ|GE|LE|NE|GT|LT|RG>prefix_len_ranges[0].compare_operator... entry <seq> prefix-len-range value <0-128>prefix_len_ranges[0].value... entry <seq> communities <community-string>PrefixListEntry.communities(set; accumulates)... entry <seq> max-allowed-subnet-count <value>PrefixListEntry.max_allowed_golden_prefix_subnet_count... entry <seq> regex <string>PrefixListEntry.regex... entry <seq> description <string>PrefixListEntry.descriptiondelete ... prefix-list <name>prefix_lists[]entrydelete ... prefix-list <name> entry <seq-num>Design
entry <seq-num>selecting the nestedPrefixListEntry); writesbgp_policy.BgpPolicies.prefix_lists[]through the typedConfigSession. The delete dispatcher reusesparseListMemberSelector()withentryas the member keyword and parses the member token as a non-negative int32.base-prefixis validated withfolly::IPAddress::tryCreateNetworkand additionally requires an explicit/len(folly silently fills in an address-family default mask for a bare address); the string is stored as typed, not normalized.prefix_len_ranges); the documented CLI exposes a single range, soprefix-len-range compare-operator|valueboth target the oneCompareNumericValuethe CLI maintains atprefix_len_ranges[0](created on first use, rolled back if the value is rejected).communitiesis a thriftset<string>: repeated invocations accumulate members; re-adding an existing member reports it without duplicating.intAttrfactory (bounded int32) to the sharedBgpCliAttrHandlers.h, alongside the existing string/bool/enum ones. The composedprefix-len-range <sub-attr>display names get static storage so the factories'string_viewname capture cannot dangle (the same hazard class as thevalueDescfix that shipped with community-list).Test Plan
Unit —
CmdConfigBgpPolicyPrefixListTest(22) andCmdDeleteBgpPolicyPrefixListTest(8): attribute round-trips at both levels, seq-num keying, communities accumulation, rejection messages (including the explicit-/lenrequirement and the 0-128 range bound), phantom-rollback for list/entry/range, and single-entry delete — all pass.Integration on device (real
bgpddaemon),ConfigBgpPolicyPrefixListTest: 4/4 pass on a lab device. Every test stages AND commits, then asserts the value landed inbgpd's running config via thegetRunningConfigRPC — list attributes, nested entry attributes (includingprefix_len_ranges[0]andcommunities), whole-list delete, and single-entry delete with the sibling entry surviving.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. Extrabase-prefixvalidation coverage (out-of-range mask, second slash, v6 accept) was added at the same time. No remaining findings above the confidence threshold.