Implement Java 11's HttpClient within RestClient [HZ-2460] #25654

JamesHazelcast · 2023-10-09T18:42:09Z

During the investigation of - and solution generation for - #24613, it was noted that Java 11 provides the new HttpClient that features interruptible connections - the original solution was kept in place for 5.3, but with 5.4 we have now dropped JDK 8 support, so we can introduce this JDK 11 feature 🎉

This PR does exactly that, introducing HttpClient within the RestClient implementation we maintain as a Proof of Concept - this allows connections to be interrupted, as demonstrated in the KubernetesClient implementation, which is where the majority of focus for this PoC has been. Changes are generally fairly minimal outside of the RestClient itself and KubernetesClient implementation, with the main areas requiring changes being those that used #withCaCertificates() (now replaced by a separate constructor for clients using SSL), and timeout changes (connection/read timeouts are now unified).

This PR now also includes a regression test for the original issue that was encountered, ensuring the HttpClient solution still resolves the issue.

This is WIP and currently a first stage attempt; compiles/runs and KubernetesClient functions correctly (without an extra thread 🎉), but RestClientTest has 2 failures

… not null

With the old RestClient implementation, the connection formed in this test is never `HttpsURLConnection`, so the certificate was never used. In the new RestClient, the SSL context is created before the connection, so passing an invalid certificate breaks functionality. Since this test did not utilise the caCertificate value before, there should be no impact from removing it.

# Conflicts: # hazelcast/src/main/java/com/hazelcast/kubernetes/KubernetesClient.java # hazelcast/src/main/java/com/hazelcast/spi/utils/RestClient.java # hazelcast/src/test/java/com/hazelcast/kubernetes/StsMonitorTest.java # hazelcast/src/test/java/com/hazelcast/spi/utils/RestClientTest.java

JamesHazelcast · 2023-10-10T08:29:49Z

run-lab-run

JamesHazelcast · 2023-10-11T11:44:42Z

run-lab-run

kwart · 2023-10-11T12:19:21Z

hazelcast/src/main/java/com/hazelcast/aws/AwsRequestUtils.java

@@ -47,8 +47,7 @@ static String currentTimestamp(Clock clock) {

    static RestClient createRestClient(String url, AwsConfig awsConfig) {
        return RestClient.create(url)
-            .withConnectTimeoutSeconds(awsConfig.getConnectionTimeoutSeconds())
-            .withReadTimeoutSeconds(awsConfig.getReadTimeoutSeconds())
+            .withPreferredTimeoutSeconds(awsConfig.getConnectionTimeoutSeconds(), awsConfig.getReadTimeoutSeconds())


Due to the AWS config, It probably makes sense to have both - the timeout and the connectionTimeout (using HttpClient.Builder.connectTimeout(Duration)). If we want to support just one, we should improve the AWS config too.

Good point, thanks @kwart - I had originally dropped separate values since they seemed to be the same in all areas of the codebase, but this doesn't account for customizable values such as an AWS config. I've restored functionality for both timeouts in this commit 👍 6ea8e09

Also returns functionality where they were previously split values.

JamesHazelcast · 2023-10-12T08:42:12Z

run-lab-run

HttpClient in J11 will raise `java.lang.IllegalArgumentException: restricted header name: "Host"` if a "Host" header is provided - there are overrides for this in J12 onwards, but not possible in J11.

JamesHazelcast · 2023-10-13T12:28:43Z

Aws API tests were failing due to the setting of "Host" header which is not allowed with HttpClient in Java 11 - there are override properties available in J12, but not with J11. I've removed the "Host" header definitions and tests are passing locally (the "Host" header is set to the endpoint value provided in the URL already): 53289d4

@SeriyBg please let me know if this is a breaking change, and if so we may need to re-assess the viability of using HttpClient.

hz-devops-test · 2023-10-13T13:33:08Z

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

---------ERRORS-----------
--------------------------
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.180 s <<< FAILURE! -- in com.hazelcast.internal.metrics.jmx.MemberJmxMetricsTest
--------------------------
[ERROR] com.hazelcast.internal.metrics.jmx.MemberJmxMetricsTest.testNoMBeanLeak -- Time elapsed: 1.179 s <<< FAILURE!
--------------------------
[ERROR] Failures: 
--------------------------
[ERROR]   MemberJmxMetricsTest.testNoMBeanLeak:64 expected:<0> but was:<1>
--------------------------
[ERROR] Tests run: 4623, Failures: 1, Errors: 0, Skipped: 15
--------------------------
[ERROR] There are test failures.
--------------------------
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   MemberJmxMetricsTest.testNoMBeanLeak:64 expected:<0> but was:<1>
[INFO] 
[ERROR] Tests run: 4623, Failures: 1, Errors: 0, Skipped: 15
[INFO] 
[ERROR] There are test failures.

JamesHazelcast · 2023-10-13T14:01:05Z

run-lab-run

SeriyBg · 2023-10-16T07:54:58Z

@JamesHazelcast, the changes don't seem to be breaking one, as the Host header is set automatically anyway. However, the AWS tests are failing. It might be related to your changes and could be caused by the instability of the test suites, but I will spend a bit more time checking it

vbekiaris

LGTM - @JamesHazelcast can you make sure we run the kubernetes+persistence integration tests on EE side once this is merged to verify no regression?

SeriyBg · 2023-10-17T10:15:22Z

@JamesHazelcast, it seems that the change is, in fact, breaking the AWS integration. It fails during the cluster formation with an exception:

com.hazelcast.spi.exception.RestClientException: Failure executing: POST at: https://ecs.us-east-1.amazonaws.com. Message: {"__type":"InvalidSignatureException","message":"'Host' or ':authority' must be a 'SignedHeader' in the AWS Authorization."}. HTTP Error Code: 400

It seems that there is a fix for Java 12, but no workaround for Java 11:
https://bugs.openjdk.org/browse/JDK-8213189

Trim `host` header at RestClient level, leaving it in place for all other usages during signing.

JamesHazelcast · 2023-10-17T14:49:01Z

@JamesHazelcast, it seems that the change is, in fact, breaking the AWS integration. It fails during the cluster formation with an exception:
com.hazelcast.spi.exception.RestClientException: Failure executing: POST at: https://ecs.us-east-1.amazonaws.com. Message: {"__type":"InvalidSignatureException","message":"'Host' or ':authority' must be a 'SignedHeader' in the AWS Authorization."}. HTTP Error Code: 400
It seems that there is a fix for Java 12, but no workaround for Java 11: https://bugs.openjdk.org/browse/JDK-8213189

My latest commit appears to have resolved the issue for the AWS discovery tests - hopefully no more failures 🤞

SeriyBg

There are still some failures in the K8s Test suite, but it seems to be a problem in the suite itself. I've checked the K8s discovery and can confirm that it's working, and the member discovery is working. LGTM

JamesHazelcast · 2023-10-18T11:26:37Z

Thanks for the reviews everyone, greatly appreciated :)

…#25654) During the investigation of - and solution generation for - hazelcast#24613, it was noted that Java 11 provides the new `HttpClient` that features interruptible connections - the original solution was kept in place for 5.3, but with 5.4 we have now dropped JDK 8 support, so we can introduce this JDK 11 feature 🎉 This PR does exactly that, introducing `HttpClient` within the `RestClient` implementation we maintain as a Proof of Concept - this allows connections to be interrupted, as demonstrated in the `KubernetesClient` implementation, which is where the majority of focus for this PoC has been. Changes are generally fairly minimal outside of the `RestClient` itself and `KubernetesClient` implementation, with the main areas requiring changes being those that used `#withCaCertificates()` (now replaced by a separate constructor for clients using SSL), and timeout changes (connection/read timeouts are now unified). This PR now also includes a regression test for the original issue that was encountered, ensuring the `HttpClient` solution still resolves the issue.

JamesHazelcast added 12 commits May 23, 2023 19:35

PoC implement HttpClient instead of HttpURLConnection in RestClient

f8ec59d

This is WIP and currently a first stage attempt; compiles/runs and KubernetesClient functions correctly (without an extra thread 🎉), but RestClientTest has 2 failures

Restore 1000ms shutdown await timeout in KubernetesClient

0b1407e

Correct checkResponseCode usage in RestClient

c0a94cb

Emulate HttpURLConnection behaviour for GET requests with body

7526e14

KubernetesClient: only create RestClient with SSL if caCertificate is…

688b52f

… not null

RestClient#checkResponseCode error message fix

172437a

Fix obvious merge issues

685593f

Fix missed merge issue

07fc734

Minor cleanup for KubernetesClientTest

00710a8

Fix monitor thread shutdown caveats & add regression test

48bfe4f

JamesHazelcast added Type: Enhancement Team: Core Source: Internal PR or issue was opened by an employee Not Release Notes content Module: REST labels Oct 9, 2023

JamesHazelcast added this to the 5.4.0 milestone Oct 9, 2023

JamesHazelcast requested review from kwart, vbekiaris and semihbkgr October 9, 2023 18:42

JamesHazelcast self-assigned this Oct 9, 2023

Checkstyle

60b6889

hazelcast deleted a comment from hz-devops-test Oct 9, 2023

kwart reviewed Oct 11, 2023

View reviewed changes

JamesHazelcast added 2 commits October 11, 2023 13:33

Add back separate connection/read timeouts

6ea8e09

Also returns functionality where they were previously split values.

Typo fix

47e76bc

hazelcast deleted a comment from hz-devops-test Oct 11, 2023

hasancelik requested review from SeriyBg and removed request for semihbkgr October 13, 2023 12:20

hasancelik added Module: Kubernetes Team: Cloud Native labels Oct 13, 2023

JamesHazelcast added 2 commits October 13, 2023 13:25

Fix line leftover by mistake

3c52c06

Remove "Host" header in Aws API calls

53289d4

HttpClient in J11 will raise `java.lang.IllegalArgumentException: restricted header name: "Host"` if a "Host" header is provided - there are overrides for this in J12 onwards, but not possible in J11.

JamesHazelcast requested a review from kwart October 13, 2023 12:38

SeriyBg added the run-discovery-tests Used to run discovery plugins tests label Oct 16, 2023

SeriyBg added run-discovery-tests Used to run discovery plugins tests and removed run-discovery-tests Used to run discovery plugins tests labels Oct 16, 2023

vbekiaris approved these changes Oct 17, 2023

View reviewed changes

SeriyBg added run-discovery-tests Used to run discovery plugins tests and removed run-discovery-tests Used to run discovery plugins tests labels Oct 17, 2023

Potential AWS signing fix

a285171

Trim `host` header at RestClient level, leaving it in place for all other usages during signing.

JamesHazelcast added run-discovery-tests Used to run discovery plugins tests and removed run-discovery-tests Used to run discovery plugins tests labels Oct 17, 2023

kwart approved these changes Oct 18, 2023

View reviewed changes

SeriyBg approved these changes Oct 18, 2023

View reviewed changes

JamesHazelcast merged commit 13b466c into hazelcast:master Oct 18, 2023
14 of 17 checks passed

JamesHazelcast deleted the poc/5.4/hz-2460 branch October 18, 2023 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Java 11's HttpClient within RestClient [HZ-2460] #25654

Implement Java 11's HttpClient within RestClient [HZ-2460] #25654

JamesHazelcast commented Oct 9, 2023 •

edited

JamesHazelcast commented Oct 10, 2023

JamesHazelcast commented Oct 11, 2023

kwart Oct 11, 2023

JamesHazelcast Oct 11, 2023

JamesHazelcast commented Oct 12, 2023

JamesHazelcast commented Oct 13, 2023 •

edited

hz-devops-test commented Oct 13, 2023

JamesHazelcast commented Oct 13, 2023

SeriyBg commented Oct 16, 2023

vbekiaris left a comment

SeriyBg commented Oct 17, 2023 •

edited

JamesHazelcast commented Oct 17, 2023

SeriyBg left a comment

JamesHazelcast commented Oct 18, 2023

Implement Java 11's HttpClient within RestClient [HZ-2460] #25654

Implement Java 11's HttpClient within RestClient [HZ-2460] #25654

Conversation

JamesHazelcast commented Oct 9, 2023 • edited

JamesHazelcast commented Oct 10, 2023

JamesHazelcast commented Oct 11, 2023

kwart Oct 11, 2023

Choose a reason for hiding this comment

JamesHazelcast Oct 11, 2023

Choose a reason for hiding this comment

JamesHazelcast commented Oct 12, 2023

JamesHazelcast commented Oct 13, 2023 • edited

hz-devops-test commented Oct 13, 2023

JamesHazelcast commented Oct 13, 2023

SeriyBg commented Oct 16, 2023

vbekiaris left a comment

Choose a reason for hiding this comment

SeriyBg commented Oct 17, 2023 • edited

JamesHazelcast commented Oct 17, 2023

SeriyBg left a comment

Choose a reason for hiding this comment

JamesHazelcast commented Oct 18, 2023

JamesHazelcast commented Oct 9, 2023 •

edited

JamesHazelcast commented Oct 13, 2023 •

edited

SeriyBg commented Oct 17, 2023 •

edited