Stronger types for BGP unnumbered peer addressing

[jgallagher]

(This is followup from review comments on #9570 that can be deferred to after R18, but I think we should consider it tech debt that we should pay down as soon as is reasonable. I believe addressing it is blocked by #9801.)

#9570 introduces support for BGP unnumbered peers, which have no address. This is mostly expressed by using an unspecified address (0.0.0.0 or ::, depending ipv4/ipv6) to mean "no address". In some spots we use an Option<IpAddr> instead, where None means "no address", but the inconsistency is unfortunate:

• If the address isn't optional, any user of the address that needs to consider this now needs to check whether the address is unspecified. I'll propose that this seems easy to forget. b211fc9 is an example of the kind of thing I'm worried about; this adds a check to tread unspecified addresses specially, and it seems quite difficult to know (a) whether we've updated all sites that need to be updated and (b) that we'll have confidence that we continue to do so in the future.
• If the address is optional, what does Some(addr) mean if addr is itself unspecified?
• The datastore model types use Option<IpNetwork>, but then convert to an unspecified address as a sentinel value in the database, which has all the typical "sentinel value" issues (in particular: we have to correctly convert them at every insertion and query).

This is definitely not my area of expertise, but I'd tentatively propose something shaped sort of like this, with more clear names from someone who knows the domain better:

enum MaybeAddress {
    BgpUnnumbered,
    Address(SpecifiedIpAddr),
}

where SpecifiedIpAddr is a newtype around IpAddr similar to NonZeroU8 and friends (i.e., it validates that the IP address it wraps is not unspecified).

[jgallagher]

Another spot where this came up in testing #9570; we should make sure whatever we do here would've caught this one: acd95fb. I think using a stronger type in UplinkAddressConfig would've forced us to consider this case in the display impl.

[jgallagher]

Some notes from chatting with @internet-diglett - at a couple spots when inserting switch port settings into the db, we build an in-memory map keyed by the peer address, and we convert None to Ipv4Addr::UNSPECIFIED; e.g.,

omicron/nexus/db-queries/src/db/datastore/switch_port.rs

Lines 1407 to 1411 in 72e1f85

	// Track peers for policy lookup. For unnumbered peers (addr is None),
	// use UNSPECIFIED as the sentinel value.
	let lookup_addr =
	p.addr.unwrap_or(IpAddr::V4(Ipv4Addr::UNSPECIFIED));
	peer_by_addr.insert(lookup_addr, &p);

I asked if it was possible to have multiple unnumbered peers, and the answer is that it shouldn't be (each port can have at most one unnumbered peer). This ought to be enforced by these db constraints:

omicron/schema/crdb/dbinit.sql

Lines 3695 to 3703 in 72e1f85

	-- Unique constraint for numbered BGP peers (those with an address)
	CREATE UNIQUE INDEX IF NOT EXISTS switch_port_settings_bgp_peer_config_numbered_unique
	ON omicron.public.switch_port_settings_bgp_peer_config (port_settings_id, interface_name, addr)
	WHERE addr IS NOT NULL;
	-- Unique constraint for unnumbered BGP peers (one per interface)
	CREATE UNIQUE INDEX IF NOT EXISTS switch_port_settings_bgp_peer_config_unnumbered_unique
	ON omicron.public.switch_port_settings_bgp_peer_config (port_settings_id, interface_name)
	WHERE addr IS NULL;

However, IIUC, those constraints allow as many as three "unnumbered" peer rows to exist for a given port: 0.0.0.0, ::, and NULL. I'm a little surprised we were using Ipv4Addr::UNSPECIFIED here when we've used Ipv6Addr::UNSPECIFIED in other places, so I'm not sure if this is a purely theoretical potential problem or an actual one we haven't noticed yet.

[jgallagher]

#10082 introduces stronger Rust types as described in the parent issue, but doesn't tackle any of the database issues. I tried to take a lay of the land of what's going on there, and came up with this.

I think there are four tables that store a BGP peer address that need to be able to represent "unnumbered":

• switch_port_settings_bgp_peer_config.addr properties (these also apply to bgp_peer_view.addr, which is a view over this table):
  • column is NULLable
  • "unnumbered" is represented as NULL
  • has separate indices with WHERE addr IS NOT NULL / WHERE addr IS NULL for numbered / unnumbered peers; latter tries to enforce "at most one unnumbered peer per interface"
  • no CHECK constraints that we don't have 0.0.0.0- or ::-valued columns. If we did, those would go into the "numbered" index (because they're not NULL), but the Rust code that consumes this table would treat them as "unnumbered", allowing us to have up to three "unnumbered" addresses for any given interface
• switch_port_settings_bgp_peer_config_communities.addr, switch_port_settings_bgp_peer_config_allow_import.addr, switch_port_settings_bgp_peer_config_allow_export.addr all have the same properties:
  • column is NOT NULL
  • column is part of the composite primary key of its table
  • unnumbered peers are expected to be represented as Ipv4Addr::UNSPECIFIED (0.0.0.0)
    • datastore methods that operate on this table work on Option<IpNetwork> and convert None -> Ipv4Addr::UNSPECIFIED
  • no CHECK constraints that we don't have ::-valued columns
    • if someone called these methods with Some(Ipv6Addr::UNSPECIFIED), I'm not sure what would happen - probably we would insert this (and then could have two unnumbered addresses for a given interface), but it would also have to be queried with Some(Ipv6Addr::UNSPECIFIED) to find it?

I only found one table where we store an UplinkAddressConfig::address, which has to be able to represent "link-local". I'm confused about this one though (the last bullet here):

• switch_port_settings_address_config.address
  • column doesn't have a NOT NULL annotation, but is implicitly NOT NULL because it's part of the table's composite primary key, although a TODO comment in the schema references Establish conventions for network resource primary keys in Nexus #3013 (maybe switching to synthetic id keys)
  • on rack init, we convert "link-local" to Ipv6Addr::UNSPECIFIED (::)
  • external API requests create these with arbitrary values, so I think we could have either 0.0.0.0 or ::
  • I don't actually understand how we could insert unspecified addresses at all: inserts go through address_lot::try_reserve_block(), which looks like it will reject any address that isn't covered by an address lot block. Does that mean if we have link-local uplink addresses, we also have an address lot block that includes the unspecified address? Or do these inserts bypass this check in some way I don't understand?

---

For the four BGP tables, my high level proposal is "pick one representation for unnumbered, use it consistently, and enforce that the other representations are rejected via CHECK constraints". The obvious options for the representation we pick are NULL, Ipv4Addr::UNSPECIFIED, or Ipv6Addr::UNSPECIFIED. Aesthetically I have a preference for NULL, but that's maybe the most awkward choice to use in practice, because it would require reworking the three tables above in which addr has a NOT NULL constraint because it's part of the primary key of the table. Maybe we should instead use Ipv4Addr::UNSPECIFIED, because that would involve the least db schema migration work and churn? If we did, I think the changes would be:

• For the three tables in which addr is already NOT NULL, add a CHECK constraint that rejects Ipv6Addr::UNSPECIFIED. Applying this migration requires that the table not have any rows containing that value; the easiest thing to do where would be DELETE FROM ... WHERE addr = "::", if we don't expect any of those to actually exist. A more complicated thing we could try is "convert :: to 0.0.0.0, but only if there isn't already another row on this interface with an address of 0.0.0.0"?
• For switch_port_settings_bgp_peer_config:
  • Update the table, converting any NULL addrs to 0.0.0.0
  • Make the column NOT NULL
  • Add a CHECK constraint rejecting :: (as above, this would need to either delete or convert any existing rows containing ::)
  • Change the unnumbered / numbered unique indices to split on "addr does or does not equal 0.0.0.0" instead of "addr is or is not NULL"

I'd make a similar high-level proposal for tightening up switch_port_settings_address_config.address, but don't understand enough about how that's working today to list out anything concrete.