Dual stack ephemeral IP#9714
Conversation
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
It would have felt nuts to type all this out by hand but it seems great for the user!
c7c619c to
05dab02
Compare
| return Err(Error::invalid_request(format!( | ||
| "two ephemeral IPs would both be {pool_version}: \ | ||
| auto selects default pool, which is {pool_version}" | ||
| ))); |
There was a problem hiding this comment.
Having to do these lookups here and then again during the actual allocation feels like the worst part of this change. I'm looking into what it would take to avoid this by, e.g., resolving the pool here and passing the result into the saga.
There was a problem hiding this comment.
This all feels kind of susceptible to TOCTTOUs. I don't see a good way around that, with the way we're structuring the DB tables themselves though.
There was a problem hiding this comment.
I had Claude prototype moving the pool resolution out of the sagas (a not-small +400/-300 change), and it had to touch a lot of parts. It would not be easy to be confident the change is correct. I don't think it's worth doing — we do a bunch more queries inside the saga anyway.
|
Oh also I need to do the whole API schema regen. I left it out so I could use |
bnaecker
left a comment
There was a problem hiding this comment.
Seems good to me. A few vague questions, but shouldn't block integrating this. Thanks for taking this on.
| return Err(Error::invalid_request(format!( | ||
| "two ephemeral IPs would both be {pool_version}: \ | ||
| auto selects default pool, which is {pool_version}" | ||
| ))); |
There was a problem hiding this comment.
This all feels kind of susceptible to TOCTTOUs. I don't see a good way around that, with the way we're structuring the DB tables themselves though.
`instance_lookup_ephemeral_ip` conflated two concerns: looking up an ephemeral IP by version (pure data retrieval) and validating that user requests are unambiguous (error when multiple IPs exist and no version specified). This validation only makes sense for user-facing operations like detach, not for internal callers that already know which version they want. Changed the function to require an IP version, making it a pure lookup. Move the "multiple ephemeral IPs" validation into the detach saga, the only caller that needs it. better error message for detach with ambiguous version
1ee28cf to
2c2c30d
Compare
| ), | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
This was initially added to instance_lookup_ephemeral_ip, but that was confusing because that's a lookup function, plus this validation is only relevant when the IP version can be none, which only happens (among calls to instance_lookup_ephemeral_ip) in the detach path. So this logic only needs to happen here. The other paths (instance create and ephemeral IP attach) can also start out without an explicit IP version, but by the time they got to instance_lookup_ephemeral_ip, the IP version has already been resolved.
Closes #9665
Had the robots work on this while I got ready for bed.
API schema diff
Notably this did not require changes to the instance create API, only detach.