From 38be2812b1534f3d79fb89f28c5bbd3903a07158 Mon Sep 17 00:00:00 2001 From: Henning Andersen <33268011+henningandersen@users.noreply.github.com> Date: Thu, 25 Jun 2020 20:37:56 +0200 Subject: [PATCH] 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. --- .../painless/PainlessPlugin.java | 11 +- .../plugins/ExtensiblePlugin.java | 20 ++- .../elasticsearch/plugins/PluginsService.java | 94 +++++++++- .../plugins/PluginsServiceTests.java | 165 ++++++++++++++++++ ...ins.PluginsServiceTests$TestExtensionPoint | 21 +++ .../core/security/SecurityExtension.java | 21 --- .../xpack/security/Security.java | 4 +- 7 files changed, 300 insertions(+), 36 deletions(-) create mode 100644 server/src/test/resources/META-INF/services/org.elasticsearch.plugins.PluginsServiceTests$TestExtensionPoint diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java index fc4ddfa3eca..218cb85a647 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java +++ b/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) { - for (PainlessExtension extension : ServiceLoader.load(PainlessExtension.class, loader)) { - for (Map.Entry, List> entry : extension.getContextWhitelists().entrySet()) { + public void loadExtensions(ExtensionLoader loader) { + loader.loadExtensions(PainlessExtension.class).stream() + .flatMap(extension -> extension.getContextWhitelists().entrySet().stream()) + .forEach(entry -> { List existing = whitelists.computeIfAbsent(entry.getKey(), c -> new ArrayList<>(Whitelist.BASE_WHITELISTS)); existing.addAll(entry.getValue()); - } - } + }); } @Override diff --git a/server/src/main/java/org/elasticsearch/plugins/ExtensiblePlugin.java b/server/src/main/java/org/elasticsearch/plugins/ExtensiblePlugin.java index 73618305129..503ebbbf251 100644 --- a/server/src/main/java/org/elasticsearch/plugins/ExtensiblePlugin.java +++ b/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 { + /** + * 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 extension point type + * @return all implementing extensions. + */ + List loadExtensions(Class extensionPointType); + } + /** - * Reload any SPI implementations from the given classloader. + * 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 reloadSPI(ClassLoader loader) {} + default void loadExtensions(ExtensionLoader loader) {} } diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java index 4c3be448afc..1ad97d50223 100644 --- a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/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 { Map loaded = new HashMap<>(); Map> transitiveUrls = new HashMap<>(); List sortedBundles = sortBundles(bundles); - for (Bundle bundle : sortedBundles) { checkBundleJarHell(JarHell.parseClassPath(), bundle, transitiveUrls); @@ -474,9 +474,92 @@ public class PluginsService implements ReportingService { plugins.add(new Tuple<>(bundle.plugin, plugin)); } + loadExtensions(plugins); return Collections.unmodifiableList(plugins); } + // package-private for test visibility + static void loadExtensions(List> plugins) { + Map> 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 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 extendingPlugins) { + ExtensiblePlugin.ExtensionLoader extensionLoader = new ExtensiblePlugin.ExtensionLoader() { + @Override + public List loadExtensions(Class extensionPointType) { + List result = new ArrayList<>(); + for (Plugin extendingPlugin : extendingPlugins) { + result.addAll(createExtensions(extensionPointType, extendingPlugin)); + } + return Collections.unmodifiableList(result); + } + }; + + extensiblePlugin.loadExtensions(extensionLoader); + } + + private static List createExtensions(Class extensionPointType, Plugin plugin) { + SPIClassIterator classIterator = SPIClassIterator.get(extensionPointType, plugin.getClass().getClassLoader()); + List extensions = new ArrayList<>(); + while (classIterator.hasNext()) { + Class extensionClass = classIterator.next(); + extensions.add(createExtension(extensionClass, extensionPointType, plugin)); + } + return extensions; + } + + // package-private for test visibility + static T createExtension(Class extensionClass, Class extensionPointType, Plugin plugin) { + //noinspection unchecked + Constructor[] constructors = (Constructor[]) 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 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 String extensionSignatureMessage(Class extensionClass, Class extensionPointType, Plugin plugin) { + return "signature of " + extensionConstructorMessage(extensionClass, extensionPointType) + + " must be either () or (" + plugin.getClass().getName() + ")"; + } + + private static String extensionConstructorMessage(Class extensionClass, Class 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 classpath, Bundle bundle, Map> transitiveUrls) { @@ -549,12 +632,13 @@ public class PluginsService implements ReportingService { // 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 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; diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index 760e3128640..702b7768dcd 100644 --- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/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 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"); + } + } } diff --git a/server/src/test/resources/META-INF/services/org.elasticsearch.plugins.PluginsServiceTests$TestExtensionPoint b/server/src/test/resources/META-INF/services/org.elasticsearch.plugins.PluginsServiceTests$TestExtensionPoint new file mode 100644 index 00000000000..e00a55ba1bb --- /dev/null +++ b/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 diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityExtension.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityExtension.java index b45d4ad99cc..a6ee40e2c69 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityExtension.java +++ b/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 loadExtensions(ClassLoader loader) { - SPIClassIterator iterator = SPIClassIterator.get(SecurityExtension.class, loader); - List extensions = new ArrayList<>(); - while (iterator.hasNext()) { - final Class 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; - } - } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 2d207630f28..046bf80611a 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/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) { - securityExtensions.addAll(SecurityExtension.loadExtensions(loader)); + public void loadExtensions(ExtensionLoader loader) { + securityExtensions.addAll(loader.loadExtensions(SecurityExtension.class)); } private synchronized NioGroupFactory getNioGroupFactory(Settings settings) {