[JENKINS-73130] Upgrade core from Jetty 10.x to 12.x (EE 8) #9590

basil · 2024-08-08T21:33:55Z

Context

See JENKINS-73130 and its linked epic.

Problem

January 1, 2024 marked the end of community support for the Jetty 10 and 11 releases. Jenkins currently delivers Jetty 10.

Solution

Upgrade to Jetty 12 (EE 8).

This release does not have any significant compatibility implications for plugins—the javax Servlet API packages remain in use. A small number of plugins rely on Jetty implementation details in their test suites; these test suites must be adapted to the new Jetty 12 APIs, but the production code does not. The main API differences between Jetty 10 and Jetty 12 (EE 8) are that the Jetty codebase has been turned into a modular architecture that can support multiple Servlet API versions. Previous versions of Jetty were tied to a particular version of the Servlet API.

Implementation

In general, replace dependencies with EE 8 versions in pom.xml and update all relevant imports. More notes are left in the self-review below.

Testing done

Full PCT/ATH

Future work

A forthcoming PR will migrate from EE 8 to EE 9.

Proposed changelog entries

Upgrade Jetty from 10.0.22 to 12.0.12.

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.
Options

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options

Co-authored-by: Olivier Lamy <[email protected]>

basil · 2024-08-08T21:35:03Z

.github/renovate.json

-      ],
-      "enabled": false,
-      "matchPackageNames": [
+        "jakarta.servlet:jakarta.servlet-api",
        "jakarta.servlet.jsp.jstl:jakarta.servlet.jsp.jstl-api"


This exclusion remains valid, but not for the reason currently given. The exclusion remains valid for the reason given in the previous exclusion for jakarta.servlet:jakarta.servlet-api. Indeed, that reason applies to both jakarta.servlet:jakarta.servlet-api and jakarta.servlet.jsp.jstl:jakarta.servlet.jsp.jstl-api, so move these exclusions to the same section.

basil · 2024-08-08T21:35:51Z

.idea/encodings.xml

+    <file url="file://$PROJECT_DIR$/websocket/jetty12-ee8/src/filter/resources" charset="UTF-8" />
+    <file url="file://$PROJECT_DIR$/websocket/jetty12-ee8/src/main/java" charset="UTF-8" />
+    <file url="file://$PROJECT_DIR$/websocket/jetty12-ee8/src/main/resources" charset="UTF-8" />


Add a new Jetty 12 (EE 8) WebSocket implementation. Both the Jetty 10 and Jetty 12 (EE 8) implementations will be dropped in a forthcoming PR when we add support for Jetty 12 (EE 9).

basil · 2024-08-08T21:36:01Z

bom/pom.xml

@@ -40,7 +40,8 @@ THE SOFTWARE.
  <properties>
    <commons-fileupload2.version>2.0.0-M2</commons-fileupload2.version>
    <slf4jVersion>2.0.15</slf4jVersion>
-    <stapler.version>1892.v73465f3d074d</stapler.version>
+    <!-- TODO JENKINS-73122 https://github.com/jenkinsci/stapler/pull/537 -->


jenkinsci/stapler#537

basil · 2024-08-08T21:36:10Z

pom.xml

@@ -97,7 +98,8 @@ THE SOFTWARE.
    <bridge-method-injector.version>1.29</bridge-method-injector.version>
    <spotless.check.skip>false</spotless.check.skip>
    <!-- Make sure to keep the jetty-maven-plugin version in war/pom.xml in sync with the Jetty release in Winstone: -->
-    <winstone.version>6.21</winstone.version>
+    <!-- TODO JENKINS-73126 https://github.com/jenkinsci/winstone/pull/383 -->
+    <winstone.version>7.0-rc921.b_dc6422f61b_6</winstone.version>


jenkinsci/winstone#383

basil · 2024-08-08T21:36:38Z

test/pom.xml

@@ -149,7 +149,7 @@ THE SOFTWARE.
    <dependency>
      <groupId>${project.groupId}</groupId>
      <artifactId>jenkins-test-harness</artifactId>
-      <version>2225.2230.v6210cb_b_827f9</version>
+      <version>2250.v03a_1295b_0a_30</version>


The latest version of Jenkins Test Harness (JTH), which requires Java 17 and is based on Jetty 12, using reflection to choose between EE 8 and EE 9.

basil · 2024-08-08T21:41:00Z

test/src/test/java/jenkins/install/SetupWizardTest.java

@@ -336,7 +338,7 @@ protected JSONSignatureValidator getJsonSignatureValidator(String name) {
        }
    }

-    private static class RemoteUpdateSiteHandler extends AbstractHandler {
+    private static class RemoteUpdateSiteHandler extends Handler.Abstract {


basil · 2024-08-08T21:41:38Z

test/src/test/java/jenkins/security/Security3030Test.java

+                // The client might still be sending us more of the request, but we have had enough of it already and
+                // have decided to stop processing it. Drain the read end of the socket so that the client can finish
+                // sending its request in order to read the response we are about to provide.
+                try (OutputStream os = OutputStream.nullOutputStream()) {
+                    req.getInputStream().transferTo(os);
+                }


See JENKINS-73127 for an explanation of this change.

basil · 2024-08-08T21:41:49Z

test/src/test/java/jenkins/util/SystemPropertiesTest.java

@@ -103,7 +103,7 @@ public void shouldReturnWebAppPropertyIfSystemPropertyNotSetAndDefaultIsSet() th
     * @param value value of the property
     */
    protected void setWebAppInitParameter(String property, String value) {
-        Assume.assumeThat(j.jenkins.servletContext, Matchers.instanceOf(ContextHandler.Context.class));
-        ((ContextHandler.Context) j.jenkins.servletContext).getContextHandler().getInitParams().put(property, value);
+        Assume.assumeThat(j.jenkins.servletContext, Matchers.instanceOf(WebAppContext.Context.class));


Adapting to new Jetty 12 APIs as per the migration guide.

basil · 2024-08-08T21:44:23Z

websocket/jetty12-ee8/pom.xml

@@ -0,0 +1,85 @@
+<?xml version="1.0" encoding="UTF-8"?>


To ease reviewing, a diff from Jetty 10:

The MIT License -Copyright (c) 2019, CloudBees, Inc. +Copyright (c) 2019, 2024 CloudBees, Inc. Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal @@ -32,9 +32,9 @@ THE SOFTWARE. <relativePath>../..</relativePath> </parent> - <artifactId>websocket-jetty10</artifactId> - <name>Jetty 10 implementation for WebSocket</name> - <description>An implementation of the WebSocket handler that works with Jetty 10.</description> + <artifactId>websocket-jetty12-ee8</artifactId> + <name>Jetty 12 (EE 8) implementation for WebSocket</name> + <description>An implementation of the WebSocket handler that works with Jetty 12 (EE 8).</description> <dependencyManagement> <dependencies> @@ -52,7 +52,7 @@ THE SOFTWARE. <dependency> <groupId>org.jenkins-ci</groupId> <artifactId>winstone</artifactId> - <version>6.21</version> + <version>${winstone.version}</version> <optional>true</optional> </dependency> <dependency>

basil · 2024-08-08T21:45:15Z

websocket/jetty12-ee8/src/main/java/jenkins/websocket/Jetty12EE8Provider.java

@@ -0,0 +1,178 @@
+/*


To ease reviewing, a diff from Jetty 10:

diff --git a/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java b/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java index b5243e627f..cc006b180f 100644 --- a/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java +++ b/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java @@ -1,7 +1,7 @@ * The MIT License * - * Copyright 2022 CloudBees, Inc. + * Copyright 2022, 2024 CloudBees, Inc. * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -31,35 +31,35 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.Future; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import org.eclipse.jetty.websocket.api.Session; -import org.eclipse.jetty.websocket.api.WebSocketListener; -import org.eclipse.jetty.websocket.api.WriteCallback; -import org.eclipse.jetty.websocket.server.JettyServerUpgradeRequest; -import org.eclipse.jetty.websocket.server.JettyServerUpgradeResponse; -import org.eclipse.jetty.websocket.server.JettyWebSocketServerContainer; +import org.eclipse.jetty.ee8.websocket.api.Session; +import org.eclipse.jetty.ee8.websocket.api.WebSocketListener; +import org.eclipse.jetty.ee8.websocket.api.WriteCallback; +import org.eclipse.jetty.ee8.websocket.server.JettyServerUpgradeRequest; +import org.eclipse.jetty.ee8.websocket.server.JettyServerUpgradeResponse; +import org.eclipse.jetty.ee8.websocket.server.JettyWebSocketServerContainer; import org.kohsuke.MetaInfServices; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @Restricted(NoExternalUse.class) @MetaInfServices(Provider.class) -public class Jetty10Provider implements Provider { +public class Jetty12EE8Provider implements Provider { /** * Number of seconds a WebsocketConnection may stay idle until it expires. * Zero to disable. * This value must be higher than the <code>jenkins.websocket.pingInterval</code>. - * Per <a href=https://www.eclipse.org/jetty/documentation/jetty-10/programming-guide/index.html#pg-websocket-session-ping>Jetty 10 documentation</a> + * Per <a href=https://eclipse.dev/jetty/documentation/jetty-12/programming-guide/index.html#pg-websocket-session-ping>Jetty 12 documentation</a> * a ping mechanism should keep the websocket active. Therefore, the idle timeout must be higher than the ping * interval to avoid timeout issues. */ private static long IDLE_TIMEOUT_SECONDS = Long.getLong("jenkins.websocket.idleTimeout", 60L); - private static final String ATTR_LISTENER = Jetty10Provider.class.getName() + ".listener"; + private static final String ATTR_LISTENER = Jetty12EE8Provider.class.getName() + ".listener"; private boolean initialized = false; - public Jetty10Provider() { + public Jetty12EE8Provider() { JettyWebSocketServerContainer.class.hashCode(); } @@ -74,12 +74,12 @@ public class Jetty10Provider implements Provider { public Handler handle(HttpServletRequest req, HttpServletResponse rsp, Listener listener) throws Exception { init(req); req.setAttribute(ATTR_LISTENER, listener); - // TODO Jetty 10 has no obvious equivalent to WebSocketServerFactory.isUpgradeRequest; RFC6455Negotiation? + // TODO Jetty 10+ has no obvious equivalent to WebSocketServerFactory.isUpgradeRequest; RFC6455Negotiation? if (!"websocket".equalsIgnoreCase(req.getHeader("Upgrade"))) { rsp.sendError(HttpServletResponse.SC_BAD_REQUEST, "only WS connections accepted here"); return null; } - if (!JettyWebSocketServerContainer.getContainer(req.getServletContext()).upgrade(Jetty10Provider::createWebSocket, req, rsp)) { + if (!JettyWebSocketServerContainer.getContainer(req.getServletContext()).upgrade(Jetty12EE8Provider::createWebSocket, req, rsp)) { rsp.sendError(HttpServletResponse.SC_BAD_REQUEST, "did not manage to upgrade"); return null; } @@ -175,5 +175,4 @@ public class Jetty10Provider implements Provider { } }; } - }

basil · 2024-08-08T21:49:15Z

war/pom.xml

+              <config implementation="org.eclipse.jetty.maven.MavenResource">
+                <resourceAsString>${basedir}/src/realm.properties</resourceAsString>
+              </config>


https://jetty.org/docs/jetty/12/programming-guide/maven-jetty/jetty-maven-plugin.html#configuring-security-settings

basil · 2024-08-08T21:49:40Z

war/pom.xml

        <configuration>
          <!--
            Reload webapp when you hit ENTER. (See JETTY-282 for more)
          -->
-          <reload>manual</reload>
+          <scan>0</scan>


https://jetty.org/docs/jetty/12/programming-guide/maven-jetty/jetty-maven-plugin.html#configuration

MarkEWaite

Changes look good to me.

I see that the websocket/jetty10 module is retained in the pom.xml at the root of the repository. Is that needed for API compatibility so that the jenkins.websocket.Jetty10Provider class is not removed from the Jenkins API or is there something else that I'm not understanding about the Jetty 10 websocket provider being built and packaged in the Jenkins war file along with the Jetty 12 websocket provider?

basil · 2024-08-09T04:36:44Z

I see that the websocket/jetty10 module is retained in the pom.xml at the root of the repository. Is that needed for API compatibility so that the jenkins.websocket.Jetty10Provider class is not removed from the Jenkins API or is there something else that I'm not understanding about the Jetty 10 websocket provider being built and packaged in the Jenkins war file along with the Jetty 12 websocket provider?

Historically we have retained older WebSocket Jetty modules in core to allow for older test harnesses (with older Jetty versions) to be used with newer cores, and this PR follows past precedent. In any case this state of affairs will only last for a few weeks, as we'll be removing both the Jetty 10 and Jetty 12 EE 8 WebSocket modules when adding support for Jetty 12 EE 9 in core. I haven't made any attempt to get a Jetty 12 EE 9 core to work with an older test harness. Instead I have updated PCT to dynamically upgrade the test harness to one that supports Jetty 12 EE 8 and EE 9.

BorisYaoA

LGTM

MarkEWaite · 2024-08-09T11:30:37Z

Historically we have retained older WebSocket Jetty modules in core to allow for older test harnesses (with older Jetty versions) to be used with newer cores, and this PR follows past precedent. In any case this state of affairs will only last for a few weeks, as we'll be removing both the Jetty 10 and Jetty 12 EE 8 WebSocket modules when adding support for Jetty 12 EE 9 in core. I haven't made any attempt to get a Jetty 12 EE 9 core to work with an older test harness. Instead I have updated PCT to dynamically upgrade the test harness to one that supports Jetty 12 EE 8 and EE 9.

Thanks! I think you had even said that in your earlier comments on the change. I must have missed that earlier comment when I reviewed the code.

alecharp

Once we have release stapler, winstone and JTH we can update the versions in the poms here.

…html#handler-response-content-apis

github-actions · 2024-08-10T18:21:00Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

Kevin-CB

Security wise, it looks good to me!

MarkEWaite · 2024-08-12T12:42:43Z

This PR is ready for merge once the versions are updated for the upcoming winstone and stapler releases:

basil · 2024-08-12T14:41:21Z

Thanks all! I am merging this to do one final set of tests before it is released tomorrow.

daniel-beck · 2024-08-12T15:39:50Z

I'm trying to mvn -pl war jetty:run and it doesn't work in 121c0ae (but does in d176756).

HTTP ERROR 500 javax.servlet.ServletException: org.apache.commons.jelly.JellyTagException: file:/…/core/src/main/resources/lib/layout/icon.jelly:71:93: <j:invokeStatic> method getSymbol threw exception: org/apache/xml/serializer/OutputPropertiesFactory

Caused by: java.lang.ClassNotFoundException: org.apache.xml.serializer.OutputPropertiesFactory
    at org.apache.xalan.templates.OutputProperties.<init> (OutputProperties.java:84)
    at org.apache.xalan.transformer.TransformerIdentityImpl.<init> (TransformerIdentityImpl.java:93)
    at org.apache.xalan.processor.TransformerFactoryImpl.newTransformer (TransformerFactoryImpl.java:818)
    at org.jenkins.ui.symbol.Symbol.loadSymbol (Symbol.java:129)

Is this happening for anyone else?

MarkEWaite · 2024-08-12T16:06:43Z

Is this happening for anyone else?

I'm seeing the same thing when I run with mvn -pl war jetty:run after first running mvn -am -pl war,bom -Pquick-build clean install

I don't see that when I run with:

JENKINS_HOME=war/work java -jar war/target/jenkins.war

basil · 2024-08-12T16:29:05Z

Is this happening for anyone else?

It is happening for me as well.

daniel-beck · 2024-08-12T18:00:49Z

@basil Is this easily followed up with a fix, or should we revert the PR? Any idea?

basil · 2024-08-12T18:48:26Z

@daniel-beck Not sure offhand. Have you done anything to debug the issue?

MarkEWaite · 2024-08-12T18:51:44Z

My attempts in the debugger have not shown me anything yet, but I'll continue after lunch to see if I can discover something

[JENKINS-73130] Upgrade core from Jetty 10.x to 12.x (EE 8)

778f2ed

Co-authored-by: Olivier Lamy <[email protected]>

basil added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted ath-successful This PR has successfully passed the full acceptance-test-harness suite pct-successful This PR has successfully passed the full plugin-compatibility-test suite labels Aug 8, 2024

basil commented Aug 8, 2024

View reviewed changes

basil added the needs-security-review Awaiting review by a security team member label Aug 8, 2024

basil requested a review from a team August 8, 2024 21:45

basil commented Aug 8, 2024

View reviewed changes

basil requested review from MarkEWaite, alecharp and BorisYaoA August 8, 2024 21:53

MarkEWaite approved these changes Aug 9, 2024

View reviewed changes

daniel-beck requested review from daniel-beck and Kevin-CB August 9, 2024 09:46

BorisYaoA approved these changes Aug 9, 2024

View reviewed changes

alecharp approved these changes Aug 9, 2024

View reviewed changes

https://jetty.org/docs/jetty/12/programming-guide/migration/11-to-12.…

7b0bf37

…html#handler-response-content-apis

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 10, 2024

Merge branch 'master' into prototype

6091dab

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 10, 2024

Merge branch 'master' into prototype

a1d9b05

NotMyFault approved these changes Aug 11, 2024

View reviewed changes

Kevin-CB approved these changes Aug 12, 2024

View reviewed changes

Kevin-CB added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Aug 12, 2024

Use released versions

0045c65

basil merged commit 121c0ae into master Aug 12, 2024
8 of 9 checks passed

basil deleted the prototype branch August 12, 2024 14:42

basil mentioned this pull request Aug 12, 2024

Fix jetty:run #9603

Merged

basil mentioned this pull request Nov 7, 2024

URL encoding issue in test report links with square brackets jenkinsci/junit-plugin#659

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73130] Upgrade core from Jetty 10.x to 12.x (EE 8) #9590

[JENKINS-73130] Upgrade core from Jetty 10.x to 12.x (EE 8) #9590

basil commented Aug 8, 2024 •

edited by MarkEWaite

Loading

Submitter checklist

Maintainer checklist

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

basil Aug 8, 2024

MarkEWaite left a comment

basil commented Aug 9, 2024

BorisYaoA left a comment

MarkEWaite commented Aug 9, 2024

alecharp left a comment

github-actions bot commented Aug 10, 2024

Kevin-CB left a comment

MarkEWaite commented Aug 12, 2024 •

edited

Loading

basil commented Aug 12, 2024

daniel-beck commented Aug 12, 2024 •

edited

Loading

MarkEWaite commented Aug 12, 2024 •

edited

Loading

basil commented Aug 12, 2024

daniel-beck commented Aug 12, 2024

basil commented Aug 12, 2024

MarkEWaite commented Aug 12, 2024

[JENKINS-73130] Upgrade core from Jetty 10.x to 12.x (EE 8) #9590

[JENKINS-73130] Upgrade core from Jetty 10.x to 12.x (EE 8) #9590

Conversation

basil commented Aug 8, 2024 • edited by MarkEWaite Loading

Context

Problem

Solution

Implementation

Testing done

Future work

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

basil commented Aug 9, 2024

BorisYaoA left a comment

Choose a reason for hiding this comment

MarkEWaite commented Aug 9, 2024

alecharp left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 10, 2024

Kevin-CB left a comment

Choose a reason for hiding this comment

MarkEWaite commented Aug 12, 2024 • edited Loading

basil commented Aug 12, 2024

daniel-beck commented Aug 12, 2024 • edited Loading

MarkEWaite commented Aug 12, 2024 • edited Loading

basil commented Aug 12, 2024

daniel-beck commented Aug 12, 2024

basil commented Aug 12, 2024

MarkEWaite commented Aug 12, 2024

basil commented Aug 8, 2024 •

edited by MarkEWaite

Loading

MarkEWaite commented Aug 12, 2024 •

edited

Loading

daniel-beck commented Aug 12, 2024 •

edited

Loading

MarkEWaite commented Aug 12, 2024 •

edited

Loading