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.

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

[JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 #9259

Merged
merged 7 commits into from
May 18, 2024

Conversation

basil
Copy link
Member

@basil basil commented May 10, 2024

JENKINS-73170 Upgrade Commons FileUpload from 1.5 to 2.0.0-M2

Context

JENKINS-73169 describes the long-term goal of removing Commons FileUpload from Stapler and Jenkins core, decreasing API surface area and maintenance burden by eliminating an unneeded library.

This long-term strategy carries a certain amount of risk and labor:

  • New APIs need to be designed and validated across a variety of consumers (both core consumers and plugin consumers), which is more laborious than a textual package/class rename operation.
  • New implementations need to be written and tested, ensuring that existing behavior remains the same and no new security issues are introduced. Given the general lack of realistic automated testing for file uploading, there is a high risk of behavioral regression. Even worse is the potential for a security regression that a new implementation brings. Testing all of these scenarios thoroughly will require a large amount of manual testing and is far more laborious than staying with a battle-tested implementation with known (and tested) security modalities.
  • Implementing this change throughout the ecosystem would require the new APIs to ship in a weekly/LTS release, for plugins to adopt that LTS release, and then for those plugins to be adapted to using the new APIs. The list is:
    plain-credentials|278830
    subversion|86698
    uno-choice|37949
    google-oauth-plugin|12220
    m2release|9733
    scriptler|6767
    sidebar-link|6678
    file-parameters|5507
    xcode-plugin|4506
    kpp-management-plugin|2262
    miniorange-saml-sp|2099
    hp-application-automation-tools-plugin|1611
    custom-folder-icon|1158
    promoted-builds-simple|1038
    custom-job-icon|712
    jclouds-jenkins|583
    patch-parameter|559
    svn-partial-release-mgr|517
    htmlresource|515
    BlameSubversion|405
    deployer-framework|271
    avatar|214
    ios-device-connector|125
    pom2config|90
    recipe|51
    clif-performance-testing|22
    
    Our experience has shown that it takes many months for such changes to propagate through the ecosystem and to be adopted by plugins. Upgrading a plugin's baseline, patching it to use new APIs, and getting the result merged and released can be a time-consuming exercise, if not a frustrating one—especially for the long tail of plugins.

Problem

We have a desire to upgrade Spring Security to a recent version in a relatively timely fashion. Recent versions of Spring Security require Java EE 9 or later, while Commons FileUpload 1.x only supports Java EE 8 and earlier, implying in the long term the completion of the abovementioned project as a prerequisite to moving to Java EE 9 or later (or, in the short term, creating a custom fork of Commons FileUpload 1.x with the javax imports changed to jakarta imports—a high-maintenance prospect that would also regress our previous efforts to unfork this library). Yet this long-term solution is laborious and carries a high risk of regression (both functionally and security-wise); moreover, file uploading is not a central feature of the software, and focusing a huge amount of effort on it seems out of place relative to the value to end users it will provide—certainly a higher priority would be to focus on the actual Jakarta EE 9 migration itself, which brings with it the tangible benefits of security updates for Spring Security and thus a clear benefit to Jenkins users.

Solution

A medium-term compromise is possible via Commons FileUpload 2.x, which supports both javax and jakarta imports. Consider that Stapler and Jenkins core provide a number of APIs that work with Commons FileUpload FileItem objects, in contrast to the low-level DiskFileItem objects and the classes that produce them (like DiskFileItemFactory). These APIs somewhat shelter their consumers from low-level implementation details, like the instantiation of a factory against a particular servlet API. The key observation is that the vast majority of plugins (all open-source plugins with more than 3,000 installations, and all but two proprietary plugins) consume these higher-level Jenkins APIs, which still tie us to Commons FileUpload, but not necessarily to low-level implementation details like DiskFileItem; moreover, any cases I found of plugins that directly consume the low-level APIs can be trivially converted to the higher-level Stapler APIs. Unfortunately, the package name has changed between 1.x and 2.x; however, since the FileItem interface is very similar between 1.x and 2.x, and since it is an interface, we can bridge between the two versions somewhat transparently.

The implications are as follows: once a handful of plugins are migrated from direct usages of DiskFileItem to the simpler FileItem wrappers provided by Stapler (which is a trivial and low-risk change, and moreover only needs to be done for 2-4 plugins in the short term), we can upgrade core to FileUpload 2.x (with both javax and jakarta support) maintaining full plugin compatibility (in both production and build/test) with any plugins that consume the 1.x APIs via FileItem (and not through the low-level DiskFileItem). These existing plugins, which link against FileUpload 1.x, will call into Stapler to get their FileItem, which will be a 2.x object wrapped in a 1.x compatibility object, and any methods they call on that 1.x compatibility object will vector back into the 2.x methods at runtime.

In other words, core will be fully running on FileUpload 2.x, with both javax and jakarta support, unblocking the Java EE 9 migration project. Even better, this can be achieved through a fraction of the labor and the risk of the long-term strategy. Commons FileUpload 2.x is largely a package/class rename release, but the code is basically the battle-tested implementation we have always relied on, so the risk of functional and security regression is very low. Rather than having to sweep through dozens of plugins, we'll have to sweep through a handful at most—and the results of that sweep can be released proactively, rather than having to wait for a new API to be available (perhaps even in LTS) in order to consume it. Finally, this gets file uploading out of the way for the time being so that Jenkins contributors can focus on the higher-priority task of dealing with the Jakarta EE 9 migration.

Some might object to the use of a milestone release of Commons FileUpload, and this is a fair point. Yet it is advertised on both the project's home page and their GitHub page; also, it is published to Maven Central (unlike a SNAPSHOT release). It would appear as if the authors are getting close to a release but simply looking for feedback on the API. The Jenkins project could provide a valuable service to the Apache Commons project by confirming that the API is working for our use case. Moreover, I suspect the Spring 5.3.x EOL will likely push the whole Java ecosystem toward Java EE 9+, and I suspect (or at least hope) that this will push Commons FileUpload 2.x toward GA this year. Nevertheless, since it is still in milestone status, I am not recommending that we go ahead and migrate plugins from FileUpload 1.x to 2.x APIs at this time. The 1.x compatibility methods are tried and true, and not subject to change, so it is prudent to remain on them at least until the GA release of 2.x. Moreover, going on a sweep of plugin consumers is exactly the kind of activity that we wanted to avoid doing as part of this exercise; and if we are going to do a sweep of plugin consumers, then we might as well simply implement the long-term solution—something which this medium-term solution in no way precludes, but simply postpones to a later date to focus on more pressing concerns. Finally, with the exception of the FileItem compatibility interface, this is a lateral move in API surface area (removal of 1.x and addition of 2.x) rather than an increase (addition of 2.x to the existing 1.x).

Looking at the bigger picture, shipping this immediately will cross out one major portion of the risk and labor associated with the Jakarta EE 9 upgrade—that which is associated with file uploading. Getting this out into weekly releases early will ensure that we have plenty of time to deal with any issues. The next major change after this will be to require Java 17, at which point we can switch to Jetty 12.x with Jakarta EE 8 and get another big wad of changes out into production. Then, the only remaining part of the problem will be to switch from Jetty 12 (EE 8) and FileUpload 2 (EE 8) to Jetty 12 (EE 9) and FileUpload 2 (EE 9), a much more bounded problem, whose risk will also be a lot lower, since some of its major prerequisites will have already been shipping for weeks earlier. In contrast, trying to ship a FileUpload change, a Jetty upgrade, and a Jakarta migration all in the same release sounds like a scope that is far too great to be practical.

Implementation

Usages in core and Stapler are migrated to 2.x. A full compatibility layer is provided for plugins that use the high-level FileItem API (but not the low-level DiskFileItem API). This compatibilty layer extends to both production as well as compilation/tests.

The compatibility chart is here (there are also two CloudBees plugins, and I will find someone to update those). The only open-source plugins with over 1,000 installations are miniorange-saml-sp and hp-application-automation-tools-plugin—those are the only two I plan to help with or mention in the release notes, but even those are installed on only 0.72% of controllers so I don't think they should block the integration of this PR into the weekly line (this is just too important a milestone and we are just on too tight of a timeline to wait).

Testing done

  • Full PCT and ATH (both passing)
  • Manual testing of file uploading in both core (via the upload plugin feature) and two plugins (Scriptler and Sidebar Link) using the compatibility methods
  • Existing security-related unit tests still pass, as does the unit test I added recently for file upload persistence.

Proposed changelog entries

  • Upgrade Commons FileUpload from 1.5 to 2.0.0-M2. Users of the miniorange-saml-sp plugin should upgrade to a compatible version in lockstep with upgrading Jenkins core.

Proposed upgrade guidelines

  • Upgrade Commons FileUpload from 1.5 to 2.0.0-M2. Users of the miniorange-saml-sp plugin should upgrade to a compatible version in lockstep with upgrading Jenkins core.

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

@basil basil added the work-in-progress The PR is under active development, not ready to the final review label May 10, 2024
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label May 12, 2024
Copy link
Contributor

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

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label May 12, 2024
@basil basil force-pushed the fileupload2 branch 5 times, most recently from 9454dd8 to 181f931 Compare May 13, 2024 18:48
basil added a commit to basil/bom that referenced this pull request May 13, 2024
basil added a commit to basil/bom that referenced this pull request May 14, 2024
basil added a commit to basil/bom that referenced this pull request May 14, 2024
basil added a commit to basil/bom that referenced this pull request May 14, 2024
basil added a commit to basil/bom that referenced this pull request May 14, 2024
basil added a commit to basil/acceptance-test-harness that referenced this pull request May 14, 2024
@basil basil added 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 May 14, 2024
@basil basil added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed and removed work-in-progress The PR is under active development, not ready to the final review labels May 14, 2024
@basil basil changed the title Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 [JENKINS-73170] Upgrade Commons FileUpload from 1.5 to 2.0.0-M2 May 14, 2024
@basil basil added the needs-security-review Awaiting review by a security team member label May 14, 2024
@basil basil marked this pull request as ready for review May 14, 2024 23:59
@basil basil requested review from MarkEWaite, alecharp and a team May 15, 2024 00:00
Comment on lines +189 to +223
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-fileupload2</artifactId>
<version>${commons-fileupload2.version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-fileupload2-core</artifactId>
<version>${commons-fileupload2.version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-fileupload2-distribution</artifactId>
<version>${commons-fileupload2.version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-fileupload2-jakarta-servlet5</artifactId>
<version>${commons-fileupload2.version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-fileupload2-jakarta-servlet6</artifactId>
<version>${commons-fileupload2.version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-fileupload2-javax</artifactId>
<version>${commons-fileupload2.version}</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-fileupload2-portlet</artifactId>
<version>${commons-fileupload2.version}</version>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice if there was a BOM, but I didn't find one, nor did I find an RFE bug asking for one, so I included every package in this library in the Jenkins BOM for completeness.

@@ -139,10 +150,15 @@ public String getOriginalFileName() {
return originalFileName;
}

@WithBridgeMethods(value = org.apache.commons.fileupload.FileItem.class, adapterMethod = "fromFileUpload2FileItem")
Copy link
Member Author

Choose a reason for hiding this comment

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

See the commentary in the Stapler PR for an explanation of how this works.

public static final class FileItemImpl implements FileItem {
private final File file;
@Deprecated
public static final class FileItemImpl implements org.apache.commons.fileupload.FileItem {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is basically only consumed by tests, and might even make more sense in the test harness, though I am not tackling that here. However, to retain compatibility at compile time and in tests, it has to retain the old API. I am providing a FileItemImpl2 that implements the new API for use by core itself and implement this in terms of that (through the delegate pattern) in order to reuse code.

@@ -82,20 +87,20 @@ public MultipartFormDataParser(HttpServletRequest request, int maxParts, long ma
throw new ServletException("Error creating temporary directory", e);
}
tmpDir.deleteOnExit();
ServletFileUpload upload = new ServletFileUpload(new DiskFileItemFactory(DiskFileItemFactory.DEFAULT_SIZE_THRESHOLD, tmpDir));
JavaxServletFileUpload<DiskFileItem, DiskFileItemFactory> upload = new JavaxServletDiskFileUpload(DiskFileItemFactory.builder().setFile(tmpDir).get());
Copy link
Member Author

Choose a reason for hiding this comment

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

See the explanation for this in the Stapler PR.

@daniel-beck daniel-beck 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 May 15, 2024
@daniel-beck
Copy link
Member

Reviewed wrt SECURITY-3030 and SECURITY-3073, seems fine.

basil added 2 commits May 15, 2024 10:41
bridge methods work at runtime but there is still a compilation error.
Since I don't want to force plugins to adopt the new API when bumping
their core baseline, simpler to avoid bridge methods here.
bom/pom.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks!

@basil
Copy link
Member Author

basil commented May 18, 2024

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process.

@basil basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 18, 2024
@basil basil merged commit 0117bf3 into jenkinsci:master May 18, 2024
17 checks passed
@basil basil deleted the fileupload2 branch May 18, 2024 16:42
basil added a commit to basil/bom that referenced this pull request May 18, 2024
basil added a commit to basil/acceptance-test-harness that referenced this pull request May 18, 2024
basil added a commit to basil/acceptance-test-harness that referenced this pull request May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants