-
Notifications
You must be signed in to change notification settings - Fork 59
Add operator APIs for manipulating system IP Pools #9225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
4a65598
to
eed8606
Compare
} | ||
|
||
async fn ip_pool_range_list( | ||
async fn system_ip_pool_range_list( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self -- I think this should be a "normal", non-system endpoint. Users who can see an IP Pool should be able to list its ranges. But it does make the whole prefix / no-prefix thing more confusing.
- Add Nexus endpoints for reserving IP Pools for Oxide use or for external customer Silos - Remove check used to hide internal IP Pools, since they're all visible to an operator now - Rename operator IP Pool endpoints by prefixing `system_*`, to avoid conflicts with previous "silo-scoped" endpoints. - Handle possibility of any number of Oxide-internal IP Pools in various places where we previously assumed exactly 2 of them. - Fixes #8947 - Fixes #8226
eed8606
to
143f98a
Compare
We should probably hold this PR until after we cut the R17 release. I'm also adding a PR to the CLI repo handling these breaking changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bnaecker! Comments follow.
} | ||
|
||
// List Oxide reserved IP Pools and corresponding authz objects. | ||
async fn list_all_service_ip_pools( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not really doing what the name/comment suggests. It's associating particular IP pools with operator specified RSS ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, I'll change it. I think it's only call-site is when doing the initial deployment after rack setup, which means the whole function should go away when we supply the pools through RSS (rather than just the ranges, which we do today).
/// The pool is reserved for use by external customer Silos. | ||
ExternalSilos, | ||
/// The pool is reserved for Oxide internal use. | ||
OxideInternal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend using SystemInternal
for outward facing stuff to be consistent with system scoped APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's better. I'll rename this. It will make this PR a bit larger though. I already named it like that in the database, so I'll see if I can easily rename the variant there. That can be tricky IIRC, so it might not work.
system_*
, to avoid conflicts with previous "silo-scoped" endpoints.project_ip_pool_list
andproject_ip_pool_view
#8226