[FIXED JENKINS-37041] Ensure that detached plugins are always at least their minimum version #2489

stephenc · 2016-07-28T15:14:05Z

Ok, so here was the test scenario:

Upgrade Jenkins (this will upgrade the detached plugin)
Stop Jenkins
Overwrite the detached plugin with the older version
Start Jenkins
Watch as the world burns

With this fix, under my (slightly contrived) test scenario I now get

INFO: Started initialization
Jul 28, 2016 4:07:38 PM hudson.PluginManager loadDetachedPlugins
WARNING: Detached plugin bouncycastle-api found at version 1.648.3, required minimum version is 2.16.0
...
INFO: Initializing Bouncy Castle security provider.
Jul 28, 2016 4:07:39 PM jenkins.bouncycastle.api.SecurityProviderInitializer addSecurityProvider
INFO: Bouncy Castle security provider initialized.
Jul 28, 2016 4:07:42 PM jenkins.InitReactorRunner$1 onAttained

So this fixed the only code path that I could find that remained whereby a plugin that has been detached would not get upgraded (namely if the admin mistakenly overwrites the detached plugin with an older version than the stated minimum version)

CRITICAL IMHO that this lands in 2.16

@reviewbybees @jenkinsci/code-reviewers


        [FIXED JENKINS-37041] Ensure that detached plugins are always at leas…

…t their minimum version

reviewbybees · 2016-07-28T15:15:32Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick · 2016-07-28T15:25:35Z

core/src/main/java/hudson/ClassicPluginStrategy.java

+         * @return the minimum required version for the current version of Jenkins.
+         * @sice 2.16
+         */
+        public VersionNumber getRequireVersion() {


🐜 rename to getRequiredVersion (and rename field & constructor parameter to match)

daniel-beck · 2016-07-28T15:26:13Z

Isn't this usually something that usually shows up in the detached plugin admin monitor?

Not opposed, just wondering how they relate.

jglick · 2016-07-28T15:27:06Z

core/src/main/java/hudson/PluginManager.java

+            final Set<ClassicPluginStrategy.DetachedPlugin> forceUpgrade = new HashSet<>();
+            for (ClassicPluginStrategy.DetachedPlugin p : ClassicPluginStrategy.getDetachedPlugins()) {
+                VersionNumber installedVersion = getPluginVersion(rootDir, p.getShortName());
+                VersionNumber requireVersion = p.getRequireVersion();


🐜 ditto here

jglick · 2016-07-28T15:29:00Z

Argh

npm ERR! shasum check failed for /tmp/npm-1725-b4cb96c5/registry.npmjs.org/jshint/-/jshint-2.9.2.tgz

jglick · 2016-07-28T15:29:07Z

🐝


        [JENKINS-37041] Address Code review comments

stephenc · 2016-07-28T15:33:39Z

@daniel-beck so I believe the scenario that I have fixed addresses the root issue that @oleg-nenashev was trying to fix in #2436 (though that particular PR seems overly heavyweight and complex to fix this problem)

In the specific case of 2.16... you will be in a world of pain if bouncycastle does not load and hence you may not (depending on the other plugins you have installed) be even able to see the admin monitor... given that we have the plugin in the detached list and can therefore fix things for the user before unpacking the plugin archives, seems only prudent to fix things for them

abayer · 2016-07-28T15:45:39Z

🐝 assuming tests pass.

jglick · 2016-07-28T15:53:41Z

re🐝

stephenc · 2016-07-28T15:57:30Z

@reviewbybees done

stephenc · 2016-07-28T15:58:32Z

I'm going to wait for the tests... @jenkinsci/code-reviewers as I view this fix as critical, anyone who wants to object has until the tests pass before I hit merge

stephenc · 2016-07-28T15:59:31Z

@jenkinsci/code-reviewers if you want extra time to review, just say so and I will wait... but if I see the tests have passed and nobody else has either objected or asked for more time to review, then I will hit merge ;-)

reviewbybees · 2016-07-28T16:02:31Z

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

stephenc · 2016-07-28T20:30:42Z

Ok tests passed

oleg-nenashev · 2016-07-28T20:49:55Z

5 hours timeout is definitely not enough for @jenkinsci/code-reviewers :sadpanda:

oleg-nenashev · 2016-07-28T20:53:48Z

The code looks good to me, but I'm not so happy about merging such highly-important changes without any tests

daniel-beck · 2016-08-18T11:27:49Z

This may have caused JENKINS-37332.

[FIXED JENKINS-37041] Ensure that detached plugins are always at leas…

Loading status checks…

2a450e5

…t their minimum version

jglick reviewed Jul 28, 2016
View changes

jglick closed this Jul 28, 2016

jglick reopened this Jul 28, 2016

[JENKINS-37041] Address Code review comments

Loading status checks…

d92fd4c

daniel-beck mentioned this pull request Jul 28, 2016

[JENKINS-36441] - Provide a bundling mechanism for plugins being unbundled from the core #2436

Closed

3 of 5 tasks complete

stephenc merged commit 841cba5 into jenkinsci:master Jul 28, 2016
1 check passed

1 check passed

Jenkins This pull request looks good
Details

stephenc deleted the stephenc:jenkins-37041 branch Jul 28, 2016

daniel-beck mentioned this pull request Mar 19, 2017

[JENKINS-42724] - Restore the windows-service/jenkins.xml resource to restore compatibility with windows-slaves 1.2 #2803

Merged

4 of 4 tasks complete

jglick mentioned this pull request Mar 23, 2017

Update detached plugins to reflect security releases #2817

Merged

daniel-beck mentioned this pull request Oct 7, 2019

Do not update detached plugins on Jenkins update #4275

Closed

0 of 4 tasks complete

jenkinsci / jenkins

[FIXED JENKINS-37041] Ensure that detached plugins are always at least their minimum version #2489

[FIXED JENKINS-37041] Ensure that detached plugins are always at least their minimum version #2489

stephenc commented Jul 28, 2016

reviewbybees commented Jul 28, 2016

This comment has been minimized.

daniel-beck commented Jul 28, 2016

This comment has been minimized.

jglick commented Jul 28, 2016

jglick commented Jul 28, 2016

stephenc commented Jul 28, 2016

abayer commented Jul 28, 2016

jglick commented Jul 28, 2016

stephenc commented Jul 28, 2016

stephenc commented Jul 28, 2016

stephenc commented Jul 28, 2016

reviewbybees commented Jul 28, 2016

stephenc commented Jul 28, 2016

oleg-nenashev commented Jul 28, 2016

oleg-nenashev commented Jul 28, 2016 •

edited

daniel-beck commented Aug 18, 2016

jenkinsci / jenkins

Sponsor jenkinsci/jenkins

Join GitHub today

[FIXED JENKINS-37041] Ensure that detached plugins are always at least their minimum version #2489

[FIXED JENKINS-37041] Ensure that detached plugins are always at least their minimum version #2489

Conversation

stephenc commented Jul 28, 2016

reviewbybees commented Jul 28, 2016

This comment has been minimized.

jglick Jul 28, 2016 • edited

daniel-beck commented Jul 28, 2016

This comment has been minimized.

jglick Jul 28, 2016

jglick commented Jul 28, 2016

jglick commented Jul 28, 2016

stephenc commented Jul 28, 2016

abayer commented Jul 28, 2016

jglick commented Jul 28, 2016

stephenc commented Jul 28, 2016

stephenc commented Jul 28, 2016

stephenc commented Jul 28, 2016

reviewbybees commented Jul 28, 2016

stephenc commented Jul 28, 2016

oleg-nenashev commented Jul 28, 2016

oleg-nenashev commented Jul 28, 2016 • edited

daniel-beck commented Aug 18, 2016

jglick Jul 28, 2016 •

edited

oleg-nenashev commented Jul 28, 2016 •

edited