Add dual-stack IP config for internal NICs#9389
Conversation
9d04b7a to
83a21bb
Compare
83a21bb to
2033231
Compare
|
Thanks in advance to the reviewers. It turns out we use the |
- Add dual-stack VPC private address configuration type and include it in the shared NetworkInterface type. - Add database model support for reading / writing the dual-stack NIC type to the database, handling serialization into optional fields. - Update all the callsites to handle the new dual-stack-aware type. - Add a bunch of conversions for the APIs which rely on that new type, of which there are many. This also adds the conversions and older types into the `sled-agent-types` crate, so they can be used in a few places that don't directly depend on the `sled-agent-api` crate itself, notably reconfigurator and `nexus-inventory`. - Update the sled-agent reconciler to deserialize previous versions of its sled-configuration ledgers, convert them, and write them out again as the new versions. - Updates the OPTE `PortManager` type with the new dual-stack support. This is only for private IP addresses, though. We still need some work to support OPTE ports with dual stack external addresses. This is about half of #9247, the VPC-private part. - Closes #9246
2033231 to
fe87906
Compare
|
I'll figure out how to resolve the conflicts with the multicast work on Monday. |
|
I'm going to wait to fix the conflicts until we address some changes in the way the conflicting code was structured. I'll still have conflicts to resolve that point, but that will be entirely mechanical. The PR is still ready for review as it stands. |
rcgoodfellow
left a comment
There was a problem hiding this comment.
Sorry for the delay in getting this posted @bnaecker. Review comments follow. However, this touches on a lot of Omicron systems I'm not very familiar with like inventory, blueprints, ledgers and reconfigurator. It would be good to have someone who works in those systems to take a look.
| path = "/vmms/{propolis_id}", | ||
| versions = ..VERSION_ADD_DUAL_STACK_SHARED_NETWORK_INTERFACES, | ||
| }] | ||
| async fn v6_vmm_register( |
There was a problem hiding this comment.
This was very confusing to me at first as i was reading v6 as IPv6 and did not realize this was the old method being moved to API version 6.
There was a problem hiding this comment.
Luckily once this catches up with main it'll be somewhere between v8 and v10 instead 😅
| path = "/vmms/{propolis_id}", | ||
| versions = ..VERSION_ADD_DUAL_STACK_SHARED_NETWORK_INTERFACES, | ||
| }] | ||
| async fn v6_vmm_register( |
There was a problem hiding this comment.
Luckily once this catches up with main it'll be somewhere between v8 and v10 instead 😅
- Avoid panics in DB conversion and Omicron-to-OPTE IP configuration conversion - Clean up some error messages - Add helpers to return any IP address for an OPTE port / gateway - Remove explicit v2 module for current version of NIC type
This adds only a v8 into the public sled-agent-types crate with the previous version. It adds infallible conversions from older API types to and from v8, rather than changing those conversions to be directly to the latest type. Also call the "next" version of the actual API handler too, rather than jumping to the latest.
|
Ok @jgallagher @rcgoodfellow I believe I've addressed your comments and fixed up all the merge conflicts with main. This should be ready for another look when you get a chance. Thanks! |
| .subnet() | ||
| .iter() | ||
| // The 0th address is the network address, and OPTE's | ||
| // gateway sits at the next. See |
There was a problem hiding this comment.
Can this call v6.opte_gateway() instead?
There was a problem hiding this comment.
Doh! Yeah, I added those methods and never used them. Thanks!
| floating_ips: &[IpAddr], | ||
| is_ipv4_only: bool, | ||
| ) -> Result<Ipv4Cfg, Error> { | ||
| let gateway_ip = v4.subnet().first_host().into(); |
There was a problem hiding this comment.
Can this call v4.opte_gateway() instead?
| versions = | ||
| VERSION_ADD_PROBE_PUT_ENDPOINT..VERSION_ADD_DUAL_STACK_SHARED_NETWORK_INTERFACES, | ||
| }] | ||
| async fn v6_probes_put( |
There was a problem hiding this comment.
Should this be v9 instead of v6?
There was a problem hiding this comment.
It should, yes. Thanks.
This is a follow-up to #9389 that I missed. We were already separating transit IPs by version in the code before and after the database. This pushes that separation all the way into the database, which will make it easier to implement updates to dual-stack NICs in the public API. Separating them lets clients update the transit IPs for only one version, without some janky read-modify-write code that ensures we don't clobber the transit IPs for the _other_ version.
This is a follow-up to #9389 that I missed. We were already separating transit IPs by version in the code before and after the database. This pushes that separation all the way into the database, which will make it easier to implement updates to dual-stack NICs in the public API. Separating them lets clients update the transit IPs for only one version, without some janky read-modify-write code that ensures we don't clobber the transit IPs for the _other_ version.
sled-agent-typescrate, so they can be used in a few places that don't directly depend on thesled-agent-apicrate itself, notably reconfigurator andnexus-inventory.PortManagertype with the new dual-stack support. This is only for private IP addresses, though. We still need some work to support OPTE ports with dual stack external addresses. This is about half of Enable dual-stack OPTE ports in the sled-agent #9247, the VPC-private part.