[SpannerToSourceDb] Reverse replication Integration test with SSL based connection - #4192
[SpannerToSourceDb] Reverse replication Integration test with SSL based connection#4192darshan-sj wants to merge 3 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the integration testing framework for the Spanner-to-SourceDB migration template by adding support for SSL-encrypted connections to MySQL. It includes a new resource manager that handles the automated generation and conversion of SSL certificates into Java KeyStore (JKS) format, allowing the pipeline to securely connect to the database during tests. The changes also involve updating existing test infrastructure to support these secure connections and improving code flexibility by decoupling test utilities from specific MySQL implementations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces SSLMySQLResourceManager to enable SSL support for MySQL test containers, dynamically extracting generated certificates into JKS keystores and truststores. It also updates the SpannerToSourceDbIT integration test to utilize this new resource manager and refactors several test utilities to use the generic JDBCResourceManager interface. Feedback focuses on improving process execution robustness and resource cleanup in SSLMySQLResourceManager, using existing GCS path helpers, and updating URL encoding to use the modern Charset-based API.
| ProcessBuilder pbTrust = | ||
| new ProcessBuilder( | ||
| keytool, | ||
| "-importcert", | ||
| "-alias", | ||
| "mysqlca", | ||
| "-file", | ||
| tempDir.resolve("ca.pem").toString(), | ||
| "-keystore", | ||
| truststore.toString(), | ||
| "-storepass", | ||
| password, | ||
| "-noprompt"); | ||
| Process trustProcess = pbTrust.start(); | ||
| trustProcess.waitFor(); | ||
| if (trustProcess.exitValue() != 0) { | ||
| throw new RuntimeException("Failed to generate truststore.jks"); | ||
| } | ||
|
|
||
| // Convert PKCS12 to Keystore JKS | ||
| Path keystore = tempDir.resolve("keystore.jks"); | ||
| LOG.info("Generating keystore.jks"); | ||
| ProcessBuilder pbKey = | ||
| new ProcessBuilder( | ||
| keytool, | ||
| "-importkeystore", | ||
| "-srckeystore", | ||
| tempDir.resolve("keystore.p12").toString(), | ||
| "-srcstoretype", | ||
| "PKCS12", | ||
| "-srcstorepass", | ||
| password, | ||
| "-destkeystore", | ||
| keystore.toString(), | ||
| "-deststoretype", | ||
| "JKS", | ||
| "-deststorepass", | ||
| password, | ||
| "-noprompt"); | ||
| Process keyProcess = pbKey.start(); | ||
| keyProcess.waitFor(); | ||
| if (keyProcess.exitValue() != 0) { | ||
| throw new RuntimeException("Failed to generate keystore.jks"); | ||
| } | ||
|
|
||
| this.keystorePath = keystore.toAbsolutePath().toString(); | ||
| this.truststorePath = truststore.toAbsolutePath().toString(); | ||
| LOG.info( | ||
| "Successfully generated Keystore: {} and Truststore: {}", keystorePath, truststorePath); | ||
| } |
There was a problem hiding this comment.
The process execution for keytool is duplicated, does not capture the error stream (making failures hard to debug), and does not guarantee process destruction in a finally block if interrupted or if an exception is thrown.
We can refactor this into a robust helper method runProcess that redirects the error stream, captures the output on failure, and ensures the process is destroyed.
ProcessBuilder pbTrust =
new ProcessBuilder(
keytool,
"-importcert",
"-alias",
"mysqlca",
"-file",
tempDir.resolve("ca.pem").toString(),
"-keystore",
truststore.toString(),
"-storepass",
password,
"-noprompt");
runProcess(pbTrust, "generate truststore.jks");
// Convert PKCS12 to Keystore JKS
Path keystore = tempDir.resolve("keystore.jks");
LOG.info("Generating keystore.jks");
ProcessBuilder pbKey =
new ProcessBuilder(
keytool,
"-importkeystore",
"-srckeystore",
tempDir.resolve("keystore.p12").toString(),
"-srcstoretype",
"PKCS12",
"-srcstorepass",
password,
"-destkeystore",
keystore.toString(),
"-deststoretype",
"JKS",
"-deststorepass",
password,
"-noprompt");
runProcess(pbKey, "generate keystore.jks");
this.keystorePath = keystore.toAbsolutePath().toString();
this.truststorePath = truststore.toAbsolutePath().toString();
LOG.info(
"Successfully generated Keystore: {} and Truststore: {}", keystorePath, truststorePath);
}
private void runProcess(ProcessBuilder pb, String stepName) throws IOException, InterruptedException {
pb.redirectErrorStream(true);
Process process = pb.start();
try {
int exitCode = process.waitFor();
if (exitCode != 0) {
String output;
try (java.io.InputStream is = process.getInputStream()) {
output = new String(is.readAllBytes(), java.nio.charset.StandardCharsets.UTF_8);
} catch (Exception e) {
output = "Could not read process output";
}
throw new RuntimeException("Failed to " + stepName + ". Exit code: " + exitCode + ". Output: " + output);
}
} finally {
process.destroy();
}
}| public String getTruststorePath() { | ||
| return truststorePath; | ||
| } |
There was a problem hiding this comment.
The temporary directory created via Files.createTempDirectory is never cleaned up, which can lead to disk space leaks on the test runners. Override the cleanup() method to delete the temporary directory and its contents when the resource manager is closed.
public String getTruststorePath() {
return truststorePath;
}
@Override
public void cleanup() {
try {
super.cleanup();
} finally {
if (keystorePath != null) {
Path tempDir = Paths.get(keystorePath).getParent();
if (tempDir != null && Files.exists(tempDir)) {
try (java.util.stream.Stream<Path> walk = Files.walk(tempDir)) {
walk.sorted(java.util.Comparator.reverseOrder())
.map(Path::toFile)
.forEach(java.io.File::delete);
} catch (IOException e) {
LOG.warn("Failed to delete temporary SSL directory: {}", tempDir, e);
}
}
}
}
}| String truststoreGcsUrl = | ||
| "gs://" | ||
| + gcsResourceManager.getBucket() | ||
| + "/" | ||
| + gcsResourceManager | ||
| .uploadArtifact( | ||
| "input/truststore_Shard1.jks", jdbcResourceManager.getTruststorePath()) | ||
| .name(); |
There was a problem hiding this comment.
Instead of manually constructing the GCS URI by concatenating the bucket name and the uploaded artifact name, use the existing getGcsPath helper method. This is cleaner, less error-prone, and consistent with how other files (like session.json) are handled in this test.
gcsResourceManager.uploadArtifact(
"input/truststore_Shard1.jks", jdbcResourceManager.getTruststorePath());
String truststoreGcsUrl = getGcsPath("input/truststore_Shard1.jks", gcsResourceManager);References
- In template integration tests,
getGcsPathhas an overloaded signature that accepts aGcsResourceManageras a second argument, in addition to the single-argument version.
| String props = | ||
| String.format( | ||
| "sslMode=VERIFY_CA&allowPublicKeyRetrieval=true&trustCertificateKeyStoreUrl=%s&trustCertificateKeyStorePassword=%s", | ||
| URLEncoder.encode(truststoreLocalUrl, "UTF-8"), | ||
| URLEncoder.encode(jdbcResourceManager.getPassword(), "UTF-8")); |
There was a problem hiding this comment.
Using URLEncoder.encode(String, String) with a hardcoded string "UTF-8" is discouraged in modern Java as it throws a checked UnsupportedEncodingException. Use URLEncoder.encode(String, Charset) with StandardCharsets.UTF_8 instead.
| String props = | |
| String.format( | |
| "sslMode=VERIFY_CA&allowPublicKeyRetrieval=true&trustCertificateKeyStoreUrl=%s&trustCertificateKeyStorePassword=%s", | |
| URLEncoder.encode(truststoreLocalUrl, "UTF-8"), | |
| URLEncoder.encode(jdbcResourceManager.getPassword(), "UTF-8")); | |
| String props = | |
| String.format( | |
| "sslMode=VERIFY_CA&allowPublicKeyRetrieval=true&trustCertificateKeyStoreUrl=%s&trustCertificateKeyStorePassword=%s", | |
| URLEncoder.encode(truststoreLocalUrl, java.nio.charset.StandardCharsets.UTF_8), | |
| URLEncoder.encode(jdbcResourceManager.getPassword(), java.nio.charset.StandardCharsets.UTF_8)); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4192 +/- ##
============================================
- Coverage 55.94% 55.92% -0.03%
+ Complexity 7485 7028 -457
============================================
Files 1134 1134
Lines 70175 70175
Branches 8023 8023
============================================
- Hits 39261 39244 -17
- Misses 28367 28381 +14
- Partials 2547 2550 +3
🚀 New features to boost your workflow:
|
1 participant
No description provided.