Apache configuration cleanup

Author: varunnvs92

core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/http/timers/AsyncHttpClientApiCallTimeoutTests.java

@@ -147,7 +147,7 @@ public void successfulResponse_SlowResponseHandler_ThrowsApiCallTimeoutException
     }
 
     @Test
-    public void  slowApiAttempt_ThrowsApiCallAttemptTimeoutException() {
+    public void slowApiAttempt_ThrowsApiCallAttemptTimeoutException() {
         httpClient = testAsyncClientBuilder()
             .apiCallTimeout(API_CALL_TIMEOUT)
             .apiCallAttemptTimeout(Duration.ofMillis(100))

http-clients/apache-client/pom.xml

@@ -67,6 +67,11 @@
             <artifactId>junit</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.assertj</groupId>
+            <artifactId>assertj-core</artifactId>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.hamcrest</groupId>
             <artifactId>hamcrest-all</artifactId>

http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ApacheHttpClient.java

@@ -18,6 +18,7 @@
 import static java.util.stream.Collectors.groupingBy;
 import static java.util.stream.Collectors.mapping;
 import static java.util.stream.Collectors.toList;
+import static software.amazon.awssdk.http.SdkHttpConfigurationOption.CONNECTION_ACQUIRE_TIMEOUT;
 import static software.amazon.awssdk.http.SdkHttpConfigurationOption.CONNECTION_TIMEOUT;
 import static software.amazon.awssdk.http.SdkHttpConfigurationOption.GLOBAL_HTTP_DEFAULTS;
 import static software.amazon.awssdk.http.SdkHttpConfigurationOption.MAX_CONNECTIONS;
@@ -78,6 +79,7 @@
 import software.amazon.awssdk.http.apache.internal.utils.ApacheUtils;
 import software.amazon.awssdk.utils.AttributeMap;
 import software.amazon.awssdk.utils.Logger;
+import software.amazon.awssdk.utils.Validate;
 
 /**
  * An implementation of {@link SdkHttpClient} that uses Apache HTTP client to communicate with the service. This is the most
@@ -140,10 +142,11 @@ private void addProxyConfig(HttpClientBuilder builder,
                                 ProxyConfiguration proxyConfiguration) {
         if (isProxyEnabled(proxyConfiguration)) {
 
-            log.debug(() -> "Configuring Proxy. Proxy Host: " + proxyConfiguration.endpoint());
+            log.debug(() -> "Configuring Proxy. Proxy Host: " + proxyConfiguration.host());
 
-            builder.setRoutePlanner(new SdkProxyRoutePlanner(proxyConfiguration.endpoint().getHost(),
-                                                             proxyConfiguration.endpoint().getPort(),
+            builder.setRoutePlanner(new SdkProxyRoutePlanner(proxyConfiguration.host(),
+                                                             proxyConfiguration.port(),
+                                                             proxyConfiguration.scheme(),
                                                              proxyConfiguration.nonProxyHosts()));
 
             if (isAuthenticatedProxy(proxyConfiguration)) {
@@ -164,9 +167,8 @@ private boolean isAuthenticatedProxy(ProxyConfiguration proxyConfiguration) {
     }
 
     private boolean isProxyEnabled(ProxyConfiguration proxyConfiguration) {
-        return proxyConfiguration.endpoint() != null
-               && proxyConfiguration.endpoint().getHost() != null
-               && proxyConfiguration.endpoint().getPort() > 0;
+        return proxyConfiguration.host() != null
+               && proxyConfiguration.port() > 0;
     }
 
     @Override
@@ -240,6 +242,7 @@ private ApacheHttpRequestConfig createRequestConfig(DefaultBuilder builder,
         return ApacheHttpRequestConfig.builder()
                                       .socketTimeout(resolvedOptions.get(READ_TIMEOUT))
                                       .connectionTimeout(resolvedOptions.get(CONNECTION_TIMEOUT))
+                                      .connectionAcquireTimeout(resolvedOptions.get(CONNECTION_ACQUIRE_TIMEOUT))
                                       .proxyConfiguration(builder.proxyConfiguration)
                                       .localAddress(Optional.ofNullable(builder.localAddress).orElse(null))
                                       .expectContinueEnabled(Optional.ofNullable(builder.expectContinueEnabled)
@@ -273,6 +276,13 @@ public interface Builder extends SdkHttpClient.Builder<ApacheHttpClient.Builder>
          */
         Builder connectionTimeout(Duration connectionTimeout);
 
+        /**
+         * The amount of time to wait when acquiring a connection from the pool before giving up and timing out.
+         * @param connectionAcquisitionTimeout the timeout duration
+         * @return this builder for method chaining.
+         */
+        Builder connectionAcquisitionTimeout(Duration connectionAcquisitionTimeout);
+
         /**
          * The maximum number of connections allowed in the connection pool. Each built HTTP client has it's own private
          * connection pool.
@@ -336,6 +346,22 @@ public void setConnectionTimeout(Duration connectionTimeout) {
             connectionTimeout(connectionTimeout);
         }
 
+        /**
+         * The amount of time to wait when acquiring a connection from the pool before giving up and timing out.
+         * @param connectionAcquisitionTimeout the timeout duration
+         * @return this builder for method chaining.
+         */
+        @Override
+        public Builder connectionAcquisitionTimeout(Duration connectionAcquisitionTimeout) {
+            Validate.isPositive(connectionAcquisitionTimeout, "connectionAcquisitionTimeout");
+            standardOptions.put(CONNECTION_ACQUIRE_TIMEOUT, connectionAcquisitionTimeout);
+            return this;
+        }
+
+        public void setConnectionAcquisitionTimeout(Duration connectionAcquisitionTimeout) {
+            connectionAcquisitionTimeout(connectionAcquisitionTimeout);
+        }
+
         @Override
         public Builder maxConnections(Integer maxConnections) {
             standardOptions.put(MAX_CONNECTIONS, maxConnections);

http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ProxyConfiguration.java

@@ -18,11 +18,14 @@
 import static software.amazon.awssdk.utils.StringUtils.isEmpty;
 
 import java.net.URI;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
-import software.amazon.awssdk.annotations.ReviewBeforeRelease;
+import java.util.stream.Collectors;
 import software.amazon.awssdk.annotations.SdkPublicApi;
+import software.amazon.awssdk.utils.ProxySystemSetting;
+import software.amazon.awssdk.utils.StringUtils;
 import software.amazon.awssdk.utils.ToString;
 import software.amazon.awssdk.utils.Validate;
 import software.amazon.awssdk.utils.builder.CopyableBuilder;
@@ -31,7 +34,6 @@
 /**
  * Configuration that defines how to communicate via an HTTP proxy.
  */
-@ReviewBeforeRelease("Review which options are required and which are optional.")
 @SdkPublicApi
 public final class ProxyConfiguration implements ToCopyableBuilder<ProxyConfiguration.Builder, ProxyConfiguration> {
 
@@ -42,6 +44,10 @@ public final class ProxyConfiguration implements ToCopyableBuilder<ProxyConfigur
     private final String ntlmWorkstation;
     private final Set<String> nonProxyHosts;
     private final Boolean preemptiveBasicAuthenticationEnabled;
+    private final Boolean useSystemPropertyValues;
+    private final String host;
+    private final int port;
+    private final String scheme;
 
     /**
      * Initialize this configuration. Private to require use of {@link #builder()}.
@@ -52,18 +58,38 @@ private ProxyConfiguration(DefaultClientProxyConfigurationBuilder builder) {
         this.password = builder.password;
         this.ntlmDomain = builder.ntlmDomain;
         this.ntlmWorkstation = builder.ntlmWorkstation;
-        this.nonProxyHosts = Collections.unmodifiableSet(new HashSet<>(builder.nonProxyHosts));
+        this.nonProxyHosts = builder.nonProxyHosts;
         this.preemptiveBasicAuthenticationEnabled = builder.preemptiveBasicAuthenticationEnabled == null ? Boolean.FALSE :
                 builder.preemptiveBasicAuthenticationEnabled;
+        this.useSystemPropertyValues = builder.useSystemPropertyValues;
+        this.host = resolveHost();
+        this.port = resolvePort();
+        this.scheme = resolveScheme();
     }
 
     /**
-     * The endpoint of the proxy server that the SDK should connect through.
+     * Returns the proxy host name either from the configured endpoint or
+     * from the "http.proxyHost" system property if {@link Builder#useSystemPropertyValues(Boolean)} is set to true.
+     */
+    public String host() {
+        return host;
+    }
+
+    /**
+     * Returns the proxy port either from the configured endpoint or
+     * from the "http.proxyPort" system property if {@link Builder#useSystemPropertyValues(Boolean)} is set to true.
      *
-     * @see Builder#endpoint(URI)
+     * If no value is found in neither of the above options, the default value of 0 is returned.
+     */
+    public int port() {
+        return port;
+    }
+
+    /**
+     * Returns the {@link URI#scheme} from the configured endpoint. Otherwise return null.
      */
-    public URI endpoint() {
-        return endpoint;
+    public String scheme() {
+        return scheme;
     }
 
     /**
@@ -72,7 +98,7 @@ public URI endpoint() {
      * @see Builder#password(String)
      */
     public String username() {
-        return username;
+        return resolveValue(username, ProxySystemSetting.PROXY_USERNAME);
     }
 
     /**
@@ -81,7 +107,7 @@ public String username() {
      * @see Builder#password(String)
      */
     public String password() {
-        return password;
+        return resolveValue(password, ProxySystemSetting.PROXY_PASSWORD);
     }
 
     /**
@@ -105,11 +131,16 @@ public String ntlmWorkstation() {
     /**
      * The hosts that the client is allowed to access without going through the proxy.
      *
+     * If the value is not set on the object, the value represent by "http.nonProxyHosts" system property is returned.
+     * If system property is also not set, an unmodifiable empty set is returned.
+     *
      * @see Builder#nonProxyHosts(Set)
      */
-    @ReviewBeforeRelease("Revisit the presentation of this option and support http.nonProxyHosts property")
     public Set<String> nonProxyHosts() {
-        return nonProxyHosts;
+        Set<String> hosts = nonProxyHosts == null && useSystemPropertyValues ? parseNonProxyHostsProperty()
+                                                                             : nonProxyHosts;
+
+        return Collections.unmodifiableSet(hosts != null ? hosts : Collections.emptySet());
     }
 
     /**
@@ -152,6 +183,54 @@ public String toString() {
                        .build();
     }
 
+
+    private String resolveHost() {
+        return endpoint != null ? endpoint.getHost()
+                                : resolveValue(null, ProxySystemSetting.PROXY_HOST);
+    }
+
+    private int resolvePort() {
+        int port = 0;
+
+        if (endpoint != null) {
+            port = endpoint.getPort();
+        } else if (useSystemPropertyValues) {
+            port = ProxySystemSetting.PROXY_PORT.getStringValue()
+                                                .map(Integer::parseInt)
+                                                .orElse(0);
+        }
+
+        return port;
+    }
+
+    public String resolveScheme() {
+        return endpoint != null ? endpoint.getScheme() : null;
+    }
+
+    /**
+     * Uses the configuration options, system setting property and returns the final value of the given member.
+     */
+    private String resolveValue(String value, ProxySystemSetting systemSetting) {
+        return value == null && useSystemPropertyValues ? systemSetting.getStringValue().orElse(null)
+                                                        : value;
+    }
+
+    /**
+     * Returns the Java system property for nonProxyHosts as set of Strings.
+     * See http://docs.oracle.com/javase/7/docs/api/java/net/doc-files/net-properties.html.
+     */
+    private Set<String> parseNonProxyHostsProperty() {
+        String nonProxyHosts = ProxySystemSetting.NON_PROXY_HOSTS.getStringValue().orElse(null);
+
+        if (!StringUtils.isEmpty(nonProxyHosts)) {
+            return Arrays.stream(nonProxyHosts.split("\\|"))
+                         .map(String::toLowerCase)
+                         .map(s -> s.replace("*", ".*?"))
+                         .collect(Collectors.toSet());
+        }
+        return Collections.emptySet();
+    }
+
     /**
      * A builder for {@link ProxyConfiguration}.
      *
@@ -202,6 +281,15 @@ public interface Builder extends CopyableBuilder<Builder, ProxyConfiguration> {
          */
         Builder preemptiveBasicAuthenticationEnabled(Boolean preemptiveBasicAuthenticationEnabled);
 
+        /**
+         * Option whether to use system property values from {@link ProxySystemSetting} if any of the config options are missing.
+         *
+         * This value is set to "true" by default which means SDK will automatically use system property values
+         * for options that are not provided during building the {@link ProxyConfiguration} object. To disable this behavior,
+         * set this value to "false".
+         */
+        Builder useSystemPropertyValues(Boolean useSystemPropertyValues);
+
     }
 
     /**
@@ -214,8 +302,9 @@ private static final class DefaultClientProxyConfigurationBuilder implements Bui
         private String password;
         private String ntlmDomain;
         private String ntlmWorkstation;
-        private Set<String> nonProxyHosts = new HashSet<>();
+        private Set<String> nonProxyHosts;
         private Boolean preemptiveBasicAuthenticationEnabled;
+        private Boolean useSystemPropertyValues = Boolean.TRUE;
 
         @Override
         public Builder endpoint(URI endpoint) {
@@ -282,6 +371,9 @@ public Builder nonProxyHosts(Set<String> nonProxyHosts) {
 
         @Override
         public Builder addNonProxyHost(String nonProxyHost) {
+            if (this.nonProxyHosts == null) {
+                this.nonProxyHosts = new HashSet<>();
+            }
             this.nonProxyHosts.add(nonProxyHost);
             return this;
         }
@@ -300,6 +392,16 @@ public void setPreemptiveBasicAuthenticationEnabled(Boolean preemptiveBasicAuthe
             preemptiveBasicAuthenticationEnabled(preemptiveBasicAuthenticationEnabled);
         }
 
+        @Override
+        public Builder useSystemPropertyValues(Boolean useSystemPropertyValues) {
+            this.useSystemPropertyValues = useSystemPropertyValues;
+            return this;
+        }
+
+        public void setUseSystemPropertyValues(Boolean useSystemPropertyValues) {
+            useSystemPropertyValues(useSystemPropertyValues);
+        }
+
         @Override
         public ProxyConfiguration build() {
             return new ProxyConfiguration(this);

http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/ApacheHttpRequestConfig.java

@@ -29,13 +29,15 @@ public final class ApacheHttpRequestConfig {
 
     private final Duration socketTimeout;
     private final Duration connectionTimeout;
+    private final Duration connectionAcquireTimeout;
     private final InetAddress localAddress;
     private final boolean expectContinueEnabled;
     private final ProxyConfiguration proxyConfiguration;
 
     private ApacheHttpRequestConfig(Builder builder) {
         this.socketTimeout = builder.socketTimeout;
         this.connectionTimeout = builder.connectionTimeout;
+        this.connectionAcquireTimeout = builder.connectionAcquireTimeout;
         this.localAddress = builder.localAddress;
         this.expectContinueEnabled = builder.expectContinueEnabled;
         this.proxyConfiguration = builder.proxyConfiguration;
@@ -49,6 +51,10 @@ public Duration connectionTimeout() {
         return connectionTimeout;
     }
 
+    public Duration connectionAcquireTimeout() {
+        return connectionAcquireTimeout;
+    }
+
     public InetAddress localAddress() {
         return localAddress;
     }
@@ -75,6 +81,7 @@ public static final class Builder {
 
         private Duration socketTimeout;
         private Duration connectionTimeout;
+        private Duration connectionAcquireTimeout;
         private InetAddress localAddress;
         private Boolean expectContinueEnabled;
         private ProxyConfiguration proxyConfiguration;
@@ -92,6 +99,11 @@ public Builder connectionTimeout(Duration connectionTimeout) {
             return this;
         }
 
+        public Builder connectionAcquireTimeout(Duration connectionAcquireTimeout) {
+            this.connectionAcquireTimeout = connectionAcquireTimeout;
+            return this;
+        }
+
         public Builder localAddress(InetAddress localAddress) {
             this.localAddress = localAddress;
             return this;

http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/SdkProxyRoutePlanner.java

@@ -37,9 +37,9 @@ public class SdkProxyRoutePlanner extends DefaultRoutePlanner {
     private HttpHost proxy;
     private Set<String> hostPatterns;
 
-    public SdkProxyRoutePlanner(String proxyHost, int proxyPort, Set<String> nonProxyHosts) {
+    public SdkProxyRoutePlanner(String proxyHost, int proxyPort, String proxyProtocol, Set<String> nonProxyHosts) {
         super(DefaultSchemePortResolver.INSTANCE);
-        proxy = new HttpHost(proxyHost, proxyPort);
+        proxy = new HttpHost(proxyHost, proxyPort, proxyProtocol);
         this.hostPatterns = nonProxyHosts;
     }