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

Implement HTTP Proxy for non SSL HttpClient #1498

Closed
vietj opened this issue Jul 6, 2016 · 12 comments · Fixed by #1550
Closed

Implement HTTP Proxy for non SSL HttpClient #1498

vietj opened this issue Jul 6, 2016 · 12 comments · Fixed by #1550

Comments

@vietj
Copy link
Contributor

@vietj vietj commented Jul 6, 2016

At the moment the HttpClient proxy configuration uses CONNECT for non SSL HttpClient and instead it should do a straight connection with a target requestURI, i.e

client.get(80, "www.google.com", resp -> {});

should be equivalent to:

client.get(80, "the-proxy", "http://www.google.com", resp -> {}).host("www.google.com:80");

so we do have request like:

GET http://www.google.com HTTP/1.1
Host: www.google.com

while the client connects to the-proxy host.

Note: this does not affect NetClient that will always continue to create a tunnel using CONNECT

@techpavan

This comment has been minimized.

Copy link

@techpavan techpavan commented Jul 12, 2016

My upvote for this issue. Consumers should find it easy and Vert.x should handle it internally.

@alexlehm

This comment has been minimized.

Copy link
Contributor

@alexlehm alexlehm commented Jul 15, 2016

@vietj should we consider this in the 3.3.3 version?

@vietj

This comment has been minimized.

Copy link
Contributor Author

@vietj vietj commented Jul 15, 2016

that's doable imho, depending on the complexity of the solution

@alexlehm

This comment has been minimized.

Copy link
Contributor

@alexlehm alexlehm commented Jul 15, 2016

I implemented a unit test and a first version of the fix

https://github.com/alexlehm/vert.x/tree/issues/%231498-http-proxy-request

needs some more tests probably

@pmlopes pmlopes mentioned this issue Jul 15, 2016
58 of 60 tasks complete
@vietj

This comment has been minimized.

Copy link
Contributor Author

@vietj vietj commented Jul 28, 2016

what is the current status of this fix ?

@alexlehm

This comment has been minimized.

Copy link
Contributor

@alexlehm alexlehm commented Jul 28, 2016

I rebased my changes onto the current master from eclipse/vert.x, I will rerun the tests, I think its almost finished

alexlehm added a commit to alexlehm/vert.x that referenced this issue Jul 28, 2016
@vietj

This comment has been minimized.

Copy link
Contributor Author

@vietj vietj commented Jul 28, 2016

you said you were missing tests before, that's why I'm asking there

@alexlehm

This comment has been minimized.

Copy link
Contributor

@alexlehm alexlehm commented Jul 28, 2016

Sorry, I forgot about that. I have implemented additional tests and added handling for login on the proxy. I could do a PR now I think.

@vietj

This comment has been minimized.

Copy link
Contributor Author

@vietj vietj commented Jul 28, 2016

would be great

@alexlehm

This comment has been minimized.

Copy link
Contributor

@alexlehm alexlehm commented Jul 28, 2016

#1540

for some reason the eclipse ip check fails on one commit, I think the format is correct though.

@alexlehm

This comment has been minimized.

Copy link
Contributor

@alexlehm alexlehm commented Jul 30, 2016

new pr #1548, this one is accepted by the eclipse check

alexlehm added a commit to alexlehm/vert.x that referenced this issue Jul 31, 2016
…to issues/eclipse-vertx#1498-merged

# Conflicts:
#	src/test/java/io/vertx/test/core/ConnectHttpProxy.java

Signed-off-by: alexlehm <alexlehm@gmail.com>
@alexlehm

This comment has been minimized.

Copy link
Contributor

@alexlehm alexlehm commented Jul 31, 2016

new pr merged into the current master #1550

@vietj vietj closed this in #1550 Aug 2, 2016
vietj added a commit that referenced this issue Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.