Enhance extensible plugin (#58542)

Rather than let ExtensiblePlugins know extending plugins' classloaders,
we now pass along an explicit ExtensionLoader that loads the extensions
asked for. Extensions constructed that way can optionally receive their
own Plugin instance in the constructor.

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

@@ -54,7 +54,6 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.ServiceLoader;
 import java.util.function.Supplier;
 
 /**
@@ -113,14 +112,14 @@ public final class PainlessPlugin extends Plugin implements ScriptPlugin, Extens
     }
 
     @Override
-    public void reloadSPI(ClassLoader loader) {
+    public void loadExtensions(ExtensionLoader loader) {
-        for (PainlessExtension extension : ServiceLoader.load(PainlessExtension.class, loader)) {
+        loader.loadExtensions(PainlessExtension.class).stream()
-            for (Map.Entry<ScriptContext<?>, List<Whitelist>> entry : extension.getContextWhitelists().entrySet()) {
+            .flatMap(extension -> extension.getContextWhitelists().entrySet().stream())
+            .forEach(entry -> {
                 List<Whitelist> existing = whitelists.computeIfAbsent(entry.getKey(),
                     c -> new ArrayList<>(Whitelist.BASE_WHITELISTS));
                 existing.addAll(entry.getValue());
-            }
+            });
-        }
     }
 
     @Override

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

@@ -19,6 +19,8 @@
 
 package org.elasticsearch.plugins;
 
+import java.util.List;
+
 /**
  * An extension point for {@link Plugin} implementations to be themselves extensible.
  *
@@ -27,8 +29,22 @@ package org.elasticsearch.plugins;
  */
 public interface ExtensiblePlugin {
 
+    interface ExtensionLoader {
         /**
-     * Reload any SPI implementations from the given classloader.
+         * Load extensions of the type from all extending plugins. The concrete extensions must have either a no-arg constructor
+         * or a single-arg constructor accepting the specific plugin class.
+         * @param extensionPointType the extension point type
+         * @param <T> extension point type
+         * @return all implementing extensions.
          */
-    default void reloadSPI(ClassLoader loader) {}
+        <T> List<T> loadExtensions(Class<T> extensionPointType);
+    }
+
+    /**
+     * Allow this plugin to load extensions from other plugins.
+     *
+     * This method is called once only, after initializing this plugin and all plugins extending this plugin. It is called before
+     * any other methods on this Plugin instance are called.
+     */
+    default void loadExtensions(ExtensionLoader loader) {}
 }

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

@@ -27,6 +27,7 @@ import org.apache.lucene.analysis.util.TokenizerFactory;
 import org.apache.lucene.codecs.Codec;
 import org.apache.lucene.codecs.DocValuesFormat;
 import org.apache.lucene.codecs.PostingsFormat;
+import org.apache.lucene.util.SPIClassIterator;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.admin.cluster.node.info.PluginsAndModules;
@@ -466,7 +467,6 @@ public class PluginsService implements ReportingService<PluginsAndModules> {
         Map<String, Plugin> loaded = new HashMap<>();
         Map<String, Set<URL>> transitiveUrls = new HashMap<>();
         List<Bundle> sortedBundles = sortBundles(bundles);
-
         for (Bundle bundle : sortedBundles) {
             checkBundleJarHell(JarHell.parseClassPath(), bundle, transitiveUrls);
 
@@ -474,9 +474,92 @@ public class PluginsService implements ReportingService<PluginsAndModules> {
             plugins.add(new Tuple<>(bundle.plugin, plugin));
         }
 
+        loadExtensions(plugins);
         return Collections.unmodifiableList(plugins);
     }
 
+    // package-private for test visibility
+    static void loadExtensions(List<Tuple<PluginInfo, Plugin>> plugins) {
+        Map<String, List<Plugin>> extendingPluginsByName = plugins.stream()
+            .flatMap(t -> t.v1().getExtendedPlugins().stream().map(extendedPlugin -> Tuple.tuple(extendedPlugin, t.v2())))
+            .collect(Collectors.groupingBy(Tuple::v1, Collectors.mapping(Tuple::v2, Collectors.toList())));
+        for (Tuple<PluginInfo, Plugin> pluginTuple : plugins) {
+            if (pluginTuple.v2() instanceof ExtensiblePlugin) {
+                loadExtensionsForPlugin((ExtensiblePlugin) pluginTuple.v2(),
+                    extendingPluginsByName.getOrDefault(pluginTuple.v1().getName(), Collections.emptyList()));
+            }
+        }
+    }
+
+    private static void loadExtensionsForPlugin(ExtensiblePlugin extensiblePlugin, List<Plugin> extendingPlugins) {
+        ExtensiblePlugin.ExtensionLoader extensionLoader = new ExtensiblePlugin.ExtensionLoader() {
+            @Override
+            public <T> List<T> loadExtensions(Class<T> extensionPointType) {
+                List<T> result = new ArrayList<>();
+                for (Plugin extendingPlugin : extendingPlugins) {
+                    result.addAll(createExtensions(extensionPointType, extendingPlugin));
+                }
+                return Collections.unmodifiableList(result);
+            }
+        };
+
+        extensiblePlugin.loadExtensions(extensionLoader);
+    }
+
+    private static <T> List<? extends T> createExtensions(Class<T> extensionPointType, Plugin plugin) {
+        SPIClassIterator<T> classIterator = SPIClassIterator.get(extensionPointType, plugin.getClass().getClassLoader());
+        List<T> extensions = new ArrayList<>();
+        while (classIterator.hasNext()) {
+            Class<? extends T> extensionClass = classIterator.next();
+            extensions.add(createExtension(extensionClass, extensionPointType, plugin));
+        }
+        return extensions;
+    }
+
+    // package-private for test visibility
+    static <T> T createExtension(Class<? extends T> extensionClass, Class<T> extensionPointType, Plugin plugin) {
+        //noinspection unchecked
+        Constructor<T>[] constructors = (Constructor<T>[]) extensionClass.getConstructors();
+        if (constructors.length == 0) {
+            throw new IllegalStateException("no public " + extensionConstructorMessage(extensionClass, extensionPointType));
+        }
+
+        if (constructors.length > 1) {
+            throw new IllegalStateException("no unique public " + extensionConstructorMessage(extensionClass, extensionPointType));
+        }
+
+        final Constructor<T> constructor = constructors[0];
+        if (constructor.getParameterCount() > 1) {
+            throw new IllegalStateException(extensionSignatureMessage(extensionClass, extensionPointType, plugin));
+        }
+
+        if (constructor.getParameterCount() == 1 && constructor.getParameterTypes()[0] != plugin.getClass()) {
+            throw new IllegalStateException(extensionSignatureMessage(extensionClass, extensionPointType, plugin) +
+                ", not (" + constructor.getParameterTypes()[0].getName() + ")");
+        }
+
+        try {
+            if (constructor.getParameterCount() == 0) {
+                return constructor.newInstance();
+            } else {
+                return constructor.newInstance(plugin);
+            }
+        } catch (ReflectiveOperationException e) {
+            throw new IllegalStateException(
+                "failed to create extension [" + extensionClass.getName() + "] of type [" + extensionPointType.getName() + "]", e
+            );
+        }
+    }
+
+    private static <T> String extensionSignatureMessage(Class<? extends T> extensionClass, Class<T> extensionPointType, Plugin plugin) {
+        return "signature of " + extensionConstructorMessage(extensionClass, extensionPointType) +
+            " must be either () or (" + plugin.getClass().getName() + ")";
+    }
+
+    private static <T> String extensionConstructorMessage(Class<? extends T> extensionClass, Class<T> extensionPointType) {
+        return "constructor for extension [" + extensionClass.getName() + "] of type [" + extensionPointType.getName() + "]";
+    }
+
     // jar-hell check the bundle against the parent classloader and extended plugins
     // the plugin cli does it, but we do it again, in case lusers mess with jar files manually
     static void checkBundleJarHell(Set<URL> classpath, Bundle bundle, Map<String, Set<URL>> transitiveUrls) {
@@ -549,12 +632,13 @@ public class PluginsService implements ReportingService<PluginsAndModules> {
 
         // reload SPI with any new services from the plugin
         reloadLuceneSPI(loader);
-        for (String extendedPluginName : bundle.plugin.getExtendedPlugins()) {
-            // note: already asserted above that extended plugins are loaded and extensible
-            ExtensiblePlugin.class.cast(loaded.get(extendedPluginName)).reloadSPI(loader);
-        }
 
         Class<? extends Plugin> pluginClass = loadPluginClass(bundle.plugin.getClassname(), loader);
+        if (loader != pluginClass.getClassLoader()) {
+            throw new IllegalStateException("Plugin [" + name + "] must reference a class loader local Plugin class ["
+                + bundle.plugin.getClassname()
+                + "] (class loader [" + pluginClass.getClassLoader() + "])");
+        }
         Plugin plugin = loadPlugin(pluginClass, settings, configPath);
         loaded.put(name, plugin);
         return plugin;

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

@@ -24,6 +24,7 @@ import org.apache.lucene.util.Constants;
 import org.apache.lucene.util.LuceneTestCase;
 import org.elasticsearch.Version;
 import org.elasticsearch.bootstrap.JarHell;
+import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.io.PathUtils;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
@@ -34,6 +35,7 @@ import org.hamcrest.Matchers;
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.lang.reflect.InvocationTargetException;
 import java.net.URL;
 import java.nio.file.FileSystemException;
 import java.nio.file.Files;
@@ -54,8 +56,11 @@ import java.util.zip.ZipOutputStream;
 
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.sameInstance;
 
 @LuceneTestCase.SuppressFileSystems(value = "ExtrasFS")
 public class PluginsServiceTests extends ESTestCase {
@@ -690,4 +695,164 @@ public class PluginsServiceTests extends ESTestCase {
                         .build();
         newPluginsService(settings);
     }
+
+    public void testPluginFromParentClassLoader() throws IOException {
+        final Path pathHome = createTempDir();
+        final Path plugins = pathHome.resolve("plugins");
+        final Path fake = plugins.resolve("fake");
+
+        PluginTestUtil.writePluginProperties(
+            fake,
+            "description", "description",
+            "name", "fake",
+            "version", "1.0.0",
+            "elasticsearch.version", Version.CURRENT.toString(),
+            "java.version", System.getProperty("java.specification.version"),
+            "classname", TestPlugin.class.getName()); // set a class defined outside the bundle (in parent class-loader of plugin)
+
+        final Settings settings =
+            Settings.builder()
+                .put("path.home", pathHome)
+                .put("plugin.mandatory", "fake")
+                .build();
+        IllegalStateException exception = expectThrows(IllegalStateException.class, () -> newPluginsService(settings));
+        assertThat(exception, hasToString(containsString("Plugin [fake] must reference a class loader local Plugin class [" +
+            TestPlugin.class.getName() + "] (class loader [" + PluginsServiceTests.class.getClassLoader() + "])")));
+    }
+
+    public void testExtensiblePlugin() {
+        TestExtensiblePlugin extensiblePlugin = new TestExtensiblePlugin();
+        PluginsService.loadExtensions(Collections.singletonList(
+            Tuple.tuple(new PluginInfo("extensible", null, null, null, null, null, Collections.emptyList(), false), extensiblePlugin)
+        ));
+
+        assertThat(extensiblePlugin.extensions, notNullValue());
+        assertThat(extensiblePlugin.extensions, hasSize(0));
+
+        extensiblePlugin = new TestExtensiblePlugin();
+        TestPlugin testPlugin = new TestPlugin();
+        PluginsService.loadExtensions(Arrays.asList(
+            Tuple.tuple(new PluginInfo("extensible", null, null, null, null, null, Collections.emptyList(), false), extensiblePlugin),
+            Tuple.tuple(new PluginInfo("test", null, null, null, null, null, Collections.singletonList("extensible"), false), testPlugin)
+        ));
+
+        assertThat(extensiblePlugin.extensions, notNullValue());
+        assertThat(extensiblePlugin.extensions, hasSize(2));
+        assertThat(extensiblePlugin.extensions.get(0), instanceOf(TestExtension1.class));
+        assertThat(extensiblePlugin.extensions.get(1), instanceOf(TestExtension2.class));
+        assertThat(((TestExtension2) extensiblePlugin.extensions.get(1)).plugin, sameInstance(testPlugin));
+    }
+
+    public void testNoExtensionConstructors() {
+        TestPlugin plugin = new TestPlugin();
+        class TestExtension implements TestExtensionPoint {
+            private TestExtension() {
+            }
+        }
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(TestExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e, hasToString(containsString("no public constructor for extension [" + TestExtension.class.getName() +
+            "] of type [" + TestExtensionPoint.class.getName() + "]")));
+    }
+
+    public void testMultipleExtensionConstructors() {
+        TestPlugin plugin = new TestPlugin();
+        class TestExtension implements TestExtensionPoint {
+            public TestExtension() {
+            }
+            public TestExtension(TestPlugin plugin) {
+
+            }
+        }
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(TestExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e, hasToString(containsString("no unique public constructor for extension [" + TestExtension.class.getName() +
+            "] of type [" + TestExtensionPoint.class.getName() + "]")));
+    }
+
+    public void testBadSingleParameterConstructor() {
+        TestPlugin plugin = new TestPlugin();
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(BadSingleParameterConstructorExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e,
+            hasToString(containsString("signature of constructor for extension [" + BadSingleParameterConstructorExtension.class.getName() +
+                "] of type [" + TestExtensionPoint.class.getName() + "] must be either () or (" + TestPlugin.class.getName() + "), not (" +
+                String.class.getName() + ")")));
+    }
+
+    public void testTooManyParametersExtensionConstructors() {
+        TestPlugin plugin = new TestPlugin();
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(TooManyParametersConstructorExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e,
+            hasToString(containsString("signature of constructor for extension [" + TooManyParametersConstructorExtension.class.getName() +
+                "] of type [" + TestExtensionPoint.class.getName() + "] must be either () or (" + TestPlugin.class.getName() + ")")));
+    }
+
+    public void testThrowingConstructor() {
+        TestPlugin plugin = new TestPlugin();
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(ThrowingConstructorExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e,
+            hasToString(containsString("failed to create extension [" + ThrowingConstructorExtension.class.getName() +
+                "] of type [" + TestExtensionPoint.class.getName() + "]")));
+        assertThat(e.getCause(), instanceOf(InvocationTargetException.class));
+        assertThat(e.getCause().getCause(), instanceOf(IllegalArgumentException.class));
+        assertThat(e.getCause().getCause(), hasToString(containsString("test constructor failure")));
+    }
+
+    private static class TestExtensiblePlugin extends Plugin implements ExtensiblePlugin {
+        private List<TestExtensionPoint> extensions;
+
+        @Override
+        public void loadExtensions(ExtensionLoader loader) {
+            assert extensions == null;
+            extensions = loader.loadExtensions(TestExtensionPoint.class);
+            // verify unmodifiable.
+            expectThrows(UnsupportedOperationException.class, () -> extensions.add(new TestExtension1()));
+        }
+    }
+
+    public static class TestPlugin extends Plugin {
+    }
+
+    public interface TestExtensionPoint {
+    }
+
+    public static class TestExtension1 implements TestExtensionPoint {
+    }
+
+    public static class TestExtension2 implements TestExtensionPoint {
+        public Plugin plugin;
+
+        public TestExtension2(TestPlugin plugin) {
+            this.plugin = plugin;
+        }
+    }
+
+    public static class BadSingleParameterConstructorExtension implements TestExtensionPoint {
+        public BadSingleParameterConstructorExtension(String bad) {
+        }
+    }
+
+    public static class TooManyParametersConstructorExtension implements TestExtensionPoint {
+        public TooManyParametersConstructorExtension(String bad) {
+        }
+    }
+
+    public static class ThrowingConstructorExtension implements TestExtensionPoint {
+        public ThrowingConstructorExtension() {
+            throw new IllegalArgumentException("test constructor failure");
+        }
+    }
 }

server/src/test/resources/META-INF/services/org.elasticsearch.plugins.PluginsServiceTests$TestExtensionPoint

@@ -0,0 +1,21 @@
+#
+# Licensed to Elasticsearch under one or more contributor
+# license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright
+# ownership. Elasticsearch licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+org.elasticsearch.plugins.PluginsServiceTests$TestExtension1
+org.elasticsearch.plugins.PluginsServiceTests$TestExtension2

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityExtension.java

@@ -5,7 +5,6 @@
  */
 package org.elasticsearch.xpack.core.security;
 
-import org.apache.lucene.util.SPIClassIterator;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.cluster.service.ClusterService;
@@ -20,11 +19,9 @@ import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
 import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
 import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult;
 
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.ServiceConfigurationError;
 import java.util.Set;
 import java.util.function.BiConsumer;
 
@@ -117,22 +114,4 @@ public interface SecurityExtension {
     default AuthorizationEngine getAuthorizationEngine(Settings settings) {
         return null;
     }
-
-    /**
-     * Loads the XPackSecurityExtensions from the given class loader
-     */
-    static List<SecurityExtension> loadExtensions(ClassLoader loader) {
-        SPIClassIterator<SecurityExtension> iterator = SPIClassIterator.get(SecurityExtension.class, loader);
-        List<SecurityExtension> extensions = new ArrayList<>();
-        while (iterator.hasNext()) {
-            final Class<? extends SecurityExtension> c = iterator.next();
-            try {
-                extensions.add(c.getConstructor().newInstance());
-            } catch (Exception e) {
-                throw new ServiceConfigurationError("failed to load security extension [" + c.getName() + "]", e);
-            }
-        }
-        return extensions;
-    }
-
 }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -1112,8 +1112,8 @@ public class Security extends Plugin implements SystemIndexPlugin, IngestPlugin,
     }
 
     @Override
-    public void reloadSPI(ClassLoader loader) {
+    public void loadExtensions(ExtensionLoader loader) {
-        securityExtensions.addAll(SecurityExtension.loadExtensions(loader));
+        securityExtensions.addAll(loader.loadExtensions(SecurityExtension.class));
     }
 
     private synchronized NioGroupFactory getNioGroupFactory(Settings settings) {