Skip to content

Commit 843a87b

Browse files
dagnirdagnir
committed
Review comments
1 parent c49f518 commit 843a87b

File tree

6 files changed

+170
-53
lines changed

6 files changed

+170
-53
lines changed

‎http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -347,11 +347,11 @@ public interface Builder extends SdkAsyncHttpClient.Builder<NettyNioAsyncHttpCli
347347

348348
/**
349349
* Set the proxy configuration for this client. The configured proxy will be used to proxy any HTTP request
350-
* destined for any host that does not match any of the hosts in {@link ProxyConfiguration#nonProxyHosts() the
351-
* configured non proxy hosts}.
350+
* destined for any host that does not match any of the hosts in configured non proxy hosts.
352351
*
353352
* @param proxyConfiguration The proxy configuration.
354353
* @return The builder for method chaining.
354+
* @see ProxyConfiguration#nonProxyHosts()
355355
*/
356356
Builder proxyConfiguration(ProxyConfiguration proxyConfiguration);
357357
}

‎http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/ProxyConfiguration.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,9 @@ public interface Builder extends CopyableBuilder<Builder, ProxyConfiguration> {
137137

138138
/**
139139
* The HTTP scheme to use for connecting to the proxy. Valid values are {@code http} and {@code https}.
140+
* <p>
141+
* The client defaults to {@code http} if none is given.
142+
*
140143
* @param scheme The proxy scheme.
141144
* @return This object for method chaining.
142145
*/

‎http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/AwaitCloseChannelPoolMap.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,16 +33,17 @@
3333
import java.net.URI;
3434
import java.net.URISyntaxException;
3535
import java.util.Collection;
36-
import java.util.HashMap;
3736
import java.util.Map;
3837
import java.util.concurrent.CompletableFuture;
38+
import java.util.concurrent.ConcurrentHashMap;
3939
import java.util.concurrent.ExecutionException;
4040
import java.util.concurrent.TimeUnit;
4141
import java.util.concurrent.TimeoutException;
4242
import java.util.concurrent.atomic.AtomicReference;
4343
import javax.net.ssl.SSLException;
4444
import javax.net.ssl.TrustManagerFactory;
4545
import software.amazon.awssdk.annotations.SdkInternalApi;
46+
import software.amazon.awssdk.annotations.SdkTestInternalApi;
4647
import software.amazon.awssdk.http.Protocol;
4748
import software.amazon.awssdk.http.nio.netty.ProxyConfiguration;
4849
import software.amazon.awssdk.http.nio.netty.SdkEventLoopGroup;
@@ -71,7 +72,7 @@ public void channelCreated(Channel ch) throws Exception {
7172
}
7273
};
7374

74-
private final Map<URI, Boolean> shouldProxyForHostCache = new HashMap<>();
75+
private final Map<URI, Boolean> shouldProxyForHostCache = new ConcurrentHashMap<>();
7576

7677

7778
private final SdkChannelOptions sdkChannelOptions;
@@ -92,6 +93,12 @@ private AwaitCloseChannelPoolMap(Builder builder) {
9293
this.proxyConfiguration = builder.proxyConfiguration;
9394
}
9495

96+
@SdkTestInternalApi
97+
AwaitCloseChannelPoolMap(Builder builder, Map<URI, Boolean> shouldProxyForHostCache) {
98+
this(builder);
99+
this.shouldProxyForHostCache.putAll(shouldProxyForHostCache);
100+
}
101+
95102
public static Builder builder() {
96103
return new Builder();
97104
}

‎http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClientWireMockTest.java

Lines changed: 1 addition & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@
4242
import static org.mockito.Mockito.when;
4343

4444
import com.github.tomakehurst.wiremock.http.Fault;
45-
import com.github.tomakehurst.wiremock.http.trafficlistener.WiremockNetworkTrafficListener;
4645
import com.github.tomakehurst.wiremock.junit.WireMockRule;
4746
import io.netty.channel.Channel;
4847
import io.netty.channel.ChannelFactory;
@@ -54,10 +53,8 @@
5453
import io.netty.handler.ssl.SslProvider;
5554
import io.netty.util.AttributeKey;
5655
import java.io.IOException;
57-
import java.net.Socket;
5856
import java.net.URI;
5957
import java.nio.ByteBuffer;
60-
import java.nio.charset.StandardCharsets;
6158
import java.time.Duration;
6259
import java.util.ArrayList;
6360
import java.util.Collection;
@@ -441,7 +438,7 @@ public void requestContentOnlyEqualToContentLengthHeaderFromProvider() throws In
441438
// HTTP servers will stop processing the request as soon as it reads
442439
// bytes equal to 'Content-Length' so we need to inspect the raw
443440
// traffic to ensure that there wasn't anything after that.
444-
assertThat(wiremockTrafficListener.requests.toString()).endsWith(content);
441+
assertThat(wiremockTrafficListener.requests().toString()).endsWith(content);
445442
}
446443

447444
@Test
@@ -654,33 +651,4 @@ private static AttributeMap mapWithTrustAllCerts() {
654651
.put(SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES, true)
655652
.build();
656653
}
657-
658-
private static class RecordingNetworkTrafficListener implements WiremockNetworkTrafficListener {
659-
private final StringBuilder requests = new StringBuilder();
660-
661-
662-
@Override
663-
public void opened(Socket socket) {
664-
665-
}
666-
667-
@Override
668-
public void incoming(Socket socket, ByteBuffer byteBuffer) {
669-
requests.append(StandardCharsets.UTF_8.decode(byteBuffer));
670-
}
671-
672-
@Override
673-
public void outgoing(Socket socket, ByteBuffer byteBuffer) {
674-
675-
}
676-
677-
@Override
678-
public void closed(Socket socket) {
679-
680-
}
681-
682-
public void reset() {
683-
requests.setLength(0);
684-
}
685-
}
686654
}

‎http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/RecordingNetworkTrafficListener.java

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/*
2+
* Copyright 2010-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.awssdk.http.nio.netty;
17+
18+
import com.github.tomakehurst.wiremock.http.trafficlistener.WiremockNetworkTrafficListener;
19+
import java.net.Socket;
20+
import java.nio.ByteBuffer;
21+
import java.nio.charset.StandardCharsets;
22+
23+
/**
24+
* Simple implementation of {@link WiremockNetworkTrafficListener} to record all requests received as a string for later
25+
* verification.
26+
*/
27+
public class RecordingNetworkTrafficListener implements WiremockNetworkTrafficListener {
28+
private final StringBuilder requests = new StringBuilder();
29+
30+
31+
@Override
32+
public void opened(Socket socket) {
33+
34+
}
35+
36+
@Override
37+
public void incoming(Socket socket, ByteBuffer byteBuffer) {
38+
requests.append(StandardCharsets.UTF_8.decode(byteBuffer));
39+
}
40+
41+
@Override
42+
public void outgoing(Socket socket, ByteBuffer byteBuffer) {
43+
44+
}
45+
46+
@Override
47+
public void closed(Socket socket) {
48+
49+
}
50+
51+
public void reset() {
52+
requests.setLength(0);
53+
}
54+
55+
public StringBuilder requests() {
56+
return requests;
57+
}
58+
}

‎http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/AwaitCloseChannelPoolMapTest.java

Lines changed: 97 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -16,55 +16,136 @@
1616
package software.amazon.awssdk.http.nio.netty.internal;
1717

1818

19+
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
1920
import static org.assertj.core.api.Assertions.assertThat;
2021
import static software.amazon.awssdk.http.SdkHttpConfigurationOption.GLOBAL_HTTP_DEFAULTS;
2122

23+
import com.github.tomakehurst.wiremock.junit.WireMockRule;
24+
import io.netty.channel.Channel;
2225
import io.netty.handler.ssl.SslProvider;
26+
import io.netty.util.concurrent.Future;
2327
import java.net.URI;
2428
import java.util.ArrayList;
29+
import java.util.HashMap;
2530
import java.util.List;
26-
import java.util.concurrent.ExecutionException;
31+
import java.util.Map;
32+
import java.util.stream.Collectors;
33+
import java.util.stream.Stream;
2734
import org.apache.commons.lang3.RandomStringUtils;
28-
import org.junit.BeforeClass;
35+
import org.junit.After;
36+
import org.junit.Rule;
2937
import org.junit.Test;
3038
import software.amazon.awssdk.http.Protocol;
39+
import software.amazon.awssdk.http.nio.netty.ProxyConfiguration;
40+
import software.amazon.awssdk.http.nio.netty.RecordingNetworkTrafficListener;
3141
import software.amazon.awssdk.http.nio.netty.SdkEventLoopGroup;
3242

3343
public class AwaitCloseChannelPoolMapTest {
3444

35-
private static AwaitCloseChannelPoolMap channelPoolMap;
45+
private final RecordingNetworkTrafficListener recorder = new RecordingNetworkTrafficListener();
3646

47+
private AwaitCloseChannelPoolMap channelPoolMap;
3748

38-
@BeforeClass
39-
public static void setup() {
40-
channelPoolMap = AwaitCloseChannelPoolMap.builder()
41-
.sdkChannelOptions(new SdkChannelOptions())
42-
.sdkEventLoopGroup(SdkEventLoopGroup.builder().build())
43-
.configuration(new NettyConfiguration(GLOBAL_HTTP_DEFAULTS))
44-
.protocol(Protocol.HTTP1_1)
45-
.maxStreams(100)
46-
.sslProvider(SslProvider.OPENSSL)
47-
.build();
49+
@Rule
50+
public WireMockRule mockProxy = new WireMockRule(wireMockConfig()
51+
.dynamicPort()
52+
.networkTrafficListener(recorder));
53+
54+
@After
55+
public void methodTeardown() {
56+
if (channelPoolMap != null) {
57+
channelPoolMap.close();
58+
}
59+
channelPoolMap = null;
60+
61+
recorder.reset();
4862
}
4963

5064
@Test
51-
public void close_underlyingPoolsShouldBeClosed() throws ExecutionException, InterruptedException {
65+
public void close_underlyingPoolsShouldBeClosed() {
66+
channelPoolMap = AwaitCloseChannelPoolMap.builder()
67+
.sdkChannelOptions(new SdkChannelOptions())
68+
.sdkEventLoopGroup(SdkEventLoopGroup.builder().build())
69+
.configuration(new NettyConfiguration(GLOBAL_HTTP_DEFAULTS))
70+
.protocol(Protocol.HTTP1_1)
71+
.maxStreams(100)
72+
.sslProvider(SslProvider.OPENSSL)
73+
.build();
5274

5375
int numberOfChannelPools = 5;
5476
List<SimpleChannelPoolAwareChannelPool> channelPools = new ArrayList<>();
5577

5678
for (int i = 0; i < numberOfChannelPools; i++) {
5779
channelPools.add(
58-
channelPoolMap.get(URI.create("http://" + RandomStringUtils.randomAlphabetic(2) + i + "localhost:" + numberOfChannelPools)));
80+
channelPoolMap.get(URI.create("http://" + RandomStringUtils.randomAlphabetic(2) + i + "localhost:" + numberOfChannelPools)));
5981
}
6082

6183
assertThat(channelPoolMap.pools().size()).isEqualTo(numberOfChannelPools);
62-
6384
channelPoolMap.close();
6485
channelPools.forEach(channelPool -> {
6586
assertThat(channelPool.underlyingSimpleChannelPool().closeFuture()).isDone();
6687
assertThat(channelPool.underlyingSimpleChannelPool().closeFuture().join()).isTrue();
6788
});
6889
}
6990

91+
@Test
92+
public void usingProxy_usesCachedValueWhenPresent() {
93+
URI targetUri = URI.create("https://some-awesome-service-1234.amazonaws.com");
94+
95+
Map<URI, Boolean> shouldProxyCache = new HashMap<>();
96+
shouldProxyCache.put(targetUri, true);
97+
98+
ProxyConfiguration proxyConfiguration = ProxyConfiguration.builder()
99+
.host("localhost")
100+
.port(mockProxy.port())
101+
// Deliberately set the target host as a non-proxy host to see if it will check the cache first
102+
.nonProxyHosts(Stream.of(targetUri.getHost()).collect(Collectors.toSet()))
103+
.build();
104+
105+
AwaitCloseChannelPoolMap.Builder builder = AwaitCloseChannelPoolMap.builder()
106+
.proxyConfiguration(proxyConfiguration)
107+
.sdkChannelOptions(new SdkChannelOptions())
108+
.sdkEventLoopGroup(SdkEventLoopGroup.builder().build())
109+
.configuration(new NettyConfiguration(GLOBAL_HTTP_DEFAULTS))
110+
.protocol(Protocol.HTTP1_1)
111+
.maxStreams(100)
112+
.sslProvider(SslProvider.OPENSSL);
113+
114+
channelPoolMap = new AwaitCloseChannelPoolMap(builder, shouldProxyCache);
115+
116+
// The target host does not exist so acquiring a channel should fail unless we're configured to connect to
117+
// the mock proxy host for this URI.
118+
SimpleChannelPoolAwareChannelPool channelPool = channelPoolMap.newPool(targetUri);
119+
Future<Channel> channelFuture = channelPool.underlyingSimpleChannelPool().acquire().awaitUninterruptibly();
120+
assertThat(channelFuture.isSuccess()).isTrue();
121+
channelPool.release(channelFuture.getNow()).awaitUninterruptibly();
122+
}
123+
124+
@Test
125+
public void usingProxy_noSchemeGiven_defaultsToHttp() {
126+
ProxyConfiguration proxyConfiguration = ProxyConfiguration.builder()
127+
.host("localhost")
128+
.port(mockProxy.port())
129+
.build();
130+
131+
channelPoolMap = AwaitCloseChannelPoolMap.builder()
132+
.proxyConfiguration(proxyConfiguration)
133+
.sdkChannelOptions(new SdkChannelOptions())
134+
.sdkEventLoopGroup(SdkEventLoopGroup.builder().build())
135+
.configuration(new NettyConfiguration(GLOBAL_HTTP_DEFAULTS))
136+
.protocol(Protocol.HTTP1_1)
137+
.maxStreams(100)
138+
.sslProvider(SslProvider.OPENSSL)
139+
.build();
140+
141+
SimpleChannelPoolAwareChannelPool simpleChannelPoolAwareChannelPool = channelPoolMap.newPool(
142+
URI.create("https://some-awesome-service:443"));
143+
144+
simpleChannelPoolAwareChannelPool.acquire().awaitUninterruptibly();
145+
146+
String requests = recorder.requests().toString();
147+
148+
assertThat(requests).contains("CONNECT some-awesome-service:443");
149+
}
150+
70151
}

0 commit comments

Comments
 (0)