Skip to content

Use enum instead of sentinel :: for addrconf uplink addresses#10700

Open
jgallagher wants to merge 5 commits into
mainfrom
john/uplink-address-type
Open

Use enum instead of sentinel :: for addrconf uplink addresses#10700
jgallagher wants to merge 5 commits into
mainfrom
john/uplink-address-type

Conversation

@jgallagher

Copy link
Copy Markdown
Contributor

This is a resurrected branch from the post-R18 cleanup work I was doing a few months ago that got sidelined for various reasons, and is a continuation of the work done in #10122, #10155, and related PRs.

This overlaps some with @sunshowers's existing stack of cleanup, and it was specifically #10650 (comment) that reminded me this work wasn't done. :(

Like the other cleanup work, this only tackles the API side of things: a separate PR will be needed to tighten up the database representation.

A correctness concern with this change: UplinkAddress enforces a bunch of "don't allow special classes of IP addresses", including "no loopback", "no multicast", etc.:

IpAddr::V4(ipv4) => {
// Perform the same validity checks we require in maghemite.
// We deliberately do not flag Class E (240.0.0.0/4) or
// Link-Local (169.254.0.0/16) ranges as invalid, as some
// networks have deployed these as if they were standard
// routable unicast addresses, which we need to handle.
if ipv4.is_loopback() {
InvalidIpAddrError::LoopbackAddress
} else if ipv4.is_multicast() {
InvalidIpAddrError::MulticastAddress
} else if ipv4.is_broadcast() {
InvalidIpAddrError::Ipv4Broadcast
} else if ipv4.is_unspecified() {
InvalidIpAddrError::UnspecifiedAddress
} else {
return Ok(Self(ip));
}
}
IpAddr::V6(ipv6) => {
// As above, perform validity checks we require in maghemite.
if ipv6.is_loopback() {
InvalidIpAddrError::LoopbackAddress
} else if ipv6.is_multicast() {
InvalidIpAddrError::MulticastAddress
} else if ipv6.is_unspecified() {
InvalidIpAddrError::UnspecifiedAddress
} else if ipv6.is_unicast_link_local() {
InvalidIpAddrError::Ipv6UnicastLinkLocal
} else if ipv6.to_ipv4_mapped().is_some() {
// switch to ipv6.is_ipv4_mapped() once it's stabilized
InvalidIpAddrError::Ipv4MappedIpv6
} else {
return Ok(Self(ip));
}
}

The comment there is "to be consistent with maghemite", but in this PR, we add this same validation to types that go to dendrite instead. I'm assuming we should do the same validation, but that might be wrong - in particular, it seems a little weird to reject is_loopback() addresses in a type called LoopbackAddress, but maybe those are two different meanings of "loopback" (I'm not sure how it would be meaningful for an external API request to tell us to do something on 127.0.0.1 or ::1, but I'm regularly surprised!)? If we should not be doing this validation for the types changed here (LoopbackAddress, Address, and SwitchPortAddressView), I can change these from using UplinkAddress to... some other new enum that is still an Addrconf / Static { ipnet } pair but which takes a plain vanilla oxnet::IpNet instead of one that has these restrictions.

@jgallagher jgallagher added the networking Related to the networking. label Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

networking Related to the networking.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant