Migrate Prefix{,4,6} types to oxnet types#771
Conversation
The frozen DDMv2 protocol types (UpdateV2, PullResponseV2, UnderlayUpdateV2, TunnelUpdateV2, PathVectorV2, TunnelOriginV2, IpPrefix, Ipv4Prefix, Ipv6Prefix) were scattered across ddm-api-types, mg-common, and ddm/src/exchange/mod.rs. Collect them all into a single dedicated module with a module-level comment making the freeze guarantee impossible to miss. Add an expectorate snapshot test over the JSON schema for the two top-level message types (UpdateV2, PullResponseV2) so that any accidental change to the frozen wire format fails loudly. Also remove the DDMv1 conversion helpers (UpdateV1 and its From impls) that are no longer needed.
Fixes: #764 Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
fix formatting
| }), | ||
| derives = [schemars::JsonSchema], | ||
| crates = { | ||
| "oxnet" = "0.1.5", |
There was a problem hiding this comment.
This is slick. How do we keep this version in sync with the version that the crate/workspace depends on?
There was a problem hiding this comment.
It's not a big deal if it falls behind. All this version says is: don't use types that predate this version. So far, all the oxnet types are 0.1.0. If we start using a new type in oxnet we'll bump the version of the crate and need to bump this as well.
minimize use of yet another Prefix
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
self-review; changes incoming
Resolved conflicts from main's V4_OVER_V6_STATIC_ROUTES (v10) change: - PREFIX_TO_OXNET renumbered from v10 to v11 - Added v10 to v11 shim for StaticRoute4 (nexthop IpAddr, Prefix4 to Ipv4Net) - Added static_add/remove_v4_route_v10 endpoint shims in mg-api - Updated mg-admin-client to use client_common instead of mg_common - Fixed bgp/src/session.rs DEFAULT_RIB_PRIORITY_BGP import - Regenerated v11 OpenAPI snapshot Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
LGTM, mostly just nits. One nontrivial concern is about using new_unchecked() in converting from old API request versions, although switching all of those to TryFrom may be pretty annoying :-/
| .register_type_with_name::<Ipv4Net>("Prefix4") | ||
| .register_fn("within", prefix4_within_rhai) | ||
| .register_type_with_name::<Prefix6>("Prefix6") | ||
| .register_type_with_name::<oxnet::Ipv6Net>("Prefix6") |
There was a problem hiding this comment.
Is it fine that the Rust type name doesn't match the Rhai type name anymore? (I have basically 0 context for the Rhai bits.)
There was a problem hiding this comment.
I assumed it was better to be backward compatible with (what I assume is) our vast library of rhai scripts. @rcgoodfellow may have thoughts.
There was a problem hiding this comment.
The Rhai stuff has not made it into production interfaces yet. I don't think we need to hold this up for Rhai considerations.
| fn any_ipv4_prefix_strategy() -> impl Strategy<Value = Prefix4> { | ||
| fn any_ipv4_prefix_strategy() -> impl Strategy<Value = Ipv4Net> { | ||
| (any::<u32>(), 0u8..=32u8).prop_map(|(addr_bits, length)| { | ||
| // Don't use new() - we want to test both normalized and unnormalized |
There was a problem hiding this comment.
I'd keep this comment (and the matching ipv6 one on line 341). It's a little repetitious with the comment on the function, but this is weird enough it seems worth it
There was a problem hiding this comment.
The old comment had implicit in there that new would truncate/normalize the Prefix. IpNet does not do that so I thought the comment wasn't applicable.
| asn, | ||
| prefixes: prefixes | ||
| .into_iter() | ||
| .map(|p| Ipv6Net::new_unchecked(p.value, p.length)) |
There was a problem hiding this comment.
Is new_unchecked safe here? I don't think the old Prefix* types validated their length
Edit: Same question in a bunch of other From<OldType> for NewType conversions
There was a problem hiding this comment.
I believe so. It's an invariant of the Prefix* types that the length is valid; do you think it would be better to new().unwrap()?
There was a problem hiding this comment.
It's an invariant of the Prefix* types that the length is valid
Is it? It doesn't look like they enforce any invariants; the fields are pub and they derive Deserialize, so I think we could get any u8 value here, right?
There was a problem hiding this comment.
Yeah. I should have stated this differently: this use of new_unchecked preserves existing behavior. I could unwrap() or we could migrate to TryFrom. I'm not sure what would be best.
There was a problem hiding this comment.
Ahh, gotcha. In principle I vote for TryFrom: we're getting outside data that may not be valid, and allowing construction of an IpNet (which itself does enforce invariants, as long as nobody bypasses them with new_unchecked()) that could have an invalid length seems pretty bad?
If the TryFroms are atrocious in practice, I could see an argument for new().unwrap() under the claim that mgd only gets requests from other omicron services, which should be using progenitor clients, which means they should only be sending valid IpNets.
rcgoodfellow
left a comment
rcgoodfellow
left a comment
There was a problem hiding this comment.
Broadly looks good to me. Since this has a bunch of API surface change associated with it let's get an associated omicron PR up to see how that goes.
|
@rcgoodfellow seems to work: oxidecomputer/omicron#10558 |
4 participants
Migrate the Prefix types to oxnet... although we still need them for backward compatibility with old versions of the API. This required a new version of the mg-admin API so I rolled in a fix for #767 while I was here.