Wrap up RFD 619 migration#724
Merged
Merged
Conversation
Phase 0: capture baseline (cargo nextest 241 passing, clippy clean, openapi check fresh) before structural migration to RFD 619 layout. Phase 1: scaffold three new versions crates and v6/v7/v8 module directories in mg-types-versions: - mg-common-types + mg-common-types-versions (new) - bgp-types + bgp-types-versions (new) - rdb-types/versions (new sibling under existing rdb-types) - mg-types/versions: add v6 (rib_exported_string_key), v7 (operation_id_cleanup), v8 (bgp_src_addr) module dirs No types are migrated yet; module directories are empty placeholders. The new facade crates re-export from latest. mg-common-types-versions already includes v1::net (TunnelOrigin, Ipv6Prefix, Ipv4Prefix, IpPrefix) but consumers have not yet been re-pointed; that lands in Phase 2a. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…lFilter to rdb-types-versions Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Move Path and BgpPathProperties out of rdb/src/types.rs into
rdb-types-versions: v1::path holds the pre-UNNUMBERED shape (peer is
IpAddr), v5::path holds the UNNUMBERED shape (peer is PeerId,
nexthop_interface added). Conversions live in v5 (newer module);
the BgpPathProperties -> v1 fallback for PeerId::Interface is silent
(Ipv6Addr::UNSPECIFIED). impl Ord for Path and as_stale move to
rdb-types-versions/impls/path.rs. From<StaticRouteKey> for Path stays
in rdb/src/types.rs until StaticRouteKey migrates.
Split mg-types-versions::rib::GetRibResult per version: v1 references
v1::path::Path, v5 references v5::path::Path; latest re-exports v5.
Free fn v5::rib::get_rib_result_into_v1 downgrades for shims.
Version-gate static_list_v{4,6}_routes in mg-api so prior-version docs
keep their v1-shaped Path. Latest endpoints scoped to VERSION_UNNUMBERED..;
shims static_list_v4_routes_v1 (..VERSION_UNNUMBERED) and
static_list_v6_routes_v2 (VERSION_IPV6_BASIC..VERSION_UNNUMBERED) call
the latest impl and downgrade. Both pairs pin operation_id explicitly so
generated clients see one method per endpoint across versions.
OpenAPI: all 11 blessed docs (mg-admin v1..v8, ddm-admin v1, both
*-latest) byte-identical to baseline.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…ions
Move ImportExportPolicyV1 → v1::policy::ImportExportPolicy (suffix dropped;
schemars(rename) annotation no longer needed since the Rust name now matches
the schema name naturally). Move ImportExportPolicy{,4,6} → v4::policy::*.
Conversions From<v1::policy::ImportExportPolicy> for ImportExportPolicy{4,6}
live in the v4 module. The as_ipv4_policy/as_ipv6_policy/from_per_af_policies
inherent methods on the v1 type move into impls/policy.rs.
The combined v4::policy::ImportExportPolicy enum is internal-only (no
JsonSchema), so there is no schema-name collision with v1::policy::Import
ExportPolicy in any blessed OpenAPI doc. Verified all 9 docs byte-identical.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…ypes
Move BgpRouterInfo into v1::router; BfdPeerConfig + SessionMode into
v1::bfd. BfdPeerConfig and SessionMode schemas verified byte-identical
across all 8 blessed mg-admin docs.
The remaining trio (BgpNeighborInfo, BgpUnnumberedNeighborInfo,
BgpNeighborParameters) is not present in any blessed OpenAPI document --
they are internal db-persistence types with vestigial JsonSchema
derives. BgpNeighborParameters references ImportExportPolicy{4,6}, which
live in v4 per the prior Phase 2b sub-step, so the only-prior-versions
rule forces all three into v4::neighbor.
OpenAPI: all 9 blessed docs (mg-admin v1..v8, ddm-admin v1) plus both
*-latest symlinks byte-identical to baseline.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
mg-types-versions/src/initial/bfd.rs was importing rdb::BfdPeerConfig (a floating identifier from the rdb facade), violating RFD 619 §determinations-only-prior-versions. Switch to rdb_types_versions::v1::bfd::BfdPeerConfig. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…rsions Move the 12 leaf wire-message enumerations (MessageType, OptionalParameterCode, CapabilityCode, Safi, PathAttributeTypeCode, AsPathType, ErrorCode, HeaderErrorSubcode, OpenErrorSubcode, UpdateErrorSubcode, CeaseErrorSubcode, PathOrigin) into bgp-types-versions::v1::messages, plus Afi into bgp-types-versions::v4::messages. Their Display/slog::Value impls and the Afi <-> rdb-types-versions::v1::AddressFamily conversions go to bgp-types-versions::impls::messages (orphan-rule case 1). The three From impls that take protocol types as input (From<&Message> for MessageType, From<PathAttributeValue> for PathAttributeTypeCode, From<Capability> for CapabilityCode) become free functions in bgp/src/messages.rs (message_type_of, path_attribute_type_code_of, capability_code_of) since their input types live in bgp/ and pulling them into bgp-types-versions would break the leaf-crate property (orphan-rule case 2). The handful of call sites are updated. bgp-types-versions::error::WireError is scaffolded with the parse-only + leaf wire-error variants (TooSmall, TooLarge, NoMarker, BadLength, BadVersion, Eom, Parse, Io, Unassigned, Experimental, InvalidCode, ReservedCapability/Code, ReservedOptionalParameter, InvalidMessageType, BadBgpIdentifier, MalformedAttributeList, InvalidNlriPrefix, InvalidPrefixLength, plus the 11 TryFromPrimitiveError variants for the moved enums). bgp::error::Error gains a #[from] WireError variant for future ?-upcasting; existing variants stay put because the parse code that produces them still lives in bgp/. bgp-types-versions gains nom/num_enum/thiserror/slog/rdb-types-versions deps plus an optional clap feature (Afi has #[cfg_attr(feature=clap, derive(ValueEnum))]); the bgp crate's clap feature now passes through. The migrated types' Rust paths (bgp::messages::TYPE) still resolve via pub use re-exports, so no downstream code changes beyond the call-site updates above. Verified all 11 blessed OpenAPI docs sha256-identical to baseline; cargo fmt --check, cargo clippy --workspace -- -D warnings, and cargo nextest (241 passed) all clean. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…rsions Move seven leaf wire-message types into bgp-types-versions::messages: Tlv, Header, Community, AddPathElement (v1) and BgpNexthop, Ipv6DoubleNexthop, ExtendedNexthopElement (v4). Header gains the MAX_MESSAGE_SIZE constant alongside its existing WIRE_SIZE associated const, both also re-exported from bgp::messages so existing call sites in bgp::connection_tcp continue to compile unchanged. Inherent methods on the moved types (Header::new/to_wire/from_wire, BgpNexthop::from_bytes/to_bytes/byte_len, ExtendedNexthopElement:: is_v4_over_v6/is_v6_over_v4) and their Display/From impls move to bgp-types-versions::impls::messages (orphan-rule case 1: every input type is std or already migrated). Header and BgpNexthop parse paths now produce WireError instead of bgp::Error; bgp::Error already grew a #[from] WireError variant in chunk 1, so ? propagation through the session-time call sites is unchanged. WireError gains an InvalidAddress(String) variant to cover BgpNexthop::from_bytes. bgp::messages keeps backward-compatible pub use re-exports so the crate-public Rust paths (bgp::messages::Header etc.) still resolve. The is_v4_over_v6/is_v6_over_v4 helpers become pub (they were private methods before, but only consumed from inside bgp/, so widening them is a no-op for downstream consumers). Verified all 11 OpenAPI documents fresh with no schema drift; cargo fmt --check, cargo clippy --workspace -- -D warnings, and cargo nextest (241 passed) all clean. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…s-versions Move the MP-BGP PathAttribute family into bgp-types-versions. The 12-variant PathAttributeTypeCode (incorrectly placed at v1::messages in chunk 1) moves to v4::messages where it belongs; a 10-variant V1 form is introduced at v1::messages with #[schemars(rename = "PathAttributeTypeCode")] to keep the v1 admin schema byte-identical. v4::messages additions: PathAttribute, PathAttributeType, PathAttributeValue, PathAttributeTypeCode (12-variant), Aggregator, As4Aggregator, As4PathSegment, MpReachNlri, MpReachIpv4Unicast, MpReachIpv6Unicast, MpUnreachNlri, MpUnreachIpv4Unicast, MpUnreachIpv6Unicast, and the path_attribute_flags constants module. v1::messages additions (the v1 wire-shape compat types, formerly named *V1 in bgp::messages): Prefix, PathAttribute, PathAttributeType, PathAttributeTypeCode, PathAttributeValue. These are re-exported from bgp::messages under their historical *V1 aliases to minimize call-site churn. impls/messages.rs gains Display impls for the new types, the self-contained inherent methods (Aggregator/As4Aggregator to_wire/from_wire/to_bytes; MpReach/MpUnreach afi/safi/nexthop/ ipv4_unicast/ipv6_unicast/is_empty/len), the v4 → v1 cross-version From conversions, and reabsorbed-into-From impls for path_attribute_type_code_of and From<PathAttributeValue> for PathAttribute. In bgp/src/messages.rs the type definitions become re-exports from bgp_types::messages. Wire methods that produce bgp::error::Error or UpdateParseErrorReason (PathAttribute::to_wire / from_bytes, PathAttributeType::to_wire / error_action, PathAttributeValue::to_wire / from_wire, As4PathSegment::to_wire / from_wire, MpReachNlri::to_wire / from_wire, MpUnreachNlri::to_wire / from_wire) become free fns (path_attribute_to_wire, path_attribute_from_bytes, path_attribute_type_to_wire, path_attribute_type_error_action, path_attribute_value_to_wire, path_attribute_value_from_wire, as4_path_segment_to_wire, as4_path_segment_from_wire, mp_reach_nlri_to_wire, mp_reach_nlri_from_wire, mp_unreach_nlri_to_wire, mp_unreach_nlri_from_wire). Internal call sites in bgp/src/messages.rs are updated. WireError::PathAttributeCode in bgp-types-versions::error switches from v1::messages::PathAttributeTypeCode (10-variant) to v4::messages::PathAttributeTypeCode (12-variant) since wire parsing operates on the MP-BGP form. Verification: cargo fmt --check; cargo clippy --workspace -- -D warnings; cargo nextest run --workspace (241 passed); cargo run -p xtask -- openapi check (11 fresh, 0 stale, 0 failed). All blessed mg-admin/ddm-admin schemas remain sha256-identical to baseline. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…nd to bgp-types-versions Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…rorSubcode to bgp-types-versions Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
… RouteRefreshMessage to bgp-types-versions Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…types-versions, drop UpdateMessage::errors field Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…versions Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…erParameters families to mg-types-versions Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…fo/FsmStateKindV1 to mg-types-versions Migrates the v1 and v2 forms of the PeerInfo family to mg-types-versions: - v1::bgp::FsmStateKind (was params::FsmStateKindV1, 7-variant pre-IPV6_BASIC). - v1::bgp::DynamicTimerInfo (was params::DynamicTimerInfoV1, 2-field). - v1::bgp::PeerTimers (was params::PeerTimersV1). - v1::bgp::PeerInfo (was params::PeerInfoV1). - v2::bgp::PeerInfo (was params::PeerInfoV2). - v4::bgp::DynamicTimerInfo (the latest 3-field form, re-exported as latest). Cross-version conversions in mg-types-versions/src/impls/bgp.rs: - From<bgp_types_versions::v2::session::FsmStateKind> for v1::bgp::FsmStateKind - From<v2::bgp::PeerInfo> for v1::bgp::PeerInfo The latest PeerInfo (v5 shape) and latest PeerTimers (v5 shape) remain in bgp/src/params.rs because their public fields embed BgpCapability, PeerCounters, and StaticTimerInfo, all of which are scheduled for sub-chunk 6c. mg-types-versions cannot depend on bgp, so those types must migrate together. Sub-chunk 6c will pull the remaining PeerInfo/PeerTimers latest forms across once the unversioned-but-published group lands. bgp/src/params.rs preserves the existing public surface via re-export aliases so call sites in mgd/mg-api are unchanged. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…d rdb tail to versions crates Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…ffixed convention Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…rse module Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…dules
Three orphan-rule-adjacent violations found in `mg-types-versions/src/impls/`:
- `From<latest::static_routes::StaticRoute{4,6}> for rdb::StaticRouteKey`
forced `mg-types-versions` to depend on `rdb` (a business-logic crate).
Replaced with free fns at the only call site (`mgd/src/static_admin.rs`).
- `From<rdb::db::Rib> for {latest, v1}::rib::Rib` had the same problem.
Replaced with `rib_latest_from_rdb` / `rib_v1_from_rdb` free fns in
`mg-types/src/rib.rs` (the facade crate, which already depends on both
`rdb` and `rdb-types-versions`).
`mg-types-versions` no longer depends on `rdb`. The Phase 3 audit
(`rg "use {bgp,rdb,mg_common,ddm}::"` and the corresponding `_types::`
sweep across all `*-types-versions/src/`) is now clean.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…or migrated paths Adds `docs/types-organization.md` as a tangible reference for where schema-published types now live: facade vs versions crate pairs, the crate map, a type -> crate quick reference, and recipes for adding new types and new API versions. Calls out the leaf-crate rule, the internal-vs-published distinction, and the boundary helpers (free functions in the facade crate or at call sites) used when both source and target of a conversion are foreign. `docs/bgp-architecture.md` and `docs/bgp-unnumbered.md` were scanned for stale type-path references and are unchanged: they reference types only by short name (`MessageHistory`, `FsmStateKind`, `Path`, `PeerId`), which remain valid via the facade re-exports. `README.md` was scanned for moved type paths and is unchanged. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…ports Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Adds a dedicated facade+versions crate pair for BFD types and migrates `BfdPeerState`, `BfdPeerConfig`, `BfdPeerInfo`, and `SessionMode` into `bfd-types-versions::v1`. This was the last leaf-crate-rule violation in `mg-types-versions` — its `bfd.workspace = true` dependency (and the transitive edge into `rdb`/`mg-common`) is now gone. Type relocations: - `BfdPeerState` (was `bfd::BfdPeerState`) -> `bfd_types_versions::v1`, with the `wire_format()` helper carried along as an inherent method. - `BfdPeerConfig` and `SessionMode` (was `rdb_types_versions::v1::bfd::*`) -> `bfd_types_versions::v1`. - `BfdPeerInfo` (was `mg_types_versions::v1::bfd`) -> `bfd_types_versions::v1`. `mg_types_versions::v1::bfd` keeps only `DeleteBfdPeerPathParams` (an admin-API path-params shape, not a BFD type proper). Call-site updates: `bfd`, `rdb`, `mg-api`, and `mgd` now import BFD types from the `bfd-types` facade. `bfd::BfdPeerState` continues to work via a `pub use` re-export. OpenAPI verification: `cargo run -p xtask -- openapi check` reports zero stale documents — all schemas (`BfdPeerState`, `BfdPeerConfig`, `BfdPeerInfo`, `SessionMode`) emit byte-identical JsonSchema output, so no client regeneration is required. Drive-by: also fixes a stale doc comment in `bgp/src/session.rs` that referred to the (since-renamed) `UpdateMessage::from_wire()` method; the actual function is the free fn `update_message_from_wire`. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Extends the per-domain test-* jobs to cover their corresponding type crates so any future tests added there get picked up automatically: - test-bfd: adds `bfd-types` and `bfd-types-versions`. - test-bgp: adds `bgp-types` and `bgp-types-versions` (alongside bgp); adds `mg-types` and `mg-types-versions` (alongside mgd). - test-rdb: adds `rdb-types` and `rdb-types-versions` (alongside rdb). Bare `-p rdb-types` is ambiguous because omicron transitively pins an upstream copy of rdb-types via mg-admin-client, leaving two rdb-types nodes in Cargo.lock. The Package ID Spec form `path+file://$PWD/../rdb-types` selects the local crate unambiguously. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Squashes adjacent same-prefix `pub use`/`use` lines into single
braced statements and lifts scattered branch-added re-exports to the
top of each file. Touches only items the branch already added or
moved; pre-existing imports and test-module imports are left alone.
- bgp/src/messages.rs: 20 scattered `pub use bgp_types::messages::*`
and `pub use bgp_types_versions::*` lines -> four consolidated
blocks at the top.
- bgp/src/session.rs: three scattered uses -> top of file; trims a
stale doc-comment block whose content is now redundant.
- bgp/src/params.rs: drops single-use `v{1,2,4,5,8}_bgp` aliases in
favor of fully-qualified `pub use mg_types_versions::vN::bgp::{...}`.
- mg-types/versions/src/latest.rs, mg-common-types/versions/src/latest.rs,
mg-types/src/rib.rs, bfd/src/lib.rs: minor adjacent-run merges.
OpenAPI verified byte-identical (`cargo xtask openapi check`: all 10
documents fresh). 152 of 152 bgp+bfd tests still pass.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
The migrating-to-versions-crate guide is explicit: within the impls/
module, always refer to types using floating latest:: identifiers.
Several files reached the latest types via the vN:: path they happen
to be defined in (v1::bgp::messages::*, v4::bgp::config::*, etc.) —
these silently glue the impls to a specific version and rot when the
type moves to a new version.
Rewrite use-statements in impls/ from vN:: to latest:: wherever the
two paths resolve to the same concrete type:
- messages.rs, bgp/error.rs, bgp/parse.rs: v1/v4 bgp::messages -> latest
- session.rs: v2::bgp::session::*, v4::bgp::messages::Message -> latest
- policy.rs: ImportExportPolicy{4,6} and Prefix -> latest
- bgp/mod.rs: v4 bgp::policy, bgp::config, rdb::neighbor, v1 rdb::prefix
-> latest; latest::bgp::NeighborSelector::to_peer_id return type
Kept as vN:: (legitimately distinct types or cross-version conversion
sources):
- v1::bgp::policy::ImportExportPolicy (latest is the v4 per-AF variant,
not the same type)
- v1::bgp::config::PeerInfo, v1::bgp::config::Neighbor inherent impls
(compat shapes still served by v1 endpoints; latest is v5/v8)
- impl From<v1::...> / From<v2::...> / From<v4::...> / From<v5::...>
cross-version conversions (sources must name their version)
OpenAPI schema unchanged; cargo check workspace passes.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
The migrating-to-versions-crate guide flags this as common-mistake #1: prior-version endpoints must name their types via vN:: paths, not the floating latest:: alias. Using latest:: here resolves to the same concrete type today but breaks the protective contract — when the selector type changes shape in a future API version, the latest:: binding silently follows, and the v5 endpoint's expected schema diverges from its signature with no compile error. Three v5 trait method signatures: - read_neighbor_v5: Path<latest::bgp::NeighborSelector> -> Path<v5::bgp::NeighborSelector> - read_neighbors_v5: Path<latest::bgp::AsnSelector> -> Path<v1::bgp::config::AsnSelector> (AsnSelector lives in v1) - delete_neighbor_v5: Path<latest::bgp::NeighborSelector> -> Path<v5::bgp::NeighborSelector> Three call-site constructions in v4/v1 default methods that forward to read/delete_neighbor_v5: - latest::bgp::NeighborSelector { .. }.into() -> v5::bgp::NeighborSelector { .. }.into() OpenAPI schema unchanged; workspace check passes. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Per the RFD 619 migrating-to-versions-crate guide: "Prior versions
have an operation_id set to endpoint_name." The unnumbered_neighbor
family, bgp_apply, and static_list_v{4,6}_routes already carried
this; the rest of the prior-version endpoints (neighbor v{1,4,5},
clear_neighbor_v1, get_exported_v{1,5}, get_rib_imported_v2,
get_imported_v1, get_rib_selected_v2, get_selected_v1,
get_neighbors_v{1,2,4}, message_history_v{1,2,4}, fsm_history_v2,
read/update_bestpath_fanout_v1) relied on dropshot's implicit
operation_id derivation.
Adds operation_id = "<base>" to 31 endpoint attributes, where
<base> is the method name with the trailing _vN stripped. The
generated schema is byte-identical (cargo xtask openapi check passes
with 12 fresh documents) — this just promotes a previously implicit
convention into the source so the rule is visible at every call site
and merge conflicts on version-range changes don't silently drop it.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Per RFD 619 § "Re-export types from latest into the types crate": each types crate "acts as a facade for the latest versions." The facades had no //! doc explaining this; readers landing in mg-api-types/src/lib.rs (or ddm-api-types) only saw bare `pub mod` declarations with no clue why the crate exists, when to depend on it vs. the -versions crate, or where the actual definitions live. Adds a short crate-level doc to each facade root pointing at the versions crate and RFD 619. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Same class as 28d9b43 (drop latest:: from v5 prior-version endpoint signatures), four sites missed by that pass: - read_unnumbered_neighbors_v5: Query<latest::bgp::AsnSelector> -> Query<v1::bgp::config::AsnSelector> - read_unnumbered_neighbor_v5: Query<latest::bgp::UnnumberedNeighborSelector> -> Query<v5::bgp::UnnumberedNeighborSelector> - delete_unnumbered_neighbor_v5: Query<latest::bgp::UnnumberedNeighborSelector> -> Query<v5::bgp::UnnumberedNeighborSelector> - get_neighbors_v4: HashMap<IpAddr, latest::bgp::PeerInfo> -> HashMap<IpAddr, v5::bgp::PeerInfo> Mirror the trait change in mgd's MgAdminApi impl and the bgp_admin::get_neighbors_v4 helper so the prior-version impl signatures exactly match the trait (common-mistake #5 in the migrating-to-versions-crate guide). OpenAPI schema unchanged. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
taspelund
force-pushed
the
trey/rfd_619_wrapup
branch
from
c59c854 to
cf82beb
Compare
Contributor
Author
|
@sunshowers thank you for reviewing! I believe all your feedback has been addressed. I took your advice and used your prompt locally, which ended up finding a handful of other items that are now addressed too |
sunshowers
reviewed
May 13, 2026
Contributor
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
Just a few more comments, thanks.
Per RFD 619 "Each published type is defined in the earliest version it is present in": PeerInfo and PeerTimers live under v5 (unnumbered/bgp.rs) but their current wire shape was first introduced in v4 (MP_BGP) and carried forward unchanged into v5. The v4 endpoint get_neighbors_v4 already serves this exact shape, so the type's true first-introduction version is v4, not v5. The placement under v5 is a leftover from the unnumbered work where both lived in the same file as the newly-added UnnumberedNeighborSelector/UnnumberedNeighbor. Mechanical move: - PeerInfo, PeerTimers: unnumbered/bgp.rs -> mp_bgp/bgp/config.rs - latest.rs: re-export from v4::bgp::config instead of v5::bgp - mg-api trait, mgd impl + helper: get_neighbors_v4 signature now names v4::bgp::config::PeerInfo, matching its v4 method-suffix Type definitions are byte-identical; the only field reference that crosses modules is PeerInfo.timers: PeerTimers (now same-module via super::). All four PeerTimers field types (DynamicTimerInfo, StaticTimerInfo, JitterRange) already live in mp_bgp/bgp/config.rs. OpenAPI schema unchanged. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Per RFD 619 "Versions crates re-export the latest versions of each
type": within each module of latest.rs, re-exports must be grouped by
version, ascending, with blank-line separators (rationale: makes the
originating version obvious at a glance and forces merge conflicts on
colliding changes — RFD 619 §rationale-reexports).
The previous "high-traffic submodule types hoisted for convenience"
block at the top of latest::rdb violated that property — those re-exports
weren't grouped by version (they couldn't be: they're grouped by
submodule), which broke uniformity with the rest of latest.rs and with
latest::bgp's messages/session submodules (which are not hoisted).
Drop the hoists from latest::rdb. To preserve ergonomic
mg_api_types::rdb::Prefix etc. access for business logic, add the
flattening to the mg-api-types facade instead — the facade isn't
version-grouped to begin with, so flattening there carries no RFD-619
cost.
mg-admin-client's progenitor replace = {} block names the previously
hoisted types directly via versions paths (per RFD 619 §
determinations-clients: clients use floating identifiers from the
versions crate's latest module); update those three entries to the
new submodule-qualified paths (latest::rdb::prefix::Prefix, etc.).
No business-logic call sites change (the facade keeps the flat names).
OpenAPI schema unchanged.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <t.k.aspelund@gmail.com>
taspelund
force-pushed
the
trey/rfd_619_wrapup
branch
from
cf82beb to
cd5023f
Compare
Contributor
Author
|
Alrighty, the latest rounds of comments have been addressed. Claude believes we are fully compliant with RFD 619 😁 |
sunshowers
reviewed
May 14, 2026
Contributor
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
Almost! This is all I could find.
…<->v4 conversions
Resolves three RFD 619 placement violations flagged in the review:
1. `v2::bgp::session` referenced `v4::bgp::messages::Message` for its
`MessageHistory{,Entry}` types. Per RFD 619 ("types live in the
first version they were defined in"), the post-MP_BGP wire shape of
`MessageHistory` was first introduced at v4 (when `Message` changed
shape) and only carried through v5 unchanged. Move
`MessageHistory`, `MessageHistoryEntry`, and `MAX_MESSAGE_HISTORY`
from `v2::bgp::session` to a new `v4::bgp::session` module. Same
class of fix as the earlier PeerInfo/PeerTimers relocation.
2. Two distinct `MessageHistory` types coexisted in v2 (one in
`session.rs`, one in `history.rs`) — the session one is the latest
wire shape and moved to v4 per (1); the history one is the genuine
v2-era wire shape (carries the v2-local `Message`/`UpdateMessage`,
whose `UpdateMessage` uses the V4/V6 rdb `Prefix` enum rather than
the v1 wire-format length+octets struct) and stays in v2.
3. v2<->v4 cross-version conversions previously living in v2:
- `From<v4::session::MessageHistory{,Entry}> for v2::history::*` →
moved to v4::bgp::session.
- `From<v4::messages::Message> for v2::history::Message` and
`From<v4::messages::UpdateMessage> for v2::history::UpdateMessage`
→ moved to v4::bgp::messages.
Per RFD 619, cross-version conversions live with the newer version.
Update `latest.rs` to re-export `MessageHistory{,Entry}` and
`MAX_MESSAGE_HISTORY` from v4. Update in-crate consumers
(`v4::bgp::config::MessageHistoryResponse`, `v5::bgp::*HistoryResponse`)
to import from the new `v4::bgp::session` location.
OpenAPI schema unchanged (`cargo xtask openapi check` clean across all
12 generated documents).
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Per RFD 619, the impls module is reserved for functional code (Display, FromStr, inherent helpers) on the latest types and refers to those types via floating `latest::` identifiers. Cross-version conversions belong in the version module that owns the newer of the two types. `latest::rib::Rib` resolves to `v5::rib::Rib` (defined in `unnumbered/rib.rs`), so the existing `impl From<latest::rib::Rib> for v1::rib::Rib` downgrade was misplaced in `impls/rib.rs`. Move the impl alongside the v5 `Rib` definition; this also removes the only file in `impls/` (previously `pub(crate) mod rib;`) whose contents were purely version conversions. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
…into v4
Same class of fix as the previous `impls/rib.rs` move. The impls module
is for functional code (Display, FromStr, inherent helpers) referring to
types via floating `latest::` identifiers; cross-version conversions
belong in the version module that owns the newer of the two types.
`From<MessageHistory> for v1::bgp::session::MessageHistory` and
`From<MessageHistoryEntry> for v1::bgp::session::MessageHistoryEntry`
in `impls/session.rs` were downgrades from the latest history shape
to v1. After the previous commit moved `MessageHistory{,Entry}` to
`v4::bgp::session`, these impls now naturally belong there too —
move them alongside the latest type definitions. The functional impls
(`Display`, `ConnectionId::new`, `MessageHistory::receive`/`send`,
etc.) stay in `impls/session.rs`.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
taspelund
force-pushed
the
trey/rfd_619_wrapup
branch
3 times, most recently
from
d16ec4f to
c1311a2
Compare
Contributor
Author
|
Alrighty, latest is up for review. There's only one thing that was flagged by claude in the latest run, and it's not entirely clear what the "right" move is. Do you have any opinions?
|
taspelund
force-pushed
the
trey/rfd_619_wrapup
branch
from
c1311a2 to
917ebb2
Compare
Contributor
Ideally these would be mirrored and consistent. i.e. for every identifier, replacing |
taspelund
force-pushed
the
trey/rfd_619_wrapup
branch
from
917ebb2 to
c63f374
Compare
This change finishes the RFD 619 migration by clearing the last
structural deviations from the spec and reorganizing the `impls/`
module to match the canonical layout.
Versions-crate restructure (originally framed as "move v4->v1
wire-message downgrades and v1 Prefix conversion out of impls/"):
- `impls/messages.rs`, `impls/peer.rs`, `impls/session.rs` → moved
under `impls/bgp/` so the `impls/` tree mirrors `latest::*`.
- `impls/path.rs`, `impls/prefix.rs` → moved under `impls/rdb/`.
- `impls/static_routes.rs` → deleted; its single From impl moved into
the v2 `static_routes` module where it belongs.
- Cross-version conversions (v4->v1 `Message`/`UpdateMessage`/
`PathAttribute*` downgrades, `v1::rdb::Prefix` → `v1::bgp::messages::
Prefix` conversion) moved out of `impls/` into the newer version's
module per RFD 619 §"each version module contains code to convert
from/to one prior version".
- `MessageHistory`/`MessageHistoryEntry` post-v4 shape relocated from
`v2` into `v4`; v2 retains its original wire shape.
- `latest::Rib` -> `v1::Rib` downgrade moved from `impls/rib.rs` into
`v5/rib.rs`.
Review feedback (from a fresh RFD 619 audit of the branch):
- B: documented why `v4::UpdateMessage::errors` references
`crate::impls::bgp::parse::*` (the one acknowledged exception to
"version modules only reference versioned identifiers" — the field
is `#[serde(skip)] #[schemars(skip)]` and exists purely for in-process
RFC 7606 signaling).
- C: split the inline `impl latest::bgp::{Neighbor,UnnumberedNeighbor,
NeighborSelector,NeighborResetRequest}` blocks out of `impls/bgp/
mod.rs` into a dedicated `impls/bgp/neighbor.rs`, leaving the mod
file as `mod`-decls-only to match `impls/rdb/mod.rs` and the RFD's
template.
- D: `impls/bgp/messages.rs` now imports `WireError`/`MessageConvertError`
via `crate::latest::bgp::error::*` instead of `crate::impls::bgp::
error::*` — within `impls/` types are referred to using floating
`latest::` identifiers.
- F: fixed stale `rdb_types::` doc-test imports and a broken intra-doc
link to `crate::BgpPathProperties` (now points at
`crate::v5::rdb::path::BgpPathProperties`).
- G: wired up `ddm-admin-client`'s progenitor `replace = {}` block
against `ddm_api_types_versions::latest::net::*`; deleted the
hand-written `PartialEq`/`Eq`/`Hash` impls that compensated for
duplicated auto-generated types. Updated `ddm_admin_client::types::
TunnelOrigin` consumers in `mg-lower`, `tests`, and `ddmadm` to
import from `ddm_api_types_versions::latest::net` directly.
- H: added per-cluster rationale comments to the required-method
endpoints in the API trait (v1 neighbor CRUD, get_exported_v{1,5},
get_rib_imported/selected_v{1,2}, get_neighbors_v{1,2,4},
message_history_v{1,2,4}/fsm_history_v2) explaining why each cannot
be a provided method (each draws response data from `Self::Context`
with a per-version peer-identity shape).
- I: replaced the wholesale `pub use crate::impls::bgp::error;` /
`parse;` module re-exports in `latest.rs` with explicit `pub mod {
error,parse } { pub use ...; }` blocks listing each public item by
name. The underlying modules in `impls/bgp/` are now `pub(crate) mod`
rather than `pub mod` — reachable internally for the UpdateMessage
errors-field embedding, but externally only via the curated
`latest::bgp::{error,parse}` namespace.
- J: corrected `docs/types-organization.md` paragraphs that misstated
the locations of `parse.rs`/`error.rs` (under `impls/bgp/`, not the
crate root) and the public path of the wire-parse helpers
(`latest::bgp::{parse,error}`, not `mg_api_types_versions::parse`).
Strict-RFD findings (deviations beyond the original review scope):
- A: same-version references inside `vN/` modules now use `super::`
instead of `crate::vN::`, matching the migration guide's prescription.
The branch had been using `crate::vN::` uniformly for both same-version
and prior-version refs, which collapses the RFD's intentional
semantic distinction (super:: = "same version, whichever that is";
crate::vM:: = "specifically version M"). Touched ~14 use-statements
and 4 inline references across `initial/`, `ipv6_basic/`, `mp_bgp/`,
and `unnumbered/`.
- E: removed the flat re-exports in `mg-api-types/src/rdb.rs` that
added a parallel access path (`mg_api_types::rdb::Prefix` *and*
`mg_api_types::rdb::prefix::Prefix` both resolved to the same type).
The facade now matches every other domain module: a single
`pub use ...latest::rdb::*;` line. Updated ~30 call sites across
`bgp/`, `mgd/`, `rdb/`, `mgadm/`, `mg-lower/`, and `falcon-lab/`
to use the canonical submodule paths (`mg_api_types::rdb::prefix::*`,
`mg_api_types::rdb::path::*`, `mg_api_types::rdb::neighbor::*`,
`mg_api_types::rdb::router::*`).
Documentation update:
`docs/types-organization.md` now describes the strict-RFD structure as
a faithful summary rather than documenting deviations: the
"two structural rules" passage says `super::` for same-version /
`crate::vM::` for prior-version (item A); the facade section says
"no flat re-exports — the canonical path through the facade mirrors
the canonical path through latest" (item E); the type quick-reference
table lists `mg_api_types::rdb::prefix::Prefix` etc. as the canonical
access paths.
OpenAPI: v9 schema regenerated to pick up corrected doc comments on
`PeerId` and `AddressFamily` (now reference `mg_api_types::*` instead
of the deleted `rdb_types` crate). No wire-level changes.
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
taspelund
force-pushed
the
trey/rfd_619_wrapup
branch
from
c63f374 to
0c9a04c
Compare
Contributor
Author
Ok, this is now in place |
sunshowers
approved these changes
May 15, 2026
Contributor
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
Thank you! I know this has been a lot
Contributor
Author
|
Thank you for all the rounds of review! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Many types were not fully migrated to the RFD 619 conventions in #611 and #665. This PR seeks to remedy that.
As a whole, this PR is simply a re-organization of existing code rather than a functional change.
It does so by splitting some crates apart (introducing
$CRATE-typesand$CRATE-types-versionsfor it) and attempting to bring some order to this chaos. In some cases, types were moved into a more fitting home (e.g. BFD types moving out of rdb/mgd crates and into bfd-types).The exception to no functional changes is that the operation_id in the OpenAPI spec does change for a handful of methods.
These are all broken out into a new dropshot API version for mg-api:
ENDPOINT_RENAME.This results in clients seeing a different method name when importing the latest API version via mg-admin-client.
Aside from the method/operation_id name change, the API remains byte-compatible.
I had claude help with the grunt work on this one, and the commit messages describe the implementation plan it followed (including some course corrections).