-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add the server kind to the Nexus API context #5751
Conversation
- Adds a server kind enum, used to distinguish which API server is running any particular handler. - Wraps the existing `ServerContext` into a higher-level `ApiContext` type, which includes the former in addition to the kind of server handling the request. - Fixes #5651
The proximal reason for doing this is #5735 , to elide the check when setting the source IP allow-list that ensures you can't lock yourself out. That should help in recovery situations. I'm a bit on the fence about this. It feels like the nuclear option for a nearly trivial fix (adding a check on the server kind in the allow-list update handler). While it does make the server kind available in an API handler, that has to be passed in as an argument to any other function which needs it. That's nice and explicit, but is a bit counter to the shared state we've got smeared around Nexus in most other situations. Still, it does solve the original problem, and might be useful in the future for code that needs to know more about which server is handling any particular request. |
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.
I get why you're on the fence with this (and thank you for doing such a monotonous set of changes to do this), but I think I'm on board. I would not be offended if you want to get a second or third opinion, though! :)
} | ||
} | ||
|
||
impl std::borrow::Borrow<ServerContext> for ApiContext { |
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.
Did we need to implement Borrow
? It's a little off-the-beaten-path and I thought was more about Cow / ToOwned and being used as a key in collections, none of which seems like it'd be relevant for ApiContext
.
I could see an argument for implementing Deref
, though, to allow calling ServerContext
methods directly on an ApiContext
; I wonder if that would have made some of the other mechanical changes unnecessary? I probably wouldn't go back and do this, though - Deref
can be nice but can also be a little surprising.
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.
We do need borrow here, to satisfy this trait bound:
omicron/nexus/db-queries/src/authn/external/mod.rs
Lines 49 to 54 in 8c38ad1
pub async fn authn_request<Q>( | |
&self, | |
rqctx: &dropshot::RequestContext<Q>, | |
) -> Result<authn::Context, authn::Error> | |
where | |
Q: Borrow<T> + Send + Sync + 'static, |
I agree that it seems a bit weird, and I'm not sure I like the trait bound itself. I kept it simply to minimize churn, but I'm happy to reevaluate.
I also considered implementing Deref
. I think it might make some other changes unnecessary, but there actually didn't seem to be many ServerContext
methods used themselves. Most of the fields are visible and used directly. I also agree it's a bit magical sometimes, so I think it makes sense to defer it.
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.
TIL! I'd definitely defer any reevaluation of that for some followup work (which is probably not necessary at all - implementing Borrow
is surprising but actually required - that's fine).
- Use full match instead of `matches!` - Use `server_context()` method - Rename `ServerKind::for_internal()` constructor
Whoops, I've linked to the wrong issue here. This fixes #5735. I'll amend the commit message to reflect that now. |
ServerContext
into a higher-levelApiContext
type, which includes the former in addition to the kind of server handling the request.