From 233c0ce8a1e089c1af2dec19ca35b2b521e0a9e4 Mon Sep 17 00:00:00 2001 From: Matthew Stevenson Date: Tue, 4 Apr 2023 16:14:13 -0700 Subject: [PATCH 1/5] alts: Enable user to configure max number of concurrent ALTS handshakes. --- .../alts/internal/AltsProtocolNegotiator.java | 31 ++++++++++++++++--- .../internal/AltsProtocolNegotiatorTest.java | 28 +++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java index 8df9363f99f..5737c90e818 100644 --- a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java +++ b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java @@ -54,10 +54,12 @@ // TODO(carl-mastrangelo): rename this AltsProtocolNegotiators. public final class AltsProtocolNegotiator { private static final Logger logger = Logger.getLogger(AltsProtocolNegotiator.class.getName()); - // Avoid performing too many handshakes in parallel, as it may cause queuing in the handshake - // server and cause unbounded blocking on the event loop (b/168808426). This is a workaround until - // there is an async TSI handshaking API to avoid the blocking. - private static final AsyncSemaphore handshakeSemaphore = new AsyncSemaphore(32); + @VisibleForTesting + static final String ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE = + "GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES"; + @VisibleForTesting static final int DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES = 32; + private static final AsyncSemaphore handshakeSemaphore = + new AsyncSemaphore(getAltsMaxConcurrentHandshakes()); @Grpc.TransportAttr public static final Attributes.Key TSI_PEER_KEY = @@ -424,5 +426,26 @@ public SecurityDetails validatePeerObject(Object peerObject) throws GeneralSecur } } + @VisibleForTesting + static int getAltsMaxConcurrentHandshakes() { + String altsMaxConcurrentHandshakes = System.getenv(ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE); + if (altsMaxConcurrentHandshakes == null) { + return DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES; + } + try { + int effectiveMaxConcurrentHandshakes = Integer.parseInt(altsMaxConcurrentHandshakes); + if (effectiveMaxConcurrentHandshakes < 0) { + logger.warning( + "GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES environment variable set to invalid value."); + return DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES; + } + return effectiveMaxConcurrentHandshakes; + } catch (NumberFormatException e) { + logger.warning( + "GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES environment variable set to invalid value."); + return DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES; + } + } + private AltsProtocolNegotiator() {} } diff --git a/alts/src/test/java/io/grpc/alts/internal/AltsProtocolNegotiatorTest.java b/alts/src/test/java/io/grpc/alts/internal/AltsProtocolNegotiatorTest.java index b2506288efc..c0432207d2b 100644 --- a/alts/src/test/java/io/grpc/alts/internal/AltsProtocolNegotiatorTest.java +++ b/alts/src/test/java/io/grpc/alts/internal/AltsProtocolNegotiatorTest.java @@ -354,6 +354,34 @@ public void peerPropagated() throws Exception { .isEqualTo(SecurityLevel.PRIVACY_AND_INTEGRITY); } + @Test + public void getAltsMaxConcurrentHandshakes_success() throws Exception { + System.setProperty(AltsProtocolNegotiator.ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE, "10"); + assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes()).isEqualTo(10); + } + + @Test + public void getAltsMaxConcurrentHandshakes_envVariableNotSet() throws Exception { + System.clearProperty(AltsProtocolNegotiator.ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE); + assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes()) + .isEqualTo(AltsProtocolNegotiator.DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES); + } + + @Test + public void getAltsMaxConcurrentHandshakes_envVariableNotANumber() throws Exception { + System.setProperty( + AltsProtocolNegotiator.ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE, "not-a-number"); + assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes()) + .isEqualTo(AltsProtocolNegotiator.DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES); + } + + @Test + public void getAltsMaxConcurrentHandshakes_envVariableNegative() throws Exception { + System.setProperty(AltsProtocolNegotiator.ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE, "-10"); + assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes()) + .isEqualTo(AltsProtocolNegotiator.DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES); + } + private void doHandshake() throws Exception { // Capture the client frame and add to the server. assertEquals(1, channel.outboundMessages().size()); From aa1e56a0deda4e4b3452ba120d381080d0cdff23 Mon Sep 17 00:00:00 2001 From: Matthew Stevenson Date: Thu, 6 Apr 2023 08:46:14 -0700 Subject: [PATCH 2/5] alts: Expose separate API for parsing env variable, for testing purposes. --- .../grpc/alts/internal/AltsProtocolNegotiator.java | 10 +++++++--- .../alts/internal/AltsProtocolNegotiatorTest.java | 13 ++++--------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java index 5737c90e818..682c9c8123b 100644 --- a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java +++ b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java @@ -427,9 +427,8 @@ public SecurityDetails validatePeerObject(Object peerObject) throws GeneralSecur } @VisibleForTesting - static int getAltsMaxConcurrentHandshakes() { - String altsMaxConcurrentHandshakes = System.getenv(ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE); - if (altsMaxConcurrentHandshakes == null) { + static int getAltsMaxConcurrentHandshakes(String altsMaxConcurrentHandshakes) { + if (altsMaxConcurrentHandshakes == null) { return DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES; } try { @@ -447,5 +446,10 @@ static int getAltsMaxConcurrentHandshakes() { } } + private static int getAltsMaxConcurrentHandshakes() { + return getAltsMaxConcurrentHandshakes( + System.getenv(ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE)); + } + private AltsProtocolNegotiator() {} } diff --git a/alts/src/test/java/io/grpc/alts/internal/AltsProtocolNegotiatorTest.java b/alts/src/test/java/io/grpc/alts/internal/AltsProtocolNegotiatorTest.java index c0432207d2b..24392af75fd 100644 --- a/alts/src/test/java/io/grpc/alts/internal/AltsProtocolNegotiatorTest.java +++ b/alts/src/test/java/io/grpc/alts/internal/AltsProtocolNegotiatorTest.java @@ -356,29 +356,24 @@ public void peerPropagated() throws Exception { @Test public void getAltsMaxConcurrentHandshakes_success() throws Exception { - System.setProperty(AltsProtocolNegotiator.ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE, "10"); - assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes()).isEqualTo(10); + assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes("10")).isEqualTo(10); } @Test public void getAltsMaxConcurrentHandshakes_envVariableNotSet() throws Exception { - System.clearProperty(AltsProtocolNegotiator.ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE); - assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes()) + assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes(null)) .isEqualTo(AltsProtocolNegotiator.DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES); } @Test public void getAltsMaxConcurrentHandshakes_envVariableNotANumber() throws Exception { - System.setProperty( - AltsProtocolNegotiator.ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE, "not-a-number"); - assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes()) + assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes("not-a-number")) .isEqualTo(AltsProtocolNegotiator.DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES); } @Test public void getAltsMaxConcurrentHandshakes_envVariableNegative() throws Exception { - System.setProperty(AltsProtocolNegotiator.ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE, "-10"); - assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes()) + assertThat(AltsProtocolNegotiator.getAltsMaxConcurrentHandshakes("-10")) .isEqualTo(AltsProtocolNegotiator.DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES); } From fa6365df14282f09003b530af65c0ce468bb6d3c Mon Sep 17 00:00:00 2001 From: Matthew Stevenson Date: Thu, 6 Apr 2023 09:32:31 -0700 Subject: [PATCH 3/5] alts: Lint fix. --- .../main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java index 682c9c8123b..fc0a43dc608 100644 --- a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java +++ b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java @@ -428,7 +428,7 @@ public SecurityDetails validatePeerObject(Object peerObject) throws GeneralSecur @VisibleForTesting static int getAltsMaxConcurrentHandshakes(String altsMaxConcurrentHandshakes) { - if (altsMaxConcurrentHandshakes == null) { + if (altsMaxConcurrentHandshakes == null) { return DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES; } try { From 1d07b3fc1347251dd7ef2cc86d53813667657a54 Mon Sep 17 00:00:00 2001 From: Matthew Stevenson Date: Mon, 10 Apr 2023 09:14:16 -0700 Subject: [PATCH 4/5] alts: Remove VisibleForTesting annotation. --- .../main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java | 1 - 1 file changed, 1 deletion(-) diff --git a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java index fc0a43dc608..883518c3729 100644 --- a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java +++ b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java @@ -54,7 +54,6 @@ // TODO(carl-mastrangelo): rename this AltsProtocolNegotiators. public final class AltsProtocolNegotiator { private static final Logger logger = Logger.getLogger(AltsProtocolNegotiator.class.getName()); - @VisibleForTesting static final String ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE = "GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES"; @VisibleForTesting static final int DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES = 32; From b7b2441228880110b308497e899ccc3c4f140216 Mon Sep 17 00:00:00 2001 From: Matthew Stevenson Date: Mon, 10 Apr 2023 09:41:11 -0700 Subject: [PATCH 5/5] Add missing blank line. --- .../main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java | 1 + 1 file changed, 1 insertion(+) diff --git a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java index 883518c3729..e0343f83c51 100644 --- a/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java +++ b/alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java @@ -54,6 +54,7 @@ // TODO(carl-mastrangelo): rename this AltsProtocolNegotiators. public final class AltsProtocolNegotiator { private static final Logger logger = Logger.getLogger(AltsProtocolNegotiator.class.getName()); + static final String ALTS_MAX_CONCURRENT_HANDSHAKES_ENV_VARIABLE = "GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES"; @VisibleForTesting static final int DEFAULT_ALTS_MAX_CONCURRENT_HANDSHAKES = 32;