Skip to content
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

Allow to exclude host from being proxying #2600 #2614

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/main/asciidoc/dataobjects.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -2032,6 +2032,10 @@ Set the store as a buffer
[frame="topbot"]
|===
^|Name | Type ^| Description
|[[excludedHosts]]`excludedHosts`|`Array of String`|
+++
Set list of excluded hosts.
+++
|[[host]]`host`|`String`|
+++
Set proxy host.
Expand Down
4 changes: 2 additions & 2 deletions src/main/asciidoc/java/override/dependencies.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ project descriptor to access the Vert.x Core API:
<dependency>
<groupId>io.vertx</groupId>
<artifactId>vertx-core</artifactId>
<version>3.5.3</version>
<version>3.5.4-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove all changes to this file.

</dependency>
----

Expand All @@ -17,6 +17,6 @@ project descriptor to access the Vert.x Core API:
[source,groovy,subs="+attributes"]
----
dependencies {
compile 'io.vertx:vertx-core:3.5.3'
compile 'io.vertx:vertx-core:3.5.4-SNAPSHOT'
}
----
13 changes: 13 additions & 0 deletions src/main/generated/io/vertx/core/net/ProxyOptionsConverter.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@
class ProxyOptionsConverter {

static void fromJson(JsonObject json, ProxyOptions obj) {
if (json.getValue("excludedHosts") instanceof JsonArray) {
java.util.LinkedHashSet<java.lang.String> list = new java.util.LinkedHashSet<>();
json.getJsonArray("excludedHosts").forEach( item -> {
if (item instanceof String)
list.add((String)item);
});
obj.setExcludedHosts(list);
}
if (json.getValue("host") instanceof String) {
obj.setHost((String)json.getValue("host"));
}
Expand All @@ -45,6 +53,11 @@ static void fromJson(JsonObject json, ProxyOptions obj) {
}

static void toJson(ProxyOptions obj, JsonObject json) {
if (obj.getExcludedHosts() != null) {
JsonArray array = new JsonArray();
obj.getExcludedHosts().forEach(item -> array.add(item));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could use JsonArray::add instead of item -> array.add(item)

json.put("excludedHosts", array);
}
if (obj.getHost() != null) {
json.put("host", obj.getHost());
}
Expand Down
10 changes: 9 additions & 1 deletion src/main/java/io/vertx/core/http/impl/HttpChannelConnector.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.vertx.core.http.impl.pool.ConnectionListener;
import io.vertx.core.http.impl.pool.ConnectionProvider;
import io.vertx.core.impl.ContextImpl;
import io.vertx.core.net.ProxyOptions;
import io.vertx.core.net.ProxyType;
import io.vertx.core.net.SocketAddress;
import io.vertx.core.net.impl.ChannelProvider;
Expand Down Expand Up @@ -116,6 +117,13 @@ public void deactivate(HttpClientConnection conn) {
}
}

private boolean isExcluded(ProxyOptions options, String targetProxy) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this as it's no longer needed; see comment for HttpClientImpl.useProxy().

if(options.getExcludedHosts() == null || options.getExcludedHosts().isEmpty()) {
return false;
}
return options.getExcludedHosts().contains(targetProxy);
}

private void doConnect(
ConnectionListener<HttpClientConnection> listener,
ContextImpl context,
Expand All @@ -129,7 +137,7 @@ private void doConnect(

ChannelProvider channelProvider;
// http proxy requests are handled in HttpClientImpl, everything else can use netty proxy handler
if (options.getProxyOptions() == null || !ssl && options.getProxyOptions().getType()== ProxyType.HTTP ) {
if (options.getProxyOptions() == null || !ssl && options.getProxyOptions().getType() == ProxyType.HTTP || isExcluded(options.getProxyOptions(), host)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment for HttpClientImpl.useProxy().

channelProvider = ChannelProvider.INSTANCE;
} else {
channelProvider = ProxyChannelProvider.INSTANCE;
Expand Down
13 changes: 12 additions & 1 deletion src/main/java/io/vertx/core/http/impl/HttpClientImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,7 @@ private HttpClientRequest createRequest(HttpMethod method, String protocol, Stri
}
checkClosed();
HttpClientRequest req;
boolean useProxy = !useSSL && proxyType == ProxyType.HTTP;
boolean useProxy = useProxy(useSSL, options.getProxyOptions(), host);
if (useProxy) {
final int defaultPort = protocol.equals("ftp") ? 21 : 80;
final String addPort = (port != -1 && port != defaultPort) ? (":" + port) : "";
Expand All @@ -1030,6 +1030,17 @@ private HttpClientRequest createRequest(HttpMethod method, String protocol, Stri
return req;
}

private boolean useProxy(boolean isSsl, ProxyOptions proxyOptions, String targetHost) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality is duplicated in HttpChannelConnector.doConnect() that you also updated. Please move this helper method to e.g. a new class src/main/java/io/vertx/core/http/impl/ProxyHelper.java. Then reuse that helper in doConnect() as well. (You'll have to extract the proxy type frokm the proxyOptions instead, but that's fine.)

if(!isSsl && proxyType == ProxyType.HTTP) {
if(proxyOptions.getExcludedHosts().isEmpty()) {
return true;
} else {
return !proxyOptions.getExcludedHosts().contains(targetHost);
}
}
return false;
}

private synchronized void checkClosed() {
if (closed) {
throw new IllegalStateException("Client is closed");
Expand Down
40 changes: 39 additions & 1 deletion src/main/java/io/vertx/core/net/ProxyOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

package io.vertx.core.net;

import java.util.Objects;
import java.util.*;

import io.vertx.codegen.annotations.DataObject;
import io.vertx.core.json.JsonObject;
Expand Down Expand Up @@ -46,6 +46,7 @@ public class ProxyOptions {
private String username;
private String password;
private ProxyType type;
private Set<String> exludedHosts;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - please rename to excludedHosts.


/**
* Default constructor.
Expand All @@ -54,6 +55,7 @@ public ProxyOptions() {
host = DEFAULT_HOST;
port = DEFAULT_PORT;
type = DEFAULT_TYPE;
exludedHosts = new HashSet<>(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Collections.emptySet() instead if new HashSet<>(0). Also, you should follow the same pattern as other default values and declare a DEFAULT_EXCLUDEDHOSTS constant instead.

}

/**
Expand All @@ -67,6 +69,7 @@ public ProxyOptions(ProxyOptions other) {
username = other.getUsername();
password = other.getPassword();
type = other.getType();
exludedHosts = other.getExcludedHosts();
}

/**
Expand Down Expand Up @@ -199,6 +202,39 @@ public ProxyOptions setType(ProxyType type) {
return this;
}

/**
* Set list of excluded hosts.
*
* @param excludedHosts list of hosts to exclude from proxy logic
* @return a reference to this, so the API can be used fluently
*/
public ProxyOptions setExcludedHosts(final Set<String> excludedHosts) {
Objects.requireNonNull(excludedHosts, "Excluded host cannot be null");
this.exludedHosts = excludedHosts;
return this;
}

/**
* Add host to existing excludedHost list.
*
* @param excludedHost host excluded from proxy logic
* @return a reference to this, so the API can be used fluently
*/
public ProxyOptions addExcludedHost(final String excludedHost) {
Objects.requireNonNull(excludedHost, "Excluded host cannot be null");
this.exludedHosts.add(excludedHost);
return this;
}

/**
* Get excluded hosts.
*
* @return list of hosts that should not be proxied.
*/
public Set<String> getExcludedHosts() {
return exludedHosts;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
Expand All @@ -212,6 +248,7 @@ public boolean equals(Object o) {
if (port != that.port) return false;
if (!Objects.equals(password, that.password)) return false;
if (!Objects.equals(username, that.username)) return false;
if (!Objects.equals(exludedHosts, that.exludedHosts)) return false;

return true;
}
Expand All @@ -224,6 +261,7 @@ public int hashCode() {
result = 31 * result + port;
result = 31 * result + (password != null ? password.hashCode() : 0);
result = 31 * result + (username != null ? username.hashCode() : 0);
result = 31 * result + exludedHosts.hashCode();
return result;
}
}
26 changes: 26 additions & 0 deletions src/test/java/io/vertx/test/core/Http1xTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2955,6 +2955,32 @@ public void testHttpProxyRequest() throws Exception {
testHttpProxyRequest2(client.get(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, "/"));
}

@Test
public void testHttpHostExclusionOfProxyRequest() throws Exception {
startProxy(null, ProxyType.HTTP);
client.close();
client = vertx.createHttpClient(new HttpClientOptions()
.setProxyOptions(new ProxyOptions().setType(ProxyType.HTTP).addExcludedHost("localhost").setHost("google.com").setPort(proxy.getPort())));

HttpClientRequest clientReq = client.get(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, "/");

server.requestHandler(req -> {
req.response().end();
});

//because we excluded localhost from proxying logic, it will not reach google.com but localhost.
server.listen(onSuccess(s -> {
clientReq.handler(resp -> {
assertEquals(200, resp.statusCode());
testComplete();
});
clientReq.exceptionHandler(this::fail);
clientReq.end();
}));

await();
}

@Test
public void testHttpProxyRequestOverrideClientSsl() throws Exception {
startProxy(null, ProxyType.HTTP);
Expand Down