feat(rest_catalog::auth) Contextual Sessions - #3081

Storing catalog session here is fine but feels a bit excessive. Why not build contextual sessions in the catalog, and then pass them to RestClient::query_catalog?

Really only because the catalog is already doing quite a lot and the RestClient isn't.
I agree that conceptually it belongs closer to the catalog though.
I can make the change tomorrow 👌

There's actually something more to it. The catalog session depends on the /v1/config handshake and on the init session. Because the RestClient is initialized lazily, it's not trivial to simply move the field to the RestSessionCatalog.

I see three options:

1. keep them in the OnceCell<RestClient> and re-use the lazy intialization
2. introduce new OnceCell<Arc<dyn AuthManager>> and OnceCell<Arc<dyn AuthSession>> and have their initialization depend on the rest client's initialization
3. use a wrapper to encapsulate the rest client, auth manager and catalog session into a single lazy initialization

My thoughts on them:
2. is spreading lazy initialization logic around the code where it's really only one lazy chain that could be simplified
3. I could see this one work out but I haven't found a good name yet. I also am concerned that we're building a deep tree of encapsulating types: RestCatalog -> RestSessionCatalog -> <SomeNewType> -> RestClient -> HttpClient. The distinction between the RestClient and HttpClient isn't too well-defined yet (it's essentially an http client with rest-catalog specific config). Maybe this could be an opportunity to give it a more specific responsibility to save us from another layer -> that would get us to a hybrid of 1. and 3.

This makes sense, thanks for the detailed explanation!

I think keeping it as is for now is good. Maybe we need to rename RestClient to something else in the future since it represents more than just a client

I'd rename it to RestCatalogClient for now to emphasize it's specificity to the catalog, will push a commit in a few minutes.
I have some refactors in mind that can go on top of this PR

1. the auth manager has no dependency on sessions and shouldn't have to be loaded lazily -> it should be constructed by the builder
2. going a step further, auth manager resolution only requires the RestConfig's properties, I'm exploring whether we shouldn't move the auth manager to the config and change it from a TypedBuilder pattern to a custom builder that returns a Result<RestConfig> with a resolved auth manager. This would avoid an Arc<dyn AuthManager> on the catalog just to pass it to RestCatalogClient initialization (and then not use it again)

Rename commit -> 8785ac8

Let me know what you think 🙏

naming is hard and I like the new name more!

Same here, seems like we can pass the pre-built contextual session to this function directly

We can build the contextual session using the auth manager here

This makes sense, thanks for the detailed explanation!

I think keeping it as is for now is good. Maybe we need to rename RestClient to something else in the future since it represents more than just a client

I think this quietly doubles the per-request HeaderMap clone. with_auth_session does Self { auth_session, ..self.clone() }, which clones extra_headers, and then HttpClient::query_catalog clones extra_headers again when it extends the request. Before this PR the session was baked in at init and we paid one clone per request.

An internal method that authenticates in place keeps it to a single clone:

pub(crate) async fn query_catalog_with_session(
    &self,
    session: &dyn AuthSession,
    mut request: HttpRequest,
) -> Result<HttpResponse> {
    session.authenticate(&mut request).await?;
    let mut inner = request.into_inner();
    inner.headers_mut().extend(self.extra_headers.clone());
    HttpResponse::read(self.client.execute(inner).await?).await
}

Not a blocker, but it's a regression on the hot path for single-context callers, so I'd fix it here.

The benefit was negligible in my benchmarks but the clone still shows a design issue: a contextual auth session is now passed with every catalog request, but the old client that was designed around catalog sessions assumed they were static for the lifetime of the client.

d33821a and 085bf79 get this straight. A session is now passed on every request to query_catalog and the client doesn't hold any sessions anymore.
The catalog's use of the HttpClient now also diverges from the AuthManager's use. The HttpClient::post_form method is public for implementors of auth managers. It's now not using any catalog sessions anymore because it's meant to derive them. If implementors need to authenticate their requests, they can still do so with client-level headers.