Current IP pool utilization endpoint #5258
Conversation
| } | ||
|
|
||
| // TODO: should this just return the vec of ranges and live in ip_pool.rs? | ||
| // TODO: generally we never retrieve all of anything, how bad is that |
|
It probably will not be unusual at all. The IPv6 space is so huge it’s almost laughable. The most common subnet size is a /64, and technically you’re not even supposed to carve out smaller ones at all. We will certainly want a way to report the utilization of ranges up to the full /48 or whatever that we carve out for each VPC.
|
|
Thanks. I will look into a more elaborate BigInt setup. Probably a string with a custom format like |
openapi/nexus.json
Outdated
| "allocated": { | ||
| "description": "The number of IPs allocated from the pool\n\nLike total, this is a numeric string with a custom \"uint128\" format representing a 128-bit integer, though in practice it is extremely unlikely to be bigger than 32 bits (2^32 = ~4.29 billion).", | ||
| "type": "string", | ||
| "format": "uint128" |
There was a problem hiding this comment.
This is new. Client generators will have to look for this and handle it appropriately.
There was a problem hiding this comment.
Actually... this is non-trivial for the TS client. The only transformation of response JSON we currently do is parsing dates from strings, and we do that based solely on information present in the response, i.e., a) that the value is a string, b) the key starts with time_ or equals timestamp, and the value parses successfully as a date (source).
I had forgotten that while we do use the format info in the schema for generating the types (and the runtime validators we currently only use in the mock API), we do not have any of that info at request/response time, or more precisely, we do not currently encode that info the generated code that runs at that time. So I will have to think about how we're going to handle running this through BigInt. Some options involve making a change in this PR to make it easier for us in the generator, for example adding a special prefix or suffix on the field name.
cc @ahl
FelixMcFelix
left a comment
FelixMcFelix
left a comment
There was a problem hiding this comment.
Thanks a lot! I think this looks good on the omicron side, please ping me if you need to rework the u128 handling to make it easier for consumers and I can take another look.
| // This is a bit strange but it should be ok. | ||
| // a) SQL tends to represent counts as signed integers even though | ||
| // they can't really be negative, and Diesel follows that. We | ||
| // assume here that it will be a positive i64. |
There was a problem hiding this comment.
I think practically this is a safe assumption; if we're approaching
|
|
||
| use db::schema::ip_pool_range; | ||
|
|
||
| let ranges: Vec<IpPoolRange> = ip_pool_range::table |
There was a problem hiding this comment.
This is combining v4 and v6 capacity in a given pool -- is that the intended behaviour long-term? It's fine today since v6 isn't yet functional.
There was a problem hiding this comment.
Interesting question! It hadn't occurred to me to give separate allocated and capacity counts for v4 and v6, but it makes a lot of sense, doesn't it. I will think about doing that here — it's always nice to avoid followup API changes where we can. One nice thing about that is that the IPv4 counts can be u32s.
Another way we could do this would be to have a given pool be IPv4 or IPv6 only, but I don't really like that. Right now you can add both IPv4 and IPv6 ranges to a pool, and it would be kind of complicated and/or confusing to require pools to have only one or the other.
There was a problem hiding this comment.
All right, check out 39edb8f. A little complicated, maybe, but the end result is
{
ipv4: { allocated, capacity },
ipv6: { allocated, capacity },
}
Sound good?
There was a problem hiding this comment.
I think so, thanks for doing this! This also helps a lot since external v4s can be quite constrained in practice.
| "Failed to convert i64 {} IPv4 address count to u32", | ||
| allocated.ipv4 | ||
| )) | ||
| })?, |
There was a problem hiding this comment.
I'm slightly nervous about this but I feel like it's going to be really, really hard to hit this 500!
| // exactly as vulnerable as if it were paginated. If there are more | ||
| // than 10,000 ranges in a pool, we will undercount, but I have a | ||
| // hard time seeing that as a practical problem. | ||
| .limit(10000) |
There was a problem hiding this comment.
In private chat we discussed the alternative of doing this calculation in SQL so we don't have to limit it here. What we found is that neither CRDB nor Postgres support subtraction of IPv6 addresses when the result is greater than a 64 bit integer, and we couldn't figure out how to convert the addresses to 128 bit integers first and then subtract. So it seems we are forced to do this in Rust.
> select 'ffff::'::inet - '::1';
ERROR: result out of range
Closes #5189
The strangest thing here is that IPv6
allocatedandcapacityare both u128 that we put in the JSON spec as"type": "string", "format": "uint128", whereuint128is a custom format our client generators will have to look for and handle.capacityis huge because IPv6 ranges are huge. IPv4 allocated and capacity areu32.The strangest thing here is the fact that we have to handle the unlikely possibility that an IPv6 range is too large for us to serialize its length in a JSON response. IPv6 range size is au128, and we seem to be capping our numbers in API responses atu32. My proposal for handling this is simply to make the field nullable in the response, and we make clear that a null value means it was too large to serialize. I think this is fine because this should only happen if the operator does something really bad, and it is well understood that the operator can mess up networking with a misconfiguration. Another approach would be to always return the size as a string, but I think given the unlikelihood of the number being too large (and the fact that it's pathological if it is) means returning a numeric string puts undue burden on clients to handle it.