Repository navigation
Conversation
Test init catalog session requirement Test session caching by id
Test no caching with 0 TTL
|
@plusplusjiajia would be great if I could get your eyes on this! 🙏 (the CI failure is unrelated, I'm looking into it) |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
This is a nice piece of work — the OAuth2Params → OAuth2Config split reads cleanly, the CachedToken expiry handling for #3160 is the right shape, and the tests are refreshingly deterministic (mockito + expires_in: 0 instead of sleeps). contextual_session tracks Java's contextualSession closely too.
I don't think it should merge yet, though, mostly around the contextual cache.
The one that worries me most: the cache keys only on session_id and never compares the incoming credential on a hit. A session_id reused with a rotated or different token silently keeps the old session — a static context token is never refreshed and keeps getting sent until it 401s or the idle window (1h default) evicts it, and if two contexts ever collide on an id, one tenant gets the other's bearer. test_context_token_takes_precedence_and_is_cached_by_session_id locks that in as desired. I'd fold a fingerprint of the credential into the key (or store it in the entry and rebuild on mismatch), and state the "session_id is unique per principal" contract explicitly like Java does.
Second, catalog_session's one-shot init isn't atomic with its side effects: build_session_from_properties has already written self.token by the time set() loses or session_idle_timeout errors, so a failed or duplicate call mutates shared state past the error. Validating the timeout first and committing state with a single set() fixes it.
Before merge, I'd want:
- credential folded into the cache key (or rebuild-on-mismatch), with the session-id contract documented
- a
max_capacity(and ideally atime_to_live) on the cache — right now it's unbounded and moka only evicts on access, so a per-requestsession_idgrows memory for the full idle window - the parent-authenticate-first path tightened: a static-token context shouldn't trigger or await a parent exchange, and the
Nonearm shouldn't fall through to the parent's tenant token - the timeout validated before any token mutation in
catalog_session - a concurrency test for the
or_insertwinner-reuse claim, plus the second-catalog_sessionand credential-isolation cases
The fail-open on unrecognized credential keys (a context carrying only an id_token runs as the catalog identity) is worth at least a doc + warning this pass too. The rest in the inline comments are nits. One more thing worth a line in the PR description: nothing outside auth/ calls contextual_session yet, so this is validated through unit tests rather than an end-to-end request — either add one integration test or note the scope.
Once the cache keying and the catalog_session ordering are sorted I'll take another pass.
| ) | ||
| })?; | ||
|
|
||
| if let Some(session) = contextual_state.session_cache.get(context.session_id()) { |
There was a problem hiding this comment.
A cache hit returns the cached session without ever comparing the incoming token/credential to the one it was built from, so a session_id reused with a different or rotated credential silently gets the old session. For a static context token that means it's never refreshed — any session touched once per idle window (1h default) keeps sending the stale token until it 401s — and if two contexts ever collide on an id, one tenant gets the other's bearer. test_context_token_takes_precedence_and_is_cached_by_session_id locks this in as the intended behavior.
I'd fold a fingerprint of the credential (hash of kind + value) into the key, or store the credential in the entry and rebuild on mismatch. Java gets away with id-only keying because its contract is that session_id is unique per principal — if we're relying on that too, let's state it explicitly in the trait doc.
| props: &HashMap<String, String>, | ||
| ) -> Result<Arc<dyn AuthSession>> { | ||
| Ok(Arc::new(self.session_from(client, props).await?)) | ||
| if self.contextual_state.get().is_some() { |
There was a problem hiding this comment.
The get().is_some() pre-check isn't atomic with the later set(), and more importantly build_session_from_properties has already written self.token (when props carries token) by the time set() loses the race or session_idle_timeout(props)? errors — so a failed or duplicate call mutates the shared token cell past the error. I'd pull session_idle_timeout(props)? to the first line, build the state fully, then commit with a single set() as the only gate.
Separately, this now fails permanently on a second call — worth confirming no builder path (with_auth_manager followed by multiple load()s, or a retried RestClient::init) can reach it twice, and documenting the one-shot contract.
| /// token, no authentication is attached. | ||
| async fn authenticate(&self, req: &mut HttpRequest) -> Result<()> { | ||
| if let Some(parent) = &self.parent { | ||
| parent.authenticate(req).await?; |
There was a problem hiding this comment.
Every contextual request runs parent.authenticate(req) first and then overwrites Authorization. For a static-token context that's a wasted parent call; if the catalog session uses client credentials it can trigger or wait on a parent token exchange on each request, and a parent failure then breaks a request that carried a perfectly valid contextual token. There's also a latent leak — if the contextual token is ever absent, the None => Ok(()) path leaves the parent's tenant token on the request.
The credential exchange already authenticates through with_auth_session(catalog_session), so the session-level parent is only needed to carry the parent's non-auth headers. I'd set parent only where that's actually required, or strip Authorization before calling the parent.
| .set(ContextualAuthState { | ||
| token_exchange, | ||
| session_cache: Cache::builder() | ||
| .time_to_idle(session_idle_timeout(props)?) |
There was a problem hiding this comment.
The cache sets only time_to_idle — no max_capacity and no time_to_live — and moka's sync cache only runs maintenance on access, so an idle manager never frees anything. In a multi-tenant server with a per-user or per-request session_id, memory grows with every distinct id seen inside the idle window (1h default, and the property goes up to the 1000-year cap). I'd add a max_capacity (configurable, something like 1k–10k) and ideally a time_to_live to bound absolute age.
| let session = contextual_state | ||
| .session_cache | ||
| .entry(context.session_id().to_string()) | ||
| .or_insert(candidate_session) // If a concurrent insert won, we'll use it instead because it may already be in use. |
There was a problem hiding this comment.
This comment makes a concurrency claim — winner is reused, single exchange — that nothing tests. I'd add a join_all test on a multi-thread runtime with .expect(1) on the token mock so the single-exchange guarantee is actually pinned.
A few other gaps worth closing this pass: a second catalog_session call hitting PreconditionFailed, two different credential contexts exchanging separately (we cover token isolation but not credential isolation), and a token-endpoint failure propagating and then retrying.
| } else if let Some(credential) = context.credentials().get("credential") { | ||
| ContextualCredential::ClientCredentials(parse_contextual_credential(credential)) | ||
| } else { | ||
| return Ok(catalog_session); |
There was a problem hiding this comment.
A context carrying only an id_token / access_token / jwt — the RFC 8693 token-type keys Java's maybeCreateChildSession handles — falls through here and runs with the shared catalog identity, with no error or log. That's a fail-open: a caller who expected tenant identity silently gets the catalog credential. At minimum I'd document the supported keys; better would be to fail closed (or warn) when a context has credentials but none are recognized.
|
|
||
| let timeout_ms = timeout_prop.parse::<u64>().map_err(|e| { | ||
| Error::new( | ||
| ErrorKind::PreconditionFailed, |
There was a problem hiding this comment.
A malformed auth.session-timeout-ms is a bad user-supplied property — the rest of the crate (and CachedToken::from_response just below) uses DataInvalid for that, not PreconditionFailed. I'd switch both arms in session_idle_timeout to DataInvalid unless PreconditionFailed is deliberate. While we're here, the second format! in this function already inlines its arg, so this one can be format!("Property {AUTH_SESSION_TIMEOUT_MS_PROP} not an integer") to satisfy uninlined_format_args.
| /// explicitly configured): it is then recomputed from the merged URI in | ||
| /// [`Self::catalog_session`], since `/v1/config` may override the URI. | ||
| endpoint_is_default: bool, | ||
| /// Installed by `catalog_session`; replacing it atomically invalidates |
There was a problem hiding this comment.
contextual_state is a OnceLock, so it can never be replaced — this comment describes a design that was dropped. Reword to something like "set once by catalog_session; contextual sessions require it."
| } | ||
|
|
||
| fn parse_contextual_credential(credential: &SensitiveString) -> (Option<String>, SensitiveString) { | ||
| match credential.expose().split_once(':') { |
There was a problem hiding this comment.
split_once(':') on ":secret" yields Some("") as the client_id, which then goes out as client_id= in the form body (Java behaves the same, so it's not a divergence). Worth rejecting an empty client_id — and either way the colon-less / :secret / id: cases have no coverage, so a couple of tests around this helper would be good.
| minijinja = "2.12.0" | ||
| mockall = "0.13.1" | ||
| mockito = "1" | ||
| moka = { version = "0.12.10" } |
There was a problem hiding this comment.
This can just be moka = "0.12.10" — the braces are redundant. crates/iceberg and integrations/cache-moka still pin 0.12.10 directly with their own features; optionally move them onto workspace = true while you're here so there's a single source.
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Which issue does this PR close?
This is based on token expiration from #3160 and the
AuthManager::contextual_sessionfrom #3081. It's adding adding a first implementation for contextual sessions with the OAuth2Manager. It also validates the API introduced by that PR.What changes are included in this PR?
The OAuth2Manager now derives contextual sessions from its catalog session. Contextual sessions are cached by their session ID (that's why the token expiration PR is required to merge first). The catalog session is now expected to be initialized only once.
Effectively, users will be able to use different client credentials per request in a multi-tenant setting that leverages OAuth2.
Are these changes tested?
Yes, added tests.
AI Disclosure
I used AI to help me with code, tests and documentation.