Copilot commented on code in PR #12543:
URL: https://github.com/apache/gravitino/pull/12543#discussion_r3826731292
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogRegister.java:
##########
@@ -104,6 +115,104 @@ 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 roles = config.getTrinoJdbcRoles();
+
+ validateSslConfig(sslEnabled, verification, truststorePath);
+
+ 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(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) {
+ 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
'trino.jdbc.ssl.enabled' to be true");
+ }
+ if (StringUtils.isNotBlank(truststorePath)) {
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT,
+ "Config 'trino.jdbc.ssl.truststore.path' requires
'trino.jdbc.ssl.enabled' to be true");
+ }
+ return;
+ }
+
+ if (StringUtils.isBlank(truststorePath)) {
+ 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(
+ "Error config for 'trino.jdbc.ssl.truststore.path' %s, file not
found",
+ truststorePath));
+ }
Review Comment:
The truststore-not-found error message is awkward ("Error config for ...")
and inconsistent with other config validation messages in this file. A clearer
message helps operators quickly identify what to fix.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConfig.java:
##########
@@ -340,6 +404,88 @@ 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);
+ }
+ return "https".equalsIgnoreCase(parseDiscoveryUri().getScheme());
+ }
Review Comment:
`trino.jdbc.ssl.enabled` values with surrounding whitespace (e.g. " true ")
will be treated as `false` because `Boolean.parseBoolean` is called on the raw
string. This can silently disable TLS even though the config is present. Trim
the value before parsing so typical config-file whitespace doesn’t change
semantics.
--
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]
