KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) by cshannon · Pull Request #16754 · apache/kafka

cshannon · 2024-07-31T16:31:54Z

This commit implements the changes for KIP-1032. This updates Kafka to Jakarta specs, JavaEE 10 and Jetty 12. The changes here primarily effect Kafka Connect and MM2.

Todo/Notes:

I bumped the connect modules to JDK 17 but I also had to bump a couple other things that had a dependency on conect. The tools project depends on connect so that had to be bumped, and streams depends on tools so that needed to be bumped. This means we may need to separate some things if we don't want to enforce JDK 17 on streams.
There is an issue with a test in DedicatedMirrorIntegrationTest that I had to change for now that involves escaping characters and not quite sure what to do about it yet. The cause is the Servlet 6 spec changing what is allowed in the path. See: Jetty 12: 400: Ambiguous URI path encoding for path <%=FOO%>~1 (encoded: %3C%25%3DFOO%25%3E%7E1) jetty/jetty.project#11890
I had to configure the idle timeout in Jetty requests to match our request timeout so tests didn't fail. This was needed to fix the ConnectWorkerIntegrationTest#testPollTimeoutExpiry() test

Testing is being done by just using the existing tests for Connect and MM2 which should be sufficient.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

cshannon · 2024-07-31T16:35:47Z

@gharris1727 and @C0urante - If either (or both) of you could take a look at some point and see what you think. There's no rush but I wanted to get this PR going now that trunk has been bumped to 4.0.0-SNAPSHOT. I am going to be out of town the rest of the week but I'll be back online middle of next week.

cshannon · 2024-09-18T18:27:51Z

The checks on Java 8 and Java 11 fail because this PR requires JDK 17+

This commit implements KIP-1032 and upgrades to Jakarta, JavaEE 10 and also Jetty 12. Jetty 12 requires JDK 17 so the connect and MM2 runtimes have been bumped to JDK 17. See KIP-1032

chia7712 · 2024-11-26T15:31:40Z

@cshannon Could you please rebase code to include #17522?

cshannon · 2024-11-26T16:18:13Z

@chia7712 - I merged the latest trunk and fixed the conflicts so this PR is now up to date

mumrah

Thanks for the patch @cshannon. Overall, I think this change looks good. A few small things inline.

For big PRs like this, we rely heavily on our tests to ensure we didn't break anything. Can you run the test suite on this PR a few times just to see if anything shakes out? You can trigger a new build by pushing an empty commit (git commit --allow-empty)

cshannon · 2024-12-05T22:13:06Z

@mumrah - Thanks for the review, I pushed an update and I believe I addressed all 3 of your comments (as well as merging in the latest from trunk) and that kicked off a new build. I can also run tests a couple more times with an empty commit after the tests finish.

mumrah

I re-ran the tests three times, looks good. I'm approving this now, but will wait a few days before merging to see if @gharris1727 or @C0urante (or @chia7712) have anything to add.

cshannon · 2024-12-10T00:27:40Z

I re-ran the tests three times, looks good. I'm approving this now, but will wait a few days before merging to see if @gharris1727 or @C0urante (or @chia7712) have anything to add.

@mumrah - Sounds good, thanks for the review.

I forgot to mention that I looked more into what to do about having to change the DedicatedMirrorIntegrationTest to prevent a failure because of the Servlet 6 and Jakarta EE10 spec change that no longer allows ambiguous characters anymore in the URL path.

If we wanted to restore the previous behavior to allow it, we could set the UriComplianceMode to be LEGACY. See this comment: jetty/jetty.project#11890 (comment)

However, I would say it's better to not allow it as the spec now says it's a violation so I think the current change I made is fine, but I guess it depends if there's use cases to allow it.

Edit: Here is the relevant part of the spec https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0.html#uri-path-canonicalization

chia7712

@cshannon thanks for this patch. a couple of comments are left. PTAL

chia7712 · 2024-12-10T07:39:22Z

                }
                String forwardUrl = uriBuilder.build().toString();
                log.debug("Forwarding request {} {} {}", forwardUrl, method, body);
+                // TODO, we may need to set the reqest timeout as Idle timeout on the HttpClient


typo: reqest -> request

This comment is valid to me, as the default idle timeout for HttpClient is 30 seconds, and the default request timeout is 90 seconds.

If we want to configure the idle timeout I guess the question is the best way to do so. I think we'd either need to pass it as an argument to the request() methods or add it to the AbstractConfig for RestClient so it has access to it when constructing the HttpClient. I guess there's also the question if it should always match the request timeout or it could be configured as a separate config. Maybe this could be done as a follow on task after this is merged.

gharris1727

Thanks @cshannon for this change, thanks so much for getting this completed and working through the merge conflicts with all of the rest of the activity on trunk!

My comments are non-blocking, i'm happy to merge this as-is, but i'll let you address any other comments and merge tomorrow before feature freeze.

gharris1727 · 2024-12-10T23:04:54Z

+            // TODO: Jetty 12 throws a 400 amiguous error with the previous test string here
+            // It seems like at least some encoded characters are not allowed anymore based on
+            // https://github.com/jetty/jetty.project/issues/11890#issuecomment-2156442947
+            // See https://github.com/jetty/jetty.project/issues/11890
+            //final String b = "B- ._~:/?#[]@!$&'()*+;=\"<>%{}|\\^`618";
+            final String b = "B";


Could you keep most of these suspicious characters, and just remove the ones which are ambiguous?
From reading the linked issue, it might just be the / character, and the remainder are still valid.

It looks like this test was sensitive enough to find the underlying change in the library, so I'd like to keep it sensitive in case something like this happens again.

So there's actually a few characters that are not allowed, control characters, backslash, etc. I went ahead and removed a few problem characters so it works now. I also updated the comments for the section. You can see the exact rules in the Rejecting Suspicious Sequences section as part of https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#uri-path-canonicalization

cshannon · 2024-12-11T00:02:07Z

@gharris1727 and @chia7712 - I think I addressed all the comments so this can be merged. The only outstanding one was if we wanted to set the idle timeout on the HttpClient to match the same thing as the request timeout (or even make it configurable separately) but I am still not quite sure if we should and I think that might be better as a follow on discussion and don't think it's a blocker. I can work on a PR for that depending on what we wanted to do (if anything)

cshannon · 2024-12-11T14:47:58Z

It seems the JDK 23 build is timing out but I'm not sure why, the issue could be unrelated to the tests themselves. The build is currently running again (and appears hung but I can't see why until it finishes it says). I pulled the logs from yesterday to figure out why it was hung and it shows:

2024-12-11T02:27:43.9906670Z Gradle Test Run :streams:integration-tests:test > Gradle Test Executor 183 > VersionedKeyValueStoreIntegrationTest > shouldManualUpgradeFromNonVersionedTimestampedToVersioned() STARTED
2024-12-11T02:28:31.2902594Z 
2024-12-11T02:28:31.2904162Z Gradle Test Run :streams:integration-tests:test > Gradle Test Executor 183 > VersionedKeyValueStoreIntegrationTest > shouldManualUpgradeFromNonVersionedTimestampedToVersioned() PASSED
2024-12-11T02:28:33.2903866Z Copy JUnit XML for integration-tests to /home/runner/work/kafka/kafka/build/junit-xml/streams/integration-tests/test
2024-12-11T03:05:56.8106484Z Timed out after 175 minutes. Dumping threads now...

That output shows it finishes tests and just does nothing for 37 minutes when trying to copy the XML files and then times out

chia7712 · 2024-12-11T14:59:51Z

@cshannon There is a flaky test causing a timeout, and we are going to disable it temporarily. See #18138.

chia7712 · 2024-12-13T22:44:41Z

This pull request causes a build issue. I have opened KAFKA-18235 to address and fix it.

This commit implements the changes for KIP-1032. This updates Kafka to Jakarta specs, JavaEE 10 and Jetty 12. The changes here primarily effect Kafka Connect and MM2. Todo/Notes: 1) I bumped the connect modules to JDK 17 but I also had to bump a couple other things that had a dependency on conect. The tools project depends on connect so that had to be bumped, and streams depends on tools so that needed to be bumped. This means we may need to separate some things if we don't want to enforce JDK 17 on streams. 2) There is an issue with a test in DedicatedMirrorIntegrationTest that I had to change for now that involves escaping characters and not quite sure what to do about it yet. The cause is the Servlet 6 spec changing what is allowed in the path. See: Jetty 12: 400: Ambiguous URI path encoding for path <%=FOO%>~1 (encoded: %3C%25%3DFOO%25%3E%7E1) jetty/jetty.project#11890 3) I had to configure the idle timeout in Jetty requests to match our request timeout so tests didn't fail. This was needed to fix the ConnectWorkerIntegrationTest#testPollTimeoutExpiry() test Testing is being done by just using the existing tests for Connect and MM2 which should be sufficient. Reviewers: Greg Harris <greg.harris@aiven.io>, David Arthur <mumrah@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>

github-actions bot added tools connect mirror-maker-2 build Gradle build or GitHub Actions labels Oct 16, 2024

cshannon force-pushed the KAFKA-16437 branch from 51646e9 to 88524c4 Compare October 16, 2024 14:44

KAFKA-16437 - Upgrade to Jakarta and Jetty 12.x

5a191a0

This commit implements KIP-1032 and upgrades to Jakarta, JavaEE 10 and also Jetty 12. Jetty 12 requires JDK 17 so the connect and MM2 runtimes have been bumped to JDK 17. See KIP-1032

cshannon force-pushed the KAFKA-16437 branch from 88524c4 to 5a191a0 Compare October 16, 2024 14:52

cshannon marked this pull request as ready for review October 16, 2024 14:56

cshannon added 2 commits October 16, 2024 14:03

Fix SSLUtilsTest

96d2eac

Merge branch 'trunk' into KAFKA-16437

fafc05c

gharris1727 reviewed Oct 22, 2024

View reviewed changes

Comment thread build.gradle Outdated

cshannon added 2 commits October 22, 2024 19:27

Merge branch 'trunk' into KAFKA-16437

fa0910c

Merge branch 'trunk' into KAFKA-16437

d108f99

Merge branch 'trunk' into KAFKA-16437

ed76855

cshannon added 2 commits December 4, 2024 18:58

Merge branch 'trunk' into KAFKA-16437

acad443

Bump jetty to 12.0.15

ab01616

mumrah reviewed Dec 5, 2024

View reviewed changes

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated

Comment thread gradle/spotbugs-exclude.xml Outdated

Comment thread gradle/spotbugs-exclude.xml Outdated

cshannon added 2 commits December 5, 2024 17:09

Address PR comments

b3f5c11

Merge branch 'trunk' into KAFKA-16437

d35bec7

cshannon added 2 commits December 9, 2024 08:22

empty commit to run tests

2cc3897

Merge branch 'trunk' into KAFKA-16437

d2688d6

mumrah approved these changes Dec 10, 2024

View reviewed changes

jamie-albert mentioned this pull request Dec 10, 2024

kafka-3.8/3.9 GHSA-qh8g-58pp-2wxh advisory update wolfi-dev/advisories#9371

Merged

chia7712 reviewed Dec 10, 2024

View reviewed changes

gharris1727 approved these changes Dec 10, 2024

View reviewed changes

gharris1727 mentioned this pull request Dec 10, 2024

Add PR triage workflow #17881

Merged

address comments

2652325

github-actions bot added the dependencies Pull requests that update a dependency file label Dec 10, 2024

empty commit to re-run tests

4a9aff5

chia7712 approved these changes Dec 11, 2024

View reviewed changes

chia7712 merged commit bd6d0fb into apache:trunk Dec 11, 2024

Conversation

cshannon commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

cshannon commented Jul 31, 2024

Uh oh!

cshannon commented Sep 18, 2024

Uh oh!

Uh oh!

chia7712 commented Nov 26, 2024

Uh oh!

cshannon commented Nov 26, 2024

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cshannon commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

cshannon commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chia7712 Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

cshannon Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

gharris1727 left a comment

Choose a reason for hiding this comment

Uh oh!

gharris1727 Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

cshannon Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cshannon commented Dec 11, 2024

Uh oh!

cshannon commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chia7712 commented Dec 11, 2024

Uh oh!

chia7712 commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cshannon commented Jul 31, 2024 •

edited

Loading

cshannon commented Dec 5, 2024 •

edited

Loading

cshannon commented Dec 10, 2024 •

edited

Loading

cshannon commented Dec 11, 2024 •

edited

Loading