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

Coordinated Vert.x 4.5.11 upgrades #44515

Merged
merged 1 commit into from
Nov 17, 2024
Merged

Coordinated Vert.x 4.5.11 upgrades #44515

merged 1 commit into from
Nov 17, 2024
+43 −29

Conversation

jponge
Copy link
Member

@jponge jponge commented Nov 14, 2024 •
edited
Loading

Upgrades to:

  • Vert.x 4.5.11
  • Vert.x Mutiny bindings 3.1.6.0
  • Netty 4.1.115.Final

Fixes CVE-2024-47535 with Netty 4.1.115.Final

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/netty labels Nov 14, 2024
@jponge jponge mentioned this pull request Nov 14, 2024
Closed
cescoffier
cescoffier approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM!
Should we "undraft" it to run the full CI?

@jponge
Copy link
Member Author

jponge commented Nov 14, 2024

I just want to have a look at eclipse-vertx/vert.x#5387

@cescoffier
Copy link
Member

@mkouba could you help @jponge with eclipse-vertx/vert.x#5387? We may also need to check Quarkus HTTP (Jakarta WebSocket)

@jponge
Copy link
Member Author

jponge commented Nov 14, 2024

That being said the old API is still there but deprecated, so that might be another PR (at least @mkouba is aware that there's a change here)

@jponge jponge marked this pull request as ready for review November 14, 2024 17:32
@jponge jponge requested review from mkouba and geoand November 14, 2024 17:32
@jponge
Copy link
Member Author

jponge commented Nov 14, 2024

Looks like we have some failures, we'll see with the summary report what's to be investigated

@jponge
Copy link
Member Author

jponge commented Nov 14, 2024

The issues are SSL-related, see ./mvnw verify -f integration-tests/vertx-http -Pnative:

[ERROR] Tests run: 3, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 2.793 s <<< FAILURE! -- in io.quarkus.it.vertx.Http2TestCaseIT
[ERROR] io.quarkus.it.vertx.Http2TestCaseIT.testHttp2EnabledSsl -- Time elapsed: 0.112 s <<< ERROR!
java.util.concurrent.ExecutionException: io.vertx.core.VertxException: Connection was closed
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2073)
	at io.quarkus.it.vertx.Http2TestCase.runTest(Http2TestCase.java:108)
	at io.quarkus.it.vertx.Http2TestCase.runHttp2EnabledSsl(Http2TestCase.java:72)
	at io.quarkus.it.vertx.Http2TestCase.testHttp2EnabledSsl(Http2TestCase.java:42)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:805)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
Caused by: io.vertx.core.VertxException: Connection was closed

and:

[ERROR] Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 2.119 s <<< FAILURE! -- in io.quarkus.it.vertx.VertxProducerResourceIT
[ERROR] io.quarkus.it.vertx.VertxProducerResourceIT.testRouteRegistrationMTLS -- Time elapsed: 0.203 s <<< ERROR!
java.net.SocketException: Connection reset
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:318)
	at java.base/sun.nio.ch.NioSocketImpl.read(NioSocketImpl.java:346)
	at java.base/sun.nio.ch.NioSocketImpl$1.read(NioSocketImpl.java:796)
	at java.base/java.net.Socket$SocketInputStream.implRead(Socket.java:1108)
	at java.base/java.net.Socket$SocketInputStream.read(Socket.java:1095)
	at java.base/sun.security.ssl.SSLSocketInputRecord.read(SSLSocketInputRecord.java:489)
	at java.base/sun.security.ssl.SSLSocketInputRecord.readHeader(SSLSocketInputRecord.java:483)
	at java.base/sun.security.ssl.SSLSocketInputRecord.bytesInCompletePacket(SSLSocketInputRecord.java:70)
	at java.base/sun.security.ssl.SSLSocketImpl.readApplicationRecord(SSLSocketImpl.java:1462)
	at java.base/sun.security.ssl.SSLSocketImpl$AppInputStream.read(SSLSocketImpl.java:1068)
	at org.apache.http.impl.io.AbstractSessionInputBuffer.fillBuffer(AbstractSessionInputBuffer.java:161)
	at org.apache.http.impl.io.SocketInputBuffer.fillBuffer(SocketInputBuffer.java:82)
	at org.apache.http.impl.io.AbstractSessionInputBuffer.readLine(AbstractSessionInputBuffer.jav

(--- snip ----)

@gastaldi gastaldi mentioned this pull request Nov 14, 2024
Closed
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor

mkouba commented Nov 15, 2024

@mkouba could you help @jponge with eclipse-vertx/vert.x#5387? We may also need to check Quarkus HTTP (Jakarta WebSocket)

That being said the old API is still there but deprecated, so that might be another PR (at least @mkouba is aware that there's a change here)

AFAIK we don't use the old API at all. Basically, the endpoint handler does not attempt to perform the upgrade if an HttpUpgradeCheck does not permit to do so.

@mkouba
Copy link
Contributor

mkouba commented Nov 15, 2024

FTR the io.quarkus.extest.OverrideBuildTimeConfigTes fails with:

2024-11-15T02:09:25.5750882Z Caused by: java.util.NoSuchElementException: SRCFG00027: Could not find a mapping for io.quarkus.runtime.ConfigConfig
2024-11-15T02:09:25.5751959Z 	at io.smallrye.config.SmallRyeConfig.getConfigMapping(SmallRyeConfig.java:631)
2024-11-15T02:09:25.5752713Z 	at io.smallrye.config.SmallRyeConfig.getConfigMapping(SmallRyeConfig.java:621)
2024-11-15T02:09:25.5753553Z 	at io.quarkus.runtime.configuration.ConfigRecorder.handleConfigChange(ConfigRecorder.java:63)
2024-11-15T02:09:25.5754601Z 	at io.quarkus.deployment.steps.ConfigGenerationBuildStep$checkForBuildTimeConfigChange1532146938.deploy_8(Unknown Source)
2024-11-15T02:09:25.5755747Z 	at io.quarkus.deployment.steps.ConfigGenerationBuildStep$checkForBuildTimeConfigChange1532146938.deploy(Unknown Source)

CC @radcortez @gsmet

@cescoffier
Copy link
Member

I'm a bit worried about the SSL IT issues. It can come from a change in Netty (we know that that code changed) or the PR from Franz changing the allocator when using SSL (the PR should have been super defensive, but never know)

@cescoffier
Copy link
Member

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector is unlikely related. @ozangunalp is this a flaky test?

@jponge
Copy link
Member Author

jponge commented Nov 15, 2024 •
edited
Loading

@mkouba I've tried running ./mvnw verify -f integration-tests/test-extension across Java 22 / 21 / 17 locally and could indeed reproduce the failure on 21. In my case I got the following failure:

[INFO] --- surefire:3.5.0:test (default-test) @ quarkus-integration-test-test-extension-tests ---
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO]
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[ERROR] projects/project-using-test-template-from-extension-processed/target/test-classes/org/acme/TemplatedNormalTest (wrong name: org/acme/TemplatedNormalTest)

I'm not sure this relates to the changes in this PR, it might be a different issue IMHO.

@jponge
Copy link
Member Author

jponge commented Nov 15, 2024

Edit: it failed also with 17

@jponge
Copy link
Member Author

jponge commented Nov 15, 2024

/cc @franz1981

@franz1981
Copy link
Contributor

the PR from Franz changing the allocator when using SSL

It was passing the tests on Vertx, which by default was still using the default approach, so...it should be fine(tm).

I can take an additional look If you're blocked @jponge ?

@cescoffier
Copy link
Member

IF we can be sure if comes from that, we would need to revert it and wait for another Vert.x release.

cescoffier
cescoffier requested changes Nov 15, 2024
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

Some Test failures are related.

@ozangunalp
Copy link
Contributor

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector is unlikely related. @ozangunalp is this a flaky test?

Yes it is. I'd need to rewrite those tests when I find some time.

@franz1981
Copy link
Contributor

@cescoffier that's the lucky scenario, actually, but if is coming from Netty changes on key SSL classes - that's troublesome

@cescoffier
Copy link
Member

Trying to reproduce it on my machine.

@jponge
Copy link
Member Author

jponge commented Nov 15, 2024

Let's the new run on 17, and then I will need to edit the commits because it's not clean

@radcortez
Copy link
Member

FTR the io.quarkus.extest.OverrideBuildTimeConfigTes fails with:

Strange. I did change that very recently in #44079, which was merged yesterday, but the CI was green. I've just checked out the PR and run it locally, and it also passes.

Let me try to investigate it further.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@jponge
Copy link
Member Author

jponge commented Nov 15, 2024

(let me redo my commits)

@jponge
Copy link
Member Author

jponge commented Nov 15, 2024

Here's a single commit 🚀

@jponge
Coordinated Vert.x 4.5.11 upgrades
9fd8dcb 
- Bump to Netty 4.1.115.Final and fix SSL-related substitutions due to internal Netty breaking changes
- Bump to Vert.x 4.5.11
- Bump Mutiny Vert.x bindings 3.16.0
- Re-aligned the Vert.x versions across Quarkus modules

Fixes CVE-2024-47535 with Netty 4.1.115.Final
@jponge
Copy link
Member Author

jponge commented Nov 15, 2024

@cescoffier pending another CI run completes, do you still want to hold on?

@cescoffier
Copy link
Member

Let's wait for that run to complete.

@radcortez radcortez mentioned this pull request Nov 15, 2024
Merged
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 15, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9fd8dcb.

✅ 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

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging/deployment

✖ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

📦 integration-tests/opentelemetry-vertx-exporter

✖ io.quarkus.it.opentelemetry.vertx.exporter.grpc.SimpleGrpcNoTLSNoCompressionTest.test - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.opentelemetry.vertx.exporter.AbstractExporterTest Expecting Optional to contain a value but it was empty within 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.it.opentelemetry.vertx.exporter.AbstractExporterTest 
Expecting Optional to contain a value but it was empty within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)

⚙️ JVM Tests - JDK 21

📦 extensions/panache/hibernate-reactive-rest-data-panache/deployment

✖ io.quarkus.hibernate.reactive.rest.data.panache.deployment.repository.PanacheRepositoryResourcePutMethodTest.shouldUpdateComplexObject - History

  • 1 expectation failed. JSON path name doesn't match. Expected: is "updated collection" Actual: empty collection - java.lang.AssertionError
java.lang.AssertionError: 
1 expectation failed.
JSON path name doesn't match.
Expected: is "updated collection"
  Actual: empty collection

	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)

cescoffier
cescoffier approved these changes Nov 17, 2024
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

We are good to go. I would wait a bit before backporting
(even if it will need backport)

@cescoffier cescoffier merged commit e44e07e into quarkusio:main Nov 17, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Nov 17, 2024
@miguelborges99
Copy link

Will this be applied also in the LTS version?

@geoand
Copy link
Contributor

geoand commented Nov 17, 2024 •
edited
Loading

That's what the backport-3.15 label is for 😉.

But as Clement said, we need some bake time to ensure it doesn't cause any problems

@cescoffier
Copy link
Member

As I said in my last comment - yes, we want to backport it, but not immediately. There are some changes in Netty that need longer testing.

@franz1981
Copy link
Contributor

Yep, i Will send the PRs (or delegate) next week

@radcortez radcortez mentioned this pull request Nov 18, 2024
Merged
@gsmet gsmet modified the milestones: 3.18 - main, 3.16.4 Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

@mkouba mkouba mkouba approved these changes

@cescoffier cescoffier cescoffier approved these changes

Assignees
No one assigned
Labels
area/dependencies Pull requests that update a dependency file area/native-image area/netty area/vertx triage/backport-3.15 triage/flaky-test
Projects
None yet
Milestone
3.16.4
Development

Successfully merging this pull request may close these issues.
9 participants
@jponge @cescoffier @mkouba @franz1981 @ozangunalp @radcortez @miguelborges99 @geoand @gsmet