Reframe Omicron zone external IP creation in terms of OmicronZoneExternalIp
#5560
Conversation
Also actually add it to the planning input...
These weren't used (both RSS and blueprints choose explicit IPs), and the reconfigurator planning/execution split means we're not going to use them.
This eliminiates the possibility of having a `SourceNatConfig` with an unaligned first/last port pair, which also allows us to drop an assertion when we go to store them in the db.
| // field names and types match. (It doesn't check the _order_, but that | ||
| // should be fine as long as we're using JSON or similar formats.) | ||
| #[derive(Deserialize)] | ||
| #[serde(remote = "SourceNatConfig")] |
| /// Fails if `(first_port, last_port)` is not aligned to | ||
| /// [`NUM_SOURCE_NAT_PORTS`]. |
There was a problem hiding this comment.
So.... I'm wondering if rather than first_port and last_port this can just take the multiple of NUM_SOURCE_NAT_PORTS. Since NUM_SOURCE_NAT_PORTS is 2^14 and the number of ports is 2^16, there are only 4 possible values. So this could actually take an enum, say, SourceNatPortBlock { Block0, Block1, Block2, Block3 }. That would make this constructor infallible.
Then you can serialize the data in that fashion as well, and deserialization won't require further validation.
Some places may have a port pair, and you can have a constructor that converts that into a SourceNatPortBlock, returning the below error. But places that don't have the raw ports won't have to deal with errors.
Not sure how much it increases work for you, maybe in a followup/after r8 if it disrupts this work too much?
There was a problem hiding this comment.
I like that idea a lot, but yeah I think it's largely orthogonal to the blueprint work. I'll convert this comment into an issue to tackle later.
This is PR 1 of N (probably 3?) working toward getting blueprints to store an
OmicronZoneExternalIp(which includes both the ID and the snat cfg, if applicable) instead of only storing a rawIpAddr. It adjusts the database query function(s) that create omicron zone IPs to take anOmicronZoneExternalIpinstead of the list of arguments they took previously. The changes here include:IncompleteExternalIp::for_service_explicit{,_snat}have been replaced byIncompleteExternalIp::for_omicron_zone. It now detects based on theOmicronZoneExternalIpKindwhether it's floating or SNAT.DataStore::external_ip_allocate_service_explicit{,_snat}have been replaced byDataStore::external_ip_allocate_omicron_zone.DataStore::external_ip_allocate_service{,_snat}have been removed. These allowed allocating an IP without specifying what the IP was (i.e., by taking a free IP from the pool, if one is available). We had no non-test callers of these, and with the blueprint system wanting to make planning decisions that assign IPs without committing to anything in CRDB, I don't think we expect to have any callers of this in the future (since we'll always want to try to allocate a specific IP).IncompleteExternalIp::for_service{,_snat}are gone; same reason as above.SourceNatConfignow validates that the port pair it's given (upon creation or deserialization) are aligned correctly; previously this check was deferred to when we inserted the IP in the database, and the check was anassert!. This is now a runtime error instead. This is more of a drive-by-cleanup than strictly required, but I didn't like how many places we could be passing around a potentially-invalid snat config.PlanningInput. We were already fetching the nic rows from the DB, but forgot to actually add them. This is also more of a drive-by-fix than strictly required, but we'd need it soon anyway.