Skip to content

s2a: Single S2AChannelCredentials. #3989

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.

Sign up for GitHub

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

Open
rmehta19 wants to merge 14 commits into
base: main
Choose a base branch
from
+72 −35
Open

s2a: Single S2AChannelCredentials. #3989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8ddbe44
Single S2AChannelCredentials + use AdvancedTls.
rmehta19 Nov 14, 2025
e35914f
add comment.
rmehta19 Nov 14, 2025
217737b
address comments.
rmehta19 Dec 5, 2025
5f52e87
correct variable name.
rmehta19 Dec 5, 2025
b3d457b
Merge branch 'main' into singe-channel-creds
rmehta19 Dec 5, 2025
953e1a0
move adv tls changes to another PR.
rmehta19 Dec 5, 2025
53b8b84
reduce nesting.
rmehta19 Jan 5, 2026
6b047ae
add comment.
rmehta19 Jan 5, 2026
075db93
don't catch exception that's not thrown.
rmehta19 Jan 5, 2026
3266899
reset.
rmehta19 Jan 6, 2026
5aba3fc
format.
rmehta19 Jan 6, 2026
fc107a5
Merge branch 'main' into singe-channel-creds
rmehta19 Jan 6, 2026
c7536e6
package private.
rmehta19 Jan 7, 2026
71335fc
rename.
rmehta19 Jan 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 69 additions & 35 deletions ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,23 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
@Nullable
private final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator;

// This is initialized once for the lifetime of the application. This enables re-using
// channels to S2A.
private static volatile ChannelCredentials s2aChannelCredentialsObject;
Copy link

@xmenxk xmenxk Nov 14, 2025

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

@rmehta19 rmehta19 Nov 14, 2025

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.

Copy link
Member

@lqiu96 lqiu96 Jan 2, 2026

Choose a reason for hiding this comment

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

qq, is this intended to be re-used only per SDK client (e.g. SDK client has multiple channels created and each channel from an instance will re-use this)?

Or do you want this to be shared across each SDK client? (e.g. I have 2+ SDK clients and each have multiple channels)

Copy link
Contributor Author

@rmehta19 rmehta19 Jan 5, 2026

Choose a reason for hiding this comment

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

This is shared across all SDK clients. An application could create multiple SDK clients, and we want the S2A channel creds to be reused across all SDK clients created by an application. In other words, we want to tie the S2A channel credentials to the lifetime of the application (not to the lifetime of a single SDK client).

Copy link
Member

@lqiu96 lqiu96 Jan 5, 2026

Choose a reason for hiding this comment

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

Ok thanks! Just want to clarify. We've had some code changes in the past with static that was intended to do this per client. I think if this is to be reused across all SDK clients, then this should be fine. The comments above are mostly related to this concern.

Can we try and use Initialization-on-demand over the double checking lock? This seems to be the preferred way to create static singletons.

Copy link
Contributor Author

@rmehta19 rmehta19 Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Can we try and use Initialization-on-demand over the double checking lock? This seems to be the preferred way to create static singletons.

So I think to do this we need to do something like this for Initialization-on-demand:

private static class S2AChannelCredentialsHolder {
    private static final ChannelCredentials INSTANCE = createS2ASecuredChannelCredentials();
    private static ChannelCredentials createS2ASecuredChannelCredentials() {
      SecureSessionAgentConfig config = s2aConfigProvider.getConfig();
      ...
     } 
}
ChannelCredentials createS2ASecuredChannelCredentials() {
    return S2AChannelCredentialsHolder.INSTANCE;
}

the problem is that s2aConfigProvider is a non-static class member of InstantiatingGrpcChannelProvider. It's non-static since it is used to inject a config from tests.

Upon reviewing this code while responding to this comment, I realized there was a bug with the double checked lock implementation: we need to be able to reset the s2aChannelCredentialsObject between tests, since tests inject their own config, which is different for each test. I added a reset method which can be used from tests.

WDYT of continuing to use double checked locking with the reset?

Copy link
Member

@lqiu96 lqiu96 Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Is testing currently the only limitation? I don't think we should block on testing unless this requires a massive refactor/ introduces a change in behavior/ etc.

Is it possible to use MockStatic to mock the getInstance() or createS2ASecuredChannelCredentials() static method for each unit test?

Copy link
Contributor Author

@rmehta19 rmehta19 Jan 6, 2026

Choose a reason for hiding this comment

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

Lawrence, I didn't see your latest comment as I was typing this out. Let me review it.

Copy link
Contributor Author

@rmehta19 rmehta19 Jan 6, 2026

Choose a reason for hiding this comment

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

I think you and I are describing the same thing. One note I mentioned is that the setter should be thread-safe. And, we should expose a reset function to restore the original value that is modified during tests, also thread-safe.

Agreed that it's not ideal to expose setters like this. Happy to implement this if it's preferred over double checked locking. Will update this thread once I have made the changes in a new commit.

Copy link
Member

@lqiu96 lqiu96 Jan 6, 2026

Choose a reason for hiding this comment

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

Oh I see your point with restoring the original value thread safe. IIUC, this would limit us to run each test sequentially since we can't guarantee when the values are reset. I can't seem to think of a way around this (maybe there is and I'm unaware of an existing pattern).

I think the double checking lock wouldn't have this issue (correct me if that's incorrect). If that's the case, I think it's probably better for the double checking lock then. I think it would be a case where it would be better for us over the initialization on demand. Thanks for brainstorming with me :)

Copy link
Contributor Author

@rmehta19 rmehta19 Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, this would limit us to run each test sequentially since we can't guarantee when the values are reset.

I think we can still run the tests in parallel, it's just that the reset function would be synchronized on the class, so multiple tests can't reset at the same time.

I think the double checking lock wouldn't have this issue (correct me if that's incorrect). If that's the case, I think it's probably better for the double checking lock then. I think it would be a case where it would be better for us over the initialization on demand.

Hmm, I think double checked locking would also have this issue. This pattern also makes sure that the S2A Channel Credentials object is only initialized once (the first time it is accessed). So each test that injects its own s2aConfigProvider would have to call reset to null the credentials after running, so that the next test can trigger the credentials to be created again with a different s2aConfigProvider.

I did implement the initialization on demand method and found the reset functions do look a bit different compared to double checked locking:

Double checked locking reset:

@VisibleForTesting
 static void resetS2AChannelCredentialsObjectForTests() {
    synchronized (InstantiatingGrpcChannelProvider.class) {
      s2aChannelCredentialsObject = null;
    }
  }

initialization on demand reset:

@VisibleForTesting
  static void resetS2AChannelCredentials(SecureSessionAgent s2aConfigProvider) {
    synchronized (InstantiatingGrpcChannelProvider.class) {
      InstantiatingGrpcChannelProvider.s2aConfigProvider = s2aConfigProvider;
      S2AChannelCredentialsHolder.reset();
    }
  }

static class S2AChannelCredentialsHolder {
    private static ChannelCredentials INSTANCE = createS2ASecuredChannelCredentials();
    private static void reset() {
      INSTANCE = createS2ASecuredChannelCredentials();
    }
    ...
}

I do think double checked locking reset is a bit more intuitive (implemented in this PR):

  • just call reset to null the credentials, and then the next call to createS2ASecuredChannelCredentials will recreate them.
  • the credentials are static (not final)
    In initialization on demand:
  • the reset needs to be called before the test runs, and it also needs to call S2AChannelCredentialsHolder.reset(), since initialization on demand uses static initialization: INSTANCE is only initialized once. If we want to change INSTANCE we have to make it a non-final field and overwrite it as shown.
  • both the config and the credentials are static (not final) variables

Given that reset login is needed in both cases, I'm wondering which pattern you prefer? Personally, I find double checked locking to be simpler to reason about in this case, but I'm also not sure of the Java-preferred way to do things in this case. Also, please feel free to correct me if I got something wrong.

Thanks for brainstorming with me :)

NP! Thanks for working with us on this!

Copy link
Member

@lqiu96 lqiu96 Jan 7, 2026

Choose a reason for hiding this comment

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

Ok, I see. We need to reset the S2AChannelCredentials and not the s2aConfigProvider after each test. I don't think we need to figure out making this work for parallel tests now. I was just thinking this may be a use case to help us decide for one over the other.

I don't want to hold this up over a pattern choice and I think this is fine as is as we have a deadline. My preference is to have the JVM guarantee the singleton, but that is always something we can change/ refactor later. I think we looked the possible edge cases (e.g. resetting the static value) and the challenges (unit testing) for both. I'm fine with what you have in the PR.


/**
* Resets the s2aChannelCredentialsObject of the {@link InstantiatingGrpcChannelProvider} class
* for testing purposes.
*
* <p>This should only be called from tests.
*/
@VisibleForTesting
static void resetS2AChannelCredentials() {
synchronized (InstantiatingGrpcChannelProvider.class) {
s2aChannelCredentialsObject = null;
}
}

/*
* Experimental feature
*
Expand Down Expand Up @@ -595,43 +612,60 @@ ChannelCredentials createPlaintextToS2AChannelCredentials(String plaintextAddres
* @return {@link ChannelCredentials} configured to use S2A to create mTLS connection.
*/
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) {
// s2aChannelCredentialsObject is initialized once and shared by all instances of the class.
// To prevent a race on initialization, the object initialization is synchronized on the class
// object.
synchronized (InstantiatingGrpcChannelProvider.class) {
if (s2aChannelCredentialsObject != null) {
return s2aChannelCredentialsObject;
}
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 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;
}

private ManagedChannel createSingleChannel() throws IOException {
Expand Down
3 changes: 3 additions & 0 deletions .../gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,7 @@ void createS2ASecuredChannelCredentials_bothS2AAddressesNull_returnsNull() {
.setS2AConfigProvider(s2aConfigProvider)
.build();
assertThat(provider.createS2ASecuredChannelCredentials()).isNull();
InstantiatingGrpcChannelProvider.resetS2AChannelCredentials();
}

@Test
Expand All @@ -1175,6 +1176,7 @@ void createS2ASecuredChannelCredentials_bothS2AAddressesNull_returnsNull() {
.contains(
"Cannot establish an mTLS connection to S2A because autoconfig endpoint did not return a mtls address to reach S2A.");
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
InstantiatingGrpcChannelProvider.resetS2AChannelCredentials();
}

@Test
Expand All @@ -1197,6 +1199,7 @@ void createS2ASecuredChannelCredentials_returnsPlaintextToS2AS2AChannelCredentia
.contains(
"Cannot establish an mTLS connection to S2A because MTLS to MDS credentials do not exist on filesystem, falling back to plaintext connection to S2A");
InstantiatingGrpcChannelProvider.LOG.removeHandler(logHandler);
InstantiatingGrpcChannelProvider.resetS2AChannelCredentials();
}

@Test
Expand Down
Loading