-
Notifications
You must be signed in to change notification settings - Fork 42
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
Refactor login endpoints, instrument console endpoints #7374
Conversation
} | ||
|
||
Ok(true) |
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 still don't like this method, but at least it's a few lines shorter now
) -> Result<Response<Body>, HttpError> { | ||
console_api::login_saml_begin(rqctx, path_params, query_params).await |
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.
All this function did was call service_console_index
. That's the kind of thing we're fixing here.
} | ||
|
||
async fn login_saml_redirect( | ||
rqctx: RequestContext<Self::Context>, | ||
path_params: Path<params::LoginToProviderPathParam>, | ||
query_params: Query<params::LoginUrlQuery>, | ||
) -> Result<HttpResponseFound, HttpError> { | ||
console_api::login_saml_redirect(rqctx, path_params, query_params).await |
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.
Some of these are slightly more interesting than inlining a function call. This helper function did all the stuff you normally see here at top level in the endpoint handlers (instrumentation call, pulling stuff out of path and query params, etc), and the nitty gritty datastore logic that normally goes inside nexus/src/app
. So I pulled the former out to here and the latter into nexus/src/app/login.rs
.
The idea was to make these look more like the other handlers.
console_page!(console_silo_images); | ||
console_page!(console_silo_utilization); | ||
console_page!(console_silo_access); | ||
|
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 created these to reduce boilerplate back when the actual endpoint handlers were defined here (rather that silly little helper functions called by the endpoint handlers, which is what they are now). Because of the change to Dropshot API, we kinda have to have the boilerplate over in http_entrypoints.rs
anyway, so these are pointless. Deleting this section is why we now inline console_index_or_login_redirect(rqctx).await
calls in a ton of handlers.
e578e4e
to
8d193d5
Compare
Pulling these refactors out of #7339 because they're mechanical and just add noise. The point is to make it a cleaner diff when we add the function calls or wrapper code that creates audit log entries, as well as to clean up the
device_auth
(eliminated) andconsole_api
(shrunken substantially) files, which have always been a little out of place.Refactors
With the change to a trait-based Dropshot API, the already weird
console_api
anddevice_auth
modules became even weirder, because the actual endpoint definitions were moved out of those files and intohttp_entrypoints.rs
, but they still called functions that lived in the other files. These functions were redundant and had signatures more or less identical to the endpoint handlers. That's the main reason we lose 90 lines here.Before we had
Now we (mostly) cut out the middleman:
Some of what was in the middle moved up into the endpoint handlers, some moved "down" into the nexus "service layer" functions.
One (1) functional change
The one functional change is that the console endpoints are all instrumented now.