Nexus external API: Use stronger types for BgpPeer addresses#10122
Nexus external API: Use stronger types for BgpPeer addresses#10122jgallagher merged 29 commits intomainfrom
BgpPeer addresses#10122Conversation
jgallagher
force-pushed
the
john/stronger-unnumbered-types-2
branch
from
9f59688 to
56e7ddf
Compare
internet-diglett
left a comment
internet-diglett
left a comment
There was a problem hiding this comment.
I primarily have one burning question, but otherwise this looks pretty good!
| ReserveBlockError::AddressNotInLot, | ||
| ) => Error::invalid_request("address not in lot"), | ||
| } | ||
| Error::from(err) |
| )] | ||
| ValueTooLarge, | ||
| #[error( | ||
| "max path value must be an integer between {} and {}", |
There was a problem hiding this comment.
whoops, pretty sure this was me 😂
| // Track peers for policy lookup. | ||
| peer_by_addr | ||
| .insert(p.addr.ip_squashing_unnumbered_to_sentinel(), &p); |
There was a problem hiding this comment.
🤔 I feel like I'm missing something... or I'm forgetting a conversation that was had before but...
In the event that a SwitchPortSettingsCreate struct contains multiple unnumbered peer configurations, wouldn't indexing them by their address cause all-but-one of them to get clobbered?
(if this is indeed a bug, the previous implementation also did the same thing)
There was a problem hiding this comment.
Oh, this is noted on #10151, but: this isn't as bad as it seems at a glance. We don't use this map for database insertion; it's only used to assemble the in-memory return value. Still a bug, but it only affects the value we return from the initial insert - the stored data is correct.
2 participants
This is a followup to #10082, and extends the stronger
RouterPeerTypeout from the internal API to the Nexus external API. The primary changes here are:BgpPeerto give stronger guarantees:BgpPeer::addris nowRouterPeerTypeinstead ofOption<IpAddr>(which permitted three distinct representations of "unnumbered":None,Some(0.0.0.0), andSome(::)).BgpPeer::router_lifetimemoved from being a top-level field to being nested inside theRouterPeerType::Unnumberedvariant, and its type is nowRouterLifetimeConfiginstead ofu16, adding enforcement of bounds.BgpPeer::interface_name(closesBgpPeer::interface_nameisn't faithfully persisted in the db - we should remove it #10104).BgpPeer:BgpPeerConfigSwitchPortSettingsSwitchPortSettingsCreateThe bulk of the diff is defining new versions of all these types and unit tests for the conversions they implement. The rest is fallout where these types touch other areas (e.g., datastore methods and bg tasks) and should be relatively straightforward.