laskoviymishka commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4008200299
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -2790,6 +2850,97 @@ mod tests {
assert!(dropped.load(Ordering::SeqCst));
}
+ #[tokio::test]
+ async fn test_contextual_session_authenticates_catalog_operation() {
Review Comment:
This is the one thing I'd want before merge: the new wiring touches all
twelve operations, but only `list_namespaces` is exercised here.
The gap I care about most is `check_exists_via_head` — both `table_exists`
and the HEAD branch of `namespace_exists` route through it, and that path
silently ignored the context before this PR. An existence check sent with the
wrong tenant's session is how "does table X exist" leaks across tenants, so I'd
really like a HEAD test with `match_header` on the contextual session for both.
While we're in there, a test where `contextual_session` returns `Err`
(asserting the op fails before any HTTP request goes out) plus one write-op
test would cover the parts most likely to break silently on a mechanical slip.
The existing `ContextManager` harness makes each ~15 lines. wdyt?
##########
crates/catalog/rest/src/auth/mod.rs:
##########
@@ -73,6 +75,26 @@ pub trait AuthManager: Debug + Send + Sync {
client: &HttpClient,
props: &HashMap<String, String>,
) -> Result<Arc<dyn AuthSession>>;
+
+ /// Returns the authentication session for a specific context.
+ ///
+ /// The catalog calls this method only after [`Self::catalog_session`] has
+ /// succeeded. `catalog_session` is the catalog session returned by this
+ /// manager. If the context does not require different authentication,
+ /// implementations should return `catalog_session` unchanged.
+ ///
+ /// The catalog does not cache the returned session. Implementations should
+ /// cache context-specific sessions internally using
+ /// [`SessionContext::session_id`] and are responsible for eviction and
+ /// releasing any associated resources. Reusing a session ID with different
+ /// context may therefore return the previously cached session.
+ async fn contextual_session(
Review Comment:
Not blocking — more a design question for the series. The doc asks
implementors to cache sessions and "release any associated resources," but the
trait gives them no signal for when to do that. Java's `AuthManager extends
AutoCloseable` and `OAuth2Manager.close()` calls `sessionCache.invalidateAll()`
for exactly this.
In a long-lived multi-tenant catalog, a per-context cache grows unbounded
with no teardown hook. Worth a default `async fn close(&self) -> Result<()> {
Ok(()) }` now, or at least a doc note that `Drop` is the intended cleanup
point? Happy to leave it for a follow-up if that's the plan.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -891,13 +937,14 @@ impl RestSessionCatalog {
impl SessionCatalog for RestSessionCatalog {
async fn list_namespaces(
&self,
- _context: &SessionContext,
+ context: &SessionContext,
parent: Option<&NamespaceIdent>,
) -> Result<Vec<NamespaceIdent>> {
let client = self.client().await?;
let endpoint = client.config.namespaces_endpoint();
let mut namespaces = Vec::new();
let mut next_token = None;
+ let session = client.contextual_session(context).await?;
Review Comment:
One subtle thing — we grab the contextual session once here and `Arc::clone`
it across every page. If an implementation hands back a short-lived token and a
listing spans more pages than the token's TTL, the later pages go out with
stale auth and surface as a surprise 401 deep in the loop rather than at the
call.
Probably fine for now since no built-in manager does short-lived contextual
tokens yet, but worth either re-deriving per page or a doc line telling impls a
session may be reused across an operation's requests. wdyt? (Same applies to
`list_tables`.)
##########
crates/catalog/rest/src/auth/mod.rs:
##########
@@ -73,6 +75,26 @@ pub trait AuthManager: Debug + Send + Sync {
client: &HttpClient,
props: &HashMap<String, String>,
) -> Result<Arc<dyn AuthSession>>;
+
+ /// Returns the authentication session for a specific context.
+ ///
+ /// The catalog calls this method only after [`Self::catalog_session`] has
+ /// succeeded. `catalog_session` is the catalog session returned by this
+ /// manager. If the context does not require different authentication,
+ /// implementations should return `catalog_session` unchanged.
Review Comment:
Two things the implementor guidance leaves unsaid that I think are worth a
sentence each, since the OAuth2 impl in #3170 will hit both.
First, `contextual_session` gets no `HttpClient` — an implementation that
needs one for a token exchange has to stash a clone of the `client` passed to
`catalog_session`. That's the intended pattern (Java holds the client
internally too), but nothing here says so.
Second, the catalog doesn't serialize concurrent calls with the same
`session_id`, so a caching impl without its own synchronization can race and
build several sessions for one context. A note that impls must guard concurrent
creation would save someone that bug.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -463,29 +481,48 @@ impl RestClient {
let http_client = http_client.update_with(&config)?;
// The manager is handed an unauthenticated client: its own
// requests must not be signed by the session it is deriving.
- let session = auth_manager
+ let catalog_session = auth_manager
.catalog_session(
&http_client.without_auth_session(),
&Self::auth_props(&config),
)
.await?;
Ok(Self {
+ auth_manager,
+ catalog_session,
config,
- http_client: http_client.with_auth_session(session),
+ http_client,
endpoints,
})
}
/// Testing only: the bearer token the catalog session would attach.
#[cfg(test)]
async fn token(&self) -> Option<String> {
- self.http_client.token().await
+ self.http_client
+ .with_auth_session(Arc::clone(&self.catalog_session))
+ .token()
+ .await
+ }
+
+ /// Derives the authentication session for one catalog operation.
+ async fn contextual_session(&self, context: &SessionContext) ->
Result<Arc<dyn AuthSession>> {
+ self.auth_manager
+ .contextual_session(context, Arc::clone(&self.catalog_session))
+ .await
}
- /// Sends `request`, authenticated by the client's session.
- async fn query_catalog(&self, request: HttpRequest) ->
Result<HttpResponse> {
- self.http_client.query_catalog(request).await
+ /// Sends `request` with `session`.
+ async fn query_catalog(
+ &self,
+ session: Arc<dyn AuthSession>,
+ request: HttpRequest,
+ ) -> Result<HttpResponse> {
+ self.http_client
+ .with_auth_session(session)
Review Comment:
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:
```rust
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.
##########
crates/catalog/rest/src/auth/mod.rs:
##########
@@ -73,6 +75,26 @@ pub trait AuthManager: Debug + Send + Sync {
client: &HttpClient,
props: &HashMap<String, String>,
) -> Result<Arc<dyn AuthSession>>;
+
+ /// Returns the authentication session for a specific context.
+ ///
+ /// The catalog calls this method only after [`Self::catalog_session`] has
+ /// succeeded. `catalog_session` is the catalog session returned by this
+ /// manager. If the context does not require different authentication,
+ /// implementations should return `catalog_session` unchanged.
+ ///
+ /// The catalog does not cache the returned session. Implementations should
+ /// cache context-specific sessions internally using
+ /// [`SessionContext::session_id`] and are responsible for eviction and
+ /// releasing any associated resources. Reusing a session ID with different
+ /// context may therefore return the previously cached session.
+ async fn contextual_session(
+ &self,
+ _context: &SessionContext,
Review Comment:
Small one, but the underscore here fights the doc. The docstring tells
implementors to key their cache off `SessionContext::session_id`, yet
`_context` renders in the generated public API and in rust-analyzer as "this
argument is unused" — the opposite message.
I'd drop the underscore from the trait signature and silence the default
body instead:
```rust
async fn contextual_session(
&self,
context: &SessionContext,
catalog_session: Arc<dyn AuthSession>,
) -> Result<Arc<dyn AuthSession>> {
let _ = context;
Ok(catalog_session)
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
