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

Remove bundled External Monitor Job Type, LDAP, and PAM Authentication plugins #5445

Merged

Conversation

basil
Copy link
Member

@basil basil commented Apr 25, 2021

Previously: #4242, #5102, and #5338. Dealing with bundled plugins in Jenkins core is painful. The fewer of these we have, the better. Here we stop bundling External Monitor Job Type, LDAP, and PAM Authentication in the Jenkins WAR. As I explain below, we do not actually need to bundle these any longer, just like in #4242, #5102, and #5338.

Testing Done

First, I verified that none of the other plugins we bundle in the WAR have a (non-implied) dependency on External Monitor Job Type, LDAP, or PAM Authentication. It turns out that 0 plugins on the Update Center explicitly depend on External Monitor Job Type, LDAP, or PAM Authentication. So we are good to go there.

Next, I did a realistic smoke test by starting up Jenkins with java -jar jenkins.war (after verifying the WAR no longer contained External Monitor Job Type, LDAP, or PAM Authentication in WEB-INF/detached-plugins) and running through the wizard. This installed LDAP and PAM Authentication (which are both suggested plugins), but that was expected. The wizard completed successfully with no errors. Note that External Monitor Job Type was not present in the list of installed plugins after completing the wizard.

Finally I had to make sure this wouldn't break any plugins consuming the classes provided by External Monitor Job Type, LDAP, or PAM Authentication. The last core release still containing External Monitor Job Type, LDAP, and PAM Authentication functionality was 1.467. Trawling through the graveyard I found 291 plugins whose required core was ≤ 1.467. Of those, 286 returned no results for the following grep(1) commands:

$ grep -Hn --binary-files=without-match -ri hudson.security.pam .
$ grep -Hn --binary-files=without-match -ri jenkins.security.plugins.ldap .
$ grep -Hn --binary-files=without-match -ri hudson.security.ldap .
$ grep -Hn --binary-files=without-match -ri hudson.security.GeneralizedTime .
$ grep -Hn --binary-files=without-match -ri hudson.security.UserAttributesHelper .
$ grep -Hn --binary-files=without-match -ri org.jenkinsci.plugins.externalmonitorjob .
$ grep -Hn --binary-files=without-match -ri hudson.model.ExternalRun .
$ grep -Hn --binary-files=without-match -ri hudson.model.ExternalJob .
$ grep -Hn --binary-files=without-match -ri hudson.cli.SetExternalBuildResultCommand .
$ grep -Hn --binary-files=without-match -ri externaljob.png .

My grep(1) commands only found a match in crittercism-dsym, but it was a false positive: someone had erroneously committed the target/jenkins-for-test/ directory into the source tree.

I could not find the source code for these four plugins …

  • Prerequisite build step, a plugin last released 10 years ago with 867 installs whose required core is 1.392
  • Project Health Report, a plugin last released 10 years ago with 550 installs whose required core is 1.392
  • Meme Generator, a plugin last released 5 years ago with 149 installs whose required core is 1.428
  • DevStack a plugin last released 9 years ago with 16 installs whose required core is 1.432

… but these all seem outdated and unlikely to depend on External Monitor Job Type, LDAP, or PAM Authentication. I do not anticipate this being a major issue.

Proposed changelog entries

  • Stop bundling the External Monitor Job Type, LDAP, and PAM Authentication plugins. Jenkins will no longer automatically install the External Monitor Job Type, LDAP, or PAM Authentication plugins on startup if a plugin depending on Jenkins (then Hudson) 1.467 or earlier is discovered. If you use such a plugin that also relies on the functionality provided by the External Monitor Job Type, LDAP, or PAM Authentication plugin and manage plugins outside Jenkins' plugin manager, you will now need to ensure that a recent release of the External Monitor Job Type, LDAP, or PAM Authentication plugin is installed. Jenkins will attempt to load such plugins but may fail at any time during startup or afterwards with ClassNotFoundException or similar.

Proposed upgrade guidelines

See above.

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@daniel-beck

Maintainer checklist

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

  • There are at least 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 does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (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).

@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 labels Apr 25, 2021
@basil basil requested a review from daniel-beck April 25, 2021 17:49
@daniel-beck
Copy link
Member

Trawling through the graveyard I found 291 plugins whose required core was ≤ 1.467

Interesting, I got 255 using

$ curl --silent --location https://updates.jenkins.io/update-center.actual.json | jq '.plugins[] | select( .requiredCore < "1.468" ) | "\( .requiredCore ) - \( .name )"'

How did you do this?

(FWIW it looks like no plugin has an explicit dependency on these, which would allow use of usage-in-plugins with a custom class list to replace the grep for redundancy. I may still do this for fun, but not a requirement.)

@basil
Copy link
Member Author

basil commented Apr 26, 2021

How did you do this?

I generally use envelope-core-oc and envelope-core-mm rather than update-center.json for these types of searches in order to broaden my search as far and wide as possible.

I may still do this for fun, but not a requirement.

No need; creating /tmp/additionalClasses with

hudson/cli/SetExternalBuildResultCommand
hudson/model/ExternalJob
hudson/model/ExternalRun
hudson/security/GeneralizedTime
hudson/security/LDAPSecurityRealm
hudson/security/PAMSecurityRealm
hudson/security/UserAttributesHelper
jenkins/security/plugins/ldap/BindAuthenticator2
jenkins/security/plugins/ldap/FromGroupSearchLDAPGroupMembershipStrategy
jenkins/security/plugins/ldap/FromUserRecordLDAPGroupMembershipStrategy
jenkins/security/plugins/ldap/LDAPConfiguration
jenkins/security/plugins/ldap/LdapEntryMapper
jenkins/security/plugins/ldap/LDAPExtendedTemplate
jenkins/security/plugins/ldap/LDAPGroupMembershipStrategy
jenkins/security/plugins/ldap/LDAPGroupMembershipStrategyDescriptor
jenkins/security/plugins/ldap/SetContextClassLoader

then using jenkins-infra/usage-in-plugins to look for usages in plugins, including those in CloudBees CI, with

mvn process-classes exec:exec -Dexec.executable=java -Dexec.args='-classpath %classpath org.jenkinsci.deprecatedusage.Main --additionalClasses /tmp/additionalClasses --onlyIncludeSpecified --updateCenter https://jenkins-updates.cloudbees.com/update-center/envelope-core-oc/update-center.json?version=2.277.3.1,https://jenkins-updates.cloudbees.com/update-center/envelope-core-mm/update-center.json?version=2.277.3.1'

produced no references.

@daniel-beck
Copy link
Member

Makes sense, I'm not used to non-bees caring about CB proprietary update sites 😉

Thanks for the usage-in-plugins confirmation. Time to slim down the war.

@daniel-beck
Copy link
Member

FTR, if you plan to continue doing these PRs: Mailer usage looks annoying based on plugins it shows up in, but the three plugins between Mailer and Matrix Project look really easy to unbundle based on usage-in-plugins. So skipping Mailer for now and continuing with those afterwards could be a reasonable next step.

Classes file I used for usage-in-plugins
com/cloudbees/hudson/plugins/folder/properties/AuthorizationMatrixProperty
hudson/markup/BasicPolicy
hudson/markup/MyspacePolicy 
hudson/markup/RawHtmlMarkupFormatter
hudson/os/windows/ManagedWindowsServiceAccount
hudson/os/windows/ManagedWindowsServiceConnector
hudson/os/windows/ManagedWindowsServiceLauncher
hudson/os/windows/WindowsRemoteFileSystem
hudson/os/windows/WindowsRemoteLauncher
hudson/security/AuthorizationMatrixProperty
hudson/security/GlobalMatrixAuthorizationStrategy
hudson/security/ProjectMatrixAuthorizationStrategy
org/jenkinsci/plugins/matrixauth/AbstractAuthorizationContainerConverter
org/jenkinsci/plugins/matrixauth/AbstractAuthorizationPropertyConverter
org/jenkinsci/plugins/matrixauth/AuthorizationContainer
org/jenkinsci/plugins/matrixauth/AuthorizationContainerDescriptor
org/jenkinsci/plugins/matrixauth/AuthorizationMatrixNodeProperty
org/jenkinsci/plugins/matrixauth/AuthorizationProperty
org/jenkinsci/plugins/matrixauth/AuthorizationPropertyDescriptor
org/jenkinsci/plugins/matrixauth/inheritance/InheritanceStrategy
org/jenkinsci/plugins/matrixauth/inheritance/InheritanceStrategyDescriptor
org/jenkinsci/plugins/matrixauth/inheritance/InheritGlobalStrategy
org/jenkinsci/plugins/matrixauth/inheritance/InheritParentStrategy
org/jenkinsci/plugins/matrixauth/inheritance/NonInheritingStrategy
org/jenkinsci/plugins/matrixauth/integrations/casc/AuthorizationMatrixNodePropertyConfigurator
org/jenkinsci/plugins/matrixauth/integrations/casc/GlobalMatrixAuthorizationStrategyConfigurator
org/jenkinsci/plugins/matrixauth/integrations/casc/MatrixAuthorizationStrategyConfigurator
org/jenkinsci/plugins/matrixauth/integrations/casc/ProjectMatrixAuthorizationStrategyConfigurator
org/jenkinsci/plugins/matrixauth/integrations/PermissionFinder
org/jenkinsci/plugins/matrixauth/MatrixAuthorizationStrategy
org/jenkinsci/plugins/matrixauth/ValidationUtil

(MyspacePolicy is antisamy-markup-formatter 1.x, luckily unused)

@basil
Copy link
Member Author

basil commented May 1, 2021

FTR, if you plan to continue doing these PRs

I was considering it, but not until the next LTS cycle. Slow and steady wins the race.

Mailer usage looks annoying based on plugins it shows up in

On the other hand, Mailer is literally the most popular Jenkins plugin of all time. It has 298,206 installations and is a suggested plugin. So do we really gain much by bundling it? I honestly can't imagine a production Jenkins controller without Mailer installed.

@jvz
Copy link
Member

jvz commented May 1, 2021

Mailer was the first plug-in split out from core, hence it having the most installs due to the detached plug-in system. It's certainly a commonly used plug-in at least.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

+1 for External Monitor and PAM auth, they are not actively used AFAIK. For LDAP I am rather +0. Although there is no strict need to bundle it, it is used widely

@oleg-nenashev oleg-nenashev requested review from timja and a team May 2, 2021 05:24
@daniel-beck
Copy link
Member

daniel-beck commented May 2, 2021

Although there is no strict need to bundle it, it is used widely

Why would this make a difference? This isn't Jenkins 1.x, bundled plugins are used very rarely.

Mailer was the first plug-in split out from core

Not quite:

maven-plugin 1.296 1.296
subversion 1.310 1.0
cvs 1.340 0.1
ant 1.430 1.0
javadoc 1.430 1.0
external-monitor-job 1.467 1.0
ldap 1.467 1.0
pam-auth 1.467 1.0
mailer 1.493 1.2

Sorry for derailing your nice PR @basil , I didn't expect a discussion to happen.

@jglick
Copy link
Member

jglick commented May 3, 2021

someone had erroneously committed the target/jenkins-for-test/ directory into the source tree

BTW it would be great to clean these up in batch somehow. Makes for a lot of noise in GH searches.

@jglick
Copy link
Member

jglick commented May 3, 2021

Mailer usage looks annoying based on plugins it shows up in

Specifically https://javadoc.jenkins.io/plugin/mailer/hudson/tasks/Mailer.UserProperty.html is a widely used API.

@oleg-nenashev
Copy link
Member

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

@oleg-nenashev oleg-nenashev 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 3, 2021
@timja timja merged commit c0561b8 into jenkinsci:master May 7, 2021
@basil basil deleted the unbundle-external-monitor-job-ldap-pam-auth branch May 7, 2021 20:20
@saper

This comment has been minimized.

@timja

This comment has been minimized.

@saper

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 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.

9 participants