Skip to content

Conversation

@rmehta19
Copy link
Contributor

No description provided.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Nov 14, 2025
@gemini-code-assist
Copy link

Summary of Changes

Hello @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 InstantiatingGrpcChannelProvider to enhance security and efficiency. It integrates advanced TLS utilities for dynamic credential handling and implements a singleton pattern for the S2A channel credentials, ensuring consistent and optimized resource usage across the application.

Highlights

  • Advanced TLS Credential Management: The pull request introduces the use of AdvancedTlsX509KeyManager and AdvancedTlsX509TrustManager for handling mTLS connections to S2A. This allows for more sophisticated management of identity and trust credentials, including scheduled updates for trust bundles.
  • Singleton S2AChannelCredentials: A static volatile field s2aChannelCredentialsObject has been added to ensure that the ChannelCredentials for S2A are instantiated only once. A double-checked locking pattern is used in createS2ASecuredChannelCredentials to provide thread-safe, lazy initialization of this singleton.
  • Improved Resource Efficiency: By ensuring a single instance of S2AChannelCredentials is used, the changes aim to improve resource efficiency and prevent redundant credential object creation, especially in scenarios where the method might be called multiple times.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Comment on lines 608 to 660
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;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 ...
}

@rmehta19
Copy link
Contributor Author

@rockspore @xmenxk @gtcooke94

Copy link

@xmenxk xmenxk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Nullable
private final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator;

private static volatile ChannelCredentials s2aChannelCredentialsObject;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe add a comment here that we are using this single credentials object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, added a comment about this.

Comment on lines +609 to +611
if (s2aChannelCredentialsObject == null) {
synchronized (InstantiatingGrpcChannelProvider.class) {
if (s2aChannelCredentialsObject == null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why doing the null check on both line 609 and 611?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing! This is part of the double checked locking pattern, WDYT?

Copy link
Member

@rockspore rockspore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants