From 2a169bacb33572c00a11204102d15a511d7c3603 Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Tue, 17 Sep 2024 10:46:32 -0700 Subject: [PATCH 1/6] Mark S2A public APIs as experimental. --- s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java | 2 ++ s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java | 2 ++ 2 files changed, 4 insertions(+) diff --git a/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java b/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java index 56f612502bf..e4d14d0c549 100644 --- a/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java +++ b/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java @@ -21,6 +21,7 @@ import static com.google.common.base.Strings.isNullOrEmpty; import io.grpc.ChannelCredentials; +import io.grpc.ExperimentalApi; import io.grpc.TlsChannelCredentials; import io.grpc.util.AdvancedTlsX509KeyManager; import io.grpc.util.AdvancedTlsX509TrustManager; @@ -32,6 +33,7 @@ * Configures an {@code S2AChannelCredentials.Builder} instance with credentials used to establish a * connection with the S2A to support talking to the S2A over mTLS. */ +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/11533") public final class MtlsToS2AChannelCredentials { /** * Creates a {@code S2AChannelCredentials.Builder} builder, that talks to the S2A over mTLS. diff --git a/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java b/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java index 8a5f1f51350..fbdb9f2b8e5 100644 --- a/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java +++ b/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java @@ -24,6 +24,7 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import io.grpc.Channel; import io.grpc.ChannelCredentials; +import io.grpc.ExperimentalApi; import io.grpc.internal.ObjectPool; import io.grpc.internal.SharedResourcePool; import io.grpc.netty.InternalNettyChannelCredentials; @@ -39,6 +40,7 @@ * Configures gRPC to use S2A for transport security when establishing a secure channel. Only for * use on the client side of a gRPC connection. */ +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/11533") public final class S2AChannelCredentials { /** * Creates a channel credentials builder for establishing an S2A-secured connection. From 467ddf55cca2640f64ca5b075ba96f886251d723 Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Tue, 17 Sep 2024 14:11:00 -0700 Subject: [PATCH 2/6] Rename S2AChannelCredentials createBuilder API to newBuilder. --- .../grpc/s2a/MtlsToS2AChannelCredentials.java | 4 +-- .../io/grpc/s2a/S2AChannelCredentials.java | 2 +- .../s2a/MtlsToS2AChannelCredentialsTest.java | 34 +++++++++---------- .../grpc/s2a/S2AChannelCredentialsTest.java | 24 ++++++------- .../grpc/s2a/handshaker/IntegrationTest.java | 8 ++--- 5 files changed, 36 insertions(+), 36 deletions(-) diff --git a/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java b/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java index e4d14d0c549..de63f4f94f4 100644 --- a/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java +++ b/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java @@ -44,7 +44,7 @@ public final class MtlsToS2AChannelCredentials { * @param trustBundlePath the path to the trust bundle PEM. * @return a {@code MtlsToS2AChannelCredentials.Builder} instance. */ - public static Builder createBuilder( + public static Builder newBuilder( String s2aAddress, String privateKeyPath, String certChainPath, String trustBundlePath) { checkArgument(!isNullOrEmpty(s2aAddress), "S2A address must not be null or empty."); checkArgument(!isNullOrEmpty(privateKeyPath), "privateKeyPath must not be null or empty."); @@ -89,7 +89,7 @@ public S2AChannelCredentials.Builder build() throws GeneralSecurityException, IO .trustManager(trustManager) .build(); - return S2AChannelCredentials.createBuilder(s2aAddress) + return S2AChannelCredentials.newBuilder(s2aAddress) .setS2AChannelCredentials(channelToS2ACredentials); } } diff --git a/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java b/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java index fbdb9f2b8e5..1b9a42fb340 100644 --- a/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java +++ b/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java @@ -48,7 +48,7 @@ public final class S2AChannelCredentials { * @param s2aAddress the address of the S2A server used to secure the connection. * @return a {@code S2AChannelCredentials.Builder} instance. */ - public static Builder createBuilder(String s2aAddress) { + public static Builder newBuilder(String s2aAddress) { checkArgument(!isNullOrEmpty(s2aAddress), "S2A address must not be null or empty."); return new Builder(s2aAddress); } diff --git a/s2a/src/test/java/io/grpc/s2a/MtlsToS2AChannelCredentialsTest.java b/s2a/src/test/java/io/grpc/s2a/MtlsToS2AChannelCredentialsTest.java index 5ccc522292e..0fc4ecb3268 100644 --- a/s2a/src/test/java/io/grpc/s2a/MtlsToS2AChannelCredentialsTest.java +++ b/s2a/src/test/java/io/grpc/s2a/MtlsToS2AChannelCredentialsTest.java @@ -26,11 +26,11 @@ @RunWith(JUnit4.class) public final class MtlsToS2AChannelCredentialsTest { @Test - public void createBuilder_nullAddress_throwsException() throws Exception { + public void newBuilder_nullAddress_throwsException() throws Exception { assertThrows( IllegalArgumentException.class, () -> - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ null, /* privateKeyPath= */ "src/test/resources/client_key.pem", /* certChainPath= */ "src/test/resources/client_cert.pem", @@ -38,11 +38,11 @@ public void createBuilder_nullAddress_throwsException() throws Exception { } @Test - public void createBuilder_nullPrivateKeyPath_throwsException() throws Exception { + public void newBuilder_nullPrivateKeyPath_throwsException() throws Exception { assertThrows( IllegalArgumentException.class, () -> - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ "s2a_address", /* privateKeyPath= */ null, /* certChainPath= */ "src/test/resources/client_cert.pem", @@ -50,11 +50,11 @@ public void createBuilder_nullPrivateKeyPath_throwsException() throws Exception } @Test - public void createBuilder_nullCertChainPath_throwsException() throws Exception { + public void newBuilder_nullCertChainPath_throwsException() throws Exception { assertThrows( IllegalArgumentException.class, () -> - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ "s2a_address", /* privateKeyPath= */ "src/test/resources/client_key.pem", /* certChainPath= */ null, @@ -62,11 +62,11 @@ public void createBuilder_nullCertChainPath_throwsException() throws Exception { } @Test - public void createBuilder_nullTrustBundlePath_throwsException() throws Exception { + public void newBuilder_nullTrustBundlePath_throwsException() throws Exception { assertThrows( IllegalArgumentException.class, () -> - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ "s2a_address", /* privateKeyPath= */ "src/test/resources/client_key.pem", /* certChainPath= */ "src/test/resources/client_cert.pem", @@ -74,11 +74,11 @@ public void createBuilder_nullTrustBundlePath_throwsException() throws Exception } @Test - public void createBuilder_emptyAddress_throwsException() throws Exception { + public void newBuilder_emptyAddress_throwsException() throws Exception { assertThrows( IllegalArgumentException.class, () -> - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ "", /* privateKeyPath= */ "src/test/resources/client_key.pem", /* certChainPath= */ "src/test/resources/client_cert.pem", @@ -86,11 +86,11 @@ public void createBuilder_emptyAddress_throwsException() throws Exception { } @Test - public void createBuilder_emptyPrivateKeyPath_throwsException() throws Exception { + public void newBuilder_emptyPrivateKeyPath_throwsException() throws Exception { assertThrows( IllegalArgumentException.class, () -> - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ "s2a_address", /* privateKeyPath= */ "", /* certChainPath= */ "src/test/resources/client_cert.pem", @@ -98,11 +98,11 @@ public void createBuilder_emptyPrivateKeyPath_throwsException() throws Exception } @Test - public void createBuilder_emptyCertChainPath_throwsException() throws Exception { + public void newBuilder_emptyCertChainPath_throwsException() throws Exception { assertThrows( IllegalArgumentException.class, () -> - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ "s2a_address", /* privateKeyPath= */ "src/test/resources/client_key.pem", /* certChainPath= */ "", @@ -110,11 +110,11 @@ public void createBuilder_emptyCertChainPath_throwsException() throws Exception } @Test - public void createBuilder_emptyTrustBundlePath_throwsException() throws Exception { + public void newBuilder_emptyTrustBundlePath_throwsException() throws Exception { assertThrows( IllegalArgumentException.class, () -> - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ "s2a_address", /* privateKeyPath= */ "src/test/resources/client_key.pem", /* certChainPath= */ "src/test/resources/client_cert.pem", @@ -124,7 +124,7 @@ public void createBuilder_emptyTrustBundlePath_throwsException() throws Exceptio @Test public void build_s2AChannelCredentials_success() throws Exception { assertThat( - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ "s2a_address", /* privateKeyPath= */ "src/test/resources/client_key.pem", /* certChainPath= */ "src/test/resources/client_cert.pem", diff --git a/s2a/src/test/java/io/grpc/s2a/S2AChannelCredentialsTest.java b/s2a/src/test/java/io/grpc/s2a/S2AChannelCredentialsTest.java index a6133ed0af8..e766aa3f145 100644 --- a/s2a/src/test/java/io/grpc/s2a/S2AChannelCredentialsTest.java +++ b/s2a/src/test/java/io/grpc/s2a/S2AChannelCredentialsTest.java @@ -30,40 +30,40 @@ @RunWith(JUnit4.class) public final class S2AChannelCredentialsTest { @Test - public void createBuilder_nullArgument_throwsException() throws Exception { - assertThrows(IllegalArgumentException.class, () -> S2AChannelCredentials.createBuilder(null)); + public void newBuilder_nullArgument_throwsException() throws Exception { + assertThrows(IllegalArgumentException.class, () -> S2AChannelCredentials.newBuilder(null)); } @Test - public void createBuilder_emptyAddress_throwsException() throws Exception { - assertThrows(IllegalArgumentException.class, () -> S2AChannelCredentials.createBuilder("")); + public void newBuilder_emptyAddress_throwsException() throws Exception { + assertThrows(IllegalArgumentException.class, () -> S2AChannelCredentials.newBuilder("")); } @Test public void setLocalSpiffeId_nullArgument_throwsException() throws Exception { assertThrows( NullPointerException.class, - () -> S2AChannelCredentials.createBuilder("s2a_address").setLocalSpiffeId(null)); + () -> S2AChannelCredentials.newBuilder("s2a_address").setLocalSpiffeId(null)); } @Test public void setLocalHostname_nullArgument_throwsException() throws Exception { assertThrows( NullPointerException.class, - () -> S2AChannelCredentials.createBuilder("s2a_address").setLocalHostname(null)); + () -> S2AChannelCredentials.newBuilder("s2a_address").setLocalHostname(null)); } @Test public void setLocalUid_nullArgument_throwsException() throws Exception { assertThrows( NullPointerException.class, - () -> S2AChannelCredentials.createBuilder("s2a_address").setLocalUid(null)); + () -> S2AChannelCredentials.newBuilder("s2a_address").setLocalUid(null)); } @Test public void build_withLocalSpiffeId_succeeds() throws Exception { assertThat( - S2AChannelCredentials.createBuilder("s2a_address") + S2AChannelCredentials.newBuilder("s2a_address") .setLocalSpiffeId("spiffe://test") .build()) .isNotNull(); @@ -72,7 +72,7 @@ public void build_withLocalSpiffeId_succeeds() throws Exception { @Test public void build_withLocalHostname_succeeds() throws Exception { assertThat( - S2AChannelCredentials.createBuilder("s2a_address") + S2AChannelCredentials.newBuilder("s2a_address") .setLocalHostname("local_hostname") .build()) .isNotNull(); @@ -80,20 +80,20 @@ public void build_withLocalHostname_succeeds() throws Exception { @Test public void build_withLocalUid_succeeds() throws Exception { - assertThat(S2AChannelCredentials.createBuilder("s2a_address").setLocalUid("local_uid").build()) + assertThat(S2AChannelCredentials.newBuilder("s2a_address").setLocalUid("local_uid").build()) .isNotNull(); } @Test public void build_withNoLocalIdentity_succeeds() throws Exception { - assertThat(S2AChannelCredentials.createBuilder("s2a_address").build()) + assertThat(S2AChannelCredentials.newBuilder("s2a_address").build()) .isNotNull(); } @Test public void build_withTlsChannelCredentials_succeeds() throws Exception { assertThat( - S2AChannelCredentials.createBuilder("s2a_address") + S2AChannelCredentials.newBuilder("s2a_address") .setLocalSpiffeId("spiffe://test") .setS2AChannelCredentials(getTlsChannelCredentials()) .build()) diff --git a/s2a/src/test/java/io/grpc/s2a/handshaker/IntegrationTest.java b/s2a/src/test/java/io/grpc/s2a/handshaker/IntegrationTest.java index 859771a4afa..272f23cc2cc 100644 --- a/s2a/src/test/java/io/grpc/s2a/handshaker/IntegrationTest.java +++ b/s2a/src/test/java/io/grpc/s2a/handshaker/IntegrationTest.java @@ -194,7 +194,7 @@ public void tearDown() throws Exception { public void clientCommunicateUsingS2ACredentials_succeeds() throws Exception { ExecutorService executor = Executors.newSingleThreadExecutor(); ChannelCredentials credentials = - S2AChannelCredentials.createBuilder(s2aAddress).setLocalSpiffeId("test-spiffe-id").build(); + S2AChannelCredentials.newBuilder(s2aAddress).setLocalSpiffeId("test-spiffe-id").build(); ManagedChannel channel = Grpc.newChannelBuilder(serverAddress, credentials).executor(executor).build(); @@ -204,7 +204,7 @@ public void clientCommunicateUsingS2ACredentials_succeeds() throws Exception { @Test public void clientCommunicateUsingS2ACredentialsNoLocalIdentity_succeeds() throws Exception { ExecutorService executor = Executors.newSingleThreadExecutor(); - ChannelCredentials credentials = S2AChannelCredentials.createBuilder(s2aAddress).build(); + ChannelCredentials credentials = S2AChannelCredentials.newBuilder(s2aAddress).build(); ManagedChannel channel = Grpc.newChannelBuilder(serverAddress, credentials).executor(executor).build(); @@ -215,7 +215,7 @@ public void clientCommunicateUsingS2ACredentialsNoLocalIdentity_succeeds() throw public void clientCommunicateUsingMtlsToS2ACredentials_succeeds() throws Exception { ExecutorService executor = Executors.newSingleThreadExecutor(); ChannelCredentials credentials = - MtlsToS2AChannelCredentials.createBuilder( + MtlsToS2AChannelCredentials.newBuilder( /* s2aAddress= */ mtlsS2AAddress, /* privateKeyPath= */ "src/test/resources/client_key.pem", /* certChainPath= */ "src/test/resources/client_cert.pem", @@ -242,7 +242,7 @@ private class DoUnaryRpc extends Thread { @Override public void run() { ExecutorService executor = Executors.newSingleThreadExecutor(); - ChannelCredentials credentials = S2AChannelCredentials.createBuilder(s2aDelayAddress).build(); + ChannelCredentials credentials = S2AChannelCredentials.newBuilder(s2aDelayAddress).build(); ManagedChannel channel = Grpc.newChannelBuilder(serverAddress, credentials).executor(executor).build(); boolean result = false; From 9c44754f4280df9e99c422b33ec65782ea5e25aa Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Tue, 17 Sep 2024 16:20:54 -0700 Subject: [PATCH 3/6] Remove usage of AdvancedTls. --- .../io/grpc/s2a/MtlsToS2AChannelCredentials.java | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java b/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java index de63f4f94f4..e8eb01628ed 100644 --- a/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java +++ b/s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java @@ -23,11 +23,8 @@ import io.grpc.ChannelCredentials; import io.grpc.ExperimentalApi; import io.grpc.TlsChannelCredentials; -import io.grpc.util.AdvancedTlsX509KeyManager; -import io.grpc.util.AdvancedTlsX509TrustManager; import java.io.File; import java.io.IOException; -import java.security.GeneralSecurityException; /** * Configures an {@code S2AChannelCredentials.Builder} instance with credentials used to establish a @@ -68,7 +65,7 @@ public static final class Builder { this.trustBundlePath = trustBundlePath; } - public S2AChannelCredentials.Builder build() throws GeneralSecurityException, IOException { + public S2AChannelCredentials.Builder build() throws IOException { checkState(!isNullOrEmpty(s2aAddress), "S2A address must not be null or empty."); checkState(!isNullOrEmpty(privateKeyPath), "privateKeyPath must not be null or empty."); checkState(!isNullOrEmpty(certChainPath), "certChainPath must not be null or empty."); @@ -77,16 +74,10 @@ public S2AChannelCredentials.Builder build() throws GeneralSecurityException, IO File certChainFile = new File(certChainPath); File trustBundleFile = new File(trustBundlePath); - AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager(); - keyManager.updateIdentityCredentials(certChainFile, privateKeyFile); - - AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build(); - trustManager.updateTrustCredentials(trustBundleFile); - ChannelCredentials channelToS2ACredentials = TlsChannelCredentials.newBuilder() - .keyManager(keyManager) - .trustManager(trustManager) + .keyManager(certChainFile, privateKeyFile) + .trustManager(trustBundleFile) .build(); return S2AChannelCredentials.newBuilder(s2aAddress) From ef53441de68e1f6da03bfc8f6726702dfda56c03 Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Tue, 17 Sep 2024 16:29:59 -0700 Subject: [PATCH 4/6] Use InsecureChannelCredentials.create instead of Optional. --- .../io/grpc/s2a/S2AChannelCredentials.java | 8 ++--- .../channel/S2AHandshakerServiceChannel.java | 33 ++++++------------- .../S2AHandshakerServiceChannelTest.java | 25 +++++++------- .../S2AProtocolNegotiatorFactoryTest.java | 2 +- .../io/grpc/s2a/handshaker/S2AStubTest.java | 4 +-- 5 files changed, 29 insertions(+), 43 deletions(-) diff --git a/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java b/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java index 1b9a42fb340..ba0f6d72de1 100644 --- a/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java +++ b/s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java @@ -25,6 +25,7 @@ import io.grpc.Channel; import io.grpc.ChannelCredentials; import io.grpc.ExperimentalApi; +import io.grpc.InsecureChannelCredentials; import io.grpc.internal.ObjectPool; import io.grpc.internal.SharedResourcePool; import io.grpc.netty.InternalNettyChannelCredentials; @@ -32,7 +33,6 @@ import io.grpc.s2a.channel.S2AHandshakerServiceChannel; import io.grpc.s2a.handshaker.S2AIdentity; import io.grpc.s2a.handshaker.S2AProtocolNegotiatorFactory; -import java.util.Optional; import javax.annotation.concurrent.NotThreadSafe; import org.checkerframework.checker.nullness.qual.Nullable; @@ -58,13 +58,13 @@ public static Builder newBuilder(String s2aAddress) { public static final class Builder { private final String s2aAddress; private ObjectPool s2aChannelPool; - private Optional s2aChannelCredentials; + private ChannelCredentials s2aChannelCredentials; private @Nullable S2AIdentity localIdentity = null; Builder(String s2aAddress) { this.s2aAddress = s2aAddress; this.s2aChannelPool = null; - this.s2aChannelCredentials = Optional.empty(); + this.s2aChannelCredentials = InsecureChannelCredentials.create(); } /** @@ -109,7 +109,7 @@ public Builder setLocalUid(String localUid) { /** Sets the credentials to be used when connecting to the S2A. */ @CanIgnoreReturnValue public Builder setS2AChannelCredentials(ChannelCredentials s2aChannelCredentials) { - this.s2aChannelCredentials = Optional.of(s2aChannelCredentials); + this.s2aChannelCredentials = s2aChannelCredentials; return this; } diff --git a/s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java b/s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java index 75ec7347bb5..8b6d502b73f 100644 --- a/s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java +++ b/s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java @@ -34,7 +34,6 @@ import io.netty.channel.socket.nio.NioSocketChannel; import io.netty.util.concurrent.DefaultThreadFactory; import java.time.Duration; -import java.util.Optional; import java.util.concurrent.ConcurrentMap; import javax.annotation.concurrent.ThreadSafe; @@ -74,8 +73,9 @@ public final class S2AHandshakerServiceChannel { * running at {@code s2aAddress}. */ public static Resource getChannelResource( - String s2aAddress, Optional s2aChannelCredentials) { + String s2aAddress, ChannelCredentials s2aChannelCredentials) { checkNotNull(s2aAddress); + checkNotNull(s2aChannelCredentials); return SHARED_RESOURCE_CHANNELS.computeIfAbsent( s2aAddress, channelResource -> new ChannelResource(s2aAddress, s2aChannelCredentials)); } @@ -87,9 +87,9 @@ public static Resource getChannelResource( */ private static class ChannelResource implements Resource { private final String targetAddress; - private final Optional channelCredentials; + private final ChannelCredentials channelCredentials; - public ChannelResource(String targetAddress, Optional channelCredentials) { + public ChannelResource(String targetAddress, ChannelCredentials channelCredentials) { this.targetAddress = targetAddress; this.channelCredentials = channelCredentials; } @@ -103,25 +103,12 @@ public ChannelResource(String targetAddress, Optional channe public Channel create() { EventLoopGroup eventLoopGroup = new NioEventLoopGroup(1, new DefaultThreadFactory("S2A channel pool", true)); - ManagedChannel channel = null; - if (channelCredentials.isPresent()) { - // Create a secure channel. - channel = - NettyChannelBuilder.forTarget(targetAddress, channelCredentials.get()) - .channelType(NioSocketChannel.class) - .directExecutor() - .eventLoopGroup(eventLoopGroup) - .build(); - } else { - // Create a plaintext channel. - channel = - NettyChannelBuilder.forTarget(targetAddress) - .channelType(NioSocketChannel.class) - .directExecutor() - .eventLoopGroup(eventLoopGroup) - .usePlaintext() - .build(); - } + ManagedChannel channel = + NettyChannelBuilder.forTarget(targetAddress, channelCredentials) + .channelType(NioSocketChannel.class) + .directExecutor() + .eventLoopGroup(eventLoopGroup) + .build(); return EventLoopHoldingChannel.create(channel, eventLoopGroup); } diff --git a/s2a/src/test/java/io/grpc/s2a/channel/S2AHandshakerServiceChannelTest.java b/s2a/src/test/java/io/grpc/s2a/channel/S2AHandshakerServiceChannelTest.java index 57288be1b6f..75f5d888bee 100644 --- a/s2a/src/test/java/io/grpc/s2a/channel/S2AHandshakerServiceChannelTest.java +++ b/s2a/src/test/java/io/grpc/s2a/channel/S2AHandshakerServiceChannelTest.java @@ -28,6 +28,7 @@ import io.grpc.Channel; import io.grpc.ChannelCredentials; import io.grpc.ClientCall; +import io.grpc.InsecureChannelCredentials; import io.grpc.ManagedChannel; import io.grpc.MethodDescriptor; import io.grpc.Server; @@ -48,7 +49,6 @@ import io.netty.channel.EventLoopGroup; import java.io.File; import java.time.Duration; -import java.util.Optional; import java.util.concurrent.TimeUnit; import org.junit.Before; import org.junit.ClassRule; @@ -82,7 +82,7 @@ public void getChannelResource_success() { Resource resource = S2AHandshakerServiceChannel.getChannelResource( "localhost:" + plaintextServer.getPort(), - /* s2aChannelCredentials= */ Optional.empty()); + InsecureChannelCredentials.create()); assertThat(resource.toString()).isEqualTo("grpc-s2a-channel"); } @@ -104,11 +104,11 @@ public void getChannelResource_twoEqualChannels() { Resource resource = S2AHandshakerServiceChannel.getChannelResource( "localhost:" + plaintextServer.getPort(), - /* s2aChannelCredentials= */ Optional.empty()); + InsecureChannelCredentials.create()); Resource resourceTwo = S2AHandshakerServiceChannel.getChannelResource( "localhost:" + plaintextServer.getPort(), - /* s2aChannelCredentials= */ Optional.empty()); + InsecureChannelCredentials.create()); assertThat(resource).isEqualTo(resourceTwo); } @@ -133,10 +133,10 @@ public void getChannelResource_twoDistinctChannels() { Resource resource = S2AHandshakerServiceChannel.getChannelResource( "localhost:" + plaintextServer.getPort(), - /* s2aChannelCredentials= */ Optional.empty()); + InsecureChannelCredentials.create()); Resource resourceTwo = S2AHandshakerServiceChannel.getChannelResource( - "localhost:" + Utils.pickUnusedPort(), /* s2aChannelCredentials= */ Optional.empty()); + "localhost:" + Utils.pickUnusedPort(), InsecureChannelCredentials.create()); assertThat(resourceTwo).isNotEqualTo(resource); } @@ -161,7 +161,7 @@ public void close_success() { Resource resource = S2AHandshakerServiceChannel.getChannelResource( "localhost:" + plaintextServer.getPort(), - /* s2aChannelCredentials= */ Optional.empty()); + InsecureChannelCredentials.create()); Channel channel = resource.create(); resource.close(channel); StatusRuntimeException expected = @@ -199,7 +199,7 @@ public void newCall_performSimpleRpcSuccess() { Resource resource = S2AHandshakerServiceChannel.getChannelResource( "localhost:" + plaintextServer.getPort(), - /* s2aChannelCredentials= */ Optional.empty()); + InsecureChannelCredentials.create()); Channel channel = resource.create(); assertThat(channel).isInstanceOf(EventLoopHoldingChannel.class); assertThat( @@ -268,7 +268,7 @@ public void create_succeedsAfterCloseIsCalledOnce() throws Exception { Resource resource = S2AHandshakerServiceChannel.getChannelResource( "localhost:" + plaintextServer.getPort(), - /* s2aChannelCredentials= */ Optional.empty()); + InsecureChannelCredentials.create()); Channel channelOne = resource.create(); resource.close(channelOne); @@ -320,15 +320,14 @@ private static Server createPlaintextServer() { ServerBuilder.forPort(Utils.pickUnusedPort()).addService(service).build()); } - private static Optional getTlsChannelCredentials() throws Exception { + private static ChannelCredentials getTlsChannelCredentials() throws Exception { File clientCert = new File("src/test/resources/client_cert.pem"); File clientKey = new File("src/test/resources/client_key.pem"); File rootCert = new File("src/test/resources/root_cert.pem"); - return Optional.of( - TlsChannelCredentials.newBuilder() + return TlsChannelCredentials.newBuilder() .keyManager(clientCert, clientKey) .trustManager(rootCert) - .build()); + .build(); } private static class SimpleServiceImpl extends SimpleServiceGrpc.SimpleServiceImplBase { diff --git a/s2a/src/test/java/io/grpc/s2a/handshaker/S2AProtocolNegotiatorFactoryTest.java b/s2a/src/test/java/io/grpc/s2a/handshaker/S2AProtocolNegotiatorFactoryTest.java index f130e52aac7..404910e8be0 100644 --- a/s2a/src/test/java/io/grpc/s2a/handshaker/S2AProtocolNegotiatorFactoryTest.java +++ b/s2a/src/test/java/io/grpc/s2a/handshaker/S2AProtocolNegotiatorFactoryTest.java @@ -115,7 +115,7 @@ public void createProtocolNegotiator_nullArgument() throws Exception { S2AGrpcChannelPool.create( SharedResourcePool.forResource( S2AHandshakerServiceChannel.getChannelResource( - "localhost:8080", /* s2aChannelCredentials= */ Optional.empty()))); + "localhost:8080", InsecureChannelCredentials.create()))); NullPointerTester tester = new NullPointerTester() diff --git a/s2a/src/test/java/io/grpc/s2a/handshaker/S2AStubTest.java b/s2a/src/test/java/io/grpc/s2a/handshaker/S2AStubTest.java index bb90be12b6a..47fd154d949 100644 --- a/s2a/src/test/java/io/grpc/s2a/handshaker/S2AStubTest.java +++ b/s2a/src/test/java/io/grpc/s2a/handshaker/S2AStubTest.java @@ -21,13 +21,13 @@ import static org.junit.Assert.assertThrows; import com.google.common.truth.Expect; +import io.grpc.InsecureChannelCredentials; import io.grpc.internal.SharedResourcePool; import io.grpc.s2a.channel.S2AChannelPool; import io.grpc.s2a.channel.S2AGrpcChannelPool; import io.grpc.s2a.channel.S2AHandshakerServiceChannel; import io.grpc.stub.StreamObserver; import java.io.IOException; -import java.util.Optional; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -55,7 +55,7 @@ public void send_receiveOkStatus() throws Exception { S2AGrpcChannelPool.create( SharedResourcePool.forResource( S2AHandshakerServiceChannel.getChannelResource( - S2A_ADDRESS, /* s2aChannelCredentials= */ Optional.empty()))); + S2A_ADDRESS, InsecureChannelCredentials.create()))); S2AServiceGrpc.S2AServiceStub serviceStub = S2AServiceGrpc.newStub(channelPool.getChannel()); S2AStub newStub = S2AStub.newInstance(serviceStub); From c1375bb3d491cee3cc3ce3f175a23915a40dc38b Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Wed, 18 Sep 2024 15:30:49 -0700 Subject: [PATCH 5/6] Invoke Thread.currentThread().interrupt() in a InterruptedException block. --- .../java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java | 1 + s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java | 1 + 2 files changed, 2 insertions(+) diff --git a/s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java b/s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java index 8b6d502b73f..fad28eafa8b 100644 --- a/s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java +++ b/s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java @@ -170,6 +170,7 @@ public void close() { isDelegateTerminated = delegate.awaitTermination(DELEGATE_TERMINATION_TIMEOUT.getSeconds(), SECONDS); } catch (InterruptedException e) { + Thread.currentThread().interrupt(); isDelegateTerminated = false; } long quietPeriodSeconds = isDelegateTerminated ? 0 : 1; diff --git a/s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java b/s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java index fb113bb29cc..9762e12155d 100644 --- a/s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java +++ b/s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java @@ -121,6 +121,7 @@ private void checkPeerTrusted(X509Certificate[] chain, boolean isCheckingClientC try { resp = stub.send(reqBuilder.build()); } catch (IOException | InterruptedException e) { + Thread.currentThread().interrupt(); throw new CertificateException("Failed to send request to S2A.", e); } if (resp.hasStatus() && resp.getStatus().getCode() != 0) { From b060a4929d9d172ec287d28f50d750b502ddc8f5 Mon Sep 17 00:00:00 2001 From: Riya Mehta Date: Wed, 18 Sep 2024 17:14:19 -0700 Subject: [PATCH 6/6] only interrupt on InterruptedException. --- s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java b/s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java index 9762e12155d..aafbb94c047 100644 --- a/s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java +++ b/s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java @@ -121,7 +121,9 @@ private void checkPeerTrusted(X509Certificate[] chain, boolean isCheckingClientC try { resp = stub.send(reqBuilder.build()); } catch (IOException | InterruptedException e) { - Thread.currentThread().interrupt(); + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } throw new CertificateException("Failed to send request to S2A.", e); } if (resp.hasStatus() && resp.getStatus().getCode() != 0) {