[nexus] Remove project_id, rack_id from IP pools #2056
Conversation
| // Type used to identify a Project in request bodies, where one may not have | ||
| // the path in the request URL. | ||
| #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)] | ||
| pub struct ProjectPath { | ||
| pub organization: Name, | ||
| pub project: Name, | ||
| } | ||
|
|
There was a problem hiding this comment.
Thanks! I'll be able to remove this hacky thing I did today: https://github.com/oxidecomputer/omicron/pull/2050/files#r1048986084
|
I suspect we'll want a follow-up to this PR to add APIs for the following:
As mentioned on #2055 |
common/src/sql/dbinit.sql
Outdated
| * should probably point to an AZ or fleet, not a rack. | ||
| */ | ||
| rack_id UUID, | ||
| internal_only BOOL NOT NULL, |
There was a problem hiding this comment.
Minor bikeshedding here, no action required unless you feel like it.
This could just be called internal as only seems to me to be implied.
It'd probably be good to copy the comment from the datastore struct or just note to check that file. Do @see comments work in rust? Maybe that's just a jsdoc thing. Again, not required. Just a nice to have.
There was a problem hiding this comment.
Done in b4dd0b1
I dunno about the linked comments, but I included a summary here.
| self.db_datastore.ip_pool_create(opctx, new_pool, None).await | ||
| self.db_datastore | ||
| .ip_pool_create(opctx, new_pool, /* internal_only= */ false) | ||
| .await | ||
| } | ||
|
|
||
| pub async fn ip_pool_services_create( |
There was a problem hiding this comment.
It's outside the scope of this PR, but I wonder if IP Pool Services is the right way to frame this now. Should it just be internal ip pools?
There was a problem hiding this comment.
I hear you on this; maybe s/service/internal would be more consistent. When I originally wrote this, I think I was writing "service" as shorthand for "internal service".
| let (.., pool) = datastore | ||
| .ip_pools_lookup_by_name_no_auth(&opctx, "default") |
There was a problem hiding this comment.
Is this really just for looking up the default IP pool? If so, I wonder if we shouldn't just have a ip_pools_lookup_default that's documented to not require authz but otherwise doesn't take arbitrary names.
There was a problem hiding this comment.
I did this, and added authz checks, within 91415a4
Fixes a regression introduced by #2056 Updates polar policy to allow instance creation from regular Silo users, and adds a regression test
Before this PR
This PR
default, which is used for address allocation unless a more specific IP pool is providedIn the future
service_ip_poolAPI with the AZ UUID used for the query, but since AZs don't exist in the API yet, this has been omitted.Part of #2055