diqiu50 opened a new pull request, #12543:
URL: https://github.com/apache/gravitino/pull/12543

   ### What changes were proposed in this pull request?
   
   - Add `trino.jdbc.ssl.enabled`, `trino.jdbc.ssl.truststore.path`, 
`trino.jdbc.ssl.truststore.password`, `trino.jdbc.ssl.truststore.type`, 
`trino.jdbc.ssl.verification` and `trino.jdbc.roles` to configure the internal 
JDBC connection to the Trino coordinator.
   - Add the `trino.jdbc.properties.` prefix to pass arbitrary Trino JDBC 
driver properties through, overriding the values derived from the dedicated 
properties.
   - `trino.jdbc.ssl.enabled` defaults to the scheme of the Trino 
`discovery.uri`, so a coordinator whose `discovery.uri` is `https://...` needs 
no explicit setting. `trino.jdbc.ssl.verification` defaults to `FULL`.
   - Validate the SSL configuration before connecting, so an invalid 
verification mode, a truststore configured without TLS, or a missing truststore 
file fails with a clear message instead of an opaque handshake error.
   - Stop propagating `trino.jdbc.*` into the generated `CREATE CATALOG` 
statement. These properties are used by the coordinator only, and the statement 
is logged and persisted to the Trino catalog properties files.
   
   ### Why are the changes needed?
   
   `CatalogRegister.init()` only sets `user` and `password`, so the connection 
cannot be established against a TLS-enabled or authorization-enabled 
coordinator, and forking the connector was the only workaround.
   
   Fix: #12542
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes, seven new optional configuration properties, documented in 
`docs/trino-connector/configuration.md`. All default to the current behavior.
   
   One behavior change: `trino.jdbc.*` properties are no longer copied into the 
catalogs the connector creates. They are only used by the coordinator to 
connect back to Trino, so no functionality depends on them being propagated, 
and this stops `trino.jdbc.password` from reaching the logs and the catalog 
properties files.
   
   ### How was this patch tested?
   
   - New `TestCatalogRegister` covering the JDBC property mapping, the 
verification defaults, the passthrough prefix and every validation failure.
   - New cases in `TestGravitinoConfig`, including a regression test that 
`toCatalogConfig()` emits no `trino.jdbc.` key.
   - Verified end to end against a coordinator started with 
`http-server.https.enabled` and a generated self-signed keystore: with only the 
truststore path and password configured, the TLS handshake, `SHOW CATALOGS` and 
`CREATE CATALOG` all succeeded under `SSLVerification=FULL`, and the logged 
statement contained no `trino.jdbc.` key.
   
   Note: `trino.jdbc.roles=system:sysadmin` cannot be verified end to end on 
open-source Trino, whose default access control rejects it with `Access Denied: 
Cannot set role sysadmin`. The tests confirm the property is passed to the 
driver and applied to the session; the authorization path needs a coordinator 
that provides system roles.
   


-- 
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]

Reply via email to