ALPN H2 Support for Netty Client #5794
Conversation
...main/java/software/amazon/awssdk/codegen/model/config/customization/CustomizationConfig.java
Outdated
Show resolved
Hide resolved
...ients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClientBase.java
Outdated
Show resolved
Hide resolved
...-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java
Outdated
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelPipelineInitializer.java
Outdated
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelPipelineInitializer.java
Outdated
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelPipelineInitializer.java
Outdated
Show resolved
Hide resolved
| ApplicationProtocolConfig.SelectorFailureBehavior.NO_ADVERTISE, | ||
| ApplicationProtocolConfig.SelectedListenerFailureBehavior.ACCEPT, |
There was a problem hiding this comment.
Question: how is SelectorFailureBehavior different from SelectedListenerFailureBehavior?
Should we use FATAL_ALERT?
There was a problem hiding this comment.
Seems like SelectorFailureBehavior comes first, is the peer in the connection. SelectedListenerFailureBehavior is for a configured listener. Updating to FATAL_ALERT for both
There was a problem hiding this comment.
Turns out OpenSsl Provider does not support FATAL_ALERT
https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java#L767
Reverting back to original values if OpenSsl provider used, added tests
|
|
||
| // ALPN supported backported in u251 | ||
| // https://bugs.openjdk.org/browse/JDK-8242894 | ||
| public static boolean isAlpnSupported() { |
There was a problem hiding this comment.
What if customers are using openssl, then Java version is no longer a limitation, right?
Instead of checking java version (that seems error-prone), should we check if ALPN related method is there?
There was a problem hiding this comment.
Updated to check for ALPNExtension, copied from Netty
https://netty.io/4.0/xref/io/netty/handler/ssl/JettyAlpnSslEngine.html#44
There was a problem hiding this comment.
Nvm, this doesn't work, always throws error. Its not clear what classes/methods we need to check, the old deprecated Netty class checked from a handful of sources, the new class doesn't have checks. Reverting to checking Java version for now, will update
There was a problem hiding this comment.
This is the check implemented in in Netty 5.x fwiw https://github.com/netty/netty/blob/netty-5.0.0.Alpha5/handler/src/main/java/io/netty5/handler/ssl/JdkAlpnSslUtils.java#L108
Any updates on non JDK Ssl provider? Seems we can skip the check if SslProvider is not JDK
What if customers are using openssl, then Java version is no longer a limitation, right?
There was a problem hiding this comment.
+1, I was going to suggest a similar check to This is the check implemented in in Netty 5.x fwiw https://github.com/netty/netty/blob/netty-5.0.0.Alpha5/handler/src/main/java/io/netty5/handler/ssl/JdkAlpnSslUtils.java#L108
There was a problem hiding this comment.
Updated check to return true if SslProvider is OpenSsl
...main/java/software/amazon/awssdk/codegen/model/config/customization/CustomizationConfig.java
Outdated
Show resolved
Hide resolved
http-client-spi/src/main/java/software/amazon/awssdk/http/ProtocolNegotiation.java
Outdated
Show resolved
Hide resolved
...-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java
Outdated
Show resolved
Hide resolved
...-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java
Outdated
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelPipelineInitializer.java
Outdated
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelPipelineInitializer.java
Show resolved
Hide resolved
...io-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyUtils.java
Outdated
Show resolved
Hide resolved
|
|
||
| // ALPN supported backported in u251 | ||
| // https://bugs.openjdk.org/browse/JDK-8242894 | ||
| public static boolean isAlpnSupported() { |
There was a problem hiding this comment.
This is the check implemented in in Netty 5.x fwiw https://github.com/netty/netty/blob/netty-5.0.0.Alpha5/handler/src/main/java/io/netty5/handler/ssl/JdkAlpnSslUtils.java#L108
Any updates on non JDK Ssl provider? Seems we can skip the check if SslProvider is not JDK
What if customers are using openssl, then Java version is no longer a limitation, right?
| if (configuration.reapIdleConnections()) { | ||
| pipeline.addLast(new IdleConnectionReaperHandler(configuration.idleTimeoutMillis())); | ||
| private void configureProtocolHandlers(Channel ch, ChannelPipeline pipeline, Protocol protocol) { | ||
| switch (protocolNegotiation) { |
There was a problem hiding this comment.
Can we add functional tests for those different combinations using a local mock server?
There was a problem hiding this comment.
Added NettyClientAlpnTest using MockH2Server from benchmarks
|
Motivation and Context
Adds ALPN H2 support to Netty client. ALPN is required when using an ALB in order to negotiate H2 connection.
This can be enabled when using Netty client with h2 protocol.
ALPN support was backported to Java 8 , only works with
1.8.0_251and newerModifications
Introduces new
ProtocolNegotiationenum class:ASSUME_PROTOCOL- Default existing setting, client uses prior knowledgeALPN- Uses ALPN, without fallback. Only works with h2, throws error if set withProtocol.HTTP1_1Testing
Added unit and integ tests
Screenshots (if appropriate)
Types of changes