[JENKINS-75232] Prevent dynamic plugin installation from registering the same extension twice in some cases by dwnusbaum · Pull Request #10240 · jenkinsci/jenkins

dwnusbaum · 2025-02-04T20:17:19Z

See JENKINS-75232. The relevant ExtensionList code has not changed since it was introduced in #1673, but in the real-world case I saw the listener that is causing the issues comes from #3496.

This PR requires jenkinsci/jenkins-test-harness#920 for the new test.

Testing done

A new automated test has been created to demonstrate the bug and test the fix. @jgreffe and I also tested the fix against the real-world plugin which triggered the bug (a proprietary CloudBees plugin).

Proposed changelog entries

Prevent dynamic plugin installation from registering the same extension twice in some cases

Proposed upgrade guidelines

N/A

### Submitter checklist
- [x] The Jira issue, if it exists, is well-described.
- [x] 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](https://github.com/jenkins-infra/jenkins.io/blob/master/content/_data/changelogs/weekly.yml)). Fill in the **Proposed upgrade guidelines** section only if there are breaking changes or changes that may require extra steps from users during upgrade.
- [x] There is automated testing or an explanation as to why this change has no tests.
- [x] 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](https://www.jenkins.io/doc/developer/security/csp/)).
- [ ] 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.

Desired reviewers

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

### Maintainer checklist
- [x] 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](https://github.com/jenkinsci/jenkins/pull/4387)).
- [ ] 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](https://issues.jenkins.io/issues/?filter=12146)).

…the same extension twice in some cases Co-authored-by: Julien Greffe <jgreffe@cloudbees.com>

dwnusbaum · 2025-02-04T20:21:19Z

core/src/main/java/hudson/ExtensionList.java

+    @Restricted(NoExternalUse.class)
+    public boolean refresh(ExtensionComponentSet delta) {


This is an incompatible API change, but given the Javadoc, there really should not be any external callers, and a quick GitHub search (I also checked the cloudbees org) looked ok.

If desired, I can instead introduce a new method and preserve the old one for compatibility. We might still want to add @Restricted(NoExternalUse.class) to the old method if we take that approach though.

dwnusbaum · 2025-02-04T20:23:42Z

core/src/main/java/jenkins/model/Jenkins.java

+        // Refresh all extension lists before firing any listeners in case a listener would cause any new extension
+        // lists to be forcibly loaded, which may lead to duplicate entries for the same extension object in a list.
+        for (var el : listsToFireOnChangeListeners) {
+            el.fireOnChangeListeners();


I am also happy to consider alternative fixes if anyone has any ideas. One thing I considered was to instead check for duplicates in

jenkins/core/src/main/java/hudson/ExtensionList.java

Line 348 in 630b6b6

l.addAll(found);

but delaying the listener notifications seemed clearer in terms of what we are trying to fix. In general this bug makes me think that there might be deeper problems with dynamic extension loading, but I am not familiar enough with the extension system to know how best to fix things.

Duplicate prevention added in 6817532 to fix issues with extensions that load other extensions in their constructors.

test/plugins/README.md

test/src/test/java/hudson/ExtensionListListenerTest.java

jglick

Source code looks fine. Recommend trying another approach for test plugin maintenance.

core/src/main/java/hudson/ExtensionList.java

core/src/main/java/jenkins/model/Jenkins.java

test/plugins/README.md

test/src/test/java/hudson/ExtensionListListenerTest.java

jtnord

Whilst this fixes the test case, I can't help but feel this doesn't fix the case where an Extension loads another extension (either in its constructor, or a Thread it kicks off).

The listener causing a load of an extension by proxy of calling some code, would just be one trigger of the wider cause here?

The suggested alternative of checking for duplicates and not using addAll would seem sane, however IIUC we would create 2 objects for the same extension (not the return the same Object). So if the extension is kicking off something async in its constructor things have already got into a bad state (and the plugin is already liable highly suspect to race conditions)?

Iff we are only creating a single extension for the class but registering it twice I think the addAll fix would be more robust

dwnusbaum · 2025-02-14T17:25:44Z

Whilst this fixes the test case, I can't help but feel this doesn't fix the case where an Extension loads another extension (either in its constructor, or a Thread it kicks off).

Ok, I can check this case.

however IIUC we would create 2 objects for the same extension (not the return the same Object)

Iff we are only creating a single extension for the class but registering it twice I think the addAll fix would be more robust

As far as I can tell, right now we really are only creating one object, just registering it twice. I'll recheck though.

jtnord · 2025-02-14T17:40:35Z

Iff we are only creating a single extension for the class but registering it twice I think the addAll fix would be more robust

As far as I can tell, right now we really are only creating one object, just registering it twice. I'll recheck though.

if that is the case it may be worthwhile doing both.

not calling listeners means less churn if there is interdependency
the addAll change means the singleton would be respected if someone does funky stuff in more areas.

dwnusbaum · 2025-02-14T19:25:56Z

Whilst this fixes the test case, I can't help but feel this doesn't fix the case where an Extension loads another extension (either in its constructor, or a Thread it kicks off).

@jtnord Indeed this case is still broken. I confirmed that in the problematic cases the extension is only instantiated once but registered twice. I'll add duplicate prevention to ExtensionList.refresh as well.

jtnord · 2025-02-15T12:47:45Z

Whilst this fixes the test case, I can't help but feel this doesn't fix the case where an Extension loads another extension (either in its constructor, or a Thread it kicks off).

@jtnord Indeed this case is still broken. I confirmed that in the problematic cases the extension is only instantiated once but registered twice. I'll add duplicate prevention to ExtensionList.refresh as well.

it's a shame that ExtensionList was a List not a Set and it's likely going to break a lot of things if we changed that :-(
The internal representation (ExtensionList.extensions) could be changed, but the front end code uses indexes into the list for some things IIRC, and changing this smells very risky to me.

…r-related case

github-actions · 2025-02-18T18:51:27Z

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

stale

dwnusbaum · 2025-02-18T18:59:02Z

it's a shame that ExtensionList was a List not a Set ... changing this smells very risky to me.

Yeah, in retrospect it seems that having this class actually be a collection was not really necessary. If we had instead only defined collection-like methods to covered the critical use cases, I would feel more comfortable refactoring the internal representation.

jtnord

Seems reasonable, did not pay much attention to the test code.

core/src/main/java/hudson/ExtensionList.java

…dded

…alPackage

jglick

Main code change is opaque to me, but test looks nice.

jglick · 2025-02-20T00:04:27Z

Expected: Exited with 0 return code
     but: CLI command exited with 1
STDOUT:
Installing a plugin from standard input

STDERR:
-name is deprecated; it is no longer necessary nor honored.

ERROR: Unexpected exception occurred while performing install-plugin command.
jenkins.RestartRequiredException
 at hudson.PluginManager.dynamicLoad(PluginManager.java:957)
 at hudson.PluginManager.dynamicLoad(PluginManager.java:927)
 at hudson.cli.InstallPluginCommand.run(InstallPluginCommand.java:100)
 at hudson.cli.CLICommand.main(CLICommand.java:258)
 at hudson.cli.CLICommandInvoker.invoke(CLICommandInvoker.java:156)
 at hudson.cli.CLICommandInvoker.invokeWithArgs(CLICommandInvoker.java:135)
 at hudson.cli.EnablePluginCommandTest.installTestPlugin(EnablePluginCommandTest.java:54)
 at hudson.cli.EnablePluginCommandTest.enableNoPluginsWithRestartIsNoOp(EnablePluginCommandTest.java:143)

smells like a regression at least in test code?

github-actions · 2025-02-20T02:29:32Z

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

dwnusbaum · 2025-02-20T14:59:09Z

smells like a regression at least in test code?

Ah, yes, EnablePluginCommandTest.enableNoPluginsWithRestartIsNoOp tries to install variant dynamically, which no longer works because I had to add variant as a test-scope dependency. I'll fix it.

dwnusbaum

There were quite a few tests that were installing variant dynamically. I tried to fix the failing test and clean things up in 99b8be8.

dwnusbaum · 2025-02-20T17:48:47Z

test/src/test/java/hudson/PluginManagerTest.java

-     */
-    @Issue("JENKINS-60449")
-    @WithPlugin("variant.hpi")
-    @Test public void installDependedOptionalPluginWithoutRestart() throws Exception {


Moved into ExtensionListRjrTest.installDependedOptionalPluginWithoutRestart.

Really, most of the tests here should probably be migrated to use RealJenkinsRule and synthetic plugins, but there are a lot of them.

dwnusbaum · 2025-02-20T17:50:47Z

test/src/test/java/hudson/PluginManagerTest.java

-    //
-    @Issue("JENKINS-50336")
-    @Test
-    public void optionalExtensionCanBeFoundAfterDynamicLoadOfVariant() throws Exception {


Also moved into ExtensionListRjrTest.installDependedOptionalPluginWithoutRestart.

I did make some changes though to the test to make it more closely match the scenario described in JENKINS-50336 (also so that I could delete variant.hpi).

dwnusbaum · 2025-02-20T17:51:32Z

test/src/test/java/hudson/cli/EnablePluginCommandTest.java

    public void enableNoPluginsWithRestartIsNoOp() {
        assumeNotWindows();
-        String name = "variant";
+        String name = "icon-shim";


Just swapping for another no-op plugin that is already here. Probably all of these tests should be using RealJenkinsRule and synthetic plugins though.

jglick · 2025-02-20T18:39:00Z

test/src/test/java/hudson/ExtensionListRjrTest.java

+    @Issue({ "JENKINS-50336", "JENKINS-60449" })
+    public void installDependedOptionalPluginWithoutRestart() throws Throwable {
+        var optionalDependerJpi = rjr.createSyntheticPlugin(new SyntheticPlugin(OptionalDepender.class.getPackage())
+                .header("Plugin-Dependencies", "variant:0,dependee:0;resolution:=optional"));


A lot easier to follow the test, not to say edit it!

jglick · 2025-02-20T18:39:54Z

test/src/test/java/hudson/cli/DisablePluginCommandTest.java

@@ -190,11 +190,10 @@ public void disableAlreadyDisabledPluginNotRestart() throws Exception {
    @Ignore("TODO calling restart seems to break Surefire")


Clearly could stand to be rewritten.

dwnusbaum · 2025-02-20T21:19:14Z

I think the hudson.model.RunTest.preventXssInBadgeTooltip test failure is an unrelated flake, or at least I don't see any obvious relation to this PR.

timja · 2025-02-20T22:30:19Z

I think the hudson.model.RunTest.preventXssInBadgeTooltip test failure is an unrelated flake, or at least I don't see any obvious relation to this PR.

likely

FWIW this PR triggered narrowing down a bit jenkinsci/avatar-plugin#18.

I've tested and this PR doesn't resolve it though, but seems to be another issue related to dynamic loading.
The plugin is pretty simple although I didn't manage to trigger the same issue in e.g. theme manager.

timja · 2025-02-26T16:50:49Z

/label ready-for-merge

This PR is now ready for merge, after the upcoming security release, we will merge it if there's no negative feedback.

Thanks!

A1exKH

LGTM.

[JENKINS-75232] Prevent dynamic plugin installation from registering …

70faaaf

…the same extension twice in some cases Co-authored-by: Julien Greffe <jgreffe@cloudbees.com>

dwnusbaum added the bug For changelog: Minor bug. Will be listed after features label Feb 4, 2025

dwnusbaum commented Feb 4, 2025

View reviewed changes

dwnusbaum marked this pull request as ready for review February 4, 2025 21:01

krisstern previously approved these changes Feb 14, 2025

View reviewed changes

dwnusbaum requested review from jglick and jtnord February 14, 2025 14:42

jglick reviewed Feb 14, 2025

View reviewed changes

dwnusbaum marked this pull request as draft February 14, 2025 16:00

jtnord reviewed Feb 14, 2025

View reviewed changes

[JENKINS-75232] Simplify ExtensionList.refresh

24f86ad

dwnusbaum mentioned this pull request Feb 18, 2025

Introduce RealJenkinsRule.createSyntheticPlugin to allow synthetic plugins to be installed dynamically and break compatibility for existing callers of RealJenkinsRule.addSyntheticPlugin jenkinsci/jenkins-test-harness#920

Merged

6 tasks

dwnusbaum added 2 commits February 18, 2025 13:46

[JENKINS-75232] Prevent duplicates in ExtensionList.refresh

6817532

[JENKINS-75232] Use RJR SyntheticPlugin for test and cover constructo…

4b33059

…r-related case

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Feb 18, 2025

Merge branch 'master' into JENKINS-75232

acba892

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Feb 18, 2025

jtnord reviewed Feb 19, 2025

View reviewed changes

core/src/main/java/hudson/ExtensionList.java Outdated Show resolved Hide resolved

dwnusbaum added 4 commits February 19, 2025 12:52

[JENKINS-75232] Only fire listeners if a new component was actually a…

5b702a0

…dded

[JENKINS-75232] No need for scope in dependencyManagement

6c25f40

[JENKINS-75232] Delete initial test

b2e5e15

[JENKINS-75232] Adapt to latest JTH changes

8af7aec

jtnord approved these changes Feb 19, 2025

View reviewed changes

[JENKINS-75232] Update to non-incremental version of JTH

b4b60cd

dwnusbaum marked this pull request as ready for review February 19, 2025 18:24

dwnusbaum requested review from jglick and krisstern February 19, 2025 18:25

[JENKINS-75232] Still need to use OptionalExtension when using Option…

d23f8ba

…alPackage

jglick approved these changes Feb 20, 2025

View reviewed changes

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Feb 20, 2025

dwnusbaum and others added 2 commits February 20, 2025 12:36

[JENKINS-75232] Update tests that were installing variant dynamically

99b8be8

Merge branch 'master' into JENKINS-75232

2889db2

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Feb 20, 2025

dwnusbaum commented Feb 20, 2025

View reviewed changes

jglick approved these changes Feb 20, 2025

View reviewed changes

Merge branch 'master' into JENKINS-75232

f73026b

jglick mentioned this pull request Feb 21, 2025

Implement afterDisconnect in SimpleCommandLauncher jenkinsci/jenkins-test-harness#922

Merged

dwnusbaum added 2 commits February 24, 2025 13:14

Merge branch 'master' into JENKINS-75232

a7ac415

Merge branch 'master' into JENKINS-75232

6bf3b09

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 26, 2025

A1exKH approved these changes Mar 3, 2025

View reviewed changes

dwnusbaum mentioned this pull request Mar 4, 2025

Remove default-allowlist entries for Groovy source files related to Declarative jenkinsci/workflow-cps-plugin#538

Merged

6 tasks

krisstern merged commit e49d4e7 into jenkinsci:master Mar 5, 2025
18 checks passed

		@Restricted(NoExternalUse.class)
		public boolean refresh(ExtensionComponentSet delta) {

		@@ -190,11 +190,10 @@ public void disableAlreadyDisabledPluginNotRestart() throws Exception {
		@Ignore("TODO calling restart seems to break Surefire")

Uh oh!

Conversation

dwnusbaum commented Feb 4, 2025 • edited by krisstern Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Desired reviewers

Uh oh!

dwnusbaum Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

dwnusbaum Feb 4, 2025 • edited by jglick Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtnord left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum commented Feb 14, 2025

Uh oh!

jtnord commented Feb 14, 2025

Uh oh!

dwnusbaum commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtnord commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 18, 2025

Uh oh!

dwnusbaum commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtnord left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

jglick commented Feb 20, 2025

Uh oh!

github-actions bot commented Feb 20, 2025

Uh oh!

dwnusbaum commented Feb 20, 2025

Uh oh!

dwnusbaum left a comment

Choose a reason for hiding this comment

Uh oh!

dwnusbaum Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

dwnusbaum Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

dwnusbaum Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

jglick Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

jglick Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

dwnusbaum commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dwnusbaum commented Feb 4, 2025 •

edited by krisstern

Loading

dwnusbaum Feb 4, 2025 •

edited by jglick

Loading

jtnord left a comment •

edited

Loading

dwnusbaum commented Feb 14, 2025 •

edited

Loading

jtnord commented Feb 15, 2025 •

edited

Loading

dwnusbaum commented Feb 18, 2025 •

edited

Loading

dwnusbaum commented Feb 20, 2025 •

edited

Loading

timja commented Feb 26, 2025 •

edited

Loading