Yank out type duplication for BGP peers by taspelund · Pull Request #803 · oxidecomputer/maghemite

taspelund · 2026-06-25T18:01:46Z

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:

NeighborConfig
Neighbor
SessionInfo
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

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 · 2026-07-01T16:31:47Z

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

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

nicolaskagami

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.

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

- 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 · 2026-07-30T22:32:46Z

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 · 2026-08-04T13:20:42Z

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.

rcgoodfellow

Thanks for the consolidation work here @taspelund. Comments follow.

Since the API surface change is large for this PR, I'd like to see an omicron branch that's working e2e in CI and in voxel before this lands.

rcgoodfellow · 2026-09-02T05:22:08Z

Given all the testing infrastructure we've built up with falcon lab and the amount of additional complexity abstracting connections over multiple underlying substrates costs, i'm not sure if its worth keeping the channel model and the indirection layer around.

I think there has also been some work recently on running tests based on multiple mgd daemons on the same host peering with each other over localhost addresses. That seems like it be more worthwhile than the channel-based tests.

rcgoodfellow · 2026-09-02T05:25:48Z

        conn: &BgpConnectionTcp,
        min_ttl: Option<u8>,
        md5_key: Option<String>,
+        _listen_port: std::num::NonZeroU16,


remove leading _. You can use let _ = listen_port; in the linux stub.

rcgoodfellow · 2026-09-02T05:41:33Z

+/// Address and port configuration for a session.
+#[derive(Debug, Deserialize, Serialize, JsonSchema, Clone, PartialEq, Eq)]
+pub struct SessionAddrInfo {
+    /// TCP port configured on the remote peer. `BGP_PORT` is used when the peer


What is meant by the second sentence in this comment? remote_port is not optional here. If this is about how a user of this structure fills this field in, I think that commentary should go in the using code and not here as this can very easily get out of sync.

rcgoodfellow · 2026-09-02T06:02:17Z

+        let name = lock!(neighbor.name).clone();
+        let peer_group = lock!(neighbor.peer_group).clone();
+        *lock!(self.neighbor.name) = name;
+        *lock!(self.neighbor.peer_group) = peer_group;


This can probably simplify to

*lock!(self.neighbor.name) = lock!(neighbor.name).clone(); *lock!(self.neighbor.peer_group) = lock!(neighbor.peer_group).clone();

To reduce the chance of name and peer_group accidentally being held to long if code gets added after this later.

rcgoodfellow · 2026-09-02T06:11:20Z

+impl Default for JitterRange {
+    fn default() -> Self {
+        Self {
+            min: 0.75,


These constants deserve some comments.

rcgoodfellow · 2026-09-02T06:30:44Z

+
+// ===== upgrade conversions (v11 split -> v13 unified), total =====
+
+impl From<crate::v11::bgp::config::Neighbor> for Neighbor {


There is a LOT of version conversion going on here. It seems like we should account for this in our e2e tests like falcon lab by creating variants of our tests that drive things from clients that are one or even a few versions behind.

rcgoodfellow · 2026-09-02T06:36:47Z

I think this file can be removed?

rcgoodfellow · 2026-09-02T06:36:52Z

I think this file can be removed?

rcgoodfellow · 2026-09-02T06:38:53Z

        Self::update_neighbor_v8(rqctx, request.map(Into::into)).await
    }

-    // V1/V2 API - legacy Neighbor type with combined import/export policies.


At some point we can probably start pruning when it's guaranteed that things as far back as V1/V2 will never be in use?

rcgoodfellow · 2026-09-02T07:06:23Z

+        let (key, _) = item?;
+        tree.remove(key)?;
+    }
+    Ok(())


I think we may need a tree.flush() here to ensure that all the removes took place by the time this function returns.

taspelund self-assigned this Jun 25, 2026

taspelund added needs testing bgp Border Gateway Protocol mgd Maghemite daemon rust Pull requests that update rust code labels Jun 25, 2026

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 force-pushed the trey/bgp-peer-config-dedupe branch from d986afd to 498ce1b Compare June 25, 2026 23:03

taspelund marked this pull request as ready for review July 1, 2026 16:31

taspelund requested review from nicolaskagami, rcgoodfellow and zeeshanlakhani July 1, 2026 16:31

Add Default impl for JitterRange

f4d484f

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

taspelund force-pushed the trey/bgp-peer-config-dedupe branch from 1acc3c0 to f4d484f Compare July 2, 2026 17:52

nicolaskagami reviewed Jul 27, 2026

View reviewed changes

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

Merge branch 'main' into trey/bgp-peer-config-dedupe

a00e3bc

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

rcgoodfellow reviewed Sep 2, 2026

View reviewed changes


		// ===== upgrade conversions (v11 split -> v13 unified), total =====

		impl From<crate::v11::bgp::config::Neighbor> for Neighbor {

Uh oh!

Conversation

taspelund commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

API and type model

Internal simplification

Compatibility and review fixes

Persistence note

Validation

Uh oh!

taspelund commented Jul 1, 2026

Uh oh!

nicolaskagami left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taspelund commented Jul 30, 2026

Uh oh!

nicolaskagami commented Aug 4, 2026

Uh oh!

rcgoodfellow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

taspelund commented Jun 25, 2026 •

edited

Loading

rcgoodfellow left a comment •

edited

Loading