IP range add can only be IPv4

[david-crespo]

Closes #5085

• Change range add request body from IpRange to Ipv4Range, see what breaks
• Error on Ipv6 ranges in the service layer
• Update tests and add new tests
• May change range remove body to Ipv4Range for consistency even though it is technically fine as long as add is restricted to Ipv4 — need to think about it decided not to so we can still remove any existing IPv6 ranges through the API

[david-crespo]

Only two failures, not bad.

FAIL [   8.005s] omicron-nexus::test_all integration_tests::ip_pools::test_ip_pool_range_overlapping_ranges_fails
FAIL [   7.474s] omicron-nexus::test_all integration_tests::ip_pools::test_ip_pool_range_pagination

[bnaecker]

Nice, thanks for doing this. I've a question about whether to change the API at all and fail IPv6 requests in the handler, but otherwise LGTM.

[bnaecker]

My preference would be to have a check inside the endpoint function that failed IPv6 requests, rather than changing the API itself. It's not a strong preference, but that results in a bit less churn in the other code and the generated OpenAPI document.

[david-crespo]

I think you might be right. I was somewhat biased in thinking about this by the fact that I mostly use the API through the TypeScript client. Because TS has structural types, the IPv6 and IPv4 versions of the request body look exactly the same: they're both { first: string, last: string } (see generated TS client).

There is no special constructor either; you just do { first: "10.0.0.1", last: "10.0.0.2" }, etc. But in other SDKs like Rust, changing the API for this would change the constructor you use to make the request body from IpRange::V4(Ipv4Range::new(...)) to plain Ipv4Range::new(), and that is real churn. On the other hand, the presence of the V6 variant on the IpRange constructor sure does make it seem like you should be able to pass in a v6 IP, and that's misleading.

I'm genuinely on the fence. It would help if you could give me a ballpark estimate for how long you think we'll have to disallow IPv6 here. If we're talking v7 and v8 and possibly beyond that, I'd lean toward changing the API now and changing it back.

[bnaecker]

I'm genuinely on the fence.

Yeah, I get it. The text in the HTTP request body looks the same in either case, but the Rust code (and other similarly-typed languages) looks different. I also don't want to mislead.

It would help if you could give me a ballpark estimate for how long you think we'll have to disallow IPv6 here. If we're talking v7 and v8 and possibly beyond that, I'd lean toward changing the API now and changing it back.

I really can't say, unfortunately. The only blocker I'm currently aware of is #5090, but it wouldn't surprise me if fixing that just uncovered the next blocker. It looks like all of the instance-NAT-related functionality is currently restricted to IPv4, so that would need to be adapted or duplicated for IPv6. Maybe @internet-diglett has a better sense of the amount of work required to get IPv6 NAT support squared away?

[internet-diglett]

🤔 For sure we need to plumb it through for NAT. A cursory glance at the downstream dendrite side shows that we appear to have most of the machinery we need there. We'd also need to update some of the bootstore logic so we can keep the infra ips consistent with the rest of the ip related apis.

I think we also have other network related functionality that is currently v4-only like BGP (and that doesn't have the downstream machinery yet), but I don't think that is a hard blocker. It will primarily mean that if someone wants to advertise a v6 prefix via BGP, they won't be able to.

[internet-diglett]

@david-crespo circling back on this, it seems like the biggest issue currently is that we'd basically need to roll out v6 support for all of the networking related features once we do it for one feature. Off the top of my head:

• IPv6 for instances means IPv6 external networking
• IPv6 external networking means IPv6 network services (NAT, Static Routing, BGP, etc.)
• IPv6 network services means IPv6 bootstore logic

While the pathways for most of this have already been established with the IPv4 logic, it would probably still take a bit of time to roll out properly, and I think all of us are currently occupied with other work at the moment.

[david-crespo]

That’s very helpful and it’s enough to tip me over into thinking we should make clear in the API too that it’s IPv4 only.

[david-crespo]

Or not! I came around to Ben's view and changed it in 16f9dad so that the API stays the same and we do the rejection further down. What bothered me was less the API churn and more the inconsistency now between the add range endpoints and every other endpoint that deals with IP ranges.

I hoped to do the reject as low down as possible — in the datastore function — but in order to test the u128 business in IP pool utilization with IPv6 ranges from #5258, I still needed to be able to create IPv6 ranges at the datastore layer. So I moved the check one layer up, to the nexus service layer.