Add External API endpoints for BGP unnumbered interface manager status#10705
Add External API endpoints for BGP unnumbered interface manager status#10705taspelund wants to merge 5 commits into
Conversation
taspelund
added
enhancement
|
I have taken this through a4x2 manually and have validated that e2e connectivity works, as do the new endpoints: |
4736713 to
654fe27
Compare
| impl super::Nexus { | ||
| pub async fn bgp_unnumbered_manager_status( | ||
| &self, | ||
| _optctx: &OpContext, |
There was a problem hiding this comment.
Slight typo here (and in the other methods):
| _optctx: &OpContext, | |
| _opctx: &OpContext, |
That said, I'm not sure these methods should take an argument they don't use. I don't work in the auth area much, but my intuition is that auth checks should be performed at the "lowest level", instead of relying on callers performing them - should we be doing an auth check in this method (and therefore using the opctx arg after all)?
There was a problem hiding this comment.
I'm not sure what the right approach is here tbh. I followed the approach that the existing bfd methods used as an example since I wasn't sure what to do for authz
There was a problem hiding this comment.
@david-crespo not to single you out, but since you've already chimed in... Do you have any guidance around what the right thing to do is with authz and opctx here?
| let mut result = Vec::new(); | ||
| for switch_slot in [SwitchSlot::Switch0, SwitchSlot::Switch1] { | ||
| // Log an error if we only have one scrimlet, but keep going. | ||
| // We still want to return anything we're able to collect. |
There was a problem hiding this comment.
We're not guaranteed to have any clients here; from the docs on resolve_mgd_clients():
/// # Errors
///
/// Fails if we cannot resolve MGD in DNS.
///
/// For any MGD instance we resolve via DNS, if the MGD instance does not know
/// its own switch slot, the switch slot -> client mapping for that instance
/// will be omitted from the returned map. Callers must not assume an `Ok(_)`
/// return value contains any client.
As written, that means if we get back an empty map from self.mg_clients(), we'll return an Ok(Vec::new()) result. Is that okay? I wonder if instead of returning a Vec, we should return something like a BTreeMap<SwitchSlot, UnnumberedManagerStateOrError>? Then we could return something logically like
{ "switch0": "no client found: blah blah error", "switch1": "SomeValidStatus" }| let mut result = Vec::new(); | ||
| for switch_slot in [SwitchSlot::Switch0, SwitchSlot::Switch1] { | ||
| // Log an error if we only have one scrimlet, but keep going. | ||
| // We still want to return anything we're able to collect. |
There was a problem hiding this comment.
Same note here about maybe not having clients, and maybe returning a map keyed by switch slot so we can give back partial success?
There was a problem hiding this comment.
I initially thought about returning a map keyed on switch slot, but it looked like other endpoints for switch-specific queries (e.g. lldp) all used the approach of returning a vec of wrapper structs that have switch slot as a member.
If we do go with a map, I think we could get rid of the wrapper structs SwitchUnnumberedManagerState and SwitchUnnumberedInterface since all they do for UnnumberedManagerState and UnnumberedInterface is embed the switch slot.
I don't work in this codebase very often, so I'm happy to go whichever way is preferable to the folks who live here
|
|
||
| fn nexus_interface_name(interface: String) -> String { | ||
| if let Some(interface) = interface | ||
| .strip_prefix("tfport") |
There was a problem hiding this comment.
This method makes me nervous and worried we're baking knowledge into nexus that really doesn't belong there. Some questions:
- If we expect all interfaces to be named
tfport{X}_0, should we fail instead of returning the interface unchanged? - Is this a sign we're overusing
Strings and instead should have something like aTfPort(String)type, with methods for formatting itself either with or without thetfport ... _0trappings? - Are external API callers expected to use a
tfport{X}_0name? - I assume "tf" is from "tofino" - are we setting ourselves up for future pain whenever we have non-tofino switch?
Maybe most of this is a nonissue since this function is private and only used for API version conversions (or maybe it's even addressing some of these questions as we move the API forward)?
There was a problem hiding this comment.
If we expect all interfaces to be named tfport{X}_0, should we fail instead of returning the interface unchanged?
I'm not sure on this front. Today mgd takes a String as its query parameter and does a lookup for an interface with that exact name, without any transforms applied -- which is the same logic that's used for BGP unnumbered peers. So from the mgd side, if the String given to it matches a real interface then it'll return state for that same interface. I'm not sure how mgd would take String A and return String B or how we'd want that to appear... I'd think it'd be better to return whatever we got so we can at least still get some operational state, even if the ifname is mangled.
Is this a sign we're overusing Strings and instead should have something like a TfPort(String) type, with methods for formatting itself either with or without the tfport ... _0 trappings?
I assume "tf" is from "tofino" - are we setting ourselves up for future pain whenever we have non-tofino switch?
I would much prefer a new TfPort type that organizes all of this logic, but I don't know how toronto is going to affect things... I imagine it's probably too early to tell since I don't think we have the equivalent of the tfport driver for toronto yet. If we go through the effort to build a type like that, I would almost prefer that it lives someplace like oxnet so it can be reused by mgd, dpd, tfportd, and nexus.
Are external API callers expected to use a tfport{X}_0 name?
No, external API users supply a name like "qsfp0". I added the tfport formatting logic for this endpoint because the other link/port/peer APIs expect the bare name, not the tfport name. No need to expose the internal implementation detail if we can help it.
Maybe most of this is a nonissue since this function is private and only used for API version conversions (or maybe it's even addressing some of these questions as we move the API forward)?
I think today it's a non-issue, but I would definitely like to have more structure in place around these conversions. I just think it's going to require more coordination across the network component repos.
There was a problem hiding this comment.
I vote for using a newType (like what I mentioned above, but yeah TfPort is a better name).
Right now we're codifying an expectation in a way that would be better expressed with types. If and when we need to change that expectation we'll be better served by already having the type in place, be that for adapting to Toronto or to an oxnet type.
Also, we don't need to expose the type on the API if we are concerned about breaking changes. I'd be for a type here even if it's entirely confined to this file.
| /// When the peer was first discovered (ISO 8601 timestamp) | ||
| pub discovered_at: String, | ||
| /// When the most recent Router Advertisement was received (ISO 8601 | ||
| /// timestamp) | ||
| pub last_advertisement: String, |
There was a problem hiding this comment.
Could these be DateTime<Utc> instead of String?
If they're coming from a remote source and we're worried about parsing, it's maybe worth noting that in the doc comment. "Expected to be ISO 8601 timestamps, but presented as-is from the remote router" or something like that.
There was a problem hiding this comment.
these are all generated by mgd directly, not derived from an external source. there's no reason this can't be done cleaner -- I'll fix on the maghemite side and omicron can just inherit it
There was a problem hiding this comment.
I think I'm actually going to change the return type to use a Duration rather than an absolute timestamp.
This makes it simpler for reporting during startup (no NTP) and gives a view more inline with how the underlying component actually thinks about the world (using a monotonic Instant to calculate a Duration and comparing it against an expiry timeout).
654fe27 to
5e1011e
Compare
| /// Peer IPv6 address | ||
| pub address: Ipv6Addr, | ||
| /// When the peer was first discovered (ISO 8601 timestamp) | ||
| pub discovered_at: String, |
There was a problem hiding this comment.
The convention we try to follow with timestamps, though we don't do it universally, is time_*. This one is easy because it can be time_discovered. last_advertisement is a little weirder, would have to be something like time_last_advertised — not terrible but not great.
| networking_bgp_status GET /v1/system/networking/bgp-status | ||
| networking_bgp_unnumbered_interface_list GET /v1/system/networking/bgp/unnumbered/interfaces | ||
| networking_bgp_unnumbered_interface_view GET /v1/system/networking/bgp/unnumbered/interfaces/{switch_slot}/{interface_name} | ||
| networking_bgp_unnumbered_manager_status GET /v1/system/networking/bgp/unnumbered/manager |
There was a problem hiding this comment.
Not that we have a great pattern here, but these stand out in being the only ones to nest something under bgp. Maybe all of them should look like /bgp/*, but for now, for consistency maybe it should be /v1/system/networking/bgp-unnumbered-interfaces?
omicron/nexus/external-api/output/nexus_tags.txt
Lines 272 to 289 in 5e1011e
There was a problem hiding this comment.
I can shift over to that convention for consistency. Thanks!
| fn maghemite_interface_name(interface_name: &str) -> String { | ||
| format!("tfport{interface_name}_0") | ||
| } |
There was a problem hiding this comment.
This is a great place to use a newType to codify the format expectations of the String we're carrying around. It could look like this:
pub(crate) struct MaghemiteIfaceName(String);
impl MaghemiteIfaceName {
pub fn from_interface_name(iface_name: &str) -> Self {
Self(format!("tfport{iface_name}_0"))
}
}
impl AsRef<str> for MaghemiteIfaceName {
fn as_ref(&self) -> &str {
&self.0
}
}
impl std::fmt::Display for MaghemiteIfaceName {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(&self.0)
}
}- A semantically-explicit constructor name is better if we're not typing the input as well. That could be good if we want to enforce other things like maximum size and absence of certain special characters. The sooner we find these inconsistencies the better.
|
|
||
| fn nexus_interface_name(interface: String) -> String { | ||
| if let Some(interface) = interface | ||
| .strip_prefix("tfport") |
There was a problem hiding this comment.
I vote for using a newType (like what I mentioned above, but yeah TfPort is a better name).
Right now we're codifying an expectation in a way that would be better expressed with types. If and when we need to change that expectation we'll be better served by already having the type in place, be that for adapting to Toronto or to an oxnet type.
Also, we don't need to expose the type on the API if we are concerned about breaking changes. I'd be for a type here even if it's entirely confined to this file.
| } else { | ||
| interface | ||
| } |
There was a problem hiding this comment.
I'm not sure it's good to go on with a silent error here.
| /// When the peer was first discovered (ISO 8601 timestamp) | ||
| pub discovered_at: String, | ||
| /// When the most recent Router Advertisement was received (ISO 8601 | ||
| /// timestamp) | ||
| pub last_advertisement: String, |
Replace the stringly discovered-peer API type with typed values, in support of oxidecomputer/omicron#10705: - DiscoveredRouter (v12) now reports relative durations (time_since_discovered, time_since_last_rx, effective_reachable_time) instead of ISO 8601 timestamp strings reconstructed from Instants at query time (a derivation that was unstable across queries and shifted under clock adjustments), and drops the expired bool and human-readable time_until_expiry string entirely. An expired entry is represented as discovered_peer: None, making the expired-but-still-reported state unrepresentable. - time-until-expiry is derived, not transmitted: a DiscoveredRouter::time_until_expiration() helper lives with the API type, and mg-admin-client replaces the generated DiscoveredRouter with the real type so mgadm shares it. - The v5 NDP downgrade reconstructs the legacy wall-clock timestamps and human-readable expiry string from the durations. Expired peers are no longer visible through v5 (its only consumer is mgadm, which moves to the new endpoint). - State-to-API conversion is now From<&DiscoveredRouterState> for Option<DiscoveredRouter> in the unnumbered crate, with tests for the expired -> None and live-mapping paths. - format_duration_human moves from mg-common to client-common (zero-dep CLI utility crate), so mg-api-types-versions and the admin CLIs share one copy without ddmadm depending on mg-api-types. - mgadm drops the Reachable column (peer presence now implies reachability) and shows durations for the discovered peer detail. - DiscoveredRouterState renames `when` to `last_seen`; dead validate_peer_for_interface removed. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Now that maghemite has migrated to the RFD 619 convention, it exposes most types via the mg-api-types crate. This crate should have its hash updated alongside mg-admin-client and ddm-admin-client. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Adds external API endpoints to GET the BGP unnumbered status of interfaces and the unnumbered manager. This provides insights into the Router Discovery state (have we learned about a BGP peer via NDP) and the readiness of an interface that's been configured for BGP unnumbered (does the interface exist in the OS? does it have an IPv6 link-local IP? or is it active?). These are all important when diagnosing failures or delays in the establishment of BGP unnumbered peers, since these are all prerequisites for the BGP FSM to complete connections. This is the omicron side of maghemite#800 Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
5e1011e to
c69cc2c
Compare
4 participants
This is the omicron side of maghemite#800.
In mgd, we are refactoring the internals of BGP unnumbered which affects the mg-api by no longer requiring the BGP ASN for querying the state... However, omicron never had plumbing to query BGP unnumbered interface state to begin with, which is what this PR remedies.
The new API endpoints are purely status, with no config whatsoever.
The maghemite hash currently points to the feature branch, but will be updated to main once this PR's counterpart lands.