You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "DerGut (via GitHub)" <gi...@apache.org> on 2026/08/26 22:21:40 UTC
[PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
DerGut opened a new pull request, #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Another piece that's working towards https://github.com/apache/iceberg-rust/issues/2774.
## What changes are included in this PR?
This PR a a new `pub` trait method to the `iceberg_catalog_rest::AuthManager` with a default implementation. The new `AuthManager::contextual_session` allows implementors to convert per catalog operation `SessionContext` into HTTP-level authentication metadata.
## Are these changes tested?
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## AI Disclosure
<!--
https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions
-->
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#issuecomment-5589749932
This PR should now be ready for review, it also has it's API validated by https://github.com/apache/iceberg-rust/pull/3170 but doesn't depend on it to be merged in any way 👍
cc @CTTY
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3994265831
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
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)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3962771293
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
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`?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -687,13 +724,15 @@ impl Catalog for RestCatalog {
/// REST catalog implementation of [`SessionCatalog`].
///
-/// Each operation accepts a [`SessionContext`]. REST configuration, authentication sessions,
-/// and the HTTP client are initialized lazily once per catalog and shared across all operations.
+/// Each operation accepts a [`SessionContext`]. REST configuration, the auth manager, the catalog
+/// authentication session, and the HTTP client are initialized lazily once per catalog. Each
+/// REST request uses the contextual authentication session returned by the manager.
#[derive(Debug)]
pub struct RestSessionCatalog {
- /// Injected through [`RestSessionCatalogBuilder::with_auth_manager`]; otherwise
- /// one is resolved from `rest.auth.type` when the client is built.
- auth_manager: Option<Arc<dyn AuthManager>>,
+ /// Builder-supplied override retained so lazy client initialization can
+ /// clone it into the runtime state. When absent, a manager is resolved from
+ /// `rest.auth.type` during initialization.
+ auth_manager_override: Option<Arc<dyn AuthManager>>,
Review Comment:
naming is hard and I like the new name more!
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -829,9 +869,15 @@ impl RestSessionCatalog {
/// Issue a `HEAD` request to `url` and interpret it as an existence check:
/// `2xx` means it exists, `404` means it doesn't.
- async fn check_exists_via_head(&self, client: &RestClient, url: String) -> Result<bool> {
+ async fn check_exists_via_head(
+ &self,
+ context: &SessionContext,
Review Comment:
Same here, seems like we can pass the pre-built contextual session to this function directly
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -743,7 +782,8 @@ impl RestSessionCatalog {
}
let request = HttpRequest::build(request_builder)?;
- let http_response = client.query_catalog(request).await?;
+ let session = client.contextual_session(context).await?;
Review Comment:
We can build the contextual session using the auth manager here
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3994301830
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
Rename commit -> https://github.com/apache/iceberg-rust/pull/3081/changes/8785ac886f39d4f9f5aab2ea9598c59c79f983ff
Let me know what you think 🙏
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3973924777
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
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 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3973924777
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
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
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 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4209460563
##########
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:
The client reuse is already stated on the [`AuthManager`'s doc comment](https://github.com/DerGut/iceberg-rust/blob/15ef053d8fc27de5258689ec8c9f05112737cd42/crates/catalog/rest/src/auth/mod.rs#L50-L55):
> ```rust
> /// [`Self::init_session`] and [`Self::catalog_session`] are handed the
> /// catalog's [`HttpClient`], which an implementation may reuse for its own
> /// requests (e.g. a token exchange) so that they share the catalog's
> /// connection pool and configuration.
> ```
I made both more specific in https://github.com/apache/iceberg-rust/pull/3081/commits/bb87e36983441ebd39e73bec6f76f3e2ecc4ae39
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4209104209
##########
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:
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.
https://github.com/apache/iceberg-rust/pull/3081/commits/d33821a17f28f6a239bb199a4d50a1fb3a0b7022 and https://github.com/apache/iceberg-rust/pull/3081/commits/085bf7988a7fa76eed0adff19d9b403f9e34c199 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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4206630201
##########
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:
You're right! The comment was derived from the Java interface. With the default `Dropper`, it's not really relevant for Rust implementors and I removed it in https://github.com/apache/iceberg-rust/pull/3081/changes/8d137319ee53d0497e2c4faa2019128030fb3d45
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "alexanderbianchi (via GitHub)" <gi...@apache.org>.
alexanderbianchi commented on PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#issuecomment-5649361931
@CTTY Looks like auto-merge was disabled
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4206630201
##########
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:
You're right! The comment was derived from the Java interface. With Rust we get a behavior like `AutoCloseable` out of the box without an explicit API contract. I removed the comment in https://github.com/apache/iceberg-rust/pull/3081/changes/8d137319ee53d0497e2c4faa2019128030fb3d45
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4208254063
##########
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:
Addressed in https://github.com/apache/iceberg-rust/pull/3081/commits/93a2b8724c0b2fa4fc6fe6d8ca0c7548ce9d15ea
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY merged PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3994117445
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3994301830
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
Rename commit -> https://github.com/apache/iceberg-rust/pull/3081/commits/dd6aaaa846fd0d95a1fe90028ed9b257919025ea
Let me know what you think 🙏
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4206616863
##########
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:
Good point! Addresed it in https://github.com/apache/iceberg-rust/pull/3081/changes/db6d1069689b243dfaaf2015c06f21526b35753a
##########
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:
Good point! Addressed it in https://github.com/apache/iceberg-rust/pull/3081/changes/db6d1069689b243dfaaf2015c06f21526b35753a
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4206790648
##########
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:
https://github.com/apache/iceberg-rust/pull/3081/commits/3046937e553280ef394b71224535422b39ea02d7
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4206630201
##########
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:
You're right! With Rust we get a good default behavior already without an explicit API contract. I removed the comment about releasing resources in https://github.com/apache/iceberg-rust/pull/3081/changes/8d137319ee53d0497e2c4faa2019128030fb3d45
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4209104209
##########
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:
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.
https://github.com/apache/iceberg-rust/pull/3081/commits/d33821a17f28f6a239bb199a4d50a1fb3a0b7022 and https://github.com/apache/iceberg-rust/pull/3081/commits/15ef053d8fc27de5258689ec8c9f05112737cd42 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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#issuecomment-6071384341
Oops, I thought this was merged in a while ago. Thanks Andrei for the additional review! Now with all comments addressed I'm adding this to the merge queue now
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3963220466
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
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 👌
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3994241186
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r3994241186
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -416,10 +418,14 @@ pub(crate) fn oauth_params_from_props(props: &HashMap<String, String>) -> HashMa
params
}
-#[derive(Debug)]
struct RestClient {
- /// Carries the session the auth manager derived from the merged
- /// configuration, so every request below is authenticated.
+ /// The manager that created `catalog_session`; retained so each request can
+ /// derive authentication for its [`SessionContext`].
+ auth_manager: Arc<dyn AuthManager>,
+ /// The catalog-wide session passed to [`AuthManager::contextual_session`].
+ catalog_session: Arc<dyn AuthSession>,
Review Comment:
I'd rename it to RestCatalogClient for now, will push a commit in a few minutes.
I have some refactors in mind that can go on top of this PR
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4209460563
##########
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:
The client reuse is already stated on the [`AuthManager`'s doc comment](https://github.com/DerGut/iceberg-rust/blob/15ef053d8fc27de5258689ec8c9f05112737cd42/crates/catalog/rest/src/auth/mod.rs#L50-L55):
> ```rust
> /// [`Self::init_session`] and [`Self::catalog_session`] are handed the
> /// catalog's [`HttpClient`], which an implementation may reuse for its own
> /// requests (e.g. a token exchange) so that they share the catalog's
> /// connection pool and configuration.
> ```
I made both more specific in https://github.com/apache/iceberg-rust/pull/3081/commits/9267df86a510ffca097b5991a691e907a26a6a42
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#issuecomment-6042447104
Thanks for the review @laskoviymishka 🙇 I should have addressed all your comments!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest_catalog::auth) Contextual Sessions [iceberg-rust]
Posted by "DerGut (via GitHub)" <gi...@apache.org>.
DerGut commented on code in PR #3081:
URL: https://github.com/apache/iceberg-rust/pull/3081#discussion_r4206769098
##########
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:
That's true. This was my original implementation that I revamped with https://github.com/apache/iceberg-rust/pull/3081/changes/32a9dc5f2f1e35012bdcc48fcd9167af0fcc585b. Let me revert that commit.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org