Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
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
Copy Markdown

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
Copy Markdown
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 thread
lqiu96 marked this conversation as resolved.
Outdated
Comment thread
lqiu96 marked this conversation as resolved.
Outdated

/**
* Resets the s2aChannelCredentialsObject of the {@link InstantiatingGrpcChannelProvider} class
* for testing purposes.
*
* <p>This should only be called from tests.
*/
@VisibleForTesting
public static void resetS2AChannelCredentialsObjectForTests() {
Comment thread
lqiu96 marked this conversation as resolved.
Outdated
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) {
Comment thread
lqiu96 marked this conversation as resolved.
Outdated
// 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) {
Comment thread
lqiu96 marked this conversation as resolved.
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;
}
Comment on lines 625 to 680
Copy link
Copy Markdown
Contributor

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


private ManagedChannel createSingleChannel() throws IOException {
Expand Down
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.resetS2AChannelCredentialsObjectForTests();
}

@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.resetS2AChannelCredentialsObjectForTests();
}

@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.resetS2AChannelCredentialsObjectForTests();
}

@Test
Expand Down
Loading