Introduce stronger types for link-local addresses and unnumbered peers

[jgallagher]

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.

[jgallagher]

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)

[taspelund]

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

[jgallagher]

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?

[internet-diglett]

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).

[jgallagher]

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.

[jgallagher]

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

[jgallagher]

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

[internet-diglett]

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

[jgallagher]

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]

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.

[bnaecker]

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

[jgallagher]

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.

[internet-diglett]

Thank you so much for volunteering to do this! Even if we still need to tweak a name here or there, the overall result already seems much clearer and removes a lot a sharp corners.

[internet-diglett]

This is probably a good idea. I guess we'd need a migration to convert the existing diasallowed values to NULL as well.

[jgallagher]

@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]

@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!