Add support in plugins for a separate spi classloader

modules/lang-painless/build.gradle

@@ -23,14 +23,19 @@ testClusters.all {
   systemProperty 'es.transport.cname_in_publish_address', 'true'
 }
 
+configurations {
+  spi
+  compileOnlyApi.extendsFrom(spi)
+}
+
 dependencies {
   api 'org.antlr:antlr4-runtime:4.5.3'
   api 'org.ow2.asm:asm-util:7.2'
   api 'org.ow2.asm:asm-tree:7.2'
   api 'org.ow2.asm:asm-commons:7.2'
   api 'org.ow2.asm:asm-analysis:7.2'
   api 'org.ow2.asm:asm:7.2'
-  api project('spi')
+  spi project('spi')
 }
 
 tasks.named("dependencyLicenses").configure {
@@ -58,7 +63,7 @@ tasks.named("yamlRestCompatTest").configure {
  * Painless plugin */
 tasks.register("apiJavadoc", Javadoc) {
   source = sourceSets.main.allJava
-  classpath = sourceSets.main.runtimeClasspath
+  classpath = sourceSets.main.runtimeClasspath + configurations.spi.classpath
   include '**/org/elasticsearch/painless/api/'
   destinationDir = new File(docsDir, 'apiJavadoc')
 }
@@ -71,6 +76,13 @@ tasks.register("apiJavadocJar", Jar) {
 tasks.named("assemble").configure {
   dependsOn "apiJavadocJar"
 }
+
+tasks.named("bundlePlugin").configure {
+  it.into("spi") {
+    from(configurations.spi)
+  }
+}
+
 /**********************************************
  *           Context API Generation           *
  **********************************************/

modules/lang-painless/spi/build.gradle

@@ -13,6 +13,6 @@ group = 'org.elasticsearch.plugin'
 archivesBaseName = 'elasticsearch-scripting-painless-spi'
 
 dependencies {
-  api project(":server")
+  compileOnly project(":server")
   testImplementation project(":test:framework")
 }

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java

@@ -200,8 +200,7 @@ public void loadExtensions(ExtensionLoader loader) {
         loader.loadExtensions(PainlessExtension.class).stream()
             .flatMap(extension -> extension.getContextWhitelists().entrySet().stream())
             .forEach(entry -> {
-                List<Whitelist> existing = whitelists.computeIfAbsent(entry.getKey(),
-                    c -> new ArrayList<>(BASE_WHITELISTS));
+                List<Whitelist> existing = whitelists.computeIfAbsent(entry.getKey(), c -> new ArrayList<>());
                 existing.addAll(entry.getValue());
             });
     }

server/src/main/java/org/elasticsearch/bootstrap/PolicyUtil.java

@@ -287,6 +287,19 @@ static PluginPolicyInfo readPolicyInfo(Path pluginRoot) throws IOException {
                 }
             }
         }
+        // also add spi jars
+        // TODO: move this to a shared function, or fix plugin layout to have jar files in lib directory
+        Path spiDir = pluginRoot.resolve("spi");
+        if (Files.exists(spiDir)) {
+            try (DirectoryStream<Path> jarStream = Files.newDirectoryStream(spiDir, "*.jar")) {
+                for (Path jar : jarStream) {
+                    URL url = jar.toRealPath().toUri().toURL();
+                    if (jars.add(url) == false) {
+                        throw new IllegalStateException("duplicate module/plugin: " + url);
+                    }
+                }
+            }
+        }
 
         // parse the plugin's policy file into a set of permissions
         Policy policy = readPolicy(policyFile.toUri().toURL(), getCodebaseJarMap(jars));

server/src/main/java/org/elasticsearch/plugins/PluginsService.java

@@ -235,9 +235,24 @@ public PluginsAndModules info() {
     static class Bundle {
         final PluginInfo plugin;
         final Set<URL> urls;
+        final Set<URL> spiUrls;
+        final Set<URL> allUrls;
 
         Bundle(PluginInfo plugin, Path dir) throws IOException {
             this.plugin = Objects.requireNonNull(plugin);
+
+            Path spiDir = dir.resolve("spi");
+            // plugin has defined an explicit api for extension
+            this.spiUrls = Files.exists(spiDir) ? gatherUrls(spiDir) : null;
+            this.urls = gatherUrls(dir);
+            Set<URL> allUrls = new LinkedHashSet<>(urls);
+            if (spiUrls != null) {
+                allUrls.addAll(spiUrls);
+            }
+            this.allUrls = allUrls;
+        }
+
+        static Set<URL> gatherUrls(Path dir) throws IOException {
             Set<URL> urls = new LinkedHashSet<>();
             // gather urls for jar files
             try (DirectoryStream<Path> jarStream = Files.newDirectoryStream(dir, "*.jar")) {
@@ -249,7 +264,14 @@ static class Bundle {
                     }
                 }
             }
-            this.urls = Objects.requireNonNull(urls);
+            return urls;
+        }
+
+        Set<URL> getExtensionUrls() {
+            if (spiUrls != null) {
+                return spiUrls;
+            }
+            return urls;
         }
 
         @Override
@@ -424,7 +446,7 @@ private static void addSortedBundle(Bundle bundle, Map<String, Bundle> bundles,
 
     private List<Tuple<PluginInfo,Plugin>> loadBundles(Set<Bundle> bundles) {
         List<Tuple<PluginInfo, Plugin>> plugins = new ArrayList<>();
-        Map<String, Plugin> loaded = new HashMap<>();
+        Map<String, Tuple<Plugin, ClassLoader>> loaded = new HashMap<>();
         Map<String, Set<URL>> transitiveUrls = new HashMap<>();
         List<Bundle> sortedBundles = sortBundles(bundles);
         for (Bundle bundle : sortedBundles) {
@@ -530,67 +552,84 @@ static void checkBundleJarHell(Set<URL> classpath, Bundle bundle, Map<String, Se
 
         try {
             final Logger logger = LogManager.getLogger(JarHell.class);
-            Set<URL> urls = new HashSet<>();
+            Set<URL> extendedPluginUrls = new HashSet<>();
             for (String extendedPlugin : exts) {
                 Set<URL> pluginUrls = transitiveUrls.get(extendedPlugin);
                 assert pluginUrls != null : "transitive urls should have already been set for " + extendedPlugin;
 
-                Set<URL> intersection = new HashSet<>(urls);
+                // consistency check: extended plugins should not have duplicate codebases with each other
+                Set<URL> intersection = new HashSet<>(extendedPluginUrls);
                 intersection.retainAll(pluginUrls);
                 if (intersection.isEmpty() == false) {
                     throw new IllegalStateException("jar hell! extended plugins " + exts +
                                                     " have duplicate codebases with each other: " + intersection);
                 }
 
-                intersection = new HashSet<>(bundle.urls);
+                // jar hell check: extended plugins (so far) do not have jar hell with each other
+                extendedPluginUrls.addAll(pluginUrls);
+                JarHell.checkJarHell(extendedPluginUrls, logger::debug);
+
+                // consistency check: each extended plugin should not have duplicate codebases with implementation+spi of this plugin
+                intersection = new HashSet<>(bundle.allUrls);
                 intersection.retainAll(pluginUrls);
                 if (intersection.isEmpty() == false) {
                     throw new IllegalStateException("jar hell! duplicate codebases with extended plugin [" +
-                                                    extendedPlugin + "]: " + intersection);
+                        extendedPlugin + "]: " + intersection);
                 }
 
-                urls.addAll(pluginUrls);
-                JarHell.checkJarHell(urls, logger::debug); // check jarhell as we add each extended plugin's urls
+                // jar hell check: extended plugins (so far) do not have jar hell with implementation+spi of this plugin
+                Set<URL> implementation = new HashSet<>(bundle.allUrls);
+                implementation.addAll(extendedPluginUrls);
+                JarHell.checkJarHell(implementation, logger::debug);
             }
 
-            urls.addAll(bundle.urls);
-            JarHell.checkJarHell(urls, logger::debug); // check jarhell of each extended plugin against this plugin
-            transitiveUrls.put(bundle.plugin.getName(), urls);
+            // Set transitive urls for other plugins to extend this plugin. Note that jarhell has already been checked above.
+            // This uses the extension urls (spi if set) since the implementation will not be in the transitive classpath at runtime.
+            extendedPluginUrls.addAll(bundle.getExtensionUrls());
+            transitiveUrls.put(bundle.plugin.getName(), extendedPluginUrls);
 
             // check we don't have conflicting codebases with core
             Set<URL> intersection = new HashSet<>(classpath);
-            intersection.retainAll(bundle.urls);
+            intersection.retainAll(bundle.allUrls);
             if (intersection.isEmpty() == false) {
                 throw new IllegalStateException("jar hell! duplicate codebases between plugin and core: " + intersection);
             }
             // check we don't have conflicting classes
             Set<URL> union = new HashSet<>(classpath);
-            union.addAll(bundle.urls);
+            union.addAll(bundle.allUrls);
             JarHell.checkJarHell(union, logger::debug);
         } catch (Exception e) {
             throw new IllegalStateException("failed to load plugin " + bundle.plugin.getName() + " due to jar hell", e);
         }
     }
 
-    private Plugin loadBundle(Bundle bundle, Map<String, Plugin> loaded) {
+    private Plugin loadBundle(Bundle bundle, Map<String, Tuple<Plugin, ClassLoader>> loaded) {
         String name = bundle.plugin.getName();
 
         verifyCompatibility(bundle.plugin);
 
         // collect loaders of extended plugins
         List<ClassLoader> extendedLoaders = new ArrayList<>();
         for (String extendedPluginName : bundle.plugin.getExtendedPlugins()) {
-            Plugin extendedPlugin = loaded.get(extendedPluginName);
+            Tuple<Plugin, ClassLoader> extendedPlugin = loaded.get(extendedPluginName);
             assert extendedPlugin != null;
-            if (ExtensiblePlugin.class.isInstance(extendedPlugin) == false) {
+            if (ExtensiblePlugin.class.isInstance(extendedPlugin.v1()) == false) {
                 throw new IllegalStateException("Plugin [" + name + "] cannot extend non-extensible plugin [" + extendedPluginName + "]");
             }
-            extendedLoaders.add(extendedPlugin.getClass().getClassLoader());
+            extendedLoaders.add(extendedPlugin.v2());
         }
 
-        // create a child to load the plugin in this bundle
         ClassLoader parentLoader = PluginLoaderIndirection.createLoader(getClass().getClassLoader(), extendedLoaders);
-        ClassLoader loader = URLClassLoader.newInstance(bundle.urls.toArray(new URL[0]), parentLoader);
+        ClassLoader spiLoader = null;
+        if (bundle.spiUrls != null) {
+            spiLoader = URLClassLoader.newInstance(bundle.spiUrls.toArray(new URL[0]), parentLoader);
+        }
+
+        ClassLoader loader = URLClassLoader.newInstance(bundle.urls.toArray(new URL[0]), spiLoader == null ? parentLoader : spiLoader);
+        if (spiLoader == null) {
+            // use full implementation for plugins extending this one
+            spiLoader = loader;
+        }
 
         // reload SPI with any new services from the plugin
         reloadLuceneSPI(loader);
@@ -612,7 +651,7 @@ private Plugin loadBundle(Bundle bundle, Map<String, Plugin> loaded) {
                     + "] (class loader [" + pluginClass.getClassLoader() + "])");
             }
             Plugin plugin = loadPlugin(pluginClass, settings, configPath);
-            loaded.put(name, plugin);
+            loaded.put(name, Tuple.tuple(plugin, spiLoader));
             return plugin;
         } finally {
             AccessController.doPrivileged((PrivilegedAction<Void>) () -> {

server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java

@@ -12,13 +12,13 @@
 import org.apache.lucene.util.Constants;
 import org.apache.lucene.util.LuceneTestCase;
 import org.elasticsearch.Version;
-import org.elasticsearch.jdk.JarHell;
-import org.elasticsearch.core.Tuple;
-import org.elasticsearch.core.PathUtils;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.PathUtils;
+import org.elasticsearch.core.Tuple;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.env.TestEnvironment;
 import org.elasticsearch.index.IndexModule;
+import org.elasticsearch.jdk.JarHell;
 import org.elasticsearch.test.ESTestCase;
 import org.hamcrest.Matchers;
 
@@ -564,6 +564,50 @@ public void testJarHellTransitiveMap() throws Exception {
         assertThat(deps, containsInAnyOrder(pluginJar.toUri().toURL(), dep1Jar.toUri().toURL(), dep2Jar.toUri().toURL()));
     }
 
+    public void testJarHellSpiAddedToTransitiveDeps() throws Exception {
+        Path pluginDir = createTempDir();
+        Path pluginJar = pluginDir.resolve("plugin.jar");
+        makeJar(pluginJar, DummyClass2.class);
+        Path spiDir = pluginDir.resolve("spi");
+        Files.createDirectories(spiDir);
+        Path spiJar = spiDir.resolve("spi.jar");
+        makeJar(spiJar, DummyClass3.class);
+        Path depDir = createTempDir();
+        Path depJar = depDir.resolve("dep.jar");
+        makeJar(depJar, DummyClass1.class);
+        Map<String, Set<URL>> transitiveDeps = new HashMap<>();
+        transitiveDeps.put("dep", Collections.singleton(depJar.toUri().toURL()));
+        PluginInfo info1 = new PluginInfo("myplugin", "desc", "1.0", Version.CURRENT, "1.8",
+            "MyPlugin", Collections.singletonList("dep"), false, PluginType.ISOLATED, "", false);
+        PluginsService.Bundle bundle = new PluginsService.Bundle(info1, pluginDir);
+        PluginsService.checkBundleJarHell(JarHell.parseClassPath(), bundle, transitiveDeps);
+        Set<URL> transitive = transitiveDeps.get("myplugin");
+        assertThat(transitive, containsInAnyOrder(spiJar.toUri().toURL(), depJar.toUri().toURL()));
+    }
+
+    public void testJarHellSpiConflict() throws Exception {
+        Path pluginDir = createTempDir();
+        Path pluginJar = pluginDir.resolve("plugin.jar");
+        makeJar(pluginJar, DummyClass2.class);
+        Path spiDir = pluginDir.resolve("spi");
+        Files.createDirectories(spiDir);
+        Path spiJar = spiDir.resolve("spi.jar");
+        makeJar(spiJar, DummyClass1.class);
+        Path depDir = createTempDir();
+        Path depJar = depDir.resolve("dep.jar");
+        makeJar(depJar, DummyClass1.class);
+        Map<String, Set<URL>> transitiveDeps = new HashMap<>();
+        transitiveDeps.put("dep", Collections.singleton(depJar.toUri().toURL()));
+        PluginInfo info1 = new PluginInfo("myplugin", "desc", "1.0", Version.CURRENT, "1.8",
+            "MyPlugin", Collections.singletonList("dep"), false, PluginType.ISOLATED, "", false);
+        PluginsService.Bundle bundle = new PluginsService.Bundle(info1, pluginDir);
+        IllegalStateException e = expectThrows(IllegalStateException.class, () ->
+            PluginsService.checkBundleJarHell(JarHell.parseClassPath(), bundle, transitiveDeps));
+        assertEquals("failed to load plugin myplugin due to jar hell", e.getMessage());
+        assertThat(e.getCause().getMessage(), containsString("jar hell!"));
+        assertThat(e.getCause().getMessage(), containsString("DummyClass1"));
+    }
+
     public void testNonExtensibleDep() throws Exception {
         // This test opens a child classloader, reading a jar under the test temp
         // dir (a dummy plugin). Classloaders are closed by GC, so when test teardown

x-pack/plugin/eql/build.gradle

@@ -17,9 +17,7 @@ archivesBaseName = 'x-pack-eql'
 
 dependencies {
   compileOnly project(path: xpackModule('core'))
-  compileOnly(project(':modules:lang-painless')) {
-    exclude group: "org.ow2.asm"
-  }
+  compileOnly(project(':modules:lang-painless:spi'))
   api "org.antlr:antlr4-runtime:${antlrVersion}"
   compileOnly project(xpackModule('ql'))
 

x-pack/plugin/eql/licenses/antlr4-runtime-4.5.3.jar.sha1

@@ -0,0 +1 @@
+2609e36f18f7e8d593cc1cddfb2ac776dc96b8e0

x-pack/plugin/eql/licenses/antlr4-runtime-LICENSE.txt

@@ -0,0 +1,26 @@
+[The "BSD license"]
+Copyright (c) 2015 Terence Parr, Sam Harwell
+All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions
+are met:
+
+ 1. Redistributions of source code must retain the above copyright
+    notice, this list of conditions and the following disclaimer.
+ 2. Redistributions in binary form must reproduce the above copyright
+    notice, this list of conditions and the following disclaimer in the
+    documentation and/or other materials provided with the distribution.
+ 3. The name of the author may not be used to endorse or promote products
+    derived from this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

x-pack/plugin/sql/build.gradle

@@ -30,9 +30,7 @@ archivesBaseName = 'x-pack-sql'
 
 dependencies {
   compileOnly project(path: xpackModule('core'))
-  compileOnly(project(':modules:lang-painless')) {
-    exclude group: "org.ow2.asm"
-  }
+  compileOnly(project(':modules:lang-painless:spi'))
   api project('sql-action')
   api project(':modules:aggs-matrix-stats')
   api "org.antlr:antlr4-runtime:${antlrVersion}"

x-pack/plugin/sql/licenses/antlr4-runtime-4.5.3.jar.sha1

@@ -0,0 +1 @@
+2609e36f18f7e8d593cc1cddfb2ac776dc96b8e0

x-pack/plugin/sql/licenses/antlr4-runtime-LICENSE.txt

@@ -0,0 +1,26 @@
+[The "BSD license"]
+Copyright (c) 2015 Terence Parr, Sam Harwell
+All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions
+are met:
+
+ 1. Redistributions of source code must retain the above copyright
+    notice, this list of conditions and the following disclaimer.
+ 2. Redistributions in binary form must reproduce the above copyright
+    notice, this list of conditions and the following disclaimer in the
+    documentation and/or other materials provided with the distribution.
+ 3. The name of the author may not be used to endorse or promote products
+    derived from this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.