-
Notifications
You must be signed in to change notification settings - Fork 138
feat: add JDBC URL params and SSL config support to MySQL connector #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Conversation
|
Hi @siiddhantt please rebase your PR to staging branch |
- Add JDBCURLParams map for custom connection parameters - Add SSLConfiguration field for certificate-based SSL/TLS - Update URI() to dynamically apply JDBC params and SSL settings - Add registerTLSConfig() for certificate handling - Add comprehensive unit tests (7 tests, all passing) - Update spec.json schema and README documentation - Maintain backward compatibility with tls_skip_verify
f386d3b to
3afa192
Compare
|
Hi @vaibhav-datazip I've rebased it to the staging branch |
- Remove redundant SSL nil check in buildTLSConfig() - Change certificate handling from file paths to PEM string content - Use maps.Copy() for efficient JDBC params copying - Fix TLS config registration: buildTLSConfig() returns *tls.Config, registered inline - Remove TLSSkipVerify field completely - Simplify README examples per feedback - Update spec.json descriptions for certificate fields
|
LGTM |
|
Hi @siiddhantt, can you share the test list that you have done on this PR with MySQL? |
|
Hi @hash-data, I've verified the implementation with comprehensive testing: Unit Tests (5): All JDBC parameter and SSL config tests pass
All configurations generate correct DSN strings and connect successfully to MySQL 8.0. |
|
Hi @siiddhantt, once check the security CI, it is failing |
Hi @hash-data it should be resolved now, thanks! |
|
Hi @siiddhantt , Basically 3 things are required from your side.
|
|
Hi @siiddhantt feel free to check our slack and in contributing-to-olake channel drop in your pr if you are facing any issues our devs would love to take the things from there too . |
|
Hi @ImDoubD-datazip and @Akshay-datazip , sure I'll provide those three and take a look into slack for updates regarding the PR. Thanks! |
|
Hi @ImDoubD-datazip, Here's exactly what I did to test the connection and how you can reproduce it: What I TestedI ran two types of tests to verify the JDBC parameters and SSL configuration work correctly:
Configuration 1: SSL Disabled {
"hosts": "localhost",
"username": "mysql",
"password": "secret1234",
"database": "olake_mysql_test",
"port": 3306,
"ssl": {
"mode": "disable"
}
}Configuration 2: JDBC Parameters Only {
"hosts": "localhost",
"username": "mysql",
"password": "secret1234",
"database": "olake_mysql_test",
"port": 3306,
"jdbc_url_params": {
"charset": "utf8mb4",
"parseTime": "true"
}
}Configuration 3: Combined (JDBC + SSL) {
"hosts": "localhost",
"username": "mysql",
"password": "secret1234",
"database": "olake_mysql_test",
"port": 3306,
"jdbc_url_params": {
"charset": "utf8mb4",
"parseTime": "true",
"timeout": "10s"
},
"ssl": {
"mode": "disable"
}
}How to Run the Tests YourselfIf you want to verify everything locally, here are the exact steps:
/Applications/Docker.app/Contents/Resources/bin/docker ps | grep mysql
cd drivers/mysql
go test ./internal -run "^TestConfig" -v
go test ./internal -run "^TestConnection" -vUPDATEI've added comprehensive SSL/TLS testing for the MySQL connector. Here's what I did: Generated test certificates using OpenSSL and stored them in ssl-certs (CA cert, server cert, client cert). Then spun up an SSL-enforced MySQL container on port 3307 using docker. What I Tested :Ran the full test suite with
I've attached the |
|
@siiddhantt , will be reviewing soon |
Description
This PR implements JDBC URL parameters and SSL/TLS configuration support for the MySQL connector, addressing the need for more flexible and secure database connections.
Changes:
Configstruct withJDBCURLParams(map[string]string) for custom connection parametersSSLConfigurationfield using existingutils.SSLConfigfor certificate-based SSL/TLSURI()method to dynamically apply JDBC parameters and SSL settings to connection stringregisterTLSConfig()helper method for loading and registering custom TLS certificatesValidate()to check SSL configuration when providedspec.jsonwith new optionaljdbc_url_paramsandsslfieldstls_skip_verifyfieldFixes #378
Type of change
How Has This Been Tested?
Unit Tests:
Integration Tests with Real MySQL Database:
Code Quality:
Test Configuration Examples:
{ "hosts": "localhost", "username": "mysql", "password": "secret", "database": "test_db", "port": 3306, "jdbc_url_params": { "charset": "utf8mb4", "parseTime": "true", "timeout": "10s" }, "ssl": { "mode": "disable" } }Documentation