Introduce stronger types for link-local addresses and unnumbered peers by jgallagher · Pull Request #10082 · oxidecomputer/omicron

jgallagher · 2026-03-18T01:27:29Z

This is a big chunk of #9832. Stealing from the doc comments in sled-agent/types/versions/src/stronger_bgp_unnumbered_types/early_networking.rs, the primary changes in this PR are:

Introduce SpecifiedIpNet, a newtype wrapper around IpNet that does not allow unspecified IP addresses.
Introduce SpecifiedIpAddr, a newtype wrapper around IpAddr that does not allow unspecified IP addresses.
Introduce UplinkAddress, a stronger type for specifying possibly-link-local IP nets. This is the new type of UplinkAddressConfig::address, which was previously an Option<IpNet> where both None and Some(UNSPECIFIED) were treated as link-local.
Introduce RouterPeerAddress, a stronger type for specifying possibly-unnumbered BGP peer addresses. This is the new type of BgpPeerConfig::addr, which was previously an IpAddr where an unspecified address was treated as unnumbered.

The rest of the changes are fallout from those: adding new types for any the that contains UplinkAddressConfig or BgpPeerConfig, since those changed, and updating all the places that create or consume any of those types. I'm hoping this PR is pretty straightforward to review despite its size, because much of the size is either tests or all the noise of redefining a bunch of big structs with a single field changed.

The two main things I'd consider part of #9832 that are NOT addressed in this PR:

Database representation; the columns where we store these values still allow NULL, 0.0.0.0, or ::. Fixing this will require a db migration, so I want to do that separately.
I didn't touch the external API. My sense is that it would be good to apply these stronger types there too, but I'll defer to @internet-diglett or @rcgoodfellow for that - I'm happy to do the work if it should be done.

A third thing we could consider is whether to push this stronger typing down to maghemite too.

For now, in all these cases we convert to or from the stronger types primarily through obnoxiously-long method names that should stick out like sore thumbs (RouterPeerAddress::from_optional_ip_treating_unspecified_as_unnumbered() and friends). This should make it obvious where we're switching from strong types to weaker or vice versa.

There are a couple of breaking changes in how we specify RSS configs. I'll leave a couple comments below with more details.

…red-types-1

jgallagher · 2026-03-18T01:31:22Z

+impl UserSpecifiedUplinkAddressConfig {
+    /// String representation for [`UplinkAddress::LinkLocal`] when
+    /// serializing/deserializing [`UserSpecifiedUplinkAddressConfig`].
+    pub const LINK_LOCAL: &str = "link-local";


This is the first breaking change w.r.t. RSS. In the RSS config files passed to wicket, specifying a link-local uplink address now requires:

address = "link-local"

instead of passing "0.0.0.0" or "::"; both of those will now be rejected by wicket (with an error message suggesting "link-local" instead).

There's a similar change below for BGP peer addresses, where we're now required to say

addr = "unnumbered"

instead of "0.0.0.0" or "::" or omitting it entirely (all of which are similarly rejected with an error message suggesting "unnumbered").

(cc @taspelund who suggested "unnumbered" specifically)

One clarification on "link-local": do we support (or plan to support) explicit link-local uplink addresses? Or do/will we only support auto-derived link-local uplink addresses?

It seems like the current use of "link-local" is meant to represent an auto derived address, but the terminology seems like it would be slightly at odds with an explicit link-local address.
i.e.
"link-local" makes it seem like the only alternative is a routable address. But if we allow someone to configure a specific link-local address (e.g. address = fe80::beef) then the "link-local" string seems less meaningful since it doesn't quite capture the full nuance of it being both link-local and auto derived.

Hopefully I'm making sense and not just rambling

A good question but not for me 😅. A couple options here in terms of this representation:

UplinkAddress::Address { .. } could disallow link-local addresses in the inner IpNet just like it disallows 0.0.0.0/n and ::/n on this branch, although I'm not sure what to name the inner type if we do that; SpecifiedIpNet seems okay for "IpNet that isn't using an unspecified addr", but I don't know what to call "IpNet that isn't using an unspecified addr or a link-local address"

UplinkAddress::Address { .. } should allow explicit link-local addresses, and we rename UplinkAddress::LinkLocal to something like UplinkAddress::AutoLinkLocal

Is one of those correct in terms of what we want to support?

do we support (or plan to support) explicit link-local uplink addresses? Or do/will we only support auto-derived link-local uplink addresses?

Not sure I'm the correct person to answer this either, but looking at illumos today it would seem that we do not support explicit link local addressing, since the addrconf address type does not appear to allow an explicit address, and if you create a static ipv6 it automatically creates an addrconf link local address under the hood.

ipadm create-addr [-t] -T static [-d] -a [local|remote=]addr[/prefixlen]... addrobj Create an address on the specified IP interface using static configuration. The address will be enabled but can disabled using the ipadm disable-addr subcommand. Note that addrconf address configured on the interface is required to configure static IPv6 address on the same interface. This takes the following options: -a,--address Specify the address. The local or remote prefix can be used for a point-to-point interface. In this case, both addresses must be given. Otherwise, the equal sign ("=") should be omitted and the address should be provided by itself without second address. -d,--down The address is down. -t,--temporary Temporary, not persistent across reboots. ... ipadm create-addr [-t] -T addrconf [-i interface-id] [-p {stateful|stateless}={yes|no}]... addrobj Create an auto-configured address on the specified IP interface. This takes the following options: -i,--interface-id Specify the interface ID to be used. -p,--prop Specify which method of auto-configuration should be used. -t,--temporary Temporary, not persistent across reboots.

Something that might be worthwhile to distinguish is that this address configuration is creating a v6 link-local address, because technically there is a v4 link-local address space too (169.254.0.0/16).

Two followup questions:

Should I rename the UplinkAddress::LinkLocal variant to something like UplinkAddress::Ipv6LinkLocal, UplinkAddress::AddrConf, or UplinkAddress::Ipv6AddrConf instead?

Should I rename the SpecifiedIpNet to ... something ... and make it reject both unspecified addresses and ipv6 link local addresses? (And also ipv4 link local addresses?) If so, is it okay if this rejection is based on the std lib's Ipv6Addr::is_unicast_link_local() as opposed to something different from that (is there a "non-unicast link local"?)?

My gut feeling is that renaming the variant for clarity is an easy win, if there's a more suitable name than just LinkLocal. I'm a lot less sure about rejecting link-local addresses in the other variant.

Plot twist - I'm not sure link-local addresses work at all, currently (#9832 (comment)).

Ok they do work but only if there's an address lot that includes the address ::. Filed #10103.

I vote for UplinkAddress::AddrConf (or Addrconf?)

Addrconf is ipv6 specific

It's what is actually happening under the hood

It implies automatic configuration based on the description from the IETF

It also happens to be shorter

Summarizing an offline chat with @internet-diglett and @taspelund, our proposal is:

enum UplinkAddress { AddrConf, Static { ip_net: UplinkIpNet }, }

where UplinkIpNet has several validation checks beyond just "not unspecified" (e.g., also reject ipv6 link local and multicast addrs); copy these from maghemite (v4 checks, v6 checks).

jgallagher · 2026-03-18T01:35:53Z

 routes = [{nexthop = "192.168.1.199", destination = "0.0.0.0/0"}]
 # Addresses associated with this port.
-addresses = [{address = "192.168.1.30/24"}]
+addresses = [{address = {type = "address", ip_net = "192.168.1.30/24"}}]


This is the second breaking change w.r.t. RSS configs - in the TOML files that specify the full config (that are only used in development, which does include a4x2), we now have to use the somewhat-more-annoying tagged representation for uplink addresses and BGP peer addresses, since these are now enums instead of Option<IpAddr> or Option<IpNet>.

We could make this use the more flexible parsing we're using now in wicket, but I didn't love that because that would affect our OpenAPI schema for real RSS config handoffs from sled-agent to Nexus, and eventually the external API if we reuse these types there. The wicket representation shows up in the OpenAPI spec as just "string", which happens to have a bunch of rules around it that can't be easily expressed in OpenAPI (e.g., "must be either unnumbered or an IP address other than 0.0.0.0 or ::"). The wicketd OpenAPI spec does have this problem, but it's only used by wicket, not the rest of the control plane or the external API.

We could potentially define different types for "reading config-rss.toml from disk" and "to use in OpenAPI", but that seemed like a bunch of duplication that is maybe even more confusing than the two different types of formatting. Feedback very welcome.

Are we tracking making this change to the lab configs, or customer versions and templates for them?

This change doesn't apply to any of the "real" RSS configs, for labs or customers; the only breaking change for those is #10082 (comment), which only applies to link-local addresses or unnumbered peers ("regular" IPs still parse the same way in the real path). I don't think any of the lab configs use those; I'll ask around about customer templates once this is ready to land.

…red-types-1

jgallagher · 2026-03-20T14:52:19Z

@taspelund @internet-diglett I believe all the changes we discussed are in place. The names of the new types introduced are now:

pub enum UplinkAddress {
    AddrConf,
    Static { ip_net: UplinkIpNet },
}

pub enum RouterPeerType {
    Unnumbered { router_lifetime: v20::RouterLifetimeConfig },
    Numbered { ip: RouterPeerIpAddr },
}

where UplinkIpNet and RouterPeerIpAddr both reject:

loopback addresses
multicast addresses
unspecified addresses
IPv4 broadcast address
IPv6 unicast link local addresses

There are a bunch of changes to make this happen, but I tried to isolate the stuff that really needs another look into its own commits:

a2d9387 moves router_lifetime from BgpPeerConfig into RouterPeerType::Unnumbered. This required changing some of the helper methods and implemented Levon's suggestion of splitting out a different type for wicket (UserSpecifiedRouterPeerAddr) so we can keep user-friendly TOML.
c20e607 adds all the extra IP validation checks beyond just "not the unspecified addr"
fc93b58 adds a check (and test) for wicket specifically, that we reject a config that has a BGP numbered peer and a router_lifetime value. (This is an illegal state that can't be represented at all in the real enum, but the friendly-enum-for-TOML allows it, so we have to check for it at runtime.)

taspelund · 2026-03-21T00:34:58Z

@taspelund @internet-diglett I believe all the changes we discussed are in place. The names of the new types introduced are now:

pub enum UplinkAddress {
    AddrConf,
    Static { ip_net: UplinkIpNet },
}

pub enum RouterPeerType {
    Unnumbered { router_lifetime: v20::RouterLifetimeConfig },
    Numbered { ip: RouterPeerIpAddr },
}

where UplinkIpNet and RouterPeerIpAddr both reject:

* loopback addresses

* multicast addresses

* unspecified addresses

* IPv4 broadcast address

* IPv6 unicast link local addresses

There are a bunch of changes to make this happen, but I tried to isolate the stuff that really needs another look into its own commits:

* [a2d9387](https://github.com/oxidecomputer/omicron/pull/10082/commits/a2d9387b251b275f0719486f2d11ebab08e9d3d2) moves `router_lifetime` from `BgpPeerConfig` into `RouterPeerType::Unnumbered`. This required changing some of the helper methods and implemented Levon's suggestion of splitting out a different type for wicket (`UserSpecifiedRouterPeerAddr`) so we can keep user-friendly TOML.

* [c20e607](https://github.com/oxidecomputer/omicron/pull/10082/commits/c20e607b17f87c61fc8a81a359a98ed2736b2d29) adds all the extra IP validation checks beyond just "not the unspecified addr"

* [fc93b58](https://github.com/oxidecomputer/omicron/pull/10082/commits/fc93b589f9038586abfc2b76034082f565e14e8f) adds a check (and test) for wicket specifically, that we reject a config that has a BGP numbered peer and a `router_lifetime` value. (This is an illegal state that can't be represented at all in the real enum, but the friendly-enum-for-TOML allows it, so we have to check for it at runtime.)

I looked over these three commits and they seem good to me. Thanks for all the work John!

jgallagher · 2026-03-23T15:44:05Z

From chatting with @taspelund:

Pushed ee931a1 to also reject ipv4-mapped ipv6 addresses
Filed Static route nexthop addresses: Consider stronger types #10123 for applying these same restrictions to static route nexthop

internet-diglett

This looks great! Have we had a chance to kick the tires on this in a4x2 or on a racklette?

jgallagher · 2026-03-24T00:32:21Z

This looks great! Have we had a chance to kick the tires on this in a4x2 or on a racklette?

Yep! I tried both - static routing in a4x2, which worked fine but needed https://github.com/oxidecomputer/testbed/pull/283 for the stronger types in dev config-rss.toml files, and BGP in london, which also worked fine aside from minor infra issues unrelated to these changes https://github.com/oxidecomputer/rackletteadm/issues/108.

This is a followup to #10082, and extends the stronger `RouterPeerType` out from the internal API to the Nexus external API. The primary changes here are: * Change fields of `BgpPeer` to give stronger guarantees: * `BgpPeer::addr` is now `RouterPeerType` instead of `Option<IpAddr>` (which permitted three distinct representations of "unnumbered": `None`, `Some(0.0.0.0)`, and `Some(::)`). * `BgpPeer::router_lifetime` moved from being a top-level field to being nested inside the `RouterPeerType::Unnumbered` variant, and its type is now `RouterLifetimeConfig` instead of `u16`, adding enforcement of bounds. * Remove `BgpPeer::interface_name` (closes #10104). * Define new versions of types that transitively include `BgpPeer`: * `BgpPeerConfig` * `SwitchPortSettings` * `SwitchPortSettingsCreate` The 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.

ahl

I know I'm late, but wanted to leave a note for discussion this week

ahl · 2026-04-18T15:40:43Z

        "minimum": 0,
        "maximum": 9000
      },
+      "RouterPeerIpAddr": {


I think this showing up as its own type is arguably not what we ideally want

(note: one could suggest that we do want this type and to expose the restrictions on the IP address, but I think that's way way way too complex to express intelligibly in JSON Schema)

(note: one could suggest that we do want this type and to expose the restrictions on the IP address, but I think that's way way way too complex to express intelligibly in JSON Schema)

Yeah, I felt the same - the intent here was to tell schmars that this is "just" an IP address (but still have all the extra checking during deserialization). I thought #[schmars(with = ...)] was the best way to do that, and the type name showing up was an unfortunate side effect. ✔️ on switching to schemars(transparent) in #10286

ahl · 2026-04-18T15:52:42Z

+// detailed error messages we produce. We manually implement Deserialize per
+// https://github.com/serde-rs/serde/issues/2211#issuecomment-1627628399.
+#[serde(into = "IpAddr")]
+#[schemars(with = "IpAddr")]


Does this do anything? schemars respects the serde into attribute

IIRC it didn't really, because there wasn't a corresponding from or try_from on the serde attribute (because of the by-hand Deserialization).

ahl · 2026-04-19T01:03:57Z

+    fn try_from(ip: IpAddr) -> Result<Self, Self::Error> {
+        let err = match ip {
+            IpAddr::V4(ipv4) => {
+                // Perform the same validity checks we require in maghemite.


would this make sense to live in oxnet?

Probably! IIRC from chatting with @taspelund the idea was to make sure these were sensible in omicron, then move them to oxnet to reuse them in maghemite when someone has time to do similar cleanup work there.

jgallagher added 26 commits March 16, 2026 11:08

wip: introduce stronger unnumbered types

319605b

use new stronger types in sled-agent API

79ba415

add more briding helper methods

683343b

helper method for optional IP conversion

ecf4f66

clean up wicket interface

3ce05bb

unused imports

823ed21

fixups from rebase

97bd63e

openapi

ffc8114

address some TODOs

25ca92c

comment cleanup

64da05a

SpecifiedIpNet::addr(): return SpecifiedIpAddr

c696e2f

Merge branch 'main' into john/stronger-unnumbered-types-1

2e9fa2b

better wicket parsing + unit tests

87dcf69

make wicket example config more varied

b4ed2a2

Merge remote-tracking branch 'origin/main' into john/stronger-unnumbe…

40b7264

…red-types-1

fix display of uplink addresses (include subnet)

7aaf046

constants over string literals

7c92647

macro to reduce copy/paste in body -> envelope conversions

8e37371

add test for new early network blob format

4aca700

fix test RSS TOML files

1f0336b

add proptests for specified IP parsing and deserialization

902f7e4

Merge branch 'main' into john/stronger-unnumbered-types-1

cfc3611

add test for UplinkAddress::to_uplinkd_smf_property()

3a44f89

add tests for type conversion round trips

f5449ad

openapi

039ae86

typo

8c72b83

jgallagher requested review from bnaecker and internet-diglett March 18, 2026 01:27

jgallagher commented Mar 18, 2026

View reviewed changes

jgallagher added 12 commits March 20, 2026 00:50

rename methods link_local -> addrconf

7174d49

prefer ::default() over ::new(0).unwrap()

80fbaed

clippy

7d486b5

fix now-invalid IPs in tests

424d961

consistent error messages

dd2df67

fix missed renames in comments

d204c72

typo

372e667

wicket: add guard and test for numbered peer + router_lifetime

fc93b58

more context in error logs

cddf59a

check more invalid router peer addrs

dffe1fb

fix config-rss.toml address format in helios/deploy

d243fab

Merge remote-tracking branch 'origin/main' into john/stronger-unnumbe…

eeb1d3f

…red-types-1

jgallagher requested review from internet-diglett and taspelund March 20, 2026 14:52

jgallagher added 2 commits March 20, 2026 13:21

serde(rename) so we can use addrconf instead of addr_conf

665fd14

Merge branch 'main' into john/stronger-unnumbered-types-1

cabe1f6

jgallagher mentioned this pull request Mar 23, 2026

Nexus external API: Use stronger types for BgpPeer addresses #10122

Merged

also reject ipv4-mapped ipv6 addresses

ee931a1

jgallagher mentioned this pull request Mar 23, 2026

Static route nexthop addresses: Consider stronger types #10123

Open

bnaecker mentioned this pull request Mar 23, 2026

Add support for ICMPv6 protocol filter in firewall rules #10101

Merged

internet-diglett approved these changes Mar 24, 2026

View reviewed changes

jgallagher merged commit 04dae50 into main Mar 24, 2026
18 checks passed

jgallagher deleted the john/stronger-unnumbered-types-1 branch March 24, 2026 01:21

ahl reviewed Apr 19, 2026

View reviewed changes

Conversation

jgallagher commented Mar 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taspelund Mar 18, 2026 • 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!

jgallagher Mar 19, 2026 • 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!

jgallagher Mar 19, 2026 • 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!

jgallagher commented Mar 20, 2026

Uh oh!

taspelund commented Mar 21, 2026

Uh oh!

jgallagher commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

internet-diglett left a comment

Choose a reason for hiding this comment

Uh oh!

jgallagher commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ahl left a comment

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!

5 participants

taspelund Mar 18, 2026 •

edited

Loading

jgallagher Mar 19, 2026 •

edited

Loading

jgallagher Mar 19, 2026 •

edited

Loading

jgallagher commented Mar 23, 2026 •

edited

Loading

jgallagher commented Mar 24, 2026 •

edited

Loading