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.

Sign up for GitHub

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

Jump to bottom

Nima: WebClient Proxy Support #6441

Merged
merged 13 commits into from
Jul 3, 2023
Merged

Nima: WebClient Proxy Support #6441

merged 13 commits into from
Jul 3, 2023

Conversation

jbescos
Copy link
Member

@jbescos jbescos commented Mar 21, 2023 •
edited
Loading

#6006

Supports proxy type HTTP/1.1.

It does not support proxy authentication so far.

Here is a new issue to complete missing parts: #7143

@jbescos
Nima: WebClient Proxy Support helidon-io#6006
9f96e23 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos
Merge with master
879cc28 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 21, 2023
@jbescos jbescos marked this pull request as draft March 21, 2023 12:07
@jbescos jbescos force-pushed the issue6006 branch 2 times, most recently from 2b49dd5 to e8c8b1d Compare May 12, 2023 07:31
jbescos added 3 commits May 12, 2023 09:31
@jbescos
Merge with master
e8c8b1d 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos
Proxy working again
7d2a8bf 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos
Remove proxy authentication for the time being
eb4e32c 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos jbescos force-pushed the issue6006 branch 6 times, most recently from 6ed5e41 to 623ea27 Compare May 18, 2023 08:33
@jbescos jbescos marked this pull request as ready for review May 18, 2023 08:56
@jbescos
Remove system proxy
174cfd2 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos jbescos force-pushed the issue6006 branch 2 times, most recently from 25c6709 to 127d608 Compare June 26, 2023 05:58
@jbescos
Merge with master
127d608 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
Verdent
Verdent requested changes Jun 26, 2023
View reviewed changes
nima/tests/integration/webclient/pom.xml Outdated Show resolved Hide resolved
nima/tests/integration/webclient/webclient/pom.xml Outdated Show resolved Hide resolved
...webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/Http1ClientConnection.java Outdated Show resolved Hide resolved
nima/webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/ConnectionCache.java Outdated Show resolved Hide resolved
nima/webclient/webclient/src/main/java/io/helidon/nima/webclient/Proxy.java Outdated Show resolved Hide resolved
nima/webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/ConnectionKey.java Outdated Show resolved Hide resolved
...webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/Http1ClientConnection.java Outdated
protected abstract void connect(Http1ClientConnection connection, InetSocketAddress remoteAddress) throws IOException;

static void configure(Http1ClientConnection connection, InetSocketAddress remoteAddress) throws IOException {
EstablishConnection connector = null;
Copy link
Member

Choose a reason for hiding this comment

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

not needed null assignment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know from which JDK this is allowed, but good to know. Done

Copy link
Member

Choose a reason for hiding this comment

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

You only need to assign null (or any other initialization) if the variable is not assigned in any of the possible flows. This is since ever.

@jbescos jbescos force-pushed the issue6006 branch 2 times, most recently from bcac2e4 to 0ab0c06 Compare June 27, 2023 06:53
@jbescos
Merge branch 'main' into issue6006
0ab0c06 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos jbescos force-pushed the issue6006 branch 3 times, most recently from c601fc1 to 163c2ac Compare June 27, 2023 09:14
Verdent
Verdent previously requested changes Jun 27, 2023
View reviewed changes
Copy link
Member

@Verdent Verdent left a comment

Choose a reason for hiding this comment

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

WebClient should have a way, how to define proxy for overall client and not just per request. Please add this method as well.

...ebclient/webclient/src/test/java/io/helidon/nima/tests/integration/client/HttpProxyTest.java Outdated Show resolved Hide resolved
...bclient/webclient/src/test/java/io/helidon/nima/tests/integration/client/HttpsProxyTest.java Outdated Show resolved Hide resolved
nima/webclient/webclient/pom.xml Outdated Show resolved Hide resolved
nima/webclient/webclient/src/main/java/io/helidon/nima/webclient/Proxy.java Outdated Show resolved Hide resolved
nima/webclient/webclient/src/main/java/io/helidon/nima/webclient/Proxy.java Outdated Show resolved Hide resolved
...webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/Http1ClientConnection.java Outdated Show resolved Hide resolved
...webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/Http1ClientConnection.java Outdated Show resolved Hide resolved
...webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/Http1ClientConnection.java Outdated
BufferData data = BufferData.create(httpConnect.toString().getBytes(StandardCharsets.UTF_8));
writer.writeNow(data);
DataReader reader = new DataReader(tempSocket);
int statusCode = readStatusCode(reader);
Copy link
Member

Choose a reason for hiding this comment

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

Please use Http1StatusParser instead of this readStatusCode method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was using that one, but that one does not support HTTP 1.0. I tested that there are some proxies that, even when you send the request for HTTP 1.1, they respond with HTTP 1.0.

...webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/Http1ClientConnection.java Outdated Show resolved Hide resolved
nima/webclient/webclient/src/main/java/io/helidon/nima/webclient/Proxy.java Outdated Show resolved Hide resolved
@jbescos jbescos marked this pull request as draft June 27, 2023 13:24
@jbescos
Copy link
Member Author

jbescos commented Jun 27, 2023

I went through all your comments @Verdent

The missing part is still this comment: #6441 (review)

I set this as a draft meanwhile I work on that. The other comments can be reviewed.

@jbescos jbescos force-pushed the issue6006 branch 4 times, most recently from a2168e5 to 93206af Compare June 27, 2023 13:29
jbescos added 2 commits June 27, 2023 15:29
@jbescos
Review comments
93206af 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos
Merge with main
5c36c47 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos
Copy link
Member Author

jbescos commented Jun 29, 2023

This comment is also done: #6441 (review)

@jbescos
Include system proxy
914a874 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos jbescos marked this pull request as ready for review June 29, 2023 11:20
@jbescos
Merge with main
388ec8e 
Signed-off-by: Jorge Bescos Gascon <[email protected]>
@jbescos jbescos force-pushed the issue6006 branch 6 times, most recently from b2b1085 to 486fa0e Compare July 3, 2023 13:00
@jbescos jbescos mentioned this pull request Jul 3, 2023
Closed
5 tasks
tomas-langer
tomas-langer previously approved these changes Jul 3, 2023
View reviewed changes
@jbescos jbescos dismissed Verdent’s stale review July 3, 2023 14:33

It was already verified by Tomas and David is not available today

@jbescos jbescos merged commit 47e23a3 into helidon-io:main Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomas-langer tomas-langer tomas-langer approved these changes

@Verdent Verdent Verdent left review comments

Assignees

@jbescos jbescos

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jbescos @Verdent @tomas-langer