Single S2AChannelCredentials + use AdvancedTls. #3989
Conversation
Summary of ChangesHello @rmehta19, 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 refactors the S2A channel credentials management within the 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 two main improvements. First, it implements caching for S2A channel credentials using a thread-safe, double-checked locking pattern to avoid re-creating them on every call. Second, it updates the mTLS credential creation to use AdvancedTlsX509KeyManager and AdvancedTlsX509TrustManager for more advanced TLS configuration. My review focuses on a potential resource leak and an opportunity to simplify the new caching logic.
| AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager(); | ||
| keyManager.updateIdentityCredentials(certChain, privateKey); | ||
| AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build(); | ||
| ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); |
There was a problem hiding this comment.
Creating a new ScheduledExecutorService here without a mechanism to shut it down will cause a resource leak. The thread from this executor will run indefinitely and may prevent the JVM from shutting down gracefully.
The AdvancedTlsX509TrustManager documentation specifies that the caller is responsible for shutting down the executor. Since this executor is used for periodic credential refreshes, it should live for the duration of the application.
A better approach would be to use a shared, static ScheduledExecutorService configured with daemon threads. This ensures the background thread doesn't block application exit. For example:
private static final ScheduledExecutorService S2A_TRUST_MANAGER_EXECUTOR =
Executors.newSingleThreadScheduledExecutor(
r -> {
Thread t = new Thread(r, "s2a-trust-manager-updater");
t.setDaemon(true);
return t;
});You could then use this static executor in the updateTrustCredentials call.
| ChannelCredentials createS2ASecuredChannelCredentials() { | ||
| SecureSessionAgentConfig config = s2aConfigProvider.getConfig(); | ||
| String plaintextAddress = config.getPlaintextAddress(); | ||
| String mtlsAddress = config.getMtlsAddress(); | ||
| if (Strings.isNullOrEmpty(mtlsAddress)) { | ||
| // Fallback to plaintext connection to S2A. | ||
| LOG.log( | ||
| Level.INFO, | ||
| "Cannot establish an mTLS connection to S2A because autoconfig endpoint did not return a mtls address to reach S2A."); | ||
| return createPlaintextToS2AChannelCredentials(plaintextAddress); | ||
| } | ||
| // Currently, MTLS to MDS is only available on GCE. See: | ||
| // https://cloud.google.com/compute/docs/metadata/overview#https-mds | ||
| // Try to load MTLS-MDS creds. | ||
| File rootFile = new File(MTLS_MDS_ROOT_PATH); | ||
| File certKeyFile = new File(MTLS_MDS_CERT_CHAIN_AND_KEY_PATH); | ||
| if (rootFile.isFile() && certKeyFile.isFile()) { | ||
| // Try to connect to S2A using mTLS. | ||
| ChannelCredentials mtlsToS2AChannelCredentials = null; | ||
| try { | ||
| mtlsToS2AChannelCredentials = | ||
| createMtlsToS2AChannelCredentials(rootFile, certKeyFile, certKeyFile); | ||
| } catch (IOException ignore) { | ||
| // Fallback to plaintext-to-S2A connection on error. | ||
| LOG.log( | ||
| Level.WARNING, | ||
| "Cannot establish an mTLS connection to S2A due to error creating MTLS to MDS TlsChannelCredentials credentials, falling back to plaintext connection to S2A: " | ||
| + ignore.getMessage()); | ||
| return createPlaintextToS2AChannelCredentials(plaintextAddress); | ||
| if (s2aChannelCredentialsObject == null) { | ||
| synchronized (InstantiatingGrpcChannelProvider.class) { | ||
| if (s2aChannelCredentialsObject == null) { | ||
| SecureSessionAgentConfig config = s2aConfigProvider.getConfig(); | ||
| String plaintextAddress = config.getPlaintextAddress(); | ||
| String mtlsAddress = config.getMtlsAddress(); | ||
| if (Strings.isNullOrEmpty(mtlsAddress)) { | ||
| // Fallback to plaintext connection to S2A. | ||
| LOG.log( | ||
| Level.INFO, | ||
| "Cannot establish an mTLS connection to S2A because autoconfig endpoint did not return a mtls address to reach S2A."); | ||
| s2aChannelCredentialsObject = createPlaintextToS2AChannelCredentials(plaintextAddress); | ||
| return s2aChannelCredentialsObject; | ||
| } | ||
| // Currently, MTLS to MDS is only available on GCE. See: | ||
| // https://cloud.google.com/compute/docs/metadata/overview#https-mds | ||
| // Try to load MTLS-MDS creds. | ||
| File rootFile = new File(MTLS_MDS_ROOT_PATH); | ||
| File certKeyFile = new File(MTLS_MDS_CERT_CHAIN_AND_KEY_PATH); | ||
| if (rootFile.isFile() && certKeyFile.isFile()) { | ||
| // Try to connect to S2A using mTLS. | ||
| ChannelCredentials mtlsToS2AChannelCredentials = null; | ||
| try { | ||
| mtlsToS2AChannelCredentials = | ||
| createMtlsToS2AChannelCredentials(rootFile, certKeyFile, certKeyFile); | ||
| } catch (IOException | GeneralSecurityException ignore) { | ||
| // Fallback to plaintext-to-S2A connection on error. | ||
| LOG.log( | ||
| Level.WARNING, | ||
| "Cannot establish an mTLS connection to S2A due to error creating MTLS to MDS TlsChannelCredentials credentials, falling back to plaintext connection to S2A: " | ||
| + ignore.getMessage()); | ||
| s2aChannelCredentialsObject = | ||
| createPlaintextToS2AChannelCredentials(plaintextAddress); | ||
| return s2aChannelCredentialsObject; | ||
| } | ||
| s2aChannelCredentialsObject = | ||
| buildS2AChannelCredentials(mtlsAddress, mtlsToS2AChannelCredentials); | ||
| return s2aChannelCredentialsObject; | ||
| } else { | ||
| // Fallback to plaintext-to-S2A connection if MTLS-MDS creds do not exist. | ||
| LOG.log( | ||
| Level.INFO, | ||
| "Cannot establish an mTLS connection to S2A because MTLS to MDS credentials do not" | ||
| + " exist on filesystem, falling back to plaintext connection to S2A"); | ||
| s2aChannelCredentialsObject = createPlaintextToS2AChannelCredentials(plaintextAddress); | ||
| return s2aChannelCredentialsObject; | ||
| } | ||
| } | ||
| } | ||
| return buildS2AChannelCredentials(mtlsAddress, mtlsToS2AChannelCredentials); | ||
| } else { | ||
| // Fallback to plaintext-to-S2A connection if MTLS-MDS creds do not exist. | ||
| LOG.log( | ||
| Level.INFO, | ||
| "Cannot establish an mTLS connection to S2A because MTLS to MDS credentials do not exist on filesystem, falling back to plaintext connection to S2A"); | ||
| return createPlaintextToS2AChannelCredentials(plaintextAddress); | ||
| } | ||
| return s2aChannelCredentialsObject; | ||
| } |
There was a problem hiding this comment.
The logic inside the synchronized block has become quite complex and contains duplicated code, particularly the fallback to createPlaintextToS2AChannelCredentials(plaintextAddress). This makes the method harder to read and maintain.
Consider refactoring this block to simplify the control flow. A clearer structure would be to attempt creating mTLS credentials first, and if that fails for any reason, then fall back to creating plaintext credentials. This would centralize the fallback logic.
For example, you could extract the creation logic into a helper method that returns the created credentials, which simplifies the double-checked locking block:
// In createS2ASecuredChannelCredentials():
if (s2aChannelCredentialsObject == null) {
synchronized (InstantiatingGrpcChannelProvider.class) {
if (s2aChannelCredentialsObject == null) {
s2aChannelCredentialsObject = createS2ACredentialsOnce();
}
}
}
return s2aChannelCredentialsObject;
// New helper method:
private ChannelCredentials createS2ACredentialsOnce() {
// ... logic to try mTLS and fallback to plaintext ...
}
| @Nullable | ||
| private final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator; | ||
|
|
||
| private static volatile ChannelCredentials s2aChannelCredentialsObject; |
There was a problem hiding this comment.
nit: maybe add a comment here that we are using this single credentials object?
There was a problem hiding this comment.
Good call, added a comment about this.
| if (s2aChannelCredentialsObject == null) { | ||
| synchronized (InstantiatingGrpcChannelProvider.class) { | ||
| if (s2aChannelCredentialsObject == null) { |
There was a problem hiding this comment.
curious why doing the null check on both line 609 and 611?
There was a problem hiding this comment.
Thanks for reviewing! This is part of the double checked locking pattern, WDYT?
3 participants
No description provided.