Skip to content
Permalink
Browse files

[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
  • Loading branch information...
Vlatombe committed Mar 25, 2016
1 parent 83f51e0 commit d57db1b1f2e30917c337eabdc0c204a832fb8d0a
@@ -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();
}
@@ -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);
}
}

@@ -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);
}
}
@@ -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);
}
}
@@ -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);
}
}
Binary file not shown.
Binary file not shown.
Binary file not shown.

0 comments on commit d57db1b

Please sign in to comment.
You can’t perform that action at this time.