[nexus] Endpoint to list IP pools for silo, add is_default to silo-scoped IP pools list
#4843
Conversation
david-crespo
force-pushed
the
silo-ip-pools-endpoint
branch
from
b432677 to
06d1fea
Compare
| .filter(ip_pool::time_deleted.is_null()) | ||
| .select(db::model::IpPool::as_select()) | ||
| .select(<(IpPool, IpPoolResource)>::as_select()) |
There was a problem hiding this comment.
neat trick. initially I tried (IpPool::as_select(), IpPoolResource::as_select()) but it wanted the above
| ) | ||
| .await | ||
| .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) | ||
| } |
There was a problem hiding this comment.
I just realized this query is exactly the same as the one above — the only difference is whether you're passing in a silo or pulling the current silo from auth context. I will extract a shared function.
There was a problem hiding this comment.
This came up in some of my work too. I just dropped the current version altogether and only left on that takes a given argument. The app layer can pass in the current silo when needed.
| // TODO: this could just list views::Silo -- it's not like knowing silo_id | ||
| // and nothing else is particularly useful -- except we also want to say | ||
| // whether the pool is marked default on each silo. So one option would | ||
| // be to do the same as we did with SiloIpPool -- include is_default on | ||
| // whatever the thing is. Still... all we'd have to do to make this usable | ||
| // in both places would be to make it { ...IpPool, silo_id, silo_name, | ||
| // is_default } | ||
| ) -> Result<HttpResponseOk<ResultsPage<views::IpPoolSiloLink>>, HttpError> { |
There was a problem hiding this comment.
I wonder if views::Silo shouldn't just be updated with default_ip_pool: Option<Uuid> and the silo just be returned here. There's only ever one default pool, it seems like a good thing to latch onto the silo.
There was a problem hiding this comment.
That would mean every query for silos would have to join to the ip_pool_resource table and return both the silo and the link so we can tack on the pool ID.
There was a problem hiding this comment.
Despite the increased cost I still think it'd be a UX improvement for the API. That said, it doesn't need to be done here. I'm not a huge fan of returning the link, but it's fine for now. It gives them all the info they need to get whatever else they need.
|
|
||
| // check ability to list silos in general | ||
| opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; | ||
|
|
There was a problem hiding this comment.
Noticed this check was missing here
Fixes #4752
Fixes #4763
The main trick here is introducing
views::SiloIpPool, which is the same asviews::IpPoolexcept it also hasis_defaulton it. It only makes sense in the context of a particular silo becauseis_defaultis only defined for a (pool, silo) pair, not for a pool alone.GET /v1/system/silos/{silo}/ip-pools/v1/ip-poolsand/v1/ip-pools/{pool}should returnSiloIpPooltoo/v1/system/silos/{silo}/ip-poolsSiloIpPoolandIpPoolSilo, cleaned up by changing:views::IpPoolSilo->views::SiloIpSiloLinkparams::IpPoolSiloLink->views::IpPoolLinkSilo