[Improvement] JDBC Connection Configuration with Flexible SSL and Property Injection #681

felicity3786 · 2025-04-28T22:00:33Z

Description

Summary:
Today, the Trino Gateway only supports very basic JDBC connection initialization, assuming standard username/password settings. It lacks support for important real-world features like client-side certificates (mTLS), custom truststore setups, or fine-grained SSL configurations when connecting to databases (e.g., MySQL).
I would like to help contributing a more flexible, extensible JDBC connection management system.

Additional context and related issues

Right now, current gateway code hardcodes simple username/password based JDBC setup.
It is difficult to configure SSL/TLS for databases that require client certificates (e.g., SslMode=VERIFY_CA for MySQL). Gateway users with stricter security requirements (like us at LinkedIn) must patch or fork the code. And it is not cleanly extensible for new future other database types.

At LinkedIn, We already implemented the following internally and would like to contribute it upstream:

Introduce a JdbcPropertiesProvider interface for generating connection properties.
Add BasicJdbcPropertiesProvider (default simple username/password).
Add MySqlJdbcPropertiesProvider to handle MySQL-specific SSL properties:
Handles clientCertificateKeyStoreUrl, clientCertificateKeyStorePassword, etc.
Supports different SslMode settings (DISABLED, VERIFY_CA, etc.).
Introduce a JdbcPropertiesProviderFactory to pick the right provider automatically based on configuration.
Refactor JdbcConnectionManager to use these properties instead of hardcoded username/password.
Add Airlift-compliant @Singleton / Guice bindings for better dependency injection.

Benefits:
The changes will help

Secure and flexible database connections (support MySQL client cert auth, etc.).
Easily extensible for Oracle, or other databases in the future.
More separation of concerns: connection properties logic decoupled from connection management.
Retains full backward compatibility (H2, MySQL username/password still work out of the box).

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

* Fix some things. ({issue}`issuenumber`)

…perty Injection

xkrogen · 2025-04-29T16:02:52Z

gateway-ha/src/main/java/io/trino/gateway/ha/config/HaGatewayConfiguration.java

+
+    public boolean isOracleBackend()
+    {
+        return oracleBackend;
+    }
+
+    public void setOracleBackend(boolean oracleBackend)
+    {
+        this.oracleBackend = oracleBackend;
+    }


IIUC, with this change, now users will have to manually configure oracleBackend = true in addition to setting their JDBC URL to jdbc:oracle://..., is that right?

I have two ideas here.

First, in the interest of maintaining current behavior and reducing the burden on users, I would say we can just add a new method on DataStoreConfiguration like:

public boolean isOracleBackend() { return jdbcUrl.startsWith("jdbc:oracle:"); }

But, thinking on this more, I think we can do better. I am thinking we should create an enum for all of the supported backends:

enum DataStoreBackend { ORACLE, MYSQL, POSTGRES; }

Then in DataStoreConfiguration we can default this to being set by the JDBC URL, but allow users to override if needed:

private DataStoreBackend dataStoreBackendType; ... public DataStoreBackend getDataStoreBackendType() { if (dataStoreBackendType != null) { return dataStoreBackendType; } if (jdbcUrl.startsWith("jdbc:oracle") { return ORACLE; } else if (...) { .. } }

And then we can pass around this nice enum instead of a boolean. I suspect that over time we're going to need more DB-specific modifications throughout the system. WDYT?

Yep I think the enum DataStoreBackend is better, updated in the new commit. Thanks!

xkrogen · 2025-04-29T16:10:09Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/BasicJdbcPropertiesProvider.java

+
+public class BasicJdbcPropertiesProvider


Maybe add Javadoc explaining that this is the fallback / default behavior in case no DB-specific prop provider is available, and that this should always be the last one in the list?

xkrogen · 2025-05-01T15:05:08Z

Current diff LGTM, cc @willmostly

ebyhr · 2025-05-06T21:54:32Z

gateway-ha/src/main/java/io/trino/gateway/ha/config/DataStoreBackend.java

+ */
+package io.trino.gateway.ha.config;
+
+public enum DataStoreBackend {


DataStoreBackend -> DataStoreType

ebyhr · 2025-05-06T21:59:40Z

gateway-ha/src/main/java/io/trino/gateway/ha/config/DataStoreConfiguration.java

+        this.mysqlConfiguration = mysqlConfig;
+    }
+
+    public DataStoreBackend getDataStoreBackendType()


Please consider using java.sql.Driver#acceptsURL instead.

ebyhr · 2025-05-06T21:59:59Z

gateway-ha/src/main/java/io/trino/gateway/ha/config/MysqlConfiguration.java

+/**
+ * Configuration for MySQL SSL (client cert and truststore settings).
+ */
+public class MysqlConfiguration


Mysql -> MySql

ebyhr · 2025-05-06T22:01:59Z

gateway-ha/src/main/java/io/trino/gateway/ha/config/MysqlConfiguration.java

+    private SslMode sslMode = SslMode.DISABLED;
+    private String clientCertificateKeyStoreUrl;
+    private String clientCertificateKeyStorePassword;
+    private String clientCertificateKeyStoreType;
+    private String trustCertificateKeyStoreUrl;
+    private String trustCertificateKeyStorePassword;


Rename these fields for consistency with Trino:

tlsEnabled

keystorePath

keystorePassword

keystoreType

truststorePath

truststorePassword

The naming is from the mysql server: https://github.com/mysql/mysql-connector-j/blob/20ef8ee9eb4294a03858acccea0ddad3525f1ff9/src/main/core-api/java/com/mysql/cj/conf/PropertyKey.java#L82-L84 so the driver can easily pick it up without translating it. I felt it might be easier keeping it this way, I can add some comments for the trino concept mapping though, WDYT?

+1 with @felicity3786 -- we previously discussed that if we had these as generic, Gateway-level configs, we should use the names consistent with Trino like @ebyhr said. But currently these are MySQL-specific configs, so we should be consistent with the MySQL naming convention.

ebyhr · 2025-05-06T22:02:45Z

gateway-ha/src/main/java/io/trino/gateway/ha/module/QueryCountBasedRouterProvider.java

    {
-        return this.routingManager;
+        return new QueryCountBasedRouter(gatewayBackendManager, queryHistoryManager);


Add requireNonNull

ebyhr · 2025-05-06T22:05:15Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java

@@ -54,12 +61,18 @@ public Jdbi getJdbi(@Nullable String routingGroupDatabase)
        if (routingGroupDatabase == null) {
            return jdbi;
        }
+        Properties props = jdbcPropertiesProvider.getProperties(configuration);


props -> properties

ebyhr · 2025-05-06T22:06:04Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcPropertiesProviderFactory.java

+    @Inject
+    public JdbcPropertiesProviderFactory(List<JdbcPropertiesProvider> orderedProviders)
+    {
+        this.providers = requireNonNull(orderedProviders, "providers is null");


Remove requireNonNull and add immutable copy.

ebyhr · 2025-05-06T22:06:30Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcPropertiesProviderFactory.java

+    public JdbcPropertiesProvider forConfig(DataStoreConfiguration config)
+    {
+        return providers.stream()
+                .filter(p -> p.supports(config))


p -> provider

https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#avoid-abbreviations

ebyhr · 2025-05-06T22:07:11Z

gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcPropertiesProviderFactory.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class TestJdbcPropertiesProviderFactory


ebyhr · 2025-05-06T22:07:32Z

gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcPropertiesProviderFactory.java

+{
+    private JdbcPropertiesProviderFactory factory;
+
+    @BeforeEach


https://trino.io/docs/current/develop/tests.html#keep-test-setup-and-teardown-simple

xkrogen · 2025-05-07T18:50:51Z

gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcPropertiesProviderFactory.java

+        var cfg = makeMySqlConfig(SslMode.VERIFY_CA);
+        var factory = factoryFor(cfg);
+        var mySqlConfiguration = cfg.getMySqlConfiguration();


Avoid var per Trino guidelines (here and elsewhere): https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#avoid-var

xkrogen · 2025-05-07T18:51:27Z

gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcPropertiesProviderFactory.java


 import java.util.Properties;

 import static org.assertj.core.api.Assertions.assertThat;

-class TestJdbcPropertiesProviderFactory
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)


Why do we need this?

Hmm I guess we don't really need it given I have remove all the @BeforeAll, removed it

xkrogen · 2025-05-07T18:52:56Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcPropertiesProviderFactory.java

@@ -30,13 +29,13 @@ public class JdbcPropertiesProviderFactory
    @Inject
    public JdbcPropertiesProviderFactory(List<JdbcPropertiesProvider> orderedProviders)
    {
-        this.providers = requireNonNull(orderedProviders, "providers is null");
+        this.providers = List.copyOf(orderedProviders);


For Trino we typically use the Guava one i.e. ImmutableList.copyOf() and we should still have requireNonNull here:

Suggested change

this.providers = List.copyOf(orderedProviders);

this.providers = ImmutableList.copyOf(requireNonNull(orderedProviders, "providers is null"));

requireNonNull is redundant when we use ImmutableList.copyOf.

Why is that @ebyhr ? copyOf() will throw NPE if given a null value (at elements.toArray()):
https://github.com/google/guava/blob/6bbfc86eb2cf64600e1e30e112cad227f5e31f4b/guava/src/com/google/common/collect/ImmutableList.java#L258-L264

The Trino repo has hundreds of usages of this pattern: https://github.com/search?q=repo%3Atrinodb%2Ftrino%20%22ImmutableList.copyOf(requireNonNull(%22&type=code

xkrogen · 2025-05-07T18:55:57Z

gateway-ha/src/main/java/io/trino/gateway/ha/config/DataStoreConfiguration.java

+        if (jdbcUrl == null) {
+            throw new IllegalStateException("jdbcUrl must be set");


If jdbcUrl is required, let's add the non-null validation to getJdbcUrl(), and then just use getJdbcUrl() here, to keep that validation consistent

xkrogen · 2025-05-07T19:01:40Z

gateway-ha/src/main/java/io/trino/gateway/ha/config/DataStoreConfiguration.java

+                    String driverClass = driver.getClass().getName();
+                    if (driverClass.contains("oracle")) {
+                        return DataStoreType.ORACLE;
+                    }


This fuzzy string match against class name doesn't seem right to me. We have all of the driver classes on the classpath, can we do an actual match against the class name? Like adding a field to DataStoreType which tracks the expected instance of Class<?> for that DataStoreType. Then this iteration can become something like:

while (drivers.hasMoreElements()) { Driver driver = drivers.nextElement(); if (driver.acceptsURL(jdbcUrl)) { for (DataStoreType dataStoreType : DataStoreType.values() { if (dataStoreType.driverClass() == driver.getClass()) { return dataStoreType; } } break; } }

ebyhr · 2025-05-08T22:48:46Z

gateway-ha/src/main/java/io/trino/gateway/ha/config/MySqlConfiguration.java

+        return clientCertificateKeyStoreUrl;
+    }
+
+    public MySqlConfiguration setClientCertificateKeyStoreUrl(String url)


Please rename the parameter. Same for others.

Hi @ebyhr, @xkrogen and I both think it's would make more sense to keep the mysql naming convention as we discussed in #681 (comment), wdyt?

[Improvement] JDBC Connection Configuration with Flexible SSL and Pro…

28879c5

…perty Injection

cla-bot bot added the cla-signed label Apr 28, 2025

xkrogen reviewed Apr 29, 2025

View reviewed changes

add enmu DatastoreBackend, add javadoc

8a6ab09

ebyhr requested changes May 6, 2025

View reviewed changes

address comments

c13a8db

xkrogen reviewed May 7, 2025

View reviewed changes

ebyhr reviewed May 8, 2025

View reviewed changes

addressed comments

6e38e11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improvement] JDBC Connection Configuration with Flexible SSL and Property Injection #681

[Improvement] JDBC Connection Configuration with Flexible SSL and Property Injection #681

felicity3786 commented Apr 28, 2025 •

edited

Loading

xkrogen Apr 29, 2025

felicity3786 Apr 29, 2025

xkrogen Apr 29, 2025

xkrogen commented May 1, 2025

ebyhr May 6, 2025

ebyhr May 6, 2025

ebyhr May 6, 2025

ebyhr May 6, 2025

felicity3786 May 7, 2025

xkrogen May 7, 2025 •

edited

Loading

ebyhr May 6, 2025

ebyhr May 6, 2025

ebyhr May 6, 2025

ebyhr May 6, 2025

ebyhr May 6, 2025

ebyhr May 6, 2025

xkrogen May 7, 2025

xkrogen May 7, 2025

felicity3786 May 9, 2025

xkrogen May 7, 2025

ebyhr May 8, 2025

xkrogen May 9, 2025 •

edited

Loading

xkrogen May 7, 2025

xkrogen May 7, 2025

ebyhr May 8, 2025

felicity3786 May 9, 2025


		import static org.assertj.core.api.Assertions.assertThat;

		class TestJdbcPropertiesProviderFactory

	this.providers = List.copyOf(orderedProviders);
	this.providers = ImmutableList.copyOf(requireNonNull(orderedProviders, "providers is null"));

		if (jdbcUrl == null) {
		throw new IllegalStateException("jdbcUrl must be set");

[Improvement] JDBC Connection Configuration with Flexible SSL and Property Injection #681

Are you sure you want to change the base?

[Improvement] JDBC Connection Configuration with Flexible SSL and Property Injection #681

Conversation

felicity3786 commented Apr 28, 2025 • edited Loading

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xkrogen commented May 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xkrogen May 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xkrogen May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felicity3786 commented Apr 28, 2025 •

edited

Loading

xkrogen May 7, 2025 •

edited

Loading

xkrogen May 9, 2025 •

edited

Loading