Skip to content

Commit d8f73e0

Browse files
rmehta19rmehta19
authored
s2a: Address comments on PR#11113 (#11534)
* Mark S2A public APIs as experimental. * Rename S2AChannelCredentials createBuilder API to newBuilder. * Remove usage of AdvancedTls. * Use InsecureChannelCredentials.create instead of Optional. * Invoke Thread.currentThread().interrupt() in a InterruptedException block.
1 parent e75a044 commit d8f73e0

10 files changed

+73
-87
lines changed

‎s2a/src/main/java/io/grpc/s2a/MtlsToS2AChannelCredentials.java

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,16 @@
2121
import static com.google.common.base.Strings.isNullOrEmpty;
2222

2323
import io.grpc.ChannelCredentials;
24+
import io.grpc.ExperimentalApi;
2425
import io.grpc.TlsChannelCredentials;
25-
import io.grpc.util.AdvancedTlsX509KeyManager;
26-
import io.grpc.util.AdvancedTlsX509TrustManager;
2726
import java.io.File;
2827
import java.io.IOException;
29-
import java.security.GeneralSecurityException;
3028

3129
/**
3230
* Configures an {@code S2AChannelCredentials.Builder} instance with credentials used to establish a
3331
* connection with the S2A to support talking to the S2A over mTLS.
3432
*/
33+
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/11533")
3534
public final class MtlsToS2AChannelCredentials {
3635
/**
3736
* Creates a {@code S2AChannelCredentials.Builder} builder, that talks to the S2A over mTLS.
@@ -42,7 +41,7 @@ public final class MtlsToS2AChannelCredentials {
4241
* @param trustBundlePath the path to the trust bundle PEM.
4342
* @return a {@code MtlsToS2AChannelCredentials.Builder} instance.
4443
*/
45-
public static Builder createBuilder(
44+
public static Builder newBuilder(
4645
String s2aAddress, String privateKeyPath, String certChainPath, String trustBundlePath) {
4746
checkArgument(!isNullOrEmpty(s2aAddress), "S2A address must not be null or empty.");
4847
checkArgument(!isNullOrEmpty(privateKeyPath), "privateKeyPath must not be null or empty.");
@@ -66,7 +65,7 @@ public static final class Builder {
6665
this.trustBundlePath = trustBundlePath;
6766
}
6867

69-
public S2AChannelCredentials.Builder build() throws GeneralSecurityException, IOException {
68+
public S2AChannelCredentials.Builder build() throws IOException {
7069
checkState(!isNullOrEmpty(s2aAddress), "S2A address must not be null or empty.");
7170
checkState(!isNullOrEmpty(privateKeyPath), "privateKeyPath must not be null or empty.");
7271
checkState(!isNullOrEmpty(certChainPath), "certChainPath must not be null or empty.");
@@ -75,19 +74,13 @@ public S2AChannelCredentials.Builder build() throws GeneralSecurityException, IO
7574
File certChainFile = new File(certChainPath);
7675
File trustBundleFile = new File(trustBundlePath);
7776

78-
AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager();
79-
keyManager.updateIdentityCredentials(certChainFile, privateKeyFile);
80-
81-
AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();
82-
trustManager.updateTrustCredentials(trustBundleFile);
83-
8477
ChannelCredentials channelToS2ACredentials =
8578
TlsChannelCredentials.newBuilder()
86-
.keyManager(keyManager)
87-
.trustManager(trustManager)
79+
.keyManager(certChainFile, privateKeyFile)
80+
.trustManager(trustBundleFile)
8881
.build();
8982

90-
return S2AChannelCredentials.createBuilder(s2aAddress)
83+
return S2AChannelCredentials.newBuilder(s2aAddress)
9184
.setS2AChannelCredentials(channelToS2ACredentials);
9285
}
9386
}

‎s2a/src/main/java/io/grpc/s2a/S2AChannelCredentials.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,29 +24,31 @@
2424
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2525
import io.grpc.Channel;
2626
import io.grpc.ChannelCredentials;
27+
import io.grpc.ExperimentalApi;
28+
import io.grpc.InsecureChannelCredentials;
2729
import io.grpc.internal.ObjectPool;
2830
import io.grpc.internal.SharedResourcePool;
2931
import io.grpc.netty.InternalNettyChannelCredentials;
3032
import io.grpc.netty.InternalProtocolNegotiator;
3133
import io.grpc.s2a.channel.S2AHandshakerServiceChannel;
3234
import io.grpc.s2a.handshaker.S2AIdentity;
3335
import io.grpc.s2a.handshaker.S2AProtocolNegotiatorFactory;
34-
import java.util.Optional;
3536
import javax.annotation.concurrent.NotThreadSafe;
3637
import org.checkerframework.checker.nullness.qual.Nullable;
3738

3839
/**
3940
* Configures gRPC to use S2A for transport security when establishing a secure channel. Only for
4041
* use on the client side of a gRPC connection.
4142
*/
43+
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/11533")
4244
public final class S2AChannelCredentials {
4345
/**
4446
* Creates a channel credentials builder for establishing an S2A-secured connection.
4547
*
4648
* @param s2aAddress the address of the S2A server used to secure the connection.
4749
* @return a {@code S2AChannelCredentials.Builder} instance.
4850
*/
49-
public static Builder createBuilder(String s2aAddress) {
51+
public static Builder newBuilder(String s2aAddress) {
5052
checkArgument(!isNullOrEmpty(s2aAddress), "S2A address must not be null or empty.");
5153
return new Builder(s2aAddress);
5254
}
@@ -56,13 +58,13 @@ public static Builder createBuilder(String s2aAddress) {
5658
public static final class Builder {
5759
private final String s2aAddress;
5860
private ObjectPool<Channel> s2aChannelPool;
59-
private Optional<ChannelCredentials> s2aChannelCredentials;
61+
private ChannelCredentials s2aChannelCredentials;
6062
private @Nullable S2AIdentity localIdentity = null;
6163

6264
Builder(String s2aAddress) {
6365
this.s2aAddress = s2aAddress;
6466
this.s2aChannelPool = null;
65-
this.s2aChannelCredentials = Optional.empty();
67+
this.s2aChannelCredentials = InsecureChannelCredentials.create();
6668
}
6769

6870
/**
@@ -107,7 +109,7 @@ public Builder setLocalUid(String localUid) {
107109
/** Sets the credentials to be used when connecting to the S2A. */
108110
@CanIgnoreReturnValue
109111
public Builder setS2AChannelCredentials(ChannelCredentials s2aChannelCredentials) {
110-
this.s2aChannelCredentials = Optional.of(s2aChannelCredentials);
112+
this.s2aChannelCredentials = s2aChannelCredentials;
111113
return this;
112114
}
113115

‎s2a/src/main/java/io/grpc/s2a/channel/S2AHandshakerServiceChannel.java

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import io.grpc.internal.SharedResourceHolder.Resource;
3131
import io.grpc.netty.NettyChannelBuilder;
3232
import java.time.Duration;
33-
import java.util.Optional;
3433
import java.util.concurrent.ConcurrentMap;
3534
import java.util.logging.Level;
3635
import java.util.logging.Logger;
@@ -71,8 +70,9 @@ public final class S2AHandshakerServiceChannel {
7170
* running at {@code s2aAddress}.
7271
*/
7372
public static Resource<Channel> getChannelResource(
74-
String s2aAddress, Optional<ChannelCredentials> s2aChannelCredentials) {
73+
String s2aAddress, ChannelCredentials s2aChannelCredentials) {
7574
checkNotNull(s2aAddress);
75+
checkNotNull(s2aChannelCredentials);
7676
return SHARED_RESOURCE_CHANNELS.computeIfAbsent(
7777
s2aAddress, channelResource -> new ChannelResource(s2aAddress, s2aChannelCredentials));
7878
}
@@ -84,9 +84,9 @@ public static Resource<Channel> getChannelResource(
8484
*/
8585
private static class ChannelResource implements Resource<Channel> {
8686
private final String targetAddress;
87-
private final Optional<ChannelCredentials> channelCredentials;
87+
private final ChannelCredentials channelCredentials;
8888

89-
public ChannelResource(String targetAddress, Optional<ChannelCredentials> channelCredentials) {
89+
public ChannelResource(String targetAddress, ChannelCredentials channelCredentials) {
9090
this.targetAddress = targetAddress;
9191
this.channelCredentials = channelCredentials;
9292
}
@@ -97,21 +97,10 @@ public ChannelResource(String targetAddress, Optional<ChannelCredentials> channe
9797
*/
9898
@Override
9999
public Channel create() {
100-
ManagedChannel channel = null;
101-
if (channelCredentials.isPresent()) {
102-
// Create a secure channel.
103-
channel =
104-
NettyChannelBuilder.forTarget(targetAddress, channelCredentials.get())
105-
.directExecutor()
106-
.build();
107-
} else {
108-
// Create a plaintext channel.
109-
channel =
110-
NettyChannelBuilder.forTarget(targetAddress)
111-
.directExecutor()
112-
.usePlaintext()
113-
.build();
114-
}
100+
ManagedChannel channel =
101+
NettyChannelBuilder.forTarget(targetAddress, channelCredentials)
102+
.directExecutor()
103+
.build();
115104
return HandshakerServiceChannel.create(channel);
116105
}
117106

‎s2a/src/main/java/io/grpc/s2a/handshaker/S2ATrustManager.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,9 @@ private void checkPeerTrusted(X509Certificate[] chain, boolean isCheckingClientC
121121
try {
122122
resp = stub.send(reqBuilder.build());
123123
} catch (IOException | InterruptedException e) {
124+
if (e instanceof InterruptedException) {
125+
Thread.currentThread().interrupt();
126+
}
124127
throw new CertificateException("Failed to send request to S2A.", e);
125128
}
126129
if (resp.hasStatus() && resp.getStatus().getCode() != 0) {

‎s2a/src/test/java/io/grpc/s2a/MtlsToS2AChannelCredentialsTest.java

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,95 +26,95 @@
2626
@RunWith(JUnit4.class)
2727
public final class MtlsToS2AChannelCredentialsTest {
2828
@Test
29-
public void createBuilder_nullAddress_throwsException() throws Exception {
29+
public void newBuilder_nullAddress_throwsException() throws Exception {
3030
assertThrows(
3131
IllegalArgumentException.class,
3232
() ->
33-
MtlsToS2AChannelCredentials.createBuilder(
33+
MtlsToS2AChannelCredentials.newBuilder(
3434
/* s2aAddress= */ null,
3535
/* privateKeyPath= */ "src/test/resources/client_key.pem",
3636
/* certChainPath= */ "src/test/resources/client_cert.pem",
3737
/* trustBundlePath= */ "src/test/resources/root_cert.pem"));
3838
}
3939

4040
@Test
41-
public void createBuilder_nullPrivateKeyPath_throwsException() throws Exception {
41+
public void newBuilder_nullPrivateKeyPath_throwsException() throws Exception {
4242
assertThrows(
4343
IllegalArgumentException.class,
4444
() ->
45-
MtlsToS2AChannelCredentials.createBuilder(
45+
MtlsToS2AChannelCredentials.newBuilder(
4646
/* s2aAddress= */ "s2a_address",
4747
/* privateKeyPath= */ null,
4848
/* certChainPath= */ "src/test/resources/client_cert.pem",
4949
/* trustBundlePath= */ "src/test/resources/root_cert.pem"));
5050
}
5151

5252
@Test
53-
public void createBuilder_nullCertChainPath_throwsException() throws Exception {
53+
public void newBuilder_nullCertChainPath_throwsException() throws Exception {
5454
assertThrows(
5555
IllegalArgumentException.class,
5656
() ->
57-
MtlsToS2AChannelCredentials.createBuilder(
57+
MtlsToS2AChannelCredentials.newBuilder(
5858
/* s2aAddress= */ "s2a_address",
5959
/* privateKeyPath= */ "src/test/resources/client_key.pem",
6060
/* certChainPath= */ null,
6161
/* trustBundlePath= */ "src/test/resources/root_cert.pem"));
6262
}
6363

6464
@Test
65-
public void createBuilder_nullTrustBundlePath_throwsException() throws Exception {
65+
public void newBuilder_nullTrustBundlePath_throwsException() throws Exception {
6666
assertThrows(
6767
IllegalArgumentException.class,
6868
() ->
69-
MtlsToS2AChannelCredentials.createBuilder(
69+
MtlsToS2AChannelCredentials.newBuilder(
7070
/* s2aAddress= */ "s2a_address",
7171
/* privateKeyPath= */ "src/test/resources/client_key.pem",
7272
/* certChainPath= */ "src/test/resources/client_cert.pem",
7373
/* trustBundlePath= */ null));
7474
}
7575

7676
@Test
77-
public void createBuilder_emptyAddress_throwsException() throws Exception {
77+
public void newBuilder_emptyAddress_throwsException() throws Exception {
7878
assertThrows(
7979
IllegalArgumentException.class,
8080
() ->
81-
MtlsToS2AChannelCredentials.createBuilder(
81+
MtlsToS2AChannelCredentials.newBuilder(
8282
/* s2aAddress= */ "",
8383
/* privateKeyPath= */ "src/test/resources/client_key.pem",
8484
/* certChainPath= */ "src/test/resources/client_cert.pem",
8585
/* trustBundlePath= */ "src/test/resources/root_cert.pem"));
8686
}
8787

8888
@Test
89-
public void createBuilder_emptyPrivateKeyPath_throwsException() throws Exception {
89+
public void newBuilder_emptyPrivateKeyPath_throwsException() throws Exception {
9090
assertThrows(
9191
IllegalArgumentException.class,
9292
() ->
93-
MtlsToS2AChannelCredentials.createBuilder(
93+
MtlsToS2AChannelCredentials.newBuilder(
9494
/* s2aAddress= */ "s2a_address",
9595
/* privateKeyPath= */ "",
9696
/* certChainPath= */ "src/test/resources/client_cert.pem",
9797
/* trustBundlePath= */ "src/test/resources/root_cert.pem"));
9898
}
9999

100100
@Test
101-
public void createBuilder_emptyCertChainPath_throwsException() throws Exception {
101+
public void newBuilder_emptyCertChainPath_throwsException() throws Exception {
102102
assertThrows(
103103
IllegalArgumentException.class,
104104
() ->
105-
MtlsToS2AChannelCredentials.createBuilder(
105+
MtlsToS2AChannelCredentials.newBuilder(
106106
/* s2aAddress= */ "s2a_address",
107107
/* privateKeyPath= */ "src/test/resources/client_key.pem",
108108
/* certChainPath= */ "",
109109
/* trustBundlePath= */ "src/test/resources/root_cert.pem"));
110110
}
111111

112112
@Test
113-
public void createBuilder_emptyTrustBundlePath_throwsException() throws Exception {
113+
public void newBuilder_emptyTrustBundlePath_throwsException() throws Exception {
114114
assertThrows(
115115
IllegalArgumentException.class,
116116
() ->
117-
MtlsToS2AChannelCredentials.createBuilder(
117+
MtlsToS2AChannelCredentials.newBuilder(
118118
/* s2aAddress= */ "s2a_address",
119119
/* privateKeyPath= */ "src/test/resources/client_key.pem",
120120
/* certChainPath= */ "src/test/resources/client_cert.pem",
@@ -124,7 +124,7 @@ public void createBuilder_emptyTrustBundlePath_throwsException() throws Exceptio
124124
@Test
125125
public void build_s2AChannelCredentials_success() throws Exception {
126126
assertThat(
127-
MtlsToS2AChannelCredentials.createBuilder(
127+
MtlsToS2AChannelCredentials.newBuilder(
128128
/* s2aAddress= */ "s2a_address",
129129
/* privateKeyPath= */ "src/test/resources/client_key.pem",
130130
/* certChainPath= */ "src/test/resources/client_cert.pem",

‎s2a/src/test/java/io/grpc/s2a/S2AChannelCredentialsTest.java

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,40 +30,40 @@
3030
@RunWith(JUnit4.class)
3131
public final class S2AChannelCredentialsTest {
3232
@Test
33-
public void createBuilder_nullArgument_throwsException() throws Exception {
34-
assertThrows(IllegalArgumentException.class, () -> S2AChannelCredentials.createBuilder(null));
33+
public void newBuilder_nullArgument_throwsException() throws Exception {
34+
assertThrows(IllegalArgumentException.class, () -> S2AChannelCredentials.newBuilder(null));
3535
}
3636

3737
@Test
38-
public void createBuilder_emptyAddress_throwsException() throws Exception {
39-
assertThrows(IllegalArgumentException.class, () -> S2AChannelCredentials.createBuilder(""));
38+
public void newBuilder_emptyAddress_throwsException() throws Exception {
39+
assertThrows(IllegalArgumentException.class, () -> S2AChannelCredentials.newBuilder(""));
4040
}
4141

4242
@Test
4343
public void setLocalSpiffeId_nullArgument_throwsException() throws Exception {
4444
assertThrows(
4545
NullPointerException.class,
46-
() -> S2AChannelCredentials.createBuilder("s2a_address").setLocalSpiffeId(null));
46+
() -> S2AChannelCredentials.newBuilder("s2a_address").setLocalSpiffeId(null));
4747
}
4848

4949
@Test
5050
public void setLocalHostname_nullArgument_throwsException() throws Exception {
5151
assertThrows(
5252
NullPointerException.class,
53-
() -> S2AChannelCredentials.createBuilder("s2a_address").setLocalHostname(null));
53+
() -> S2AChannelCredentials.newBuilder("s2a_address").setLocalHostname(null));
5454
}
5555

5656
@Test
5757
public void setLocalUid_nullArgument_throwsException() throws Exception {
5858
assertThrows(
5959
NullPointerException.class,
60-
() -> S2AChannelCredentials.createBuilder("s2a_address").setLocalUid(null));
60+
() -> S2AChannelCredentials.newBuilder("s2a_address").setLocalUid(null));
6161
}
6262

6363
@Test
6464
public void build_withLocalSpiffeId_succeeds() throws Exception {
6565
assertThat(
66-
S2AChannelCredentials.createBuilder("s2a_address")
66+
S2AChannelCredentials.newBuilder("s2a_address")
6767
.setLocalSpiffeId("spiffe://test")
6868
.build())
6969
.isNotNull();
@@ -72,28 +72,28 @@ public void build_withLocalSpiffeId_succeeds() throws Exception {
7272
@Test
7373
public void build_withLocalHostname_succeeds() throws Exception {
7474
assertThat(
75-
S2AChannelCredentials.createBuilder("s2a_address")
75+
S2AChannelCredentials.newBuilder("s2a_address")
7676
.setLocalHostname("local_hostname")
7777
.build())
7878
.isNotNull();
7979
}
8080

8181
@Test
8282
public void build_withLocalUid_succeeds() throws Exception {
83-
assertThat(S2AChannelCredentials.createBuilder("s2a_address").setLocalUid("local_uid").build())
83+
assertThat(S2AChannelCredentials.newBuilder("s2a_address").setLocalUid("local_uid").build())
8484
.isNotNull();
8585
}
8686

8787
@Test
8888
public void build_withNoLocalIdentity_succeeds() throws Exception {
89-
assertThat(S2AChannelCredentials.createBuilder("s2a_address").build())
89+
assertThat(S2AChannelCredentials.newBuilder("s2a_address").build())
9090
.isNotNull();
9191
}
9292

9393
@Test
9494
public void build_withTlsChannelCredentials_succeeds() throws Exception {
9595
assertThat(
96-
S2AChannelCredentials.createBuilder("s2a_address")
96+
S2AChannelCredentials.newBuilder("s2a_address")
9797
.setLocalSpiffeId("spiffe://test")
9898
.setS2AChannelCredentials(getTlsChannelCredentials())
9999
.build())

0 commit comments

Comments
 (0)