markhoerth commented on PR #11182: URL: https://github.com/apache/gravitino/pull/11182#issuecomment-5384125154
This has been sitting without review since May, which is a shame, because the deployment pattern it describes is real and I ran into the same one independently. I have a branch that overlaps with this PR and I would rather merge with it than compete with it. What I built separately: the same `CustomTokenProvider` path, plus per-identity caching in `GravitinoCatalogManager`, because the connector's catalog cache is keyed on catalog name alone and never evicts. Once more than one identity can reach a shared driver, the first session to touch a catalog populates that cache and later sessions are served its entry, including the REST client it carries. To be clear about the exposure, that is not a live hole on main: there is only ever one client per driver today, so there is no second identity to confuse. The half that does bite today is that the cache never evicts, so a grant revoked in Gravitino keeps being honoured for the life of the driver under any auth type. The cache-key half is inert until a per-user credential mode exists, which is exactly what this PR adds, and that is my argument for landing both together rather than either alone. On configuration naming, I think you are right and I was wrong. I had used a bare `spark.sql.gravitino.tokenFile`. Yours namespaces under the auth type, which is the convention `AuthProperties` already follows with `basic.username` and `oauth2.serverUri`. Concretely I would suggest: ```properties spark.sql.gravitino.authType=token spark.sql.gravitino.token.file=/path/to/token spark.sql.gravitino.token.value=<inline token> ``` The one place I would push back is the auth type string itself, `bearer-token-file`. Two reasons: 1. That string lands in `AuthProperties`, which the Trino and Flink connectors share. The other four are named for the mechanism (`simple`, `basic`, `oauth2`, `kerberos`) rather than for where the credential came from. `bearer-token-file` bakes the source into the mechanism, which forces a second auth type the first time someone wants to pass a token inline instead of through a file. 2. An inline token is worth supporting. Not every runtime writes a file, and it is a two-line addition once `token.file` and `token.value` sit under one auth type, with the file taking precedence. Keeping the property suffixes in `AuthProperties` rather than in `GravitinoSparkConfig` would also let Trino reuse them without redefining the strings. Two things your PR does that mine does not, and that should survive any merge: stripping an existing `Bearer ` prefix from the file contents, and rejecting an empty token file. I would also suggest resolving the token on every request rather than once, so a sidecar rotating the file is picked up without restarting the driver, and reading the active `SparkSession` configuration in preference to the `SparkConf` so a shared driver can carry a different token per session. `HTTPClient` calls `authDataProvider.getTokenData()` inline while assembling each request, on the caller's thread, so that works without a refresh thread. How would you like to handle this? I am happy to either send you a patch against your branch, or open a PR that leaves the auth type entirely to you and carries only the caching half. Your call, it is your issue and your 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
