Skip ALPN if request is HTTP endpoint #5854
Conversation
...-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java
Outdated
Show resolved
Hide resolved
| @@ -99,7 +99,9 @@ public void channelCreated(Channel ch) { | |||
| ch.attr(CHANNEL_DIAGNOSTICS).set(new ChannelDiagnostics(ch)); | |||
| ch.attr(PROTOCOL_FUTURE).set(new CompletableFuture<>()); | |||
| ChannelPipeline pipeline = ch.pipeline(); | |||
| if (sslCtx != null) { | |||
|
|
|||
| boolean sslCtxPresent = sslCtx != null; | |||
There was a problem hiding this comment.
whats the reason for this change ?
There was a problem hiding this comment.
We are saving the boolean as variable since we pass to configureProtocolHandlers() to fallback to prior knowledge if sslCtx is null
There was a problem hiding this comment.
Do we have test cases for this change ?
There was a problem hiding this comment.
This is existing behavior, to use prior knowledge. Covered by test here, to make sure we don't throw UnsupportedOperationException
| throw new UnsupportedOperationException("ALPN can only be used with HTTPS, not HTTP."); | ||
| } | ||
| if (protocolNegotiation == ProtocolNegotiation.ALPN) { | ||
| log.debug(null, () -> "Default client protocol negotiation is ALPN, but request endpoint is HTTP. Prior " |
There was a problem hiding this comment.
This log will be logged for every request. What is the significance of this log, as in if the default ALPN is not supported for the service, won't we immediately get server-side exceptions?
There was a problem hiding this comment.
discussed offline, will remove logging. We fallback to prior knowledge in ChannelPipelineInitializer. This scenario of ALPN default setting with HTTP endpoint should only be present in mock tests
| @@ -99,7 +99,9 @@ public void channelCreated(Channel ch) { | |||
| ch.attr(CHANNEL_DIAGNOSTICS).set(new ChannelDiagnostics(ch)); | |||
| ch.attr(PROTOCOL_FUTURE).set(new CompletableFuture<>()); | |||
| ChannelPipeline pipeline = ch.pipeline(); | |||
| if (sslCtx != null) { | |||
|
|
|||
| boolean sslCtxPresent = sslCtx != null; | |||
There was a problem hiding this comment.
Do we have test cases for this change ?
|
Can we please add change logs . |
|
Motivation and Context
Mock tests with h2 clients making requests to HTTP endpoints fail with recent ALPN addition
Modifications
Skip adding ALPN handler if request endpoint is HTTP and the default client setting is ALPN. This maintains previous behavior of prior knowledge protocol negotiation, and avoids breaking changes for existing generated clients with h2 protocol setting
Continue to throw error if request endpoint is HTTP and user configures ALPN on client
Testing
Added unit tests
Screenshots (if appropriate)
Types of changes