Make REST Client work whether or not SSL is enabled or not in native #9773

geoand · 2020-06-04T08:32:32Z

This is done in order to enable the use of Apache HTTP Client in all
cases where the REST Client is used.
This enables us to drop the fallback to URLConnection which is rather limited

Fixes: #9342

~~Requires: resteasy/resteasy#2431~~

~~PR is a draft because without the RESTEasy fix this doesn't make sense.~~ (after talking with @gsmet it became clear to me that the fix in RESTEasy is not necessary)

…ient The fallback was originally added to get around limitation in Apache HTTP Client when SSL is completely disabled in native binaries. However we can get around these limitations in a more elegant manner in Quarkus (see quarkusio/quarkus#9773). By defaulting to Apache HTTP Client, things like HTTP Patch can be supported OOTB (see quarkusio/quarkus#9342).

geoand · 2020-06-04T08:43:54Z

I tested this along with #9773 performing the following scenaria:

rest-client IT using quarkus.ssl.native=false
rest-client IT without setting quarkus.ssl.native
rest-client quickstart using quarkus.ssl.native=false and org.acme.rest.client.CountriesService/mp-rest/url=http://restcountries.eu/rest
rest-client quickstart using quarkus.ssl.native=false and org.acme.rest.client.CountriesService/mp-rest/url=https://restcountries.eu/rest (this obviously fails with a proper message about SSL not being enabled for the native image)
rest-client IT without setting quarkus.ssl.native and org.acme.rest.client.CountriesService/mp-rest/url=http://restcountries.eu/rest
rest-client IT without setting quarkus.ssl.native and org.acme.rest.client.CountriesService/mp-rest/url=shttp://restcountries.eu/rest

geoand · 2020-06-04T08:44:32Z

cc @gsmet @kenfinnigan

kenfinnigan · 2020-06-04T10:03:27Z

extensions/rest-client/runtime/src/main/java/io/quarkus/restclient/runtime/RestClientBase.java

+
+        // we need to push a disabled SSL context when SSL has been disabled
+        // because otherwise Apache HTTP Client will try to initialize one and will fail
+        if (ImageInfo.inImageRuntimeCode() && !SslContextConfiguration.isSslNativeEnabled()) {


What does ImageInfo.inImageRuntimeCode() return if it's running on OpenJDK?

It returns false. We use that in a couple more places in Quarkus as well (for example:

quarkus/core/runtime/src/main/java/io/quarkus/runtime/ApplicationLifecycleManager.java

Line 157 in f89650b

if (ImageInfo.inImageRuntimeCode() && System.getenv(DISABLE_SIGNAL_HANDLERS) == null) {

)

Fair enough, I was half expecting it to majorly fail if it wasn't on GraalVM JDK

Under the hood it just checks for a system property, so it's not that smart :)

kenfinnigan · 2020-06-04T10:15:43Z

Looks good to me

gsmet · 2020-06-04T10:43:17Z

How is the RESTEasy patch required? It just looks like some cleanup to me?

gsmet

Looks good. I'm wondering if we should move the DisabledSslContext elsewhere though?

Making it a proper class would be a good first step. It can stay in the .graal. package as I suppose it will only be useful in a GraalVM environment.

geoand · 2020-06-04T10:48:11Z

@gsmet because without the RESTEasy patch, URLConnection is still the default. So this basically makes Apache HttpClient work OOTB even when ssl is disabled in native.

Good idea about moving the class, I'll do that

geoand · 2020-06-04T10:55:19Z

PR updated as per review comment

gsmet · 2020-06-04T11:05:57Z

Is it? It seems it's only the case if SSL_ENABLED is false and it's true by default. Or am I missing something?

geoand · 2020-06-04T11:10:04Z

The things are now Apache HttpClient only comes into play for the REST Client iff quarkus.ssl.native=true. When it's left unset, SSL_ENABLED is still false (so false by default), so URLConnection is still used.

…ient The fallback was originally added to get around limitation in Apache HTTP Client when SSL is completely disabled in native binaries. However we can get around these limitations in a more elegant manner in Quarkus (see quarkusio/quarkus#9773). By defaulting to Apache HTTP Client, things like HTTP Patch can be supported OOTB (see quarkusio/quarkus#9342).

…bled for REST Client (#2431) The fallback was originally added to get around limitation in Apache HTTP Client when SSL is completely disabled in native binaries. However we can get around these limitations in a more elegant manner in Quarkus (see quarkusio/quarkus#9773). By defaulting to Apache HTTP Client, things like HTTP Patch can be supported OOTB (see quarkusio/quarkus#9342).

geoand · 2020-06-05T18:57:17Z

@asoldano can you ping me when you release a new 4.5.x version which contains the small fix I added?

Thanks

gsmet · 2020-06-08T15:36:06Z

The things are now Apache HttpClient only comes into play for the REST Client iff quarkus.ssl.native=true. When it's left unset, SSL_ENABLED is still false (so false by default), so URLConnection is still used.

Can we discuss that when you're back from PTO. I fail to see how SSL_ENABLED is false by default given: https://github.com/resteasy/Resteasy/pull/2431/files#diff-14ad0b8f47bf5f23be58cd4a2cb1cf96L70 .

geoand · 2020-06-08T15:39:04Z

Yup, let's discuss tomorrow

geoand · 2020-06-09T13:47:51Z

Failures are related, I'll have to take a look

geoand · 2020-06-09T15:20:27Z

Fixed failing tests (and also did some manual re-testing), so this should be ready for merge

geoand · 2020-06-09T16:28:19Z

TCKs are failing in timeout tests... I have no idea what that's all about

This is done in order to enable the use of Apache HTTP Client in all cases where the REST Client is used. This enables us to drop the fallback to URLConnection which is rather limited Fixes: quarkusio#9342

geoand · 2020-06-09T17:48:34Z

@kenfinnigan I had to disable the Timeout tests in the TCK because we were failing at https://github.com/eclipse/microprofile-rest-client/blob/1.4.1/tck/src/main/java/org/eclipse/microprofile/rest/client/tck/timeout/TimeoutTest.java#L65 (the requests were taking exactly as much time as the limit).

I saw that that in newer versions of the TCK the time cushion is configurable, so when we update, we should revisit

@suspended

* 4.5.0-SNAPSHOT -> 4.5.0.Final (#2307) * 4.5.0.Final -> 4.6.0-SNAPSHOT * [RESTEASY-2496]:Add test case * [RESTEASY-2510] Split RESTUtils to links provider and injector * RESTEASY-2445 * [RESTEASY-2517] Async io fixes (#2315) * Apply same fix for SSE streams into raw streams To avoid onNext finishing after onComplete * Test for raw stream and async io * Fixed typo in onComplete/onWriteComplete * Fix potential bug spotted by Stuart * [RESTEASY-2520] CDI BeanManager fix for Quarkus (#2318) * Fix for Quarkus (quarkusio/quarkus#7639) Signed-off-by:Phillip Kruger <[email protected]> * Added check for CDI in construct. Signed-off-by:Phillip Kruger <[email protected]> * Checkstyle fixes Signed-off-by:Phillip Kruger <[email protected]> * [RESTEASY-2518] added value check and test for query param replacement * [RESTEASY-2503] Throw ViolationException for EJBs with just parameter violations. * RESTEASY-2519 (#2320) * RESTEASY-2519 * update * [RESTEASY-2535] Upgrade Jackson2 to 2.10.3 * [RESTEASY-2541] Fix RestClientListeners * [RESTEASY-2542] Excluding tests * RESTEASY-2554 Update section "Upgrading RESTEasy within WildFly" (#2349) * update * RESTEASY-2554 * remove module * update doc * minor fix * minor addition * minor fix * minor addition * fix version * minor fix * [RESTEASY-2559] Improper validation of response header in MediaTypeHeaderDelegate.java class * [RESTEASY-2551] Upgraded io.netty:netty-all to version 4.1.48.Final * [RESTEASY-2543] add regex change * [RESTEASY-2226]:NullPointerException in PatchMethodFilter * [RESTEASY-2385] Added support for embedded MultipartOutput types. Added testcase. * [RESTEASY-2480] re-enable code handling base64 * [RESTEASY-2532] replace reference to jakarta-json * [RESTEASY-2565] Mp client async interceptors (#2359) * Tie the AsyncInvocationInterceptor to each invocation * Spotted a bug, I think * [RESTEASY-2566] Set response code to 500 if writing exception fails (#2358) * [RESTEASY-2562] Incorrect status code when WebApplicationException is… (#2365) * [RESTEASY-2562] Incorrect status code when WebApplicationException is thrown while parsing @*Param * [RESTEASY-2562] Checkstyle - make arg final * [RESTEASY-2495] HeaderParam could have a ParamConverter related (#2292) AbstractInvocationCollectionProcessor is processing Array and Collection of elements being ParamConverter aware. But with single values is delegating in final class to process the value. HeaderParamProcessor is not being aware of such transformation. * [RESTEASY-2495] made HeaderParamProcessor aware of ParamConverter and provided testcase * [RESTEASY-2526] Fix by using asyncutil lib for async loops (#2368) * [RESTEASY-2466] removed profile arquillian.remote * [RESTEASY-2531] expanded and refactored chapter * [RESTEASY-2544] pom updates to arquillian-util and integration-tests * [RESTEASY-2542] ignore any IOException in SSEOutputStream.close() * Test against WildFly 19.0.0.Final instead of Beta2 * [RESTEASY-2526] Turn the com.ibm.async.asyncutil module into private * [RESTEASY-2550] Skip null parameters being processed by ParamConverter's. Also throw an NPE if a parameter annotated with @PathParam it not allowed. https://issues.redhat.com/browse/RESTEASY-2550 * [RESTEASY-2542] Restore tests * [RESTEASY-2239] updated mime4j to version 0.8.2 * Add GitHub Action for running CI builds * [RESTEASY-2561] Allow tests to run in environments other than a Linux local environment. * [RESTEASY-2564] Use a single thread executor instead of the default executor for a more graceful shutdown. * Remove TravisCI runs duplicated by Github Actions * [RESTEASY-2542] Exclude a test again, still failing * [RESTEASY-2542] Exlcude the whole SseTest, not just testReconnect() * Only upload archives for failed runs. Also fix the name to not end with ".zip" * [RESTEASY-2548] add new provider with higher priority than JaxrsFormProvider adding test * [RESTEASY-2560] set scope for must junit dependencies to test * [RESTEASY-2391] changed code to use getMethod() * [RESTEASY-2472]:@suspended AsyncResponse timeout still runs, even if … (#2372) * [RESTEASY-2472]:@suspended AsyncResponse timeout still runs, even if the response was resumed before * Update AsyncTimeoutResource.java * Update AsyncTimeoutResource.java Bogus change to rerun tests. Co-authored-by: Ron Sigal <[email protected]> * [RESTEASY-2563: SseBroadcasterImpl.close() puts notifyOnCloseListeners(eventSink) in finally block. (#2396) * [RESTEASY-2576] added client modules * [RESTEASY-2582] CompletionStageResponseTest intermittent failures (#2390) * Prevent memory visibility issues * Refactor AsyncResponseCallback to isolate data from different tests * Same sleep time for similar methods * [RESTEASY-2571] Avoid a NullPointerException of the MediaType associated with request headers is null. https://issues.redhat.com/browse/RESTEASY-2571 * [RESTEASY-2569] RestClient does not propogate incoming headers with inherited Interface * [RESTEASY-2582] Add logs to the test * [RESTEASY-2582] Fix race condition in test * [RESTEASY-1203] Fix validation for JNDI resources. (#2406) * [RESTEASY-2542][RESTEASY-2585] Workaround SseTest intermittent failures (#2404) * Enable SseTest * [RESTEASY-2542]:Fix Sse reconnect test * [RESTEASY-2542]:More Sse test fix * Fix typo and add comment * [RESTEASY-2017]:Fix SSE doesn't work with inherited annotations * Updated links to the new project site * [RESTEASY-2588] Upgrade jackson to 2.10.4 * [RESTEASY-2587] Update classmate to 1.5.1 * Fix RESTEASY-2573 by using ResteashContext.addCloseable variants * Test RESTEASY-2573 for SSE/raw/collect async streaming * [RESTEASY-2542] wait for proxy to be started * [RESTEASY-2576] Added modules to resteasy-bom/pom.xml (#2417) * Added note to testsuite/README.MD on how to run single microprofile-tck tests (#2426) * SseEventOutputImpl: avoid deadlock by avoiding getting into critical sections early RESTEASY-2585 * HttpServletResponseWrapper: try working around UNDERTOW-1713: RESTEASY-2585 * RESTEASY-2524 (#2332) * RESTEASY-2524 * trailing spaces removed * [RESTEASY-2585]:Remove Thread.sleep() workaround in SseTest * Updating target test containers * [RESTEASY-2598] Upgrade Infinispan to 10.1.8 * [RESTEASY-2598] Exclude ServerCacheInterceptorTest in WFLY19 * Fix modules test references to properly run with WFLY20 too * Exclude test failing on GithubActions CI due to microprofile/microprofile-rest-client#265 * [RESTESY-2601] Exclude test * [RESTEASY-2605] Remove the fallback to URLConnection when SSL is disabled for REST Client (#2431) The fallback was originally added to get around limitation in Apache HTTP Client when SSL is completely disabled in native binaries. However we can get around these limitations in a more elegant manner in Quarkus (see quarkusio/quarkus#9773). By defaulting to Apache HTTP Client, things like HTTP Patch can be supported OOTB (see quarkusio/quarkus#9342). Co-authored-by: Ron Sigal <[email protected]> Co-authored-by: Jim Ma <[email protected]> Co-authored-by: Gytis Trikleris <[email protected]> Co-authored-by: 阿男 <[email protected]> Co-authored-by: Stéphane Épardaud <[email protected]> Co-authored-by: Phillip Krüger <[email protected]> Co-authored-by: R Searls <[email protected]> Co-authored-by: Bartosz Spyrko-Smietanko <[email protected]> Co-authored-by: Stuart Douglas <[email protected]> Co-authored-by: Anil Gursel <[email protected]> Co-authored-by: David Santos <[email protected]> Co-authored-by: James Perkins <[email protected]> Co-authored-by: Duncan <[email protected]> Co-authored-by: bobbyphilip <[email protected]> Co-authored-by: Tommaso Borgato <[email protected]> Co-authored-by: Georgios Andrianakis <[email protected]>

boring-cyborg bot added area/core area/rest-client labels Jun 4, 2020

This was referenced Jun 4, 2020

[RESTEASY-2605] Remove the fallback to URLConnection when SSL is disabled for REST Client resteasy/resteasy#2431

Merged

Project / Spring on Quarkus / Fix broken HTTP Patch support in Quarkus REST Client snowdrop/team#90

Closed

geoand mentioned this pull request Jun 4, 2020

Add @Patch support for RestClient #9342

Closed

kenfinnigan reviewed Jun 4, 2020

View reviewed changes

gsmet approved these changes Jun 4, 2020

View reviewed changes

geoand force-pushed the #9342 branch from 8fdb041 to 8293c6b Compare June 4, 2020 10:55

gsmet added the triage/backport? label Jun 8, 2020

geoand marked this pull request as ready for review June 9, 2020 10:26

geoand force-pushed the #9342 branch from 8293c6b to d82c908 Compare June 9, 2020 15:15

Make REST Client work whether or not SSL is enabled or not in native

b406f7a

This is done in order to enable the use of Apache HTTP Client in all cases where the REST Client is used. This enables us to drop the fallback to URLConnection which is rather limited Fixes: quarkusio#9342

geoand force-pushed the #9342 branch from d82c908 to b406f7a Compare June 9, 2020 17:45

gsmet added this to the 1.6.0 - master milestone Jun 10, 2020

gsmet merged commit d05d415 into quarkusio:master Jun 10, 2020

geoand deleted the #9342 branch June 10, 2020 10:09

gsmet removed the triage/backport? label Jun 10, 2020

gsmet modified the milestones: 1.6.0 - master, 1.5.1.Final Jun 10, 2020

pebanb mentioned this pull request Jun 22, 2020

Since 1.5.1. switch from URLConnectionEngine to ApacheHttpClient43Engine impacts in very poor client performance #10160

Closed

Make REST Client work whether or not SSL is enabled or not in native #9773

Make REST Client work whether or not SSL is enabled or not in native #9773

Uh oh!

Conversation

geoand commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoand commented Jun 4, 2020

Uh oh!

geoand commented Jun 4, 2020

Uh oh!

kenfinnigan Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

geoand Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

kenfinnigan Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

geoand Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

kenfinnigan commented Jun 4, 2020

Uh oh!

gsmet commented Jun 4, 2020

Uh oh!

gsmet left a comment

Choose a reason for hiding this comment

Uh oh!

geoand commented Jun 4, 2020

Uh oh!

geoand commented Jun 4, 2020

Uh oh!

gsmet commented Jun 4, 2020

Uh oh!

geoand commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoand commented Jun 5, 2020

Uh oh!

gsmet commented Jun 8, 2020

Uh oh!

geoand commented Jun 8, 2020

Uh oh!

geoand commented Jun 9, 2020

Uh oh!

geoand commented Jun 9, 2020

Uh oh!

geoand commented Jun 9, 2020

Uh oh!

geoand commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

geoand commented Jun 4, 2020 •

edited

Loading

geoand commented Jun 4, 2020 •

edited

Loading

geoand commented Jun 9, 2020 •

edited

Loading