[JENKINS-21486] Fix plugin dependencies resolution

* Check that dependencies are enabled. A disabled optional dependency will not prevent a plugin from loading. * Check versions of dependencies declared by a plugin before loading it. If any dependency (even optional) is older than what is required, then the plugin isn't loaded. This should prevent use cases where a plugin is loaded but one of its dependencies is too old so that : * its @extension annotated classes cannot be loaded, causing the full Jenkins to blow up with crapload of exceptions which are tedious to investigate to understand the root cause. * NoSuchMethodError and the likes at runtime even though boot has completed. Version check (for setups where version list is manually crafted but yet works) can be disabled by starting Jenkins with -Dhudson.PluginWrapper.dependenciesVersionCheck.enabled=true Minor fixes done while implementing this change : * Fix version parsing in PluginWrapper.Dependency * Dynamic plugin load didn't check for disabled flag
jenkinsci · Mar 30, 2016 · d57db1b · d57db1b
1 parent 83f51e0
commit d57db1b
Show file tree

Hide file tree

Showing 8 changed files with 172 additions and 11 deletions.
diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java
@@ -489,7 +489,8 @@ public void dynamicLoad(File arc) throws IOException, InterruptedException, Rest
         // so existing plugins can't be depending on this newly deployed one.
 
         plugins.add(p);
-        activePlugins.add(p);
+        if (p.isActive())
+            activePlugins.add(p);
         synchronized (((UberClassLoader) uberClassLoader).loaded) {
             ((UberClassLoader) uberClassLoader).loaded.clear();
         }

diff --git a/core/src/main/java/hudson/PluginWrapper.java b/core/src/main/java/hudson/PluginWrapper.java
@@ -87,6 +87,12 @@
  */
 @ExportedBean
 public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
+    /**
+     * A plugin won't be loaded unless his declared dependencies are present and match the required minimal version.
+     * This can be set to false to disable the version check (legacy behaviour)
+     */
+    public static final boolean ENABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK = Boolean.parseBoolean(System.getProperty(PluginWrapper.class.getName()+"." + "dependenciesVersionCheck.enabled", "true"));
+
     /**
      * {@link PluginManager} to which this belongs to.
      */
@@ -217,22 +223,27 @@ public Dependency(String s) {
             if(idx==-1)
                 throw new IllegalArgumentException("Illegal dependency specifier "+s);
             this.shortName = s.substring(0,idx);
-            this.version = s.substring(idx+1);
-            
+            String version = s.substring(idx+1);
+
             boolean isOptional = false;
-            String[] osgiProperties = s.split(";");
+            String[] osgiProperties = version.split("[;]");
             for (int i = 1; i < osgiProperties.length; i++) {
                 String osgiProperty = osgiProperties[i].trim();
                 if (osgiProperty.equalsIgnoreCase("resolution:=optional")) {
                     isOptional = true;
                 }
             }
             this.optional = isOptional;
+            if (isOptional) {
+                this.version = osgiProperties[0];
+            } else {
+                this.version = version;
+            }
         }
 
         @Override
         public String toString() {
-            return shortName + " (" + version + ")";
+            return shortName + " (" + version + ") " + (optional ? "optional" : "");
         }        
     }
 
@@ -524,18 +535,60 @@ public boolean hasLicensesXml() {
      */
     /*package*/ void resolvePluginDependencies() throws IOException {
         List<String> missingDependencies = new ArrayList<String>();
+        List<String> obsoleteDependencies = new ArrayList<String>();
+        List<String> disabledDependencies = new ArrayList<String>();
         // make sure dependencies exist
         for (Dependency d : dependencies) {
-            if (parent.getPlugin(d.shortName) == null)
+            PluginWrapper dependency = parent.getPlugin(d.shortName);
+            if (dependency == null) {
                 missingDependencies.add(d.toString());
-        }
-        if (!missingDependencies.isEmpty())
-            throw new IOException("Dependency "+Util.join(missingDependencies, ", ")+" doesn't exist");
+            } else {
+                if (dependency.isActive()) {
+                    if (ENABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK && dependency.getVersionNumber().isOlderThan(new VersionNumber(d.version))) {
+                        obsoleteDependencies.add(dependency.getShortName() + "(" + dependency.getVersion() + " < " + d.version + ")");
+                    }
+                } else {
+                    disabledDependencies.add(d.toString());
+                }
 
+            }
+        }
         // add the optional dependencies that exists
         for (Dependency d : optionalDependencies) {
-            if (parent.getPlugin(d.shortName) != null)
-                dependencies.add(d);
+            PluginWrapper dependency = parent.getPlugin(d.shortName);
+            if (dependency != null && dependency.isActive()) {
+                if (ENABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK && dependency.getVersionNumber().isOlderThan(new VersionNumber(d.version))) {
+                    obsoleteDependencies.add(dependency.getShortName() + "(" + dependency.getVersion() + " < " + d.version + ")");
+                } else {
+                    dependencies.add(d);
+                }
+            }
+        }
+        StringBuilder messageBuilder = new StringBuilder();
+        if (!missingDependencies.isEmpty()) {
+            boolean plural = missingDependencies.size() > 1;
+            messageBuilder.append(plural ? "Dependencies " : "Dependency ")
+                    .append(Util.join(missingDependencies, ", "))
+                    .append(" ").append(plural ? "don't" : "doesn't")
+                    .append(" exist. ");
+        }
+        if (!disabledDependencies.isEmpty()) {
+            boolean plural = disabledDependencies.size() > 1;
+            messageBuilder.append(plural ? "Dependencies " : "Dependency ")
+                    .append(Util.join(missingDependencies, ", "))
+                    .append(" ").append(plural ? "are" : "is")
+                    .append(" disabled. ");
+        }
+        if (!obsoleteDependencies.isEmpty()) {
+            boolean plural = obsoleteDependencies.size() > 1;
+            messageBuilder.append(plural ? "Dependencies " : "Dependency ")
+                    .append(Util.join(obsoleteDependencies, ", "))
+                    .append(" ").append(plural ? "are" : "is")
+                    .append(" older than required.");
+        }
+        String message = messageBuilder.toString();
+        if (!message.isEmpty()) {
+            throw new IOException(message);
         }
     }
 

diff --git a/test/src/test/java/hudson/PluginManagerTest.java b/test/src/test/java/hudson/PluginManagerTest.java
@@ -45,6 +45,7 @@
 import org.apache.commons.io.FileUtils;
 import org.apache.tools.ant.filters.StringInputStream;
 import static org.junit.Assert.*;
+
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -337,6 +338,75 @@ private String callDependerValue() throws Exception {
         assertTrue(r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.dependee.DependeeExtensionPoint").isEmpty());
     }
 
+    @Issue("JENKINS-21486")
+    @Test public void installPluginWithObsoleteDependencyFails() throws Exception {
+        // Load dependee 0.0.1.
+        {
+            dynamicLoad("dependee.hpi");
+        }
+
+        // Load mandatory-depender 0.0.2, depending on dependee 0.0.2
+        try {
+            dynamicLoad("mandatory-depender-0.0.2.hpi");
+            fail("Should not have worked");
+        } catch (IOException e) {
+            // Expected
+        }
+    }
+
+    @Issue("JENKINS-21486")
+    @Test public void installPluginWithDisabledOptionalDependencySucceeds() throws Exception {
+        // Load dependee 0.0.2.
+        {
+            dynamicLoadAndDisable("dependee-0.0.2.hpi");
+        }
+
+        // Load depender 0.0.2, depending optionally on dependee 0.0.2
+        {
+            dynamicLoad("depender-0.0.2.hpi");
+        }
+
+        // dependee is not loaded so we cannot list any extension for it.
+        try {
+            r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.dependee.DependeeExtensionPoint");
+            fail();
+        } catch( ClassNotFoundException _ ){
+        }
+    }
+
+    @Issue("JENKINS-21486")
+    @Test public void installPluginWithDisabledDependencyFails() throws Exception {
+        // Load dependee 0.0.2.
+        {
+            dynamicLoadAndDisable("dependee-0.0.2.hpi");
+        }
+
+        // Load mandatory-depender 0.0.2, depending on dependee 0.0.2
+        try {
+            dynamicLoad("mandatory-depender-0.0.2.hpi");
+            fail("Should not have worked");
+        } catch (IOException e) {
+            // Expected
+        }
+    }
+
+
+    @Issue("JENKINS-21486")
+    @Test public void installPluginWithObsoleteOptionalDependencyFails() throws Exception {
+        // Load dependee 0.0.1.
+        {
+            dynamicLoad("dependee.hpi");
+        }
+
+        // Load depender 0.0.2, depending optionally on dependee 0.0.2
+        try {
+            dynamicLoad("depender-0.0.2.hpi");
+            fail("Should not have worked");
+        } catch (IOException e) {
+            // Expected
+        }
+    }
+
     @Issue("JENKINS-12753")
     @WithPlugin("tasks.jpi")
     @Test public void dynamicLoadRestartRequiredException() throws Exception {
@@ -358,4 +428,8 @@ private String callDependerValue() throws Exception {
     private void dynamicLoad(String plugin) throws IOException, InterruptedException, RestartRequiredException {
         PluginManagerUtil.dynamicLoad(plugin, r.jenkins);
     }
+
+    private void dynamicLoadAndDisable(String plugin) throws IOException, InterruptedException, RestartRequiredException {
+        PluginManagerUtil.dynamicLoad(plugin, r.jenkins, true);
+    }
 }
diff --git a/test/src/test/java/hudson/PluginManagerUtil.java b/test/src/test/java/hudson/PluginManagerUtil.java
@@ -48,9 +48,16 @@ public void before() throws Throwable {
     }
 
     public static void dynamicLoad(String plugin, Jenkins jenkins) throws IOException, InterruptedException, RestartRequiredException {
+        dynamicLoad(plugin, jenkins, false);
+    }
+
+    public static void dynamicLoad(String plugin, Jenkins jenkins, boolean disable) throws IOException, InterruptedException, RestartRequiredException {
         URL src = PluginManagerTest.class.getClassLoader().getResource("plugins/" + plugin);
         File dest = new File(jenkins.getRootDir(), "plugins/" + plugin);
         FileUtils.copyURLToFile(src, dest);
+        if (disable) {
+            new File(dest.getPath() + ".disabled").createNewFile();
+        }
         jenkins.pluginManager.dynamicLoad(dest);
     }
 }
diff --git a/test/src/test/java/hudson/PluginWrapperTest.java b/test/src/test/java/hudson/PluginWrapperTest.java
@@ -0,0 +1,26 @@
+package hudson;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+public class PluginWrapperTest {
+
+    @Test
+    public void dependencyTest() {
+        String version = "plugin:0.0.2";
+        PluginWrapper.Dependency dependency = new PluginWrapper.Dependency(version);
+        assertEquals("plugin", dependency.shortName);
+        assertEquals("0.0.2", dependency.version);
+        assertEquals(false, dependency.optional);
+    }
+
+    @Test
+    public void optionalDependencyTest() {
+        String version = "plugin:0.0.2;resolution:=optional";
+        PluginWrapper.Dependency dependency = new PluginWrapper.Dependency(version);
+        assertEquals("plugin", dependency.shortName);
+        assertEquals("0.0.2", dependency.version);
+        assertEquals(true, dependency.optional);
+    }
+}
diff --git a/test/src/test/resources/plugins/dependee-0.0.2.hpi b/test/src/test/resources/plugins/dependee-0.0.2.hpi
diff --git a/test/src/test/resources/plugins/depender-0.0.2.hpi b/test/src/test/resources/plugins/depender-0.0.2.hpi
diff --git a/test/src/test/resources/plugins/mandatory-depender-0.0.2.hpi b/test/src/test/resources/plugins/mandatory-depender-0.0.2.hpi