Handled Surface API review comments

.brazil.json

@@ -4,7 +4,7 @@
     "modules": {
         "annotations": { "packageName": "AwsJavaSdk-Core-Annotations" },
         "apache-client": { "packageName": "AwsJavaSdk-HttpClient-ApacheClient" },
-        "apache5-client": { "packageName": "AwsJavaSdk-HttpClient-Apache5Client-preview" },
+        "apache5-client": { "packageName": "AwsJavaSdk-HttpClient-Apache5Client" },
         "arns": { "packageName": "AwsJavaSdk-Core-Arns" },
         "auth": { "packageName": "AwsJavaSdk-Core-Auth" },
         "auth-crt": { "packageName": "AwsJavaSdk-Core-AuthCrt" },

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

@@ -83,6 +83,7 @@ public static void setUp() throws IOException {
 
         wireMockServer = new WireMockServer(wireMockConfig()
                 .dynamicHttpsPort()
+                .dynamicPort()
                 .needClientAuth(true)
                 .keystorePath(serverKeyStore.toAbsolutePath().toString())
                 .keystorePassword(STORE_PASSWORD)

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

@@ -25,6 +25,7 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.net.InetAddress;
 import java.security.KeyManagementException;
 import java.security.NoSuchAlgorithmException;
 import java.security.cert.CertificateException;
@@ -48,6 +49,7 @@
 import org.apache.hc.client5.http.impl.classic.HttpClients;
 import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager;
 import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManagerBuilder;
+import org.apache.hc.client5.http.impl.routing.DefaultRoutePlanner;
 import org.apache.hc.client5.http.io.HttpClientConnectionManager;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
 import org.apache.hc.client5.http.routing.HttpRoutePlanner;
@@ -57,10 +59,13 @@
 import org.apache.hc.core5.http.ClassicHttpResponse;
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpEntity;
+import org.apache.hc.core5.http.HttpException;
+import org.apache.hc.core5.http.HttpHost;
 import org.apache.hc.core5.http.HttpResponse;
 import org.apache.hc.core5.http.config.Registry;
 import org.apache.hc.core5.http.impl.io.HttpRequestExecutor;
 import org.apache.hc.core5.http.io.SocketConfig;
+import org.apache.hc.core5.http.protocol.HttpContext;
 import org.apache.hc.core5.io.CloseMode;
 import org.apache.hc.core5.pool.PoolStats;
 import org.apache.hc.core5.ssl.SSLInitializationException;
@@ -108,7 +113,7 @@
 @SdkPublicApi
 public final class Apache5HttpClient implements SdkHttpClient {
 
-    public static final String CLIENT_NAME = "Apache5";
+    public static final String CLIENT_NAME = "Apache5Preview";
 
     private static final Logger log = Logger.loggerFor(Apache5HttpClient.class);
     private static final HostnameVerifier DEFAULT_HOSTNAME_VERIFIER = new DefaultHostnameVerifier();
@@ -206,7 +211,12 @@ private void addProxyConfig(HttpClientBuilder builder,
         }
 
         if (routePlanner != null) {
+            if (configuration.localAddress != null) {
+                log.debug(() -> "localAddress configuration was ignored since Route planner was explicitly provided");
+            }
             builder.setRoutePlanner(routePlanner);
+        } else if (configuration.localAddress != null) {
+            builder.setRoutePlanner(new LocalAddressRoutePlanner(configuration.localAddress));
         }
 
         if (credentialsProvider != null) {
@@ -404,6 +414,11 @@ public interface Builder extends SdkHttpClient.Builder<Apache5HttpClient.Builder
          */
         Builder proxyConfiguration(ProxyConfiguration proxyConfiguration);
 
+        /**
+         * Configure the local address that the HTTP client should use for communication.
+         */
+        Builder localAddress(InetAddress localAddress);
+
         /**
          * Configure whether the client should send an HTTP expect-continue handshake before each request.
          */
@@ -495,6 +510,7 @@ private static final class DefaultBuilder implements Builder {
         private final AttributeMap.Builder standardOptions = AttributeMap.builder();
         private Registry<AuthSchemeFactory> authSchemeRegistry;
         private ProxyConfiguration proxyConfiguration = ProxyConfiguration.builder().build();
+        private InetAddress localAddress;
         private Boolean expectContinueEnabled;
         private HttpRoutePlanner httpRoutePlanner;
         private CredentialsProvider credentialsProvider;
@@ -560,6 +576,16 @@ public void setProxyConfiguration(ProxyConfiguration proxyConfiguration) {
             proxyConfiguration(proxyConfiguration);
         }
 
+        @Override
+        public Builder localAddress(InetAddress localAddress) {
+            this.localAddress = localAddress;
+            return this;
+        }
+
+        public void setLocalAddress(InetAddress localAddress) {
+            localAddress(localAddress);
+        }
+
         @Override
         public Builder expectContinueEnabled(Boolean expectContinueEnabled) {
             this.expectContinueEnabled = expectContinueEnabled;
@@ -794,4 +820,18 @@ private SocketConfig buildSocketConfig(AttributeMap standardOptions) {
         }
 
     }
+
+    private static class LocalAddressRoutePlanner extends DefaultRoutePlanner {
+        private final InetAddress localAddress;
+
+        LocalAddressRoutePlanner(InetAddress localAddress) {
+            super(DefaultSchemePortResolver.INSTANCE);
+            this.localAddress = localAddress;
+        }
+
+        @Override
+        protected InetAddress determineLocalAddress(HttpHost firstHop, HttpContext context) throws HttpException {
+            return localAddress;
+        }
+    }
 }

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

@@ -82,6 +82,7 @@ public static void setUp() throws IOException {
 
         wireMockServer = new WireMockServer(wireMockConfig()
                 .dynamicHttpsPort()
+                .dynamicPort()
                 .needClientAuth(true)
                 .keystorePath(serverKeyStore.toAbsolutePath().toString())
                 .keystorePassword(STORE_PASSWORD)

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

@@ -15,6 +15,8 @@
 
 package software.amazon.awssdk.http.apache5;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 import java.net.InetAddress;
 import java.time.Duration;
 import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
@@ -23,29 +25,95 @@
 import org.apache.hc.core5.http.HttpException;
 import org.apache.hc.core5.http.HttpHost;
 import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.logging.log4j.Level;
 import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
 import software.amazon.awssdk.http.SdkHttpClient;
 import software.amazon.awssdk.http.SdkHttpClientLocalAddressFunctionalTestSuite;
+import software.amazon.awssdk.testutils.LogCaptor;
 
+/**
+ * Functional tests for Apache5 HTTP Client's local address binding capabilities.
+ * Tests three scenarios:
+ * 1. Local address configured via builder
+ * 2. Local address configured via custom route planner
+ * 3. Both methods used together (route planner takes precedence)
+ */
 @DisplayName("Apache5 HTTP Client - Local Address Functional Tests")
-class Apache5HttpClientLocalAddressFunctionalTest extends SdkHttpClientLocalAddressFunctionalTestSuite {
+class Apache5HttpClientLocalAddressFunctionalTest {
+
+    @Nested
+    @DisplayName("When local address is configured via builder")
+    class LocalAddressViaBuilderTest extends SdkHttpClientLocalAddressFunctionalTestSuite {
+        @Override
+        protected SdkHttpClient createHttpClient(InetAddress localAddress, Duration connectionTimeout) {
+            return Apache5HttpClient.builder()
+                                    .localAddress(localAddress)
+                                    .connectionTimeout(connectionTimeout)
+                                    .build();
+        }
+    }
+
+    @Nested
+    @DisplayName("When local address is configured via custom route planner")
+    class LocalAddressViaRoutePlannerTest extends SdkHttpClientLocalAddressFunctionalTestSuite {
 
-    @Override
-    protected SdkHttpClient createHttpClient(InetAddress localAddress, Duration connectionTimeout) {
-        HttpRoutePlanner routePlanner = createLocalAddressRoutePlanner(localAddress);
+        @Override
+        protected SdkHttpClient createHttpClient(InetAddress localAddress, Duration connectionTimeout) {
+            HttpRoutePlanner routePlanner = createLocalAddressRoutePlanner(localAddress);
+            return Apache5HttpClient.builder()
+                                    .httpRoutePlanner(routePlanner)
+                                    .connectionTimeout(connectionTimeout)
+                                    .build();
+        }
 
-        return Apache5HttpClient.builder()
-                                .httpRoutePlanner(routePlanner)
-                                .connectionTimeout(connectionTimeout)
-                                .build();
+        private HttpRoutePlanner createLocalAddressRoutePlanner(InetAddress localAddress) {
+            return new DefaultRoutePlanner(DefaultSchemePortResolver.INSTANCE) {
+                @Override
+                protected InetAddress determineLocalAddress(HttpHost firstHop, HttpContext context) throws HttpException {
+                    return localAddress != null ? localAddress : super.determineLocalAddress(firstHop, context);
+                }
+            };
+        }
     }
 
-    private HttpRoutePlanner createLocalAddressRoutePlanner(InetAddress localAddress) {
-        return new DefaultRoutePlanner(DefaultSchemePortResolver.INSTANCE) {
-            @Override
-            protected InetAddress determineLocalAddress(HttpHost firstHop, HttpContext context) throws HttpException {
-                return localAddress != null ? localAddress : super.determineLocalAddress(firstHop, context);
+    @Nested
+    @DisplayName("When both route planner and builder local address are configured (route planner takes precedence)")
+    class RoutePlannerPrecedenceTest extends SdkHttpClientLocalAddressFunctionalTestSuite {
+
+        private final InetAddress BUILDER_LOCAL_ADDRESS = InetAddress.getLoopbackAddress();
+
+        @Override
+        protected SdkHttpClient createHttpClient(InetAddress localAddress, Duration connectionTimeout) {
+            // The localAddress parameter will be used by the route planner
+            // The builder's localAddress will be overridden
+            HttpRoutePlanner routePlanner = createLocalAddressRoutePlanner(localAddress);
+            SdkHttpClient httpClient;
+
+            try (LogCaptor logCaptor = LogCaptor.create(Level.DEBUG)) {
+                 httpClient = Apache5HttpClient.builder()
+                                                            .httpRoutePlanner(routePlanner)
+                                                            .localAddress(BUILDER_LOCAL_ADDRESS) // This will be overridden by route planner
+                                                            .connectionTimeout(connectionTimeout)
+                                                            .build();
+
+                assertThat(logCaptor.loggedEvents()).anySatisfy(logEvent -> {
+                    assertThat(logEvent.getLevel()).isEqualTo(Level.DEBUG);
+                    assertThat(logEvent.getMessage().getFormattedMessage())
+                        .contains("localAddress configuration was ignored since Route planner was explicitly provided");
+                });
             }
-        };
+            return httpClient;
+        }
+
+        private HttpRoutePlanner createLocalAddressRoutePlanner(InetAddress routePlannerAddress) {
+            return new DefaultRoutePlanner(DefaultSchemePortResolver.INSTANCE) {
+                @Override
+                protected InetAddress determineLocalAddress(HttpHost firstHop, HttpContext context) throws HttpException {
+                    // Route planner's address takes precedence over builder's address
+                    return routePlannerAddress != null ? routePlannerAddress : super.determineLocalAddress(firstHop, context);
+                }
+            };
+        }
     }
 }

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

@@ -81,7 +81,7 @@ public void prepareRequest_callableCalled_metricsReported() throws IOException {
 
         client.prepareRequest(executeRequest).call();
         MetricCollection collected = collector.collect();
-        assertThat(collected.metricValues(HTTP_CLIENT_NAME)).containsExactly("Apache5");
+        assertThat(collected.metricValues(HTTP_CLIENT_NAME)).containsExactly("Apache5Preview");
         assertThat(collected.metricValues(LEASED_CONCURRENCY)).containsExactly(1);
         assertThat(collected.metricValues(PENDING_CONCURRENCY_ACQUIRES)).containsExactly(2);
         assertThat(collected.metricValues(AVAILABLE_CONCURRENCY)).containsExactly(3);
@@ -99,7 +99,7 @@ public void prepareRequest_connectionManagerNotPooling_callableCalled_metricsRep
 
         MetricCollection collected = collector.collect();
 
-        assertThat(collected.metricValues(HTTP_CLIENT_NAME)).containsExactly("Apache5");
+        assertThat(collected.metricValues(HTTP_CLIENT_NAME)).containsExactly("Apache5Preview");
         assertThat(collected.metricValues(LEASED_CONCURRENCY)).isEmpty();
         assertThat(collected.metricValues(PENDING_CONCURRENCY_ACQUIRES)).isEmpty();
         assertThat(collected.metricValues(AVAILABLE_CONCURRENCY)).isEmpty();