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-73278] Migrate core from EE 8 to EE 9 #9672

Merged
merged 62 commits into from
Sep 3, 2024
Merged

[JENKINS-73278] Migrate core from EE 8 to EE 9 #9672

merged 62 commits into from
Sep 3, 2024

Conversation

basil
Copy link
Member

@basil basil commented Aug 27, 2024

Context

See JENKINS-73255. Goes with three other pull requests:

Problem

Jenkins bundles an outdated copy of Spring Security 5.x, with official end of open source support on August 31, 2024. Spring Security 6.x requires Spring Framework 6.x, which requires EE 9.

Solution

  • Update dependencies in pom.xml to align with Jetty 12 (EE 9)
  • Migrate imports
  • Add compatibility layer for EE 8 to support Jenkins plugins

At a high level, we add StaplerRequest2 and StaplerResponse2 classes, the Jakarta equivalents of the old versions (now deprecated). The RequestImpl and ResponseImpl implementations are now based on EE 9 rather than EE 8, but support for plugins is maintained by converting to and from the interfaces as needed.

Testing done

Full PCT and ATH (Jenkins public version and CloudBees internal version)

Proposed changelog entries

Upgrade Spring Framework from 5.3.39 to 6.1.12, upgrade Spring Security from 5.8.14 to 6.3.3, and upgrade Java EE from 8 to 9.

Proposed upgrade guidelines

Users of the LDAP plugin must upgrade it 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 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 needs-security-review Awaiting review by a security team member 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 27, 2024
@basil basil requested a review from a team August 27, 2024 23:16
Comment on lines +444 to +447
<exclusion>
<groupId>io.micrometer</groupId>
<artifactId>micrometer-observation</artifactId>
</exclusion>
Copy link
Member Author

Choose a reason for hiding this comment

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

Excluded to avoid increasing API surface area, as this seems to be unused.

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.

I've been testing versions of this for months with good results. Thanks!

Copy link

@vwagh-dev vwagh-dev left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Kevin-CB Kevin-CB left a comment

Choose a reason for hiding this comment

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

The View/doCreateItem method is now defined and should either require POST request or be made non-routable to prevent a CSRF vulnerability.

Additionally, there are several deprecated methods that no longer need to be routable, however, AFAIK they don't pose any security risk.

core/src/main/java/hudson/search/Search.java Show resolved Hide resolved
core/src/main/java/hudson/util/Graph.java Show resolved Hide resolved
core/src/main/java/hudson/util/Graph.java Show resolved Hide resolved
core/src/main/java/jenkins/model/Jenkins.java Show resolved Hide resolved
core/src/main/java/hudson/model/Api.java Show resolved Hide resolved
core/src/main/java/hudson/model/Api.java Show resolved Hide resolved
core/src/main/java/hudson/model/Job.java Show resolved Hide resolved
@basil
Copy link
Member Author

basil commented Sep 3, 2024

Should this PR be amended to also cast to the subtype when it's instanceof, consistent with CompatibleFilter?

commit 520776a

This reverts commit 3a6a576.
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

BasicHeaderAuthenticator and ParameterizedJobMixIn#doBuildWithParameters look like bugs.

There are a few cases of seemingly wrong override direction in overridable methods, which should be looked at. In addition to the line comments below, there are several additional occurrences in AbstractProject in which the old method calls the new method.

The rest of the findings are things that look off enough to warrant a mention, mostly just inconsistencies in the code with no obvious reason.

Otherwise this looks good. I was unable to find security issues, and no further compatibility issues other than those I posted as individual top comments.

core/src/main/java/hudson/PluginManager.java Outdated Show resolved Hide resolved
core/src/main/java/hudson/model/Descriptor.java Outdated Show resolved Hide resolved
core/src/main/java/jenkins/security/ApiTokenProperty.java Outdated Show resolved Hide resolved
core/src/main/java/hudson/util/DescribableList.java Outdated Show resolved Hide resolved
core/src/main/java/hudson/util/FormFieldValidator.java Outdated Show resolved Hide resolved
@basil
Copy link
Member Author

basil commented Sep 3, 2024

BasicHeaderAuthenticator and ParameterizedJobMixIn#doBuildWithParameters look like bugs.

Thanks, this is fixed in the most recent revision.

There are a few cases of seemingly wrong override direction in overridable methods, which should be looked at. In addition to the line comments below, there are several additional occurrences in AbstractProject in which the old method calls the new method.

In cases where I couldn't find any overrides or felt that overrides were unlikely, I kept things simple and had the old implementation call the new. This was a bit of a subjective choice to favor simplicity at a slight risk of incorrectness rather than polluting the code with (likely unnecessary) complexity to completely eliminate risk.

The rest of the findings are things that look off enough to warrant a mention, mostly just inconsistencies in the code with no obvious reason.

Thanks, I have fixed these oversights.

Otherwise this looks good. I was unable to find security issues, and no further compatibility issues other than those I posted as individual top comments.

Great! Thank you very much.

@basil
Copy link
Member Author

basil commented Sep 3, 2024

@daniel-beck Can you look at this final set of changes and let me know if this is ready to ship?

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

:shipit:

@daniel-beck daniel-beck added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Sep 3, 2024
This reverts commit 2a4870c.
@basil basil merged commit 7006cde into master Sep 3, 2024
8 of 9 checks passed
@basil basil deleted the jakarta branch September 3, 2024 21:59
@timja timja added major-rfe For changelog: Major enhancement. Will be highlighted on the top and removed rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Sep 4, 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 major-rfe For changelog: Major enhancement. Will be highlighted on the top pct-successful This PR has successfully passed the full plugin-compatibility-test suite security-approved @jenkinsci/core-security-review reviewed this PR for security issues squash-merge-me Unclean or useless commit history, should be merged only with squash-merge 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.

7 participants