-
Notifications
You must be signed in to change notification settings - Fork 914
fix #2458 add HTTP proxy support to UrlConnectionHttpClient #2986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
SonarCloud Quality Gate failed. |
@MikeDombo can you please add unit tests? |
* 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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is Duplicate function same as software.amazon.awssdk.http.apache.ProxyConfiguration
can we move this to Utils like in function ApacheUtils ?
import software.amazon.awssdk.utils.builder.ToCopyableBuilder; | ||
|
||
/** | ||
* Configuration that defines how to communicate via an HTTP proxy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have more descriptive Java Doc for this class similar to one ProxyConfiguration of NettyNioAsyncHttpClient
String authToken = String.format("%s:%s", this.proxyConfiguration.username(), this.proxyConfiguration.password()); | ||
String authB64 = Base64.getEncoder().encodeToString(authToken.getBytes(StandardCharsets.UTF_8)); | ||
connection.addRequestProperty("proxy-authorization", String.format("Basic %s", authB64)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to test this in junit or Integration tests ?
Did you get a chance to test this one , please let us know the steps.
We can discuss this offline on slack,
/** | ||
* Configuration that defines how to communicate via an HTTP proxy. | ||
*/ | ||
Builder proxyConfiguration(ProxyConfiguration proxyConfiguration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add Builder with consumer also ?
@@ -70,12 +75,16 @@ | |||
|
|||
private static final Logger log = Logger.loggerFor(UrlConnectionHttpClient.class); | |||
private static final String CLIENT_NAME = "UrlConnection"; | |||
private static final AttributeMap.Key<ProxyConfiguration> PROXY_CONFIGURATION = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with other http client lets assign the ProxyConfiguration directly to the class Field as done in other client constructors , I didnot get the benefit of assigning it to a AttributeMap and then doing a get.
Can we do something similar to ApacheHttpClient ?
@@ -148,7 +157,17 @@ private HttpURLConnection createAndConfigureConnection(HttpExecuteRequest reques | |||
} | |||
|
|||
private HttpURLConnection createDefaultConnection(URI uri, SSLSocketFactory socketFactory) { | |||
HttpURLConnection connection = invokeSafely(() -> (HttpURLConnection) uri.toURL().openConnection()); | |||
HttpURLConnection connection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we simplify the code as below ?
private HttpURLConnection createDefaultConnection(URI uri, SSLSocketFactory socketFactory) {
Optional<Proxy> proxy = determineProxy(uri);
HttpURLConnection connection = !proxy.isPresent() ?
invokeSafely(() -> (HttpURLConnection) uri.toURL().openConnection())
:
invokeSafely(() -> (HttpURLConnection) uri.toURL().openConnection(proxy.get()));
if (connection instanceof HttpsURLConnection) {
HttpsURLConnection httpsConnection = (HttpsURLConnection) connection;
if (options.get(SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES)) {
httpsConnection.setHostnameVerifier(NoOpHostNameVerifier.INSTANCE);
}
httpsConnection.setSSLSocketFactory(socketFactory);
}
if (proxy.isPresent() && shouldProxyAuthorize()){
connection.addRequestProperty("proxy-authorization", String.format("Basic %s", encodedAuthToken(proxyConfiguration)));
}
connection.setConnectTimeout(saturatedCast(options.get(SdkHttpConfigurationOption.CONNECTION_TIMEOUT).toMillis()));
connection.setReadTimeout(saturatedCast(options.get(SdkHttpConfigurationOption.READ_TIMEOUT).toMillis()));
return connection;
}
private boolean shouldProxyAuthorize() {
return this.proxyConfiguration != null
&& StringUtils.isEmpty(this.proxyConfiguration.username())
&& StringUtils.isEmpty(this.proxyConfiguration.password());
}
/**
* If a proxy is configured with username+password, then set the proxy-authorization header to authorize ourselves with the
* proxy
*/
private static String encodedAuthToken(ProxyConfiguration proxyConfiguration) {
String authToken = String.format("%s:%s", proxyConfiguration.username(), proxyConfiguration.password());
return Base64.getEncoder().encodeToString(authToken.getBytes(StandardCharsets.UTF_8));
}
private Optional<Proxy> determineProxy(URI uri) {
if (isProxyEnabled(proxyConfiguration) && !isProxyHostExcluded(uri)) {
return Optional.of(
new Proxy(Proxy.Type.HTTP,
new InetSocketAddress(this.proxyConfiguration.host(), this.proxyConfiguration.port())));
}
return Optional.empty();
}
Superseded by #3112. Closing. |
Motivation and Context
Resolves #2458. The goal of this change is to add full proxy support to the UrlConnectionHttpClient so that our project can remove the Apache HTTP Client and instead rely on Java builtins. This change must support HTTP and HTTPS proxies with username/password authorization.
Modifications
Adds HTTP proxy support to the UrlConnectionHttpClient by using largely the same
ProxyConfiguration
class as is in the Apache HTTP Client. If the user configures a proxy using the builder, and the request isn't going to a non-proxy host, then the request will go through the application proxy using Java's builtinProxy
class.Testing
Manually tested using a local squid proxy via HTTP without authentication.
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License