Skip to content

Commit 4aa1fdb

Browse files
authored
Handled Surface API review comments (#6224)
* Handled Surface API review comments * Added a single test for localaddress , handled review comments * Removing internal package name as -preview after internal discussion * Fix transient text case failures
1 parent a21356a commit 4aa1fdb

File tree

6 files changed

+128
-18
lines changed

6 files changed

+128
-18
lines changed

.brazil.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
"modules": {
55
"annotations": { "packageName": "AwsJavaSdk-Core-Annotations" },
66
"apache-client": { "packageName": "AwsJavaSdk-HttpClient-ApacheClient" },
7-
"apache5-client": { "packageName": "AwsJavaSdk-HttpClient-Apache5Client-preview" },
7+
"apache5-client": { "packageName": "AwsJavaSdk-HttpClient-Apache5Client" },
88
"arns": { "packageName": "AwsJavaSdk-Core-Arns" },
99
"auth": { "packageName": "AwsJavaSdk-Core-Auth" },
1010
"auth-crt": { "packageName": "AwsJavaSdk-Core-AuthCrt" },

http-clients/apache-client/src/test/java/software/amazon/awssdk/http/apache/ApacheClientTlsAuthTest.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ public static void setUp() throws IOException {
8383

8484
wireMockServer = new WireMockServer(wireMockConfig()
8585
.dynamicHttpsPort()
86+
.dynamicPort()
8687
.needClientAuth(true)
8788
.keystorePath(serverKeyStore.toAbsolutePath().toString())
8889
.keystorePassword(STORE_PASSWORD)

http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
import java.io.ByteArrayInputStream;
2727
import java.io.IOException;
28+
import java.net.InetAddress;
2829
import java.security.KeyManagementException;
2930
import java.security.NoSuchAlgorithmException;
3031
import java.security.cert.CertificateException;
@@ -48,6 +49,7 @@
4849
import org.apache.hc.client5.http.impl.classic.HttpClients;
4950
import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
5051
import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManagerBuilder;
52+
import org.apache.hc.client5.http.impl.routing.DefaultRoutePlanner;
5153
import org.apache.hc.client5.http.io.HttpClientConnectionManager;
5254
import org.apache.hc.client5.http.protocol.HttpClientContext;
5355
import org.apache.hc.client5.http.routing.HttpRoutePlanner;
@@ -57,10 +59,13 @@
5759
import org.apache.hc.core5.http.ClassicHttpResponse;
5860
import org.apache.hc.core5.http.Header;
5961
import org.apache.hc.core5.http.HttpEntity;
62+
import org.apache.hc.core5.http.HttpException;
63+
import org.apache.hc.core5.http.HttpHost;
6064
import org.apache.hc.core5.http.HttpResponse;
6165
import org.apache.hc.core5.http.config.Registry;
6266
import org.apache.hc.core5.http.impl.io.HttpRequestExecutor;
6367
import org.apache.hc.core5.http.io.SocketConfig;
68+
import org.apache.hc.core5.http.protocol.HttpContext;
6469
import org.apache.hc.core5.io.CloseMode;
6570
import org.apache.hc.core5.pool.PoolStats;
6671
import org.apache.hc.core5.ssl.SSLInitializationException;
@@ -108,7 +113,7 @@
108113
@SdkPublicApi
109114
public final class Apache5HttpClient implements SdkHttpClient {
110115

111-
public static final String CLIENT_NAME = "Apache5";
116+
public static final String CLIENT_NAME = "Apache5Preview";
112117

113118
private static final Logger log = Logger.loggerFor(Apache5HttpClient.class);
114119
private static final HostnameVerifier DEFAULT_HOSTNAME_VERIFIER = new DefaultHostnameVerifier();
@@ -206,7 +211,12 @@ private void addProxyConfig(HttpClientBuilder builder,
206211
}
207212

208213
if (routePlanner != null) {
214+
if (configuration.localAddress != null) {
215+
log.debug(() -> "localAddress configuration was ignored since Route planner was explicitly provided");
216+
}
209217
builder.setRoutePlanner(routePlanner);
218+
} else if (configuration.localAddress != null) {
219+
builder.setRoutePlanner(new LocalAddressRoutePlanner(configuration.localAddress));
210220
}
211221

212222
if (credentialsProvider != null) {
@@ -404,6 +414,11 @@ public interface Builder extends SdkHttpClient.Builder<Apache5HttpClient.Builder
404414
*/
405415
Builder proxyConfiguration(ProxyConfiguration proxyConfiguration);
406416

417+
/**
418+
* Configure the local address that the HTTP client should use for communication.
419+
*/
420+
Builder localAddress(InetAddress localAddress);
421+
407422
/**
408423
* Configure whether the client should send an HTTP expect-continue handshake before each request.
409424
*/
@@ -495,6 +510,7 @@ private static final class DefaultBuilder implements Builder {
495510
private final AttributeMap.Builder standardOptions = AttributeMap.builder();
496511
private Registry<AuthSchemeFactory> authSchemeRegistry;
497512
private ProxyConfiguration proxyConfiguration = ProxyConfiguration.builder().build();
513+
private InetAddress localAddress;
498514
private Boolean expectContinueEnabled;
499515
private HttpRoutePlanner httpRoutePlanner;
500516
private CredentialsProvider credentialsProvider;
@@ -560,6 +576,16 @@ public void setProxyConfiguration(ProxyConfiguration proxyConfiguration) {
560576
proxyConfiguration(proxyConfiguration);
561577
}
562578

579+
@Override
580+
public Builder localAddress(InetAddress localAddress) {
581+
this.localAddress = localAddress;
582+
return this;
583+
}
584+
585+
public void setLocalAddress(InetAddress localAddress) {
586+
localAddress(localAddress);
587+
}
588+
563589
@Override
564590
public Builder expectContinueEnabled(Boolean expectContinueEnabled) {
565591
this.expectContinueEnabled = expectContinueEnabled;
@@ -794,4 +820,18 @@ private SocketConfig buildSocketConfig(AttributeMap standardOptions) {
794820
}
795821

796822
}
823+
824+
private static class LocalAddressRoutePlanner extends DefaultRoutePlanner {
825+
private final InetAddress localAddress;
826+
827+
LocalAddressRoutePlanner(InetAddress localAddress) {
828+
super(DefaultSchemePortResolver.INSTANCE);
829+
this.localAddress = localAddress;
830+
}
831+
832+
@Override
833+
protected InetAddress determineLocalAddress(HttpHost firstHop, HttpContext context) throws HttpException {
834+
return localAddress;
835+
}
836+
}
797837
}

http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5ClientTlsAuthTest.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ public static void setUp() throws IOException {
8282

8383
wireMockServer = new WireMockServer(wireMockConfig()
8484
.dynamicHttpsPort()
85+
.dynamicPort()
8586
.needClientAuth(true)
8687
.keystorePath(serverKeyStore.toAbsolutePath().toString())
8788
.keystorePassword(STORE_PASSWORD)

http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientLocalAddressFunctionalTest.java

Lines changed: 82 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515

1616
package software.amazon.awssdk.http.apache5;
1717

18+
import static org.assertj.core.api.Assertions.assertThat;
19+
1820
import java.net.InetAddress;
1921
import java.time.Duration;
2022
import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
@@ -23,29 +25,95 @@
2325
import org.apache.hc.core5.http.HttpException;
2426
import org.apache.hc.core5.http.HttpHost;
2527
import org.apache.hc.core5.http.protocol.HttpContext;
28+
import org.apache.logging.log4j.Level;
2629
import org.junit.jupiter.api.DisplayName;
30+
import org.junit.jupiter.api.Nested;
2731
import software.amazon.awssdk.http.SdkHttpClient;
2832
import software.amazon.awssdk.http.SdkHttpClientLocalAddressFunctionalTestSuite;
33+
import software.amazon.awssdk.testutils.LogCaptor;
2934

35+
/**
36+
* Functional tests for Apache5 HTTP Client's local address binding capabilities.
37+
* Tests three scenarios:
38+
* 1. Local address configured via builder
39+
* 2. Local address configured via custom route planner
40+
* 3. Both methods used together (route planner takes precedence)
41+
*/
3042
@DisplayName("Apache5 HTTP Client - Local Address Functional Tests")
31-
class Apache5HttpClientLocalAddressFunctionalTest extends SdkHttpClientLocalAddressFunctionalTestSuite {
43+
class Apache5HttpClientLocalAddressFunctionalTest {
44+
45+
@Nested
46+
@DisplayName("When local address is configured via builder")
47+
class LocalAddressViaBuilderTest extends SdkHttpClientLocalAddressFunctionalTestSuite {
48+
@Override
49+
protected SdkHttpClient createHttpClient(InetAddress localAddress, Duration connectionTimeout) {
50+
return Apache5HttpClient.builder()
51+
.localAddress(localAddress)
52+
.connectionTimeout(connectionTimeout)
53+
.build();
54+
}
55+
}
56+
57+
@Nested
58+
@DisplayName("When local address is configured via custom route planner")
59+
class LocalAddressViaRoutePlannerTest extends SdkHttpClientLocalAddressFunctionalTestSuite {
3260

33-
@Override
34-
protected SdkHttpClient createHttpClient(InetAddress localAddress, Duration connectionTimeout) {
35-
HttpRoutePlanner routePlanner = createLocalAddressRoutePlanner(localAddress);
61+
@Override
62+
protected SdkHttpClient createHttpClient(InetAddress localAddress, Duration connectionTimeout) {
63+
HttpRoutePlanner routePlanner = createLocalAddressRoutePlanner(localAddress);
64+
return Apache5HttpClient.builder()
65+
.httpRoutePlanner(routePlanner)
66+
.connectionTimeout(connectionTimeout)
67+
.build();
68+
}
3669

37-
return Apache5HttpClient.builder()
38-
.httpRoutePlanner(routePlanner)
39-
.connectionTimeout(connectionTimeout)
40-
.build();
70+
private HttpRoutePlanner createLocalAddressRoutePlanner(InetAddress localAddress) {
71+
return new DefaultRoutePlanner(DefaultSchemePortResolver.INSTANCE) {
72+
@Override
73+
protected InetAddress determineLocalAddress(HttpHost firstHop, HttpContext context) throws HttpException {
74+
return localAddress != null ? localAddress : super.determineLocalAddress(firstHop, context);
75+
}
76+
};
77+
}
4178
}
4279

43-
private HttpRoutePlanner createLocalAddressRoutePlanner(InetAddress localAddress) {
44-
return new DefaultRoutePlanner(DefaultSchemePortResolver.INSTANCE) {
45-
@Override
46-
protected InetAddress determineLocalAddress(HttpHost firstHop, HttpContext context) throws HttpException {
47-
return localAddress != null ? localAddress : super.determineLocalAddress(firstHop, context);
80+
@Nested
81+
@DisplayName("When both route planner and builder local address are configured (route planner takes precedence)")
82+
class RoutePlannerPrecedenceTest extends SdkHttpClientLocalAddressFunctionalTestSuite {
83+
84+
private final InetAddress BUILDER_LOCAL_ADDRESS = InetAddress.getLoopbackAddress();
85+
86+
@Override
87+
protected SdkHttpClient createHttpClient(InetAddress localAddress, Duration connectionTimeout) {
88+
// The localAddress parameter will be used by the route planner
89+
// The builder's localAddress will be overridden
90+
HttpRoutePlanner routePlanner = createLocalAddressRoutePlanner(localAddress);
91+
SdkHttpClient httpClient;
92+
93+
try (LogCaptor logCaptor = LogCaptor.create(Level.DEBUG)) {
94+
httpClient = Apache5HttpClient.builder()
95+
.httpRoutePlanner(routePlanner)
96+
.localAddress(BUILDER_LOCAL_ADDRESS) // This will be overridden by route planner
97+
.connectionTimeout(connectionTimeout)
98+
.build();
99+
100+
assertThat(logCaptor.loggedEvents()).anySatisfy(logEvent -> {
101+
assertThat(logEvent.getLevel()).isEqualTo(Level.DEBUG);
102+
assertThat(logEvent.getMessage().getFormattedMessage())
103+
.contains("localAddress configuration was ignored since Route planner was explicitly provided");
104+
});
48105
}
49-
};
106+
return httpClient;
107+
}
108+
109+
private HttpRoutePlanner createLocalAddressRoutePlanner(InetAddress routePlannerAddress) {
110+
return new DefaultRoutePlanner(DefaultSchemePortResolver.INSTANCE) {
111+
@Override
112+
protected InetAddress determineLocalAddress(HttpHost firstHop, HttpContext context) throws HttpException {
113+
// Route planner's address takes precedence over builder's address
114+
return routePlannerAddress != null ? routePlannerAddress : super.determineLocalAddress(firstHop, context);
115+
}
116+
};
117+
}
50118
}
51119
}

http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ public void prepareRequest_callableCalled_metricsReported() throws IOException {
8181

8282
client.prepareRequest(executeRequest).call();
8383
MetricCollection collected = collector.collect();
84-
assertThat(collected.metricValues(HTTP_CLIENT_NAME)).containsExactly("Apache5");
84+
assertThat(collected.metricValues(HTTP_CLIENT_NAME)).containsExactly("Apache5Preview");
8585
assertThat(collected.metricValues(LEASED_CONCURRENCY)).containsExactly(1);
8686
assertThat(collected.metricValues(PENDING_CONCURRENCY_ACQUIRES)).containsExactly(2);
8787
assertThat(collected.metricValues(AVAILABLE_CONCURRENCY)).containsExactly(3);
@@ -99,7 +99,7 @@ public void prepareRequest_connectionManagerNotPooling_callableCalled_metricsRep
9999

100100
MetricCollection collected = collector.collect();
101101

102-
assertThat(collected.metricValues(HTTP_CLIENT_NAME)).containsExactly("Apache5");
102+
assertThat(collected.metricValues(HTTP_CLIENT_NAME)).containsExactly("Apache5Preview");
103103
assertThat(collected.metricValues(LEASED_CONCURRENCY)).isEmpty();
104104
assertThat(collected.metricValues(PENDING_CONCURRENCY_ACQUIRES)).isEmpty();
105105
assertThat(collected.metricValues(AVAILABLE_CONCURRENCY)).isEmpty();

0 commit comments

Comments
 (0)