markhoerth opened a new pull request, #12570: URL: https://github.com/apache/gravitino/pull/12570
### What changes were proposed in this pull request? Two changes to the Spark connector that only make sense together. **1. Give the caches an eviction policy.** `GravitinoCatalogManager`'s catalog cache was built with a bare `Caffeine.newBuilder().build()`, so it had no bound and no expiry. It now expires by write, controlled by `spark.sql.gravitino.catalogCacheTtlSec`. **2. Add a `token` auth type, and key the caches on the identity that token carries.** The single `GravitinoClient` built at driver init becomes a per-identity cache of clients, bounded and expiring by access, closing each client on eviction so it does not leak its HTTP connection pool. The catalog cache key becomes `identity.key() + ":" + catalogName`. The token is resolved on every request rather than captured once, reading the active Spark session's configuration in preference to the application's, and `spark.sql.gravitino.token.file` in preference to `spark.sql.gravitino.token.value`. Identity comes from the JWT claim named by `spark.sql.gravitino.token.principalFields`, defaulting to `sub`. The signature is not verified, because validating the token is the server's job and this value only partitions a cache. Opaque, non-JWT tokens fall back to a hash of the token. `GravitinoCatalogManager` stays a singleton and `getGravitinoCatalogInfo(String)` keeps its signature, so `BaseCatalog` is untouched. ### Why are the changes needed? The half that bites today, under any auth type, is the missing eviction. `loadCatalog` is a REST call the server authorizes; once it has succeeded, the result is served for the remaining life of the driver and no further request reaches Gravitino. A grant revoked in Gravitino therefore keeps being honoured by a running Spark application. The identity-blind cache key is not exploitable on main, because `simple`, `basic`, `oauth2` and `kerberos` all resolve exactly one credential per application, so there is never a second identity to confuse. It arms the moment a per-user credential mode exists. The cached value is not inert metadata: `GravitinoMetalake.loadCatalog` returns `DTOConverters.toCatalog(name, dto, restClient)`, the returned `RelationalCatalog` stores that client in `BaseSchemaCatalog`'s `protected final RESTClient restClient`, `asTableCatalog()` returns `this`, and `listTables` issues its request on that same field. So a session served another session's cached entry would transmit under the other session's credential. That is the argument for landing both halves in one PR. Eviction without an identity-aware key still mixes users once per-user tokens exist. An identity-aware key without eviction still serves revoked grants. There is also no existing way to present a bearer token obtained outside Spark, which is the gap #11181 describes, and adding one is precisely what makes the cache key defect live. Fix: #12569 Related: #11181 and its PR #11182 ask for the bearer-token-file half of this. ### Does this PR introduce _any_ user-facing change? New property keys, all optional, no change to existing behaviour: - `spark.sql.gravitino.authType=token`, a fifth valid value alongside the existing four. - `spark.sql.gravitino.token.value` and `spark.sql.gravitino.token.file`, the file taking precedence. - `spark.sql.gravitino.token.principalFields`, default `sub`, intended to match the server's `gravitino.authenticator.oauth.principalFields`. - `spark.sql.gravitino.clientCacheMaxSize` (100), `spark.sql.gravitino.clientCacheTtlSec` (3600), `spark.sql.gravitino.catalogCacheTtlSec` (300). The property suffixes are added to `AuthProperties` next to `basic.username` and `oauth2.serverUri`, namespaced by auth type, so the Trino connector can reuse them. `simple`, `basic`, `oauth2` and `kerberos` keep a single application-wide identity and are unaffected, other than the catalog cache now expiring, which is the intended fix. Two limits worth stating rather than hiding, both documented in `docs/spark-connector/spark-connector.md`: - The catalog list registered with Spark at driver startup is resolved with the application's identity, so a user may see a catalog name they are not then allowed to open. - This governs metadata resolution only. Executors read data with the credentials the underlying catalog was built with. ### How was this patch tested? `./gradlew :spark-connector:spark-common:test -PskipITs` is green, 124 tests. New `TestGravitinoCatalogManager` covers seven cases: two subjects with different `sub` build two clients and issue two loads; the same `sub` shares one client and one load; opaque non-JWT tokens partition by token value; `authType=simple` keeps one identity across two sessions; `close()` closes every cached client; exceeding `clientCacheMaxSize` evicts and closes; and a catalog entry past `catalogCacheTtlSec` is reloaded rather than served stale. The first of those is the regression test for the cache key. I checked that it actually bites by reverting `cacheKey` to return the catalog name alone, which fails four of the seven, then restored it. New cases in `TestGravitinoDriverPlugin` cover the provider: `Bearer <token>` as UTF-8 bytes, re-resolution per request so a changed value is picked up, `token.file` taking precedence and being re-read after a rewrite, and a missing token failing with a message naming both properties. Separately, to confirm that a cached `Catalog` really does carry the client that loaded it rather than re-resolving one, I ran a throwaway probe against the `client-java` MockServer harness: two clients with distinct bearer tokens, catalog loaded by A, handed to B through a plain map, `listTables()` called by B. The request went out as `Bearer TOKEN-ALICE`, while the control of B calling through its own client went out as `Bearer TOKEN-BOB`. That probe is not included here, since it tests client behaviour that is working as designed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01WAqUbBPofTzWbFYD8EGu1R -- 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]
