Skip to content

Yank out type duplication for BGP peers - #803

Open
taspelund wants to merge 4 commits into
mainfrom
trey/bgp-peer-config-dedupe
Open

Yank out type duplication for BGP peers#803
taspelund wants to merge 4 commits into
mainfrom
trey/bgp-peer-config-dedupe

Conversation

@taspelund

@taspelund taspelund commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR removes the duplicated numbered/unnumbered BGP peer configuration paths introduced as BGP unnumbered support grew. The latest implementation goes from 11 overlapping API, storage, and session shapes to four core types:

  1. NeighborConfig
  2. Neighbor
  3. SessionInfo
  4. NeighborInfo

Older shapes remain only in versioned compatibility APIs. Overall, this removes more than 1,000 lines while making numbered and unnumbered peers follow the same configuration, persistence, and session-management paths.

API and type model

The new UNIFY_BGP_NEIGHBORS API is version 13 and is exposed through latest.

  • PeerId remains solely the peer identity: Ip for numbered peers or Interface for unnumbered peers.
  • NeighborConfig contains that identity, group, the optional TCP port, and all session parameters. The former BgpPeerParameters layer is inlined without serde(flatten).
  • Neighbor is the read/stored composition { asn, config: NeighborConfig }.
  • Single-neighbor create and update operations continue to use /bgp/config/neighbor, with ASN carried by Neighbor.
  • Numbered and unnumbered clear operations use one NeighborResetRequest and one handler.

ApplyRequest is now an explicitly tagged operation:

{"action":"apply","asn":47,"originate":[],"peers":[]}
{"action":"delete","asn":47}

Apply represents complete desired state for a router. An empty peer list retains the router, while Delete explicitly removes the router and its state.

Apply peers are represented internally as IdOrdMap<NeighborConfig>, keyed only by PeerId, so one peer cannot appear in multiple groups. Serde and OpenAPI retain a normal JSON array representation, and Progenitor generates Vec<NeighborConfig> for clients. Duplicate peer identities are rejected when the request is deserialized.

Internal simplification

  • Removed bgp::config::PeerConfig and its conversion layer.
  • Removed the latest-path BgpPeerConfig, UnnumberedBgpPeerConfig, BgpPeerParameters, BgpNeighborInfo, BgpUnnumberedNeighborInfo, and BgpNeighborParameters duplication.
  • The RDB now persists Neighbor directly in one tree keyed by (ASN, PeerId) instead of maintaining separate numbered and unnumbered storage paths.
  • Router/session creation, ensure, update, reset, and teardown now dispatch through PeerId rather than parallel numbered/unnumbered implementations.
  • The mgd API handlers are collapsed onto unified create/read/update/delete/clear operations. Older split endpoints remain as version-bounded adapters.
  • mgadm and falcon-lab construct the same nested Neighbor shape used by the API and RDB.
  • deterministic_collision_resolution and timer jitter settings now survive persistence instead of being reconstructed with defaults.

Compatibility and review fixes

  • v11 requests upgrade into the unified v13 shape; a peer appearing in multiple legacy groups is rejected with HTTP 400.
  • v13 apply requests downgrade exhaustively by matching PeerId::Ip and PeerId::Interface, rather than silently dropping a peer kind. Explicit router deletion cannot be represented by v11 and returns a conversion error.
  • Compatibility read endpoints return 404 when a unified lookup finds the wrong peer kind.
  • Session updates validate timer changes before mutating peer metadata.
  • Applying an empty peer set no longer removes the router and then fails while updating origins.
  • Applying a new ASN tears down displaced router state through the normal router-deletion path.

Persistence note

The sled database is used for recovery from daemon crashes and restarts within a deployed zone. mgd upgrades replace and rebuild the zone, so no cross-version migration of the retired numbered/unnumbered neighbor tree is required.

Validation

  • cargo fmt --all --check
  • cargo test -p mg-api-types-versions
  • targeted mgd BGP apply tests
  • Clippy with warnings denied across affected crates
  • just openapi-check mgd
  • cargo check --manifest-path mg-admin-client/Cargo.toml

Fixes: #564

@taspelund taspelund self-assigned this Jun 25, 2026
@taspelund taspelund added needs testing bgp Border Gateway Protocol mgd Maghemite daemon rust Pull requests that update rust code labels Jun 25, 2026
@taspelund
taspelund force-pushed the trey/bgp-peer-config-dedupe branch 3 times, most recently from 7d328c5 to d986afd Compare June 25, 2026 22:59
@taspelund
bgp: unify numbered and unnumbered peer config
498ce1b 
This PR yanks out a ton of type duplication around BGP peer config.
We had a lot of this before, but BGP unnumbered added yet another split
for many of the types.

We started with 11 separate types:

1. Neighbor
2. UnnumberedNeighbor
3. BgpPeerConfig
4. UnnumberedBgpPeerConfig
5. BgpPeerParameters
6. BgpNeighborInfo
7. BgpUnnumberedNeighborInfo
8. BgpNeighborParameters
9. PeerConfig
10. SessionInfo
11. NeighborInfo

And ended with just 4 types:

1. NeighborConfig
2. Neighbor
3. SessionInfo
4. NeighborInfo

All in all, we end up with ~1500 fewer lines of code and a noticeably
streamlined order of operations for API handlers. The following summary
came from Claude since the full extent of the changes is so wide.
Added:

- New versioned API module mg-api-types-versions::v12
- (unify_bgp_neighbors/), exposed via latest.rs:
 + NeighborConfig: single write/input type. Peer identity plus
   all session params inlined; no asn/group, no serde(flatten).
 + Neighbor: read/stored type via composition:
   { asn, group, config: NeighborConfig }.
 + NeighborGroupSelector { asn, group } for write endpoints.
 + Unified ApplyRequest (peers: map<group, Vec>)
   and NeighborResetRequest.
 + v11<->v12 conversions: total From upgrades, partial TryFrom
   (PeerKindMismatch) downgrades.
- API version (12, UNIFY_BGP_NEIGHBORS) in mg-api api_versions!.
- Regenerated OpenAPI doc mg-admin-12.0.0.

Removed:

- bgp::config::PeerConfig and its From impls (config.rs now holds
  only RouterConfig).
- rdb storage types BgpNeighborInfo and BgpUnnumberedNeighborInfo
  (and nested BgpNeighborParameters); the DB now persists Neighbor.
- External BgpPeerConfig/UnnumberedBgpPeerConfig and the
  Neighbor/UnnumberedNeighbor split; the BgpPeerParameters struct.
- Conversion hand-copies Neighbor::from_bgp_peer_config and
  from_rdb_neighbor_info.
- Router fork: new_unnumbered_session, ensure_unnumbered_session,
  update_unnumbered_session; the peer_id param of
  new_session_locked.
- UnnumberedManagerNdp::get_neighbor_session and its dead routers
  field.
- Dead NeighborResetRequest::to_peer_id.
- Paired numbered/unnumbered mgd handlers (~10 collapsed to ~5).
- Retired the v11 OpenAPI doc to a .gitstub.

Changed:

- Identity unified on PeerId (Ip|Interface) with port: Option; asn/group
  are request coordinates, not body fields.
- SessionInfo/NeighborInfo built From<&NeighborConfig>;
  Error::UnknownPeer now carries a PeerId; update_session handles
  both peer kinds.
- rdb db.rs: one PeerId-keyed tree; add/get/remove keyed by PeerId;
  removed the unnumbered ops and BGP_UNNUMBERED_NEIGHBOR.
- mg-api: unified create/read/update/delete/clear as the required
  VERSION_UNIFY_BGP_NEIGHBORS.. methods; older split endpoints kept
  as version-bounded provided defaults. Create/update moved to
  /bgp/config/neighbor-group/{asn}/{group} (Dropshot path-collision
  fix).
- mgd: do_bgp_apply diff keyed by PeerId; dropped the
  UNSPECIFIED:179 read-path fabrication; reworked add_neighbor,
  add_neighbor_v1, get_exported_v1, and main.rs to the new shapes.
- mgadm: to_api_neighbor (was into_*) produces NeighborConfig;
  create/update pass asn+group on the path.
- falcon-lab: builds and passes the nested Neighbor.

Fixed:

- deterministic_collision_resolution and jitter are now persisted;
  previously hard-coded on rdb read and lost across reload.

Cleanups:

- Trimmed verbose compile-barrier comments to short notes
  (mp_bgp, prefix_to_oxnet, static_admin, rdb/types).
- Removed mg-api-types doc-comment text that leaked internal
  implementation into the OpenAPI spec.
- Scrubbed test.rs comments of references to removed types.

Fixes: #564

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
@taspelund
taspelund force-pushed the trey/bgp-peer-config-dedupe branch from d986afd to 498ce1b Compare June 25, 2026 23:03
@taspelund
taspelund marked this pull request as ready for review July 1, 2026 16:31
@taspelund

taspelund commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

marking this ready for review now. I still need to build out the omicron PR, but I don't think that should hold up any reviews on the maghemite side

@taspelund
Add Default impl for JitterRange
f4d484f 
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
@taspelund
taspelund force-pushed the trey/bgp-peer-config-dedupe branch from 1acc3c0 to f4d484f Compare July 2, 2026 17:52
nicolaskagami
nicolaskagami reviewed Jul 27, 2026
View reviewed changes

@nicolaskagami nicolaskagami left a comment

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.

Nice! It's always great to see code being simplified away :)

I found a couple of things that are worth addressing down below and:

  • The PR description mentions a NeighborGroupSelector which is nowhere to be found in this repo.
  • I like PeerId but I think we could do more of it, such as with act_as_a_default_ipv6_router. I'm thinking about a better format overall.

Comment thread rdb/src/db.rs
Comment thread mgd/src/bgp_admin.rs Outdated
Comment thread bgp/src/session.rs
taspelund added 2 commits July 29, 2026 20:13
@taspelund
Merge branch 'main' into trey/bgp-peer-config-dedupe
a00e3bc 
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
@taspelund
Address review feedback around the unified BGP peer configuration:
1393099 
- validate session updates before mutating peer metadata;
- retain a router when Apply drains all peers and require explicit Delete;
- enforce one NeighborConfig per PeerId across groups with IdOrdMap;
- return 404 for peer-kind mismatches in compatibility endpoints;
- reject duplicate peers from legacy apply requests with HTTP 400;
- make v13-to-v11 conversion exhaustive instead of dropping peers.

Move group into NeighborConfig, retain the array-shaped wire and generated
client representation, regenerate the v13 OpenAPI document, and add coverage
for serialization, conversions, and apply/delete behavior.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
@taspelund

taspelund commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Nice! It's always great to see code being simplified away :)

Thanks! I am much happier with this shape than I was with the parallel un/numbered paths.

* The PR description mentions a `NeighborGroupSelector` which is nowhere to be found in this repo.

I fixed up the PR description (rather, I had claude do it because there are a lot of changes in this PR) with a summary of what's been done.

* I like `PeerId` but I think we could do more of it, such as with `act_as_a_default_ipv6_router`. I'm thinking about a better format overall.

I understand the urge to move things like act_as_a_default_ipv6_router into PeerId::Interface. It's the only part of any BGP peer type that is truly unique to unnumbered peers, and structurally it would make sense for that to be where unnumbered attributes live. That said, I am sort of partial to the idea of having this be a small and simple enum that only does what it says on the tin: it identifies a peer.

Maybe there's another shape we could pursue where PeerId is only ever derived, rather than it also functioning as a config shape? That seems like it would give the best of both worlds -- PeerId stays relatively pure, un/numbered-specific config structurally prevented from being unrepresentable on the types where it doesn't apply.

@nicolaskagami

nicolaskagami commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

I understand the urge to move things like act_as_a_default_ipv6_router into PeerId::Interface. It's the only part of any BGP peer type that is truly unique to unnumbered peers, and structurally it would make sense for that to be where unnumbered attributes live. That said, I am sort of partial to the idea of having this be a small and simple enum that only does what it says on the tin: it identifies a peer.

That's not exactly what I was suggesting, exactly for the reason you give.

Maybe there's another shape we could pursue where PeerId is only ever derived, rather than it also functioning as a config shape? That seems like it would give the best of both worlds -- PeerId stays relatively pure, un/numbered-specific config structurally prevented from being unrepresentable on the types where it doesn't apply.

Yep. We could have PeerId come from a method instead of being a field and have an enum for exclusive fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolaskagami nicolaskagami nicolaskagami left review comments
@rcgoodfellow rcgoodfellow Awaiting requested review from rcgoodfellow
@zeeshanlakhani zeeshanlakhani Awaiting requested review from zeeshanlakhani

Assignees

@taspelund taspelund

Labels

bgp Border Gateway Protocol mgd Maghemite daemon needs testing rust Pull requests that update rust code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Type consolidation: remove/consolidate redundant types similar to bgp::session::SessionInfo

2 participants

@taspelund @nicolaskagami