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

Introduce the ability to automatically standup an HTTP proxy for the REST Client #41804

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jul 10, 2024

Add HTTP Proxy Dev Service for REST Client

The quarkus-rest-client starts a simple pass-through proxy by default
which can be used as a target for Wireshark (or similar tools)
in order to capture all the traffic originating from the REST Client
(this really makes sense when the REST Client is used against
HTTPS services).

However, advanced integrations that build on the REST Client can
register their own proxies by using the
DevServicesRestClientProxyProvider SPI

P.S. Originally discussed in an email sent to Quarkus Dev.

This comment was marked as resolved.

@geoand geoand force-pushed the rest-client-proxy branch from 30c5e47 to 5b977e0 Compare July 10, 2024 10:09
@maxandersen
Copy link
Member

I like the idea but feels "wrong" its at the just the rest client level...shouldn't it be vert.x http or even more globally?

like quarkus.http.devproxy=true ?

@geoand
Copy link
Contributor Author

geoand commented Jul 10, 2024

The problem is that with anything not managed (Vertx HTTP Client, JDK HTTP Client, Jakarta REST Client) you don't know what the URL is.
With the REST Client you already have all the information available to make this a one flag opt-in process

@maxandersen
Copy link
Member

The problem is that with anything not managed (Vertx HTTP Client, JDK HTTP Client, Jakarta REST Client) you don't know what the URL is.

okey - educate me here - why is that important to know? the URL gets requested via the proxy does it not?

@geoand
Copy link
Contributor Author

geoand commented Jul 11, 2024

why is that important to know? the URL gets requested via the proxy does it not?

This is not using the HTTP proxy headers - I tried that approach and could not get it to work with OpenAI's API so I abandoned it in favor of switching the URL under the covers.

@geoand
Copy link
Contributor Author

geoand commented Jul 11, 2024

One piece of feedback I had was to make the boundaries of the infrastructure layer for swapping out the URL of the REST Client and the code that actually does something with that URL (which in this case is to create the Vert.x HTTP Proxy). We could imagine that infrastructure could be used to run something like Wiremock or Microcks

@geoand geoand force-pushed the rest-client-proxy branch from 952b8a2 to cefb8a7 Compare July 11, 2024 12:26
@geoand geoand requested a review from cescoffier July 11, 2024 12:27
@geoand geoand marked this pull request as ready for review July 11, 2024 12:27

This comment has been minimized.

@geoand geoand force-pushed the rest-client-proxy branch from cefb8a7 to 6b8aa26 Compare July 11, 2024 12:33

This comment has been minimized.

@geoand
Copy link
Contributor Author

geoand commented Jul 18, 2024

@cescoffier what do you want to do with this one?

@cescoffier
Copy link
Member

I still need to review it - it's on my list for this week. Sorry for the delay.

@geoand
Copy link
Contributor Author

geoand commented Jul 18, 2024

No problem at all, just asking :)

geoand added 2 commits July 18, 2024 15:58
The quarkus-rest-client starts a simple pass-through proxy by default
which can be used as a target for Wireshark (or similar tools)
in order to capture all the traffic originating from the REST Client
(this really makes sense when the REST Client is used against
HTTPS services).

However, advances integrations that build on the REST Client can
register their own proxies by using the
DevServicesRestClientProxyProvider SPI
@geoand geoand force-pushed the rest-client-proxy branch from e6fc60b to 420d44c Compare July 18, 2024 12:58
Copy link

quarkus-bot bot commented Jul 18, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 420d44c.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ Native Tests - HTTP

📦 integration-tests/rest-client-reactive

io.quarkus.it.rest.client.BasicTestIT.shouldCreateClientSpans - History

  • expected: <1> but was: <2> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
	at io.quarkus.it.rest.client.BasicTest.shouldCreateClientSpans(BasicTest.java:216)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:812)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

@geoand
Copy link
Contributor Author

geoand commented Jul 18, 2024

Should we include this in 3.13 despite the CR having been released already?

I personally think it's completely safe

@cescoffier
Copy link
Member

No problem with me.

@geoand geoand merged commit 96e2144 into quarkusio:main Jul 18, 2024
34 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Jul 18, 2024
@geoand geoand deleted the rest-client-proxy branch July 18, 2024 14:46
@gsmet gsmet modified the milestones: 3.14 - main, 3.13.0 Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants