yuqi1129 commented on code in PR #12543:
URL: https://github.com/apache/gravitino/pull/12543#discussion_r3841815537
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java:
##########
@@ -340,6 +441,119 @@ public String getTrinoPassword() {
return config.getOrDefault(TRINO_JDBC_PASSWORD.key,
TRINO_JDBC_PASSWORD.defaultValue);
}
+ /**
+ * Returns whether the internal JDBC connection to the Trino coordinator
uses TLS.
+ *
+ * <p>If `trino.jdbc.ssl.enabled` is not set, the value is derived from the
scheme of the Trino
+ * `discovery.uri`, which is `https` on a TLS enabled coordinator.
+ *
+ * @return true if the internal JDBC connection uses TLS
+ */
+ public boolean isTrinoJdbcSslEnabled() {
+ String value = config.get(TRINO_JDBC_SSL_ENABLED.key);
+ if (StringUtils.isNotBlank(value)) {
+ return Boolean.parseBoolean(value.trim());
Review Comment:
`Boolean.parseBoolean` returns false for anything that is not `true`, so
`trino.jdbc.ssl.enabled=1` or `yes` silently means TLS off. This is worse than
not setting it at all, because with `discovery.uri=https://...` the derived
value would have been true. And if a truststore is also set, the error says
`requires TLS to be enabled either by an HTTPS 'discovery.uri' or by
'trino.jdbc.ssl.enabled=true'`, but the `discovery.uri` **is** HTTPS, so the
message points the user in the wrong direction. Can we accept only `true` and
`false` and fail on anything else?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogRegister.java:
##########
@@ -104,6 +116,179 @@ public void init(GravitinoConfig config) throws Exception
{
}
}
+ /**
+ * Builds the JDBC properties used by the internal connection to the Trino
coordinator.
+ *
+ * <p>The properties derived from the dedicated {@code trino.jdbc.*}
configurations are applied
+ * first, then the raw driver properties configured with the {@code
trino.jdbc.properties.} prefix
+ * are applied on top of them, so that any driver property can be overridden.
+ *
+ * @param config the Gravitino configuration
+ * @return the JDBC properties
+ */
+ @VisibleForTesting
+ static Properties buildJdbcProperties(GravitinoConfig config) {
+ boolean sslEnabled = config.isTrinoJdbcSslEnabled();
+ String verification = config.getTrinoJdbcSslVerification();
+ String truststorePath = config.getTrinoJdbcSslTruststorePath();
+ String truststorePassword = config.getTrinoJdbcSslTruststorePassword();
+ String truststoreType = config.getTrinoJdbcSslTruststoreType();
+ String keystorePath = config.getTrinoJdbcSslKeystorePath();
+ String keystorePassword = config.getTrinoJdbcSslKeystorePassword();
+ String keystoreType = config.getTrinoJdbcSslKeystoreType();
+ String roles = config.getTrinoJdbcRoles();
+
+ validateSslConfig(
+ sslEnabled,
+ verification,
+ truststorePath,
+ truststorePassword,
+ truststoreType,
+ keystorePath,
+ keystorePassword,
+ keystoreType);
+
+ Properties properties = new Properties();
+ properties.put("user", config.getTrinoUser());
+ String password = config.getTrinoPassword();
+ if (StringUtils.isNotEmpty(password)) {
+ properties.put("password", password);
+ }
+
+ if (sslEnabled) {
+ properties.put("SSL", "true");
+ properties.put("SSLVerification", verification);
+ if (StringUtils.isNotBlank(truststorePath)) {
+ properties.put("SSLTrustStorePath", truststorePath);
+ }
+ if (StringUtils.isNotEmpty(truststorePassword)) {
+ properties.put("SSLTrustStorePassword", truststorePassword);
+ }
+ if (StringUtils.isNotBlank(truststoreType)) {
+ properties.put("SSLTrustStoreType", truststoreType);
+ }
+ if (StringUtils.isNotBlank(keystorePath)) {
+ properties.put("SSLKeyStorePath", keystorePath);
+ }
+ if (StringUtils.isNotEmpty(keystorePassword)) {
+ properties.put("SSLKeyStorePassword", keystorePassword);
+ }
+ if (StringUtils.isNotBlank(keystoreType)) {
+ properties.put("SSLKeyStoreType", keystoreType);
+ }
+ }
+
+ if (StringUtils.isNotBlank(roles)) {
+ properties.put("roles", roles);
+ }
+
+ Map<String, String> extraProperties = config.getTrinoJdbcExtraProperties();
+ if (!extraProperties.isEmpty()) {
+ // Log the names only, the values may contain credentials.
+ LOG.debug("Applying extra Trino JDBC properties: {}",
extraProperties.keySet());
+ properties.putAll(extraProperties);
+ }
+ return properties;
+ }
+
+ private static void validateSslConfig(
+ boolean sslEnabled,
+ String verification,
+ String truststorePath,
+ String truststorePassword,
+ String truststoreType,
+ String keystorePath,
+ String keystorePassword,
+ String keystoreType) {
+ if (!SSL_VERIFICATION_MODES.contains(verification)) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT,
+ String.format(
+ "Invalid value for config 'trino.jdbc.ssl.verification':
expected one of %s, got: %s",
+ SSL_VERIFICATION_MODES, verification));
+ }
+
+ if (!sslEnabled) {
+ if (!SSL_VERIFICATION_FULL.equals(verification)) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT,
+ "Config 'trino.jdbc.ssl.verification' requires TLS to be enabled
either by an HTTPS "
+ + "'discovery.uri' or by 'trino.jdbc.ssl.enabled=true'");
+ }
+ checkRequiresSslEnabled("trino.jdbc.ssl.truststore.path",
truststorePath);
+ checkRequiresSslEnabled("trino.jdbc.ssl.truststore.password",
truststorePassword);
+ checkRequiresSslEnabled("trino.jdbc.ssl.truststore.type",
truststoreType);
+ checkRequiresSslEnabled("trino.jdbc.ssl.keystore.path", keystorePath);
+ checkRequiresSslEnabled("trino.jdbc.ssl.keystore.password",
keystorePassword);
+ checkRequiresSslEnabled("trino.jdbc.ssl.keystore.type", keystoreType);
+ return;
+ }
+
+ validateKeystoreConfig(keystorePath, keystorePassword, keystoreType);
+
+ if (StringUtils.isBlank(truststorePath)) {
+ // The driver falls back to the default JVM truststore, which the
password and the type of a
+ // truststore that was never configured have nothing to apply to.
+ checkRequires(
+ "trino.jdbc.ssl.truststore.password",
+ truststorePassword,
+ "trino.jdbc.ssl.truststore.path");
+ checkRequires(
+ "trino.jdbc.ssl.truststore.type", truststoreType,
"trino.jdbc.ssl.truststore.path");
+ return;
+ }
+
+ if (SSL_VERIFICATION_NONE.equals(verification)) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT,
+ "Config 'trino.jdbc.ssl.truststore.path' cannot be used with "
+ + "'trino.jdbc.ssl.verification' = NONE");
+ }
+ if (!Files.exists(Path.of(truststorePath))) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_MISSING_CONFIG,
+ String.format(
+ "The truststore file configured by
'trino.jdbc.ssl.truststore.path' does not exist: %s",
+ truststorePath));
+ }
+ }
+
+ private static void validateKeystoreConfig(
Review Comment:
`validateKeystoreConfig` is missing the `SSLVerification=NONE` check that
the truststore branch has on line 241. So `ssl.enabled=true` +
`ssl.verification=NONE` + `ssl.keystore.path=...` passes our validation, but
the driver rejects it later. I unpacked `trino-jdbc-469.jar` to check:
`ConnectionProperties$SslKeyStorePath` is built with
`SslVerification.validateEnabled(...)`, and that validator compares against
`SslVerificationMode.NONE` with the message `Connection property %s cannot be
set if %s is set to %s`. So the user gets an SQLException at connect time
instead of the clear config error this PR is adding everywhere else. Same for
`SSLKeyStorePassword` and `SSLKeyStoreType`.
##########
docs/trino-connector/configuration.md:
##########
@@ -14,6 +14,16 @@ license: "This software is licensed under the Apache License
version 2."
| gravitino.uri | string |
http://localhost:8090 | The `gravitino.uri` defines the connection URL of the
Gravitino server, the default value is `http://localhost:8090`. Trino connector
can detect and connect to Gravitino server once it is ready, no need to start
Gravitino server beforehand.
| No |
| trino.jdbc.user | string | admin
| The jdbc user name of current Trino.
| NO
|
| trino.jdbc.password | string | (none)
| The jdbc password of current Trino.
| NO
|
+| trino.jdbc.ssl.enabled | boolean | (derived)
| Whether the internal JDBC connection to the Trino coordinator uses TLS. If
not set, it is derived from the scheme of the Trino `discovery.uri`, so a
coordinator whose `discovery.uri` is `https://...` needs no explicit setting.
| No |
+| trino.jdbc.ssl.truststore.path | string | (none)
| Path of the truststore holding the Trino coordinator certificate. If
omitted, the default JVM truststore is used. Requires TLS, which is enabled
automatically for an HTTPS `discovery.uri` or explicitly with
`trino.jdbc.ssl.enabled=true`.
| No |
+| trino.jdbc.ssl.truststore.password | string | (none)
| Password of the truststore configured by `trino.jdbc.ssl.truststore.path`.
Requires TLS and `trino.jdbc.ssl.truststore.path`, otherwise the connector
fails to start.
| No |
+| trino.jdbc.ssl.truststore.type | string | (none)
| Type of the truststore, for example `JKS` or `PKCS12`. If omitted, the
default JVM truststore type is used. Requires TLS and
`trino.jdbc.ssl.truststore.path`, otherwise the connector fails to start.
| No |
+| trino.jdbc.ssl.keystore.path | string | (none)
| Path of the keystore holding the client certificate presented to the
coordinator, for coordinators that require mutual TLS. Requires TLS, which is
enabled automatically for an HTTPS `discovery.uri` or explicitly with
`trino.jdbc.ssl.enabled=true`. See the note on mutual TLS below.
| No |
+| trino.jdbc.ssl.keystore.password | string | (none)
| Password of the keystore configured by `trino.jdbc.ssl.keystore.path`.
Requires `trino.jdbc.ssl.keystore.path`, otherwise the connector fails to
start.
| No |
Review Comment:
These two keystore rows say only "Requires `trino.jdbc.ssl.keystore.path`",
but `checkRequiresSslEnabled` also rejects them when TLS is off, and the driver
rejects them when `verification=NONE`. The truststore rows say "Requires TLS
and ...", so these should say the same.
##########
docs/trino-connector/configuration.md:
##########
@@ -14,6 +14,16 @@ license: "This software is licensed under the Apache License
version 2."
| gravitino.uri | string |
http://localhost:8090 | The `gravitino.uri` defines the connection URL of the
Gravitino server, the default value is `http://localhost:8090`. Trino connector
can detect and connect to Gravitino server once it is ready, no need to start
Gravitino server beforehand.
| No |
| trino.jdbc.user | string | admin
| The jdbc user name of current Trino.
| NO
|
| trino.jdbc.password | string | (none)
| The jdbc password of current Trino.
| NO
|
+| trino.jdbc.ssl.enabled | boolean | (derived)
| Whether the internal JDBC connection to the Trino coordinator uses TLS. If
not set, it is derived from the scheme of the Trino `discovery.uri`, so a
coordinator whose `discovery.uri` is `https://...` needs no explicit setting.
| No |
Review Comment:
The pipes in this table are not aligned. Rows 17-20 are 2 chars wider than
the others and rows 22-26 are 1 char wider, so the 5th and 6th pipes land on 3
different columns. Please pad the whole table to the widest cell.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogRegister.java:
##########
@@ -82,15 +93,16 @@ public void init(GravitinoConfig config) throws Exception {
TrinoDriver driver = new TrinoDriver();
DriverManager.registerDriver(driver);
- Properties properties = new Properties();
- properties.put("user", config.getTrinoUser());
- properties.put("password", config.getTrinoPassword());
+ Properties properties = buildJdbcProperties(config);
+ String jdbcUri = config.getTrinoJdbcURI();
try {
- connection = driver.connect(config.getTrinoJdbcURI(), properties);
+ connection = driver.connect(jdbcUri, properties);
Review Comment:
`init()` can run more than once and it does not close the old connection. In
`GravitinoConnectorFactory`, `catalogConnectorManagerStarted` is only set to
true after `start()` succeeds, so if `start()` fails the next `create()` of the
static catalog calls `init()` again. If `driver.connect()` succeeded but the
`catalog.config-dir` check below failed, we leak one `Connection` on every
retry. We also register a new `TrinoDriver` each time. Maybe close the old
connection first, or return early when it is already connected.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogRegister.java:
##########
@@ -104,6 +116,179 @@ public void init(GravitinoConfig config) throws Exception
{
}
}
+ /**
+ * Builds the JDBC properties used by the internal connection to the Trino
coordinator.
+ *
+ * <p>The properties derived from the dedicated {@code trino.jdbc.*}
configurations are applied
+ * first, then the raw driver properties configured with the {@code
trino.jdbc.properties.} prefix
+ * are applied on top of them, so that any driver property can be overridden.
+ *
+ * @param config the Gravitino configuration
+ * @return the JDBC properties
+ */
+ @VisibleForTesting
+ static Properties buildJdbcProperties(GravitinoConfig config) {
+ boolean sslEnabled = config.isTrinoJdbcSslEnabled();
+ String verification = config.getTrinoJdbcSslVerification();
+ String truststorePath = config.getTrinoJdbcSslTruststorePath();
+ String truststorePassword = config.getTrinoJdbcSslTruststorePassword();
+ String truststoreType = config.getTrinoJdbcSslTruststoreType();
+ String keystorePath = config.getTrinoJdbcSslKeystorePath();
+ String keystorePassword = config.getTrinoJdbcSslKeystorePassword();
+ String keystoreType = config.getTrinoJdbcSslKeystoreType();
+ String roles = config.getTrinoJdbcRoles();
+
+ validateSslConfig(
+ sslEnabled,
+ verification,
+ truststorePath,
+ truststorePassword,
+ truststoreType,
+ keystorePath,
+ keystorePassword,
+ keystoreType);
+
+ Properties properties = new Properties();
+ properties.put("user", config.getTrinoUser());
+ String password = config.getTrinoPassword();
+ if (StringUtils.isNotEmpty(password)) {
+ properties.put("password", password);
+ }
+
+ if (sslEnabled) {
+ properties.put("SSL", "true");
+ properties.put("SSLVerification", verification);
+ if (StringUtils.isNotBlank(truststorePath)) {
+ properties.put("SSLTrustStorePath", truststorePath);
+ }
+ if (StringUtils.isNotEmpty(truststorePassword)) {
+ properties.put("SSLTrustStorePassword", truststorePassword);
+ }
+ if (StringUtils.isNotBlank(truststoreType)) {
+ properties.put("SSLTrustStoreType", truststoreType);
+ }
+ if (StringUtils.isNotBlank(keystorePath)) {
+ properties.put("SSLKeyStorePath", keystorePath);
+ }
+ if (StringUtils.isNotEmpty(keystorePassword)) {
+ properties.put("SSLKeyStorePassword", keystorePassword);
+ }
+ if (StringUtils.isNotBlank(keystoreType)) {
+ properties.put("SSLKeyStoreType", keystoreType);
+ }
+ }
+
+ if (StringUtils.isNotBlank(roles)) {
+ properties.put("roles", roles);
+ }
+
+ Map<String, String> extraProperties = config.getTrinoJdbcExtraProperties();
+ if (!extraProperties.isEmpty()) {
+ // Log the names only, the values may contain credentials.
+ LOG.debug("Applying extra Trino JDBC properties: {}",
extraProperties.keySet());
+ properties.putAll(extraProperties);
Review Comment:
These extra properties are applied after `validateSslConfig`, so they can
undo it. For example `trino.jdbc.properties.SSLVerification=NONE` passes
validation and then lowers the verification. I see from
`testExtraPropertiesArePassedThroughAndOverride` that the override is on
purpose, so I am not asking to block it. But the PR description says an
inconsistent TLS config now fails at startup, and that is not true for this
path. Can we say this in the docs, or log a WARN when an extra property
overwrites an `SSL*` key?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -106,16 +122,28 @@ public Connector create(
catalogRegister, catalogConnectorFactory,
this::getTrinoCatalogName);
catalogConnectorManager.config(config, client);
- if (isCoordinator(trinoConnectorContext)) {
- catalogConnectorManager.start();
- }
-
gravitinoSystemTableFactory = new
GravitinoSystemTableFactory(catalogConnectorManager);
- } catch (Exception e) {
- String message = "Initialization of the GravitinoConnector failed "
+ e.getMessage();
- LOG.error(message);
- throw new TrinoException(GRAVITINO_RUNTIME_ERROR, message, e);
}
+
+ // The `trino.jdbc.*` settings that CatalogRegister needs to connect
back to the
+ // coordinator are deliberately not propagated to the dynamic
catalogs, so they are only
+ // present in the configuration of the static connector. Trino does
not guarantee that the
+ // static catalog is loaded before the catalogs Gravitino created,
therefore the manager is
+ // started from the static connector only, re-applying its
configuration in case a dynamic
+ // connector was created first.
+ if (!catalogConnectorManagerStarted
Review Comment:
Just to confirm this is intended. The manager is now started only by the
static connector. If someone removes or renames the static `gravitino` catalog
file but the Gravitino-created catalog files are still in `etc/catalog`, the
coordinator creates the manager but never starts it, so there is no
`loadMetalake` loop and catalogs deleted on the server side are never removed.
Before this PR the dynamic connector would start it. I think it is acceptable,
but it degrades silently instead of failing, so maybe worth a line in the PR
description.
--
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]
