From fbe6a3d7cb3a63d336f17fc6789966e2e88f8e69 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 18 Dec 2017 16:17:00 -0800 Subject: [PATCH 01/16] Plugins: Add plugin extension capabilities This commit adds the infrastructure to plugin building and loading to allow one plugin to extend another. That is, one plugin may extend another by the "parent" plugin allowing itself to be extended through java SPI. When all plugins extending a plugin are finished loading, the "parent" plugin has a callback (through the ExtensiblePlugin interface) allowing it to reload SPI. This commit also adds an example plugin which uses as-yet implemented extensibility (adding to the painless whitelist). --- .../plugin/PluginPropertiesExtension.groovy | 4 + .../gradle/plugin/PluginPropertiesTask.groovy | 1 + .../resources/plugin-descriptor.properties | 3 + ...yPluginInfo.java => ExtensiblePlugin.java} | 23 +- .../org/elasticsearch/plugins/PluginInfo.java | 36 ++- .../elasticsearch/plugins/PluginsService.java | 232 ++++++++++---- .../elasticsearch/bootstrap/security.policy | 3 + .../nodesinfo/NodeInfoStreamingTests.java | 7 +- .../plugins/PluginInfoTests.java | 53 +++- .../plugins/PluginsServiceTests.java | 286 ++++++++++++++++++ .../plugins/InstallPluginCommand.java | 27 +- .../plugins/RemovePluginCommand.java | 28 +- .../plugins/InstallPluginCommandTests.java | 4 +- .../painless/PainlessPlugin.java | 3 +- .../examples/painless-whitelist/build.gradle | 33 ++ .../painlesswhitelist/MyWhitelistPlugin.java | 25 ++ ...ainlessWhitelistClientYamlTestSuiteIT.java | 38 +++ .../test/painless_whitelist/10_basic.yml | 13 + 18 files changed, 723 insertions(+), 96 deletions(-) rename core/src/main/java/org/elasticsearch/plugins/{DummyPluginInfo.java => ExtensiblePlugin.java} (63%) create mode 100644 plugins/examples/painless-whitelist/build.gradle create mode 100644 plugins/examples/painless-whitelist/src/main/java/org/elasticsearch/example/painlesswhitelist/MyWhitelistPlugin.java create mode 100644 plugins/examples/painless-whitelist/src/test/java/org/elasticsearch/example/painlesswhitelist/PainlessWhitelistClientYamlTestSuiteIT.java create mode 100644 plugins/examples/painless-whitelist/src/test/resources/rest-api-spec/test/painless_whitelist/10_basic.yml diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy index 712c8a22154c6..1fd7269237bfd 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy @@ -39,6 +39,10 @@ class PluginPropertiesExtension { @Input String classname + /** Other plugins this plugin extends through SPI */ + @Input + List extendsPlugins = [] + @Input boolean hasNativeController = false diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy index bd0765cc6763f..d3c2b059f0bcb 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy @@ -80,6 +80,7 @@ class PluginPropertiesTask extends Copy { 'elasticsearchVersion': stringSnap(VersionProperties.elasticsearch), 'javaVersion': project.targetCompatibility as String, 'classname': extension.classname, + 'extendsPlugins': extension.extendsPlugins.join(','), 'hasNativeController': extension.hasNativeController, 'requiresKeystore': extension.requiresKeystore ] diff --git a/buildSrc/src/main/resources/plugin-descriptor.properties b/buildSrc/src/main/resources/plugin-descriptor.properties index 31388a5ca79b0..8a21fae2423f1 100644 --- a/buildSrc/src/main/resources/plugin-descriptor.properties +++ b/buildSrc/src/main/resources/plugin-descriptor.properties @@ -40,6 +40,9 @@ java.version=${javaVersion} elasticsearch.version=${elasticsearchVersion} ### optional elements for plugins: # +# 'extends.plugins': other plugins this plugin extends through SPI +extends.plugins=${extendsPlugins} +# # 'has.native.controller': whether or not the plugin has a native controller has.native.controller=${hasNativeController} # diff --git a/core/src/main/java/org/elasticsearch/plugins/DummyPluginInfo.java b/core/src/main/java/org/elasticsearch/plugins/ExtensiblePlugin.java similarity index 63% rename from core/src/main/java/org/elasticsearch/plugins/DummyPluginInfo.java rename to core/src/main/java/org/elasticsearch/plugins/ExtensiblePlugin.java index 3e7442509b6e9..736183051291b 100644 --- a/core/src/main/java/org/elasticsearch/plugins/DummyPluginInfo.java +++ b/core/src/main/java/org/elasticsearch/plugins/ExtensiblePlugin.java @@ -16,18 +16,19 @@ * specific language governing permissions and limitations * under the License. */ -package org.elasticsearch.plugins; -public class DummyPluginInfo extends PluginInfo { +package org.elasticsearch.plugins; - private DummyPluginInfo(String name, String description, String version, String classname) { - super(name, description, version, classname, false, false); - } +/** + * An extension point for {@link Plugin} implementations to be themselves extensible. + * + * This class provides a callback for extensible plugins to be informed of other plugins + * which extend them. + */ +public interface ExtensiblePlugin { - public static final DummyPluginInfo INSTANCE = - new DummyPluginInfo( - "dummy_plugin_name", - "dummy plugin description", - "dummy_plugin_version", - "DummyPluginName"); + /** + * Reload any SPI implementations from the given classloader. + */ + default void reloadSPI(ClassLoader loader) {} } diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java b/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java index c301b65738b43..7937454260075 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java @@ -22,10 +22,10 @@ import org.elasticsearch.Version; import org.elasticsearch.bootstrap.JarHell; import org.elasticsearch.common.Booleans; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.xcontent.ToXContent.Params; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -33,6 +33,9 @@ import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import java.util.Locale; import java.util.Properties; @@ -48,6 +51,7 @@ public class PluginInfo implements Writeable, ToXContentObject { private final String description; private final String version; private final String classname; + private final List extendsPlugins; private final boolean hasNativeController; private final boolean requiresKeystore; @@ -58,15 +62,17 @@ public class PluginInfo implements Writeable, ToXContentObject { * @param description a description of the plugin * @param version the version of Elasticsearch the plugin is built for * @param classname the entry point to the plugin + * @param extendsPlugins other plugins this plugin extends through SPI * @param hasNativeController whether or not the plugin has a native controller * @param requiresKeystore whether or not the plugin requires the elasticsearch keystore to be created */ public PluginInfo(String name, String description, String version, String classname, - boolean hasNativeController, boolean requiresKeystore) { + List extendsPlugins, boolean hasNativeController, boolean requiresKeystore) { this.name = name; this.description = description; this.version = version; this.classname = classname; + this.extendsPlugins = Collections.unmodifiableList(extendsPlugins); this.hasNativeController = hasNativeController; this.requiresKeystore = requiresKeystore; } @@ -82,6 +88,11 @@ public PluginInfo(final StreamInput in) throws IOException { this.description = in.readString(); this.version = in.readString(); this.classname = in.readString(); + if (in.getVersion().onOrAfter(Version.V_6_1_0)) { + extendsPlugins = Collections.unmodifiableList(Arrays.asList(in.readStringArray())); + } else { + extendsPlugins = Collections.emptyList(); + } if (in.getVersion().onOrAfter(Version.V_5_4_0)) { hasNativeController = in.readBoolean(); } else { @@ -100,6 +111,9 @@ public void writeTo(final StreamOutput out) throws IOException { out.writeString(description); out.writeString(version); out.writeString(classname); + if (out.getVersion().onOrAfter(Version.V_6_1_0)) { + out.writeStringList(extendsPlugins); + } if (out.getVersion().onOrAfter(Version.V_5_4_0)) { out.writeBoolean(hasNativeController); } @@ -167,6 +181,9 @@ public static PluginInfo readFromProperties(final Path path) throws IOException "property [classname] is missing for plugin [" + name + "]"); } + final String extendsString = props.getProperty("extends.plugins", ""); + final List extendsPlugins = Arrays.asList(Strings.delimitedListToStringArray(extendsString, ",")); + final String hasNativeControllerValue = props.getProperty("has.native.controller"); final boolean hasNativeController; if (hasNativeControllerValue == null) { @@ -200,7 +217,7 @@ public static PluginInfo readFromProperties(final Path path) throws IOException " but was [" + requiresKeystoreValue + "]", e); } - return new PluginInfo(name, description, version, classname, hasNativeController, requiresKeystore); + return new PluginInfo(name, description, version, classname, extendsPlugins, hasNativeController, requiresKeystore); } /** @@ -230,6 +247,15 @@ public String getClassname() { return classname; } + /** + * Other plugins this plugin extends through SPI. + * + * @return the names of the plugins extended + */ + public List getExtendsPlugins() { + return extendsPlugins; + } + /** * The version of Elasticsearch the plugin was built for. * @@ -265,6 +291,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field("version", version); builder.field("description", description); builder.field("classname", classname); + builder.array("extended_plugins", extendsPlugins); builder.field("has_native_controller", hasNativeController); builder.field("requires_keystore", requiresKeystore); } @@ -300,7 +327,8 @@ public String toString() { .append("Version: ").append(version).append("\n") .append("Native Controller: ").append(hasNativeController).append("\n") .append("Requires Keystore: ").append(requiresKeystore).append("\n") - .append(" * Classname: ").append(classname); + .append(" * Classname: ").append(classname).append("\n") + .append(" * Extended Plugins: ").append(extendsPlugins); return information.toString(); } diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index 1a50bcc7213ed..76e9195ed9e6b 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -103,7 +103,8 @@ public PluginsService(Settings settings, Path configPath, Path modulesDirectory, // first we load plugins that are on the classpath. this is for tests and transport clients for (Class pluginClass : classpathPlugins) { Plugin plugin = loadPlugin(pluginClass, settings, configPath); - PluginInfo pluginInfo = new PluginInfo(pluginClass.getName(), "classpath plugin", "NA", pluginClass.getName(), false, false); + PluginInfo pluginInfo = new PluginInfo(pluginClass.getName(), "classpath plugin", "NA", + pluginClass.getName(), Collections.emptyList(), false, false); if (logger.isTraceEnabled()) { logger.trace("plugin loaded from classpath [{}]", pluginInfo); } @@ -243,8 +244,19 @@ static class Bundle { final PluginInfo plugin; final Set urls; - Bundle(PluginInfo plugin, Set urls) { + Bundle(PluginInfo plugin, Path dir) throws IOException { this.plugin = Objects.requireNonNull(plugin); + Set urls = new LinkedHashSet<>(); + // gather urls for jar files + try (DirectoryStream jarStream = Files.newDirectoryStream(dir, "*.jar")) { + for (Path jar : jarStream) { + // normalize with toRealPath to get symlinks out of our hair + URL url = jar.toRealPath().toUri().toURL(); + if (urls.add(url) == false) { + throw new IllegalStateException("duplicate codebase: " + url); + } + } + } this.urls = Objects.requireNonNull(urls); } @@ -262,6 +274,32 @@ public int hashCode() { } } + /** + * A classloader that is a union over the parent core classloader and classloaders of extended plugins. + */ + private static class ExtendedPluginsClassLoader extends ClassLoader { + + /** Loaders of plugins extended by a plugin. */ + private List extendedLoaders; + + ExtendedPluginsClassLoader(ClassLoader parent, List extendedLoaders) { + super(parent); + this.extendedLoaders = extendedLoaders; + } + + @Override + protected Class findClass(String name) throws ClassNotFoundException { + for (ClassLoader loader : extendedLoaders) { + try { + return loader.loadClass(name); + } catch (ClassNotFoundException e) { + // continue + } + } + throw new ClassNotFoundException(name); + } + } + // similar in impl to getPluginBundles, but DO NOT try to make them share code. // we don't need to inherit all the leniency, and things are different enough. static Set getModuleBundles(Path modulesDirectory) throws IOException { @@ -273,18 +311,7 @@ static Set getModuleBundles(Path modulesDirectory) throws IOException { try (DirectoryStream stream = Files.newDirectoryStream(modulesDirectory)) { for (Path module : stream) { PluginInfo info = PluginInfo.readFromProperties(module); - Set urls = new LinkedHashSet<>(); - // gather urls for jar files - try (DirectoryStream jarStream = Files.newDirectoryStream(module, "*.jar")) { - for (Path jar : jarStream) { - // normalize with toRealPath to get symlinks out of our hair - URL url = jar.toRealPath().toUri().toURL(); - if (urls.add(url) == false) { - throw new IllegalStateException("duplicate codebase: " + url); - } - } - } - if (bundles.add(new Bundle(info, urls)) == false) { + if (bundles.add(new Bundle(info, module)) == false) { throw new IllegalStateException("duplicate module: " + info); } } @@ -339,17 +366,7 @@ static Set getPluginBundles(Path pluginsDirectory) throws IOException { + plugin.getFileName() + "]. Was the plugin built before 2.0?", e); } - Set urls = new LinkedHashSet<>(); - try (DirectoryStream jarStream = Files.newDirectoryStream(plugin, "*.jar")) { - for (Path jar : jarStream) { - // normalize with toRealPath to get symlinks out of our hair - URL url = jar.toRealPath().toUri().toURL(); - if (urls.add(url) == false) { - throw new IllegalStateException("duplicate codebase: " + url); - } - } - } - if (bundles.add(new Bundle(info, urls)) == false) { + if (bundles.add(new Bundle(info, plugin)) == false) { throw new IllegalStateException("duplicate plugin: " + info); } } @@ -358,42 +375,153 @@ static Set getPluginBundles(Path pluginsDirectory) throws IOException { return bundles; } + /** + * Return the given bundles, sorted in dependency loading order. + * + * This sort is stable, so that if two plugins do not have any interdependency, + * their relative order from iteration of the provided set will not change. + * + * @throws IllegalStateException if a dependency cycle is found + */ + // pkg private for tests + static List sortBundles(Set bundles) { + Map namedBundles = bundles.stream().collect(Collectors.toMap(b -> b.plugin.getName(), Function.identity())); + LinkedHashSet sortedBundles = new LinkedHashSet<>(); + LinkedHashSet dependencyStack = new LinkedHashSet<>(); + for (Bundle bundle : bundles) { + addSortedBundle(bundle, namedBundles, sortedBundles, dependencyStack); + } + return new ArrayList<>(sortedBundles); + } + + // add the given bundle to the sorted bundles, first adding dependencies + private static void addSortedBundle(Bundle bundle, Map bundles, LinkedHashSet sortedBundles, + LinkedHashSet dependencyStack) { + + String name = bundle.plugin.getName(); + if (dependencyStack.contains(name)) { + StringBuilder msg = new StringBuilder("Cycle found in plugin dependencies: "); + dependencyStack.forEach(s -> { + msg.append(s); + msg.append(" -> "); + }); + msg.append(name); + throw new IllegalStateException(msg.toString()); + } + if (sortedBundles.contains(bundle)) { + // already added this plugin, via a dependency + return; + } + + dependencyStack.add(name); + for (String dependency : bundle.plugin.getExtendsPlugins()) { + Bundle depBundle = bundles.get(dependency); + if (depBundle == null) { + throw new IllegalArgumentException("Missing plugin [" + dependency + "], dependency of [" + name + "]"); + } + addSortedBundle(depBundle, bundles, sortedBundles, dependencyStack); + assert sortedBundles.contains(depBundle); + } + dependencyStack.remove(name); + + sortedBundles.add(bundle); + } + private List> loadBundles(Set bundles) { List> plugins = new ArrayList<>(); + Map loaded = new HashMap<>(); + Map> transitiveUrls = new HashMap<>(); + List sortedBundles = sortBundles(bundles); - for (Bundle bundle : bundles) { - // jar-hell check the bundle against the parent classloader - // pluginmanager does it, but we do it again, in case lusers mess with jar files manually - try { - Set classpath = JarHell.parseClassPath(); - // check we don't have conflicting codebases - Set intersection = new HashSet<>(classpath); - intersection.retainAll(bundle.urls); + for (Bundle bundle : sortedBundles) { + checkBundleJarHell(bundle, transitiveUrls); + + final Plugin plugin = loadBundle(bundle, loaded); + plugins.add(new Tuple<>(bundle.plugin, plugin)); + } + + return Collections.unmodifiableList(plugins); + } + + // 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(Bundle bundle, Map> transitiveUrls) { + // invariant: any plugins this plugin bundle extends have already been added to transitiveUrls + List exts = bundle.plugin.getExtendsPlugins(); + + try { + Set urls = new HashSet<>(); + for (String extendedPlugin : exts) { + Set pluginUrls = transitiveUrls.get(extendedPlugin); + assert pluginUrls != null : "transitive urls should have already been set for " + extendedPlugin; + + Set intersection = new HashSet<>(urls); + intersection.retainAll(pluginUrls); if (intersection.isEmpty() == false) { - throw new IllegalStateException("jar hell! duplicate codebases between" + - " plugin and core: " + intersection); + throw new IllegalStateException("jar hell! extended plugins " + exts + + " have duplicate codebases with each other: " + intersection); } - // check we don't have conflicting classes - Set union = new HashSet<>(classpath); - union.addAll(bundle.urls); - JarHell.checkJarHell(union); - } catch (Exception e) { - throw new IllegalStateException("failed to load plugin " + bundle.plugin + - " due to jar hell", e); + + intersection = new HashSet<>(bundle.urls); + intersection.retainAll(pluginUrls); + if (intersection.isEmpty() == false) { + throw new IllegalStateException("jar hell! duplicate codebases with extended plugin [" + + extendedPlugin + "]: " + intersection); + } + + urls.addAll(pluginUrls); + JarHell.checkJarHell(urls); // check jarhell as we add each extended plugin's urls } - // create a child to load the plugin in this bundle - ClassLoader loader = URLClassLoader.newInstance(bundle.urls.toArray(new URL[0]), - getClass().getClassLoader()); - // reload lucene SPI with any new services from the plugin - reloadLuceneSPI(loader); - final Class pluginClass = - loadPluginClass(bundle.plugin.getClassname(), loader); - final Plugin plugin = loadPlugin(pluginClass, settings, configPath); - plugins.add(new Tuple<>(bundle.plugin, plugin)); + urls.addAll(bundle.urls); + JarHell.checkJarHell(urls); // check jarhell of each extended plugin against this plugin + transitiveUrls.put(bundle.plugin.getName(), urls); + + Set classpath = JarHell.parseClassPath(); + // check we don't have conflicting codebases with core + Set intersection = new HashSet<>(classpath); + intersection.retainAll(bundle.urls); + if (intersection.isEmpty() == false) { + throw new IllegalStateException("jar hell! duplicate codebases between plugin and core: " + intersection); + } + // check we don't have conflicting classes + Set union = new HashSet<>(classpath); + union.addAll(bundle.urls); + JarHell.checkJarHell(union); + } catch (Exception e) { + throw new IllegalStateException("failed to load plugin " + bundle.plugin.getName() + " due to jar hell", e); } + } - return Collections.unmodifiableList(plugins); + private Plugin loadBundle(Bundle bundle, Map loaded) { + String name = bundle.plugin.getName(); + + // collect loaders of extended plugins + List extendedLoaders = new ArrayList<>(); + for (String extendedPluginName : bundle.plugin.getExtendsPlugins()) { + Plugin extendedPlugin = loaded.get(extendedPluginName); + assert extendedPlugin != null; + if (ExtensiblePlugin.class.isInstance(extendedPlugin) == false) { + throw new IllegalStateException("Plugin [" + name + "] cannot extend non-extensible plugin [" + extendedPluginName + "]"); + } + extendedLoaders.add(extendedPlugin.getClass().getClassLoader()); + } + + // create a child to load the plugin in this bundle + ExtendedPluginsClassLoader parentLoader = new ExtendedPluginsClassLoader(getClass().getClassLoader(), extendedLoaders); + ClassLoader loader = URLClassLoader.newInstance(bundle.urls.toArray(new URL[0]), parentLoader); + + // reload SPI with any new services from the plugin + reloadLuceneSPI(loader); + for (String extendedPluginName : bundle.plugin.getExtendsPlugins()) { + // 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); + Plugin plugin = loadPlugin(pluginClass, settings, configPath); + loaded.put(name, plugin); + return plugin; } /** diff --git a/core/src/main/resources/org/elasticsearch/bootstrap/security.policy b/core/src/main/resources/org/elasticsearch/bootstrap/security.policy index 603e95c3102d0..5395840b28927 100644 --- a/core/src/main/resources/org/elasticsearch/bootstrap/security.policy +++ b/core/src/main/resources/org/elasticsearch/bootstrap/security.policy @@ -131,4 +131,7 @@ grant { permission java.io.FilePermission "/sys/fs/cgroup/cpuacct/-", "read"; permission java.io.FilePermission "/sys/fs/cgroup/memory", "read"; permission java.io.FilePermission "/sys/fs/cgroup/memory/-", "read"; + + // nocommit: needed by plugin service for extensible plugins, move to a separate jar + permission java.lang.RuntimePermission "createClassLoader"; }; diff --git a/core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java b/core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java index 665c0430207e6..3de6b54c0c391 100644 --- a/core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java +++ b/core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java @@ -46,6 +46,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -143,13 +144,15 @@ private static NodeInfo createNodeInfo() { List plugins = new ArrayList<>(); for (int i = 0; i < numPlugins; i++) { plugins.add(new PluginInfo(randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10), - randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10), randomBoolean(), randomBoolean())); + randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10), Collections.emptyList(), + randomBoolean(), randomBoolean())); } int numModules = randomIntBetween(0, 5); List modules = new ArrayList<>(); for (int i = 0; i < numModules; i++) { modules.add(new PluginInfo(randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10), - randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10), randomBoolean(), randomBoolean())); + randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10), Collections.emptyList(), + randomBoolean(), randomBoolean())); } pluginsAndModules = new PluginsAndModules(plugins, modules); } diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java index 1e6cdfc722018..2a96538e83970 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java @@ -47,6 +47,7 @@ public void testReadFromProperties() throws Exception { assertEquals("fake desc", info.getDescription()); assertEquals("1.0", info.getVersion()); assertEquals("FakePlugin", info.getClassname()); + assertTrue(info.getExtendsPlugins().isEmpty()); } public void testReadFromPropertiesNameMissing() throws Exception { @@ -207,13 +208,55 @@ public void testReadFromPropertiesJvmMissingClassname() throws Exception { } } + public void testExtendsPluginsSingleExtension() throws Exception { + Path pluginDir = createTempDir().resolve("fake-plugin"); + PluginTestUtil.writeProperties(pluginDir, + "description", "fake desc", + "name", "my_plugin", + "version", "1.0", + "elasticsearch.version", Version.CURRENT.toString(), + "java.version", System.getProperty("java.specification.version"), + "classname", "FakePlugin", + "extends.plugins", "foo"); + PluginInfo info = PluginInfo.readFromProperties(pluginDir); + assertThat(info.getExtendsPlugins(), contains("foo")); + } + + public void testExtendsPluginsMultipleExtensions() throws Exception { + Path pluginDir = createTempDir().resolve("fake-plugin"); + PluginTestUtil.writeProperties(pluginDir, + "description", "fake desc", + "name", "my_plugin", + "version", "1.0", + "elasticsearch.version", Version.CURRENT.toString(), + "java.version", System.getProperty("java.specification.version"), + "classname", "FakePlugin", + "extends.plugins", "foo,bar,baz"); + PluginInfo info = PluginInfo.readFromProperties(pluginDir); + assertThat(info.getExtendsPlugins(), contains("foo", "bar", "baz")); + } + + public void testExtendsPluginsEmpty() throws Exception { + Path pluginDir = createTempDir().resolve("fake-plugin"); + PluginTestUtil.writeProperties(pluginDir, + "description", "fake desc", + "name", "my_plugin", + "version", "1.0", + "elasticsearch.version", Version.CURRENT.toString(), + "java.version", System.getProperty("java.specification.version"), + "classname", "FakePlugin", + "extends.plugins", ""); + PluginInfo info = PluginInfo.readFromProperties(pluginDir); + assertTrue(info.getExtendsPlugins().isEmpty()); + } + public void testPluginListSorted() { List plugins = new ArrayList<>(); - plugins.add(new PluginInfo("c", "foo", "dummy", "dummyclass", randomBoolean(), randomBoolean())); - plugins.add(new PluginInfo("b", "foo", "dummy", "dummyclass", randomBoolean(), randomBoolean())); - plugins.add(new PluginInfo("e", "foo", "dummy", "dummyclass", randomBoolean(), randomBoolean())); - plugins.add(new PluginInfo("a", "foo", "dummy", "dummyclass", randomBoolean(), randomBoolean())); - plugins.add(new PluginInfo("d", "foo", "dummy", "dummyclass", randomBoolean(), randomBoolean())); + plugins.add(new PluginInfo("c", "foo", "dummy", "dummyclass", Collections.emptyList(), randomBoolean(), randomBoolean())); + plugins.add(new PluginInfo("b", "foo", "dummy", "dummyclass", Collections.emptyList(),randomBoolean(), randomBoolean())); + plugins.add(new PluginInfo("e", "foo", "dummy", "dummyclass", Collections.emptyList(),randomBoolean(), randomBoolean())); + plugins.add(new PluginInfo("a", "foo", "dummy", "dummyclass", Collections.emptyList(),randomBoolean(), randomBoolean())); + plugins.add(new PluginInfo("d", "foo", "dummy", "dummyclass", Collections.emptyList(),randomBoolean(), randomBoolean())); PluginsAndModules pluginsInfo = new PluginsAndModules(plugins, Collections.emptyList()); diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index 3bd31097dcae6..f8511a568e598 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -19,25 +19,47 @@ package org.elasticsearch.plugins; +import javax.annotation.processing.Processor; +import javax.tools.JavaCompiler; +import javax.tools.JavaFileObject; +import javax.tools.SimpleJavaFileObject; +import javax.tools.ToolProvider; + +import org.apache.log4j.Level; import org.apache.lucene.util.Constants; import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.Version; +import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.index.IndexModule; import org.elasticsearch.test.ESTestCase; +import org.hamcrest.Matchers; +import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStream; +import java.net.URL; +import java.nio.charset.StandardCharsets; import java.nio.file.FileSystemException; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; +import java.util.Map; +import java.util.Set; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; +import java.util.zip.ZipOutputStream; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; @@ -269,4 +291,268 @@ public OneParameterIncorrectType(Object object) { } } + public void testSortBundlesCycleSelfReference() throws Exception { + Path pluginDir = createTempDir(); + PluginInfo info = new PluginInfo("foo", "desc", "1.0", "MyPlugin", Collections.singletonList("foo"), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir); + IllegalStateException e = expectThrows(IllegalStateException.class, () -> + PluginsService.sortBundles(Collections.singleton(bundle)) + ); + assertEquals("Cycle found in plugin dependencies: foo -> foo", e.getMessage()); + } + + public void testSortBundlesCycle() throws Exception { + Path pluginDir = createTempDir(); + Set bundles = new LinkedHashSet<>(); // control iteration order, so we get know the beginning of the cycle + PluginInfo info = new PluginInfo("foo", "desc", "1.0", "MyPlugin", Arrays.asList("bar", "other"), false, false); + bundles.add(new PluginsService.Bundle(info, pluginDir)); + PluginInfo info2 = new PluginInfo("bar", "desc", "1.0", "MyPlugin", Collections.singletonList("baz"), false, false); + bundles.add(new PluginsService.Bundle(info2, pluginDir)); + PluginInfo info3 = new PluginInfo("baz", "desc", "1.0", "MyPlugin", Collections.singletonList("foo"), false, false); + bundles.add(new PluginsService.Bundle(info3, pluginDir)); + PluginInfo info4 = new PluginInfo("other", "desc", "1.0", "MyPlugin", Collections.emptyList(), false, false); + bundles.add(new PluginsService.Bundle(info4, pluginDir)); + + IllegalStateException e = expectThrows(IllegalStateException.class, () -> PluginsService.sortBundles(bundles)); + assertEquals("Cycle found in plugin dependencies: foo -> bar -> baz -> foo", e.getMessage()); + } + + public void testSortBundlesSingle() throws Exception { + Path pluginDir = createTempDir(); + PluginInfo info = new PluginInfo("foo", "desc", "1.0", "MyPlugin", Collections.emptyList(), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir); + List sortedBundles = PluginsService.sortBundles(Collections.singleton(bundle)); + assertThat(sortedBundles, Matchers.contains(bundle)); + } + + public void testSortBundlesNoDeps() throws Exception { + Path pluginDir = createTempDir(); + Set bundles = new LinkedHashSet<>(); // control iteration order + PluginInfo info1 = new PluginInfo("foo", "desc", "1.0", "MyPlugin", Collections.emptyList(), false, false); + PluginsService.Bundle bundle1 = new PluginsService.Bundle(info1, pluginDir); + bundles.add(bundle1); + PluginInfo info2 = new PluginInfo("bar", "desc", "1.0", "MyPlugin", Collections.emptyList(), false, false); + PluginsService.Bundle bundle2 = new PluginsService.Bundle(info2, pluginDir); + bundles.add(bundle2); + PluginInfo info3 = new PluginInfo("baz", "desc", "1.0", "MyPlugin", Collections.emptyList(), false, false); + PluginsService.Bundle bundle3 = new PluginsService.Bundle(info3, pluginDir); + bundles.add(bundle3); + List sortedBundles = PluginsService.sortBundles(bundles); + assertThat(sortedBundles, Matchers.contains(bundle1, bundle2, bundle3)); + } + + public void testSortBundlesMissingDep() throws Exception { + Path pluginDir = createTempDir(); + PluginInfo info = new PluginInfo("foo", "desc", "1.0", "MyPlugin", Collections.singletonList("dne"), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + PluginsService.sortBundles(Collections.singleton(bundle)) + ); + assertEquals("Missing plugin [dne], dependency of [foo]", e.getMessage()); + } + + public void testSortBundlesCommonDep() throws Exception { + Path pluginDir = createTempDir(); + Set bundles = new LinkedHashSet<>(); // control iteration order + PluginInfo info1 = new PluginInfo("grandparent", "desc", "1.0", "MyPlugin", Collections.emptyList(), false, false); + PluginsService.Bundle bundle1 = new PluginsService.Bundle(info1, pluginDir); + bundles.add(bundle1); + PluginInfo info2 = new PluginInfo("foo", "desc", "1.0", "MyPlugin", Collections.singletonList("common"), false, false); + PluginsService.Bundle bundle2 = new PluginsService.Bundle(info2, pluginDir); + bundles.add(bundle2); + PluginInfo info3 = new PluginInfo("bar", "desc", "1.0", "MyPlugin", Collections.singletonList("common"), false, false); + PluginsService.Bundle bundle3 = new PluginsService.Bundle(info3, pluginDir); + bundles.add(bundle3); + PluginInfo info4 = new PluginInfo("common", "desc", "1.0", "MyPlugin", Collections.singletonList("grandparent"), false, false); + PluginsService.Bundle bundle4 = new PluginsService.Bundle(info4, pluginDir); + bundles.add(bundle4); + List sortedBundles = PluginsService.sortBundles(bundles); + assertThat(sortedBundles, Matchers.contains(bundle1, bundle4, bundle2, bundle3)); + } + + public static class DummyClass1 {} + + public static class DummyClass2 {} + + public static class DummyClass3 {} + + public static class NonExtensible extends Plugin {} + + void makeJar(Path jarFile, Class... classes) throws Exception { + try (ZipOutputStream out = new ZipOutputStream(Files.newOutputStream(jarFile))) { + for (Class clazz : classes) { + String relativePath = clazz.getCanonicalName().replaceAll("\\.", "/") + ".class"; + if (relativePath.contains(PluginsServiceTests.class.getSimpleName())) { + // static inner class of this test + relativePath = relativePath.replace("/" + clazz.getSimpleName(), "$" + clazz.getSimpleName()); + } + + Path codebase = PathUtils.get(clazz.getProtectionDomain().getCodeSource().getLocation().toURI()); + if (codebase.toString().endsWith(".jar")) { + // copy from jar, exactly as is + out.putNextEntry(new ZipEntry(relativePath)); + try (ZipInputStream in = new ZipInputStream(Files.newInputStream(codebase))) { + ZipEntry entry = in.getNextEntry(); + while (entry != null) { + if (entry.getName().equals(relativePath)) { + byte[] buffer = new byte[10*1024]; + int read = in.read(buffer); + while (read != -1) { + out.write(buffer, 0, read); + read = in.read(buffer); + } + break; + } + in.closeEntry(); + entry = in.getNextEntry(); + } + } + } else { + // copy from dir, and use a different canonical path to not conflict with test classpath + out.putNextEntry(new ZipEntry("test/" + clazz.getSimpleName() + ".class")); + Files.copy(codebase.resolve(relativePath), out); + } + out.closeEntry(); + } + } + } + + public void testJarHellDuplicateCodebaseWithDep() throws Exception { + Path pluginDir = createTempDir(); + Path dupJar = pluginDir.resolve("dup.jar"); + makeJar(dupJar); + Map> transitiveDeps = new HashMap<>(); + transitiveDeps.put("dep", Collections.singleton(dupJar.toUri().toURL())); + PluginInfo info1 = new PluginInfo("myplugin", "desc", "1.0", "MyPlugin", Collections.singletonList("dep"), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info1, pluginDir); + IllegalStateException e = expectThrows(IllegalStateException.class, () -> + PluginsService.checkBundleJarHell(bundle, transitiveDeps)); + assertEquals("failed to load plugin myplugin due to jar hell", e.getMessage()); + assertThat(e.getCause().getMessage(), containsString("jar hell! duplicate codebases with extended plugin")); + } + + public void testJarHellDuplicateCodebaseAcrossDeps() throws Exception { + Path pluginDir = createTempDir(); + Path pluginJar = pluginDir.resolve("plugin.jar"); + makeJar(pluginJar, DummyClass1.class); + Path otherDir = createTempDir(); + Path dupJar = otherDir.resolve("dup.jar"); + makeJar(dupJar, DummyClass2.class); + Map> transitiveDeps = new HashMap<>(); + transitiveDeps.put("dep1", Collections.singleton(dupJar.toUri().toURL())); + transitiveDeps.put("dep2", Collections.singleton(dupJar.toUri().toURL())); + PluginInfo info1 = new PluginInfo("myplugin", "desc", "1.0", "MyPlugin", Arrays.asList("dep1", "dep2"), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info1, pluginDir); + IllegalStateException e = expectThrows(IllegalStateException.class, () -> + PluginsService.checkBundleJarHell(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("duplicate codebases")); + } + + // Note: testing dup codebase with core is difficult because it requires a symlink, but we have mock filesystems and security manager + + public void testJarHellDuplicateClassWithCore() throws Exception { + // need a jar file of core dep, use log4j here + Path pluginDir = createTempDir(); + Path pluginJar = pluginDir.resolve("plugin.jar"); + makeJar(pluginJar, Level.class); + PluginInfo info1 = new PluginInfo("myplugin", "desc", "1.0", "MyPlugin", Collections.emptyList(), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info1, pluginDir); + IllegalStateException e = expectThrows(IllegalStateException.class, () -> + PluginsService.checkBundleJarHell(bundle, new HashMap<>())); + assertEquals("failed to load plugin myplugin due to jar hell", e.getMessage()); + assertThat(e.getCause().getMessage(), containsString("jar hell!")); + assertThat(e.getCause().getMessage(), containsString("Level")); + } + + public void testJarHellDuplicateClassWithDep() throws Exception { + Path pluginDir = createTempDir(); + Path pluginJar = pluginDir.resolve("plugin.jar"); + makeJar(pluginJar, DummyClass1.class); + Path depDir = createTempDir(); + Path depJar = depDir.resolve("dep.jar"); + makeJar(depJar, DummyClass1.class); + Map> transitiveDeps = new HashMap<>(); + transitiveDeps.put("dep", Collections.singleton(depJar.toUri().toURL())); + PluginInfo info1 = new PluginInfo("myplugin", "desc", "1.0", "MyPlugin", Collections.singletonList("dep"), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info1, pluginDir); + IllegalStateException e = expectThrows(IllegalStateException.class, () -> + PluginsService.checkBundleJarHell(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 testJarHellDuplicateClassAcrossDeps() throws Exception { + Path pluginDir = createTempDir(); + Path pluginJar = pluginDir.resolve("plugin.jar"); + makeJar(pluginJar, DummyClass1.class); + Path dep1Dir = createTempDir(); + Path dep1Jar = dep1Dir.resolve("dep1.jar"); + makeJar(dep1Jar, DummyClass2.class); + Path dep2Dir = createTempDir(); + Path dep2Jar = dep2Dir.resolve("dep2.jar"); + makeJar(dep2Jar, DummyClass2.class); + Map> transitiveDeps = new HashMap<>(); + transitiveDeps.put("dep1", Collections.singleton(dep1Jar.toUri().toURL())); + transitiveDeps.put("dep2", Collections.singleton(dep2Jar.toUri().toURL())); + PluginInfo info1 = new PluginInfo("myplugin", "desc", "1.0", "MyPlugin", Arrays.asList("dep1", "dep2"), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info1, pluginDir); + IllegalStateException e = expectThrows(IllegalStateException.class, () -> + PluginsService.checkBundleJarHell(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("DummyClass2")); + } + + public void testJarHellTransitiveMap() throws Exception { + Path pluginDir = createTempDir(); + Path pluginJar = pluginDir.resolve("plugin.jar"); + makeJar(pluginJar, DummyClass1.class); + Path dep1Dir = createTempDir(); + Path dep1Jar = dep1Dir.resolve("dep1.jar"); + makeJar(dep1Jar, DummyClass2.class); + Path dep2Dir = createTempDir(); + Path dep2Jar = dep2Dir.resolve("dep2.jar"); + makeJar(dep2Jar, DummyClass3.class); + Map> transitiveDeps = new HashMap<>(); + transitiveDeps.put("dep1", Collections.singleton(dep1Jar.toUri().toURL())); + transitiveDeps.put("dep2", Collections.singleton(dep2Jar.toUri().toURL())); + PluginInfo info1 = new PluginInfo("myplugin", "desc", "1.0", "MyPlugin", Arrays.asList("dep1", "dep2"), false, false); + PluginsService.Bundle bundle = new PluginsService.Bundle(info1, pluginDir); + PluginsService.checkBundleJarHell(bundle, transitiveDeps); + Set deps = transitiveDeps.get("myplugin"); + assertNotNull(deps); + assertThat(deps, containsInAnyOrder(pluginJar.toUri().toURL(), dep1Jar.toUri().toURL(), dep2Jar.toUri().toURL())); + } + + /* nocommit: make this test work, need to create a jar with a real class that does not already exist on the test classpath + public void testNonExtensibleDep() throws Exception { + Path homeDir = createTempDir(); + Settings settings = Settings.builder().put(Environment.PATH_HOME_SETTING.getKey(), homeDir).build(); + Path pluginsDir = homeDir.resolve("plugins"); + Path mypluginDir = pluginsDir.resolve("myplugin"); + PluginTestUtil.writeProperties( + mypluginDir, + "description", "whatever", + "name", "myplugin", + "version", "1.0.0", + "elasticsearch.version", Version.CURRENT.toString(), + "java.version", System.getProperty("java.specification.version"), + "extends", "nonextensible", + "classname", "test.DummyClass1"); + Path notextensibleDir = pluginsDir.resolve("nonextensible"); + PluginTestUtil.writeProperties( + notextensibleDir, + "description", "whatever", + "name", "nonextensible", + "version", "1.0.0", + "elasticsearch.version", Version.CURRENT.toString(), + "java.version", System.getProperty("java.specification.version"), + "classname", "test.NonExtensible"); + makeJar(notextensibleDir.resolve("plugin.jar"), NonExtensible.class); + IllegalStateException e = expectThrows(IllegalStateException.class, () -> newPluginsService(settings)); + assertEquals("Plugin [myplugin] cannot extend non-extensible plugin [nonextensible]", e.getMessage()); + }*/ } diff --git a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java index 7029ba048d031..accc8bd9da376 100644 --- a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java +++ b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java @@ -33,7 +33,6 @@ import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.hash.MessageDigests; -import org.elasticsearch.common.io.FileSystemUtils; import org.elasticsearch.common.settings.KeyStoreWrapper; import org.elasticsearch.env.Environment; @@ -63,9 +62,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.TreeSet; @@ -555,7 +556,7 @@ private PluginInfo verify(Terminal terminal, Path pluginRoot, boolean isBatch, E } // check for jar hell before any copying - jarHellCheck(pluginRoot, env.pluginsFile()); + jarHellCheck(info, pluginRoot, env.pluginsFile(), env.modulesFile()); // read optional security policy (extra permissions) // if it exists, confirm or warn the user @@ -568,25 +569,25 @@ private PluginInfo verify(Terminal terminal, Path pluginRoot, boolean isBatch, E } /** check a candidate plugin for jar hell before installing it */ - void jarHellCheck(Path candidate, Path pluginsDir) throws Exception { + void jarHellCheck(PluginInfo info, Path candidate, Path pluginsDir, Path modulesDir) throws Exception { // create list of current jars in classpath final Set jars = new HashSet<>(JarHell.parseClassPath()); // read existing bundles. this does some checks on the installation too. - PluginsService.getPluginBundles(pluginsDir); + Set bundles = new HashSet<>(PluginsService.getPluginBundles(pluginsDir)); + bundles.addAll(PluginsService.getPluginBundles(modulesDir)); + bundles.add(new PluginsService.Bundle(info, candidate)); + List sortedBundles = PluginsService.sortBundles(bundles); - // add plugin jars to the list - Path pluginJars[] = FileSystemUtils.files(candidate, "*.jar"); - for (Path jar : pluginJars) { - if (jars.add(jar.toUri().toURL()) == false) { - throw new IllegalStateException("jar hell! duplicate plugin jar: " + jar); - } + // check jarhell of all plugins so we know this plugin and anything depending on it are ok together + // TODO: optimize to skip any bundles not connected to the candidate plugin? + Map> transitiveUrls = new HashMap<>(); + for (PluginsService.Bundle bundle : sortedBundles) { + PluginsService.checkBundleJarHell(bundle, transitiveUrls); } + // TODO: no jars should be an error // TODO: verify the classname exists in one of the jars! - - // check combined (current classpath + new jars to-be-added) - JarHell.checkJarHell(jars); } /** diff --git a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java index 3cd5a3290439e..7c3e1f16d1d58 100644 --- a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java +++ b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java @@ -36,6 +36,7 @@ import java.util.Arrays; import java.util.List; import java.util.Locale; +import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -46,6 +47,10 @@ */ class RemovePluginCommand extends EnvironmentAwareCommand { + // exit codes for remove + /** A plugin cannot be removed because it is extended by another plugin. */ + static final int PLUGIN_STILL_USED = 11; + private final OptionSpec purgeOption; private final OptionSpec arguments; @@ -74,20 +79,31 @@ protected void execute(final Terminal terminal, final OptionSet options, final E * @throws UserException if plugin directory does not exist * @throws UserException if the plugin bin directory is not a directory */ - void execute( - final Terminal terminal, - final Environment env, - final String pluginName, - final boolean purge) throws IOException, UserException { + void execute(Terminal terminal, Environment env, String pluginName, boolean purge) throws IOException, UserException { if (pluginName == null) { throw new UserException(ExitCodes.USAGE, "plugin name is required"); } - terminal.println("-> removing [" + pluginName + "]..."); + // first make sure nothing extends this plugin + List usedBy = new ArrayList<>(); + Set bundles = PluginsService.getPluginBundles(env.pluginsFile()); + for (PluginsService.Bundle bundle : bundles) { + for (String extendedPlugin : bundle.plugin.getExtendsPlugins()) { + if (extendedPlugin.equals(pluginName)) { + usedBy.add(bundle.plugin.getName()); + } + } + } + if (usedBy.isEmpty() == false) { + throw new UserException(PLUGIN_STILL_USED, "plugin [" + pluginName + "] cannot be removed" + + " because it is extended by other plugins: " + usedBy); + } final Path pluginDir = env.pluginsFile().resolve(pluginName); final Path pluginConfigDir = env.configFile().resolve(pluginName); final Path removing = env.pluginsFile().resolve(".removing-" + pluginName); + + terminal.println("-> removing [" + pluginName + "]..."); /* * If the plugin does not exist and the plugin config does not exist, fail to the user that the plugin is not found, unless there's * a marker file left from a previously failed attempt in which case we proceed to clean up the marker file. Or, if the plugin does diff --git a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java index 8e37b10efc83f..e545609ccbff1 100644 --- a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java +++ b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/InstallPluginCommandTests.java @@ -115,7 +115,7 @@ public void setUp() throws Exception { super.setUp(); skipJarHellCommand = new InstallPluginCommand() { @Override - void jarHellCheck(Path candidate, Path pluginsDir) throws Exception { + void jarHellCheck(PluginInfo info, Path candidate, Path pluginsDir, Path modulesDir) throws Exception { // no jarhell check } }; @@ -791,7 +791,7 @@ String getStagingHash() { return stagingHash; } @Override - void jarHellCheck(Path candidate, Path pluginsDir) throws Exception { + void jarHellCheck(PluginInfo info, Path candidate, Path pluginsDir, Path modulesDir) throws Exception { // no jarhell check } }; 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 efdd36172d47e..28b860bb539ce 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 @@ -22,6 +22,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.plugins.ExtensiblePlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.plugins.ScriptPlugin; import org.elasticsearch.script.ScriptContext; @@ -34,7 +35,7 @@ /** * Registers Painless as a plugin. */ -public final class PainlessPlugin extends Plugin implements ScriptPlugin { +public final class PainlessPlugin extends Plugin implements ScriptPlugin, ExtensiblePlugin { // force to parse our definition at startup (not on the user's first script) static { diff --git a/plugins/examples/painless-whitelist/build.gradle b/plugins/examples/painless-whitelist/build.gradle new file mode 100644 index 0000000000000..6e03a3c93c2bd --- /dev/null +++ b/plugins/examples/painless-whitelist/build.gradle @@ -0,0 +1,33 @@ +/* + * 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. + */ + +apply plugin: 'elasticsearch.esplugin' + +esplugin { + name 'painless-whitelist' + description 'An example whitelisting additional classes and methods in painless' + classname 'org.elasticsearch.example.painlesswhitelist.MyWhitelistPlugin' + extendsPlugins = ['lang-painless'] +} + +integTestCluster { + distribution = 'zip' +} + +test.enabled = false diff --git a/plugins/examples/painless-whitelist/src/main/java/org/elasticsearch/example/painlesswhitelist/MyWhitelistPlugin.java b/plugins/examples/painless-whitelist/src/main/java/org/elasticsearch/example/painlesswhitelist/MyWhitelistPlugin.java new file mode 100644 index 0000000000000..877a05391ac77 --- /dev/null +++ b/plugins/examples/painless-whitelist/src/main/java/org/elasticsearch/example/painlesswhitelist/MyWhitelistPlugin.java @@ -0,0 +1,25 @@ +/* + * 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. + */ + +package org.elasticsearch.example.painlesswhitelist; + +import org.elasticsearch.plugins.Plugin; + +public class MyWhitelistPlugin extends Plugin { +} diff --git a/plugins/examples/painless-whitelist/src/test/java/org/elasticsearch/example/painlesswhitelist/PainlessWhitelistClientYamlTestSuiteIT.java b/plugins/examples/painless-whitelist/src/test/java/org/elasticsearch/example/painlesswhitelist/PainlessWhitelistClientYamlTestSuiteIT.java new file mode 100644 index 0000000000000..42642602c0b77 --- /dev/null +++ b/plugins/examples/painless-whitelist/src/test/java/org/elasticsearch/example/painlesswhitelist/PainlessWhitelistClientYamlTestSuiteIT.java @@ -0,0 +1,38 @@ +/* + * 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. + */ + +package org.elasticsearch.example.painlesswhitelist; + +import com.carrotsearch.randomizedtesting.annotations.Name; +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; +import org.elasticsearch.test.rest.yaml.ClientYamlTestCandidate; +import org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase; + +public class PainlessWhitelistClientYamlTestSuiteIT extends ESClientYamlSuiteTestCase { + + public PainlessWhitelistClientYamlTestSuiteIT(@Name("yaml") ClientYamlTestCandidate testCandidate) { + super(testCandidate); + } + + @ParametersFactory + public static Iterable parameters() throws Exception { + return ESClientYamlSuiteTestCase.createParameters(); + } +} + diff --git a/plugins/examples/painless-whitelist/src/test/resources/rest-api-spec/test/painless_whitelist/10_basic.yml b/plugins/examples/painless-whitelist/src/test/resources/rest-api-spec/test/painless_whitelist/10_basic.yml new file mode 100644 index 0000000000000..f0abcf117da15 --- /dev/null +++ b/plugins/examples/painless-whitelist/src/test/resources/rest-api-spec/test/painless_whitelist/10_basic.yml @@ -0,0 +1,13 @@ +# Integration tests for the painless whitelist example plugin +# +"Plugin loaded": + - do: + cluster.state: {} + + # Get master node id + - set: { master_node: master } + + - do: + nodes.info: {} + + - match: { nodes.$master.plugins.0.name: painless-whitelist } From a9bf9ac4167ee978087ff5d14d6543835044f47f Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 19 Dec 2017 13:41:05 -0800 Subject: [PATCH 02/16] Move pluginc classloader to a separate jar --- core/build.gradle | 4 ++ .../elasticsearch/plugins/PluginsService.java | 28 +-------- .../elasticsearch/bootstrap/security.policy | 8 ++- distribution/build.gradle | 1 + libs/plugin-classloader/build.gradle | 3 + .../plugins/ExtendedPluginsClassLoader.java | 59 +++++++++++++++++++ settings.gradle | 8 +++ 7 files changed, 81 insertions(+), 30 deletions(-) create mode 100644 libs/plugin-classloader/build.gradle create mode 100644 libs/plugin-classloader/src/main/java/org/elasticsearch/plugins/ExtendedPluginsClassLoader.java diff --git a/core/build.gradle b/core/build.gradle index 1a756e0551b29..4dc0b3075745a 100644 --- a/core/build.gradle +++ b/core/build.gradle @@ -38,6 +38,10 @@ archivesBaseName = 'elasticsearch' dependencies { + // nocommit: should be provided? + provided project(':libs:plugin-classloader') + testRuntime project(':libs:plugin-classloader') + // lucene compile "org.apache.lucene:lucene-core:${versions.lucene}" compile "org.apache.lucene:lucene-analyzers-common:${versions.lucene}" diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index 76e9195ed9e6b..5e8562324af34 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -274,32 +274,6 @@ public int hashCode() { } } - /** - * A classloader that is a union over the parent core classloader and classloaders of extended plugins. - */ - private static class ExtendedPluginsClassLoader extends ClassLoader { - - /** Loaders of plugins extended by a plugin. */ - private List extendedLoaders; - - ExtendedPluginsClassLoader(ClassLoader parent, List extendedLoaders) { - super(parent); - this.extendedLoaders = extendedLoaders; - } - - @Override - protected Class findClass(String name) throws ClassNotFoundException { - for (ClassLoader loader : extendedLoaders) { - try { - return loader.loadClass(name); - } catch (ClassNotFoundException e) { - // continue - } - } - throw new ClassNotFoundException(name); - } - } - // similar in impl to getPluginBundles, but DO NOT try to make them share code. // we don't need to inherit all the leniency, and things are different enough. static Set getModuleBundles(Path modulesDirectory) throws IOException { @@ -508,7 +482,7 @@ private Plugin loadBundle(Bundle bundle, Map loaded) { } // create a child to load the plugin in this bundle - ExtendedPluginsClassLoader parentLoader = new ExtendedPluginsClassLoader(getClass().getClassLoader(), extendedLoaders); + ExtendedPluginsClassLoader parentLoader = ExtendedPluginsClassLoader.create(getClass().getClassLoader(), extendedLoaders); ClassLoader loader = URLClassLoader.newInstance(bundle.urls.toArray(new URL[0]), parentLoader); // reload SPI with any new services from the plugin diff --git a/core/src/main/resources/org/elasticsearch/bootstrap/security.policy b/core/src/main/resources/org/elasticsearch/bootstrap/security.policy index 5395840b28927..d11d7085db575 100644 --- a/core/src/main/resources/org/elasticsearch/bootstrap/security.policy +++ b/core/src/main/resources/org/elasticsearch/bootstrap/security.policy @@ -47,6 +47,11 @@ grant codeBase "${codebase.lucene-misc}" { permission java.nio.file.LinkPermission "hard"; }; +grant codeBase "${codebase.plugin-classloader}" { + // needed to create the classloader which allows plugins to extend other plugins + permission java.lang.RuntimePermission "createClassLoader"; +}; + //// Everything else: grant { @@ -131,7 +136,4 @@ grant { permission java.io.FilePermission "/sys/fs/cgroup/cpuacct/-", "read"; permission java.io.FilePermission "/sys/fs/cgroup/memory", "read"; permission java.io.FilePermission "/sys/fs/cgroup/memory/-", "read"; - - // nocommit: needed by plugin service for extensible plugins, move to a separate jar - permission java.lang.RuntimePermission "createClassLoader"; }; diff --git a/distribution/build.gradle b/distribution/build.gradle index 26221cad5c7f6..f5f5be517a5b9 100644 --- a/distribution/build.gradle +++ b/distribution/build.gradle @@ -163,6 +163,7 @@ configure(distributions) { into 'lib' from project(':core').jar from project(':core').configurations.runtime + from { project(':libs:plugin-classloader').jar } // delay add tools using closures, since they have not yet been configured, so no jar task exists yet from { project(':distribution:tools:launchers').jar } from { project(':distribution:tools:plugin-cli').jar } diff --git a/libs/plugin-classloader/build.gradle b/libs/plugin-classloader/build.gradle new file mode 100644 index 0000000000000..e421131de8520 --- /dev/null +++ b/libs/plugin-classloader/build.gradle @@ -0,0 +1,3 @@ + + +apply plugin: 'elasticsearch.build' diff --git a/libs/plugin-classloader/src/main/java/org/elasticsearch/plugins/ExtendedPluginsClassLoader.java b/libs/plugin-classloader/src/main/java/org/elasticsearch/plugins/ExtendedPluginsClassLoader.java new file mode 100644 index 0000000000000..8346725863f68 --- /dev/null +++ b/libs/plugin-classloader/src/main/java/org/elasticsearch/plugins/ExtendedPluginsClassLoader.java @@ -0,0 +1,59 @@ +/* + * 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. + */ + +package org.elasticsearch.plugins; + +import java.security.AccessController; +import java.security.PrivilegedAction; +import java.util.Collections; +import java.util.List; + +/** + * A classloader that is a union over the parent core classloader and classloaders of extended plugins. + */ +public class ExtendedPluginsClassLoader extends ClassLoader { + + /** Loaders of plugins extended by a plugin. */ + private final List extendedLoaders; + + private ExtendedPluginsClassLoader(ClassLoader parent, List extendedLoaders) { + super(parent); + this.extendedLoaders = Collections.unmodifiableList(extendedLoaders); + } + + @Override + protected Class findClass(String name) throws ClassNotFoundException { + for (ClassLoader loader : extendedLoaders) { + try { + return loader.loadClass(name); + } catch (ClassNotFoundException e) { + // continue + } + } + throw new ClassNotFoundException(name); + } + + /** + * Return a new classloader across the parent and extended loaders. + */ + public static ExtendedPluginsClassLoader create(ClassLoader parent, List extendedLoaders) { + return AccessController.doPrivileged((PrivilegedAction) + () -> new ExtendedPluginsClassLoader(parent, extendedLoaders)); + } +} diff --git a/settings.gradle b/settings.gradle index 5150ef256a17a..d99315341c3a9 100644 --- a/settings.gradle +++ b/settings.gradle @@ -97,6 +97,14 @@ for (File example : examplePluginsDir.listFiles()) { examplePlugins.add(example.name) } +projects.add("libs") +File libsDir = new File(rootProject.projectDir, 'libs') +for (File libDir : new File(rootProject.projectDir, 'libs').listFiles()) { + if (libDir.isDirectory() == false) continue; + if (libDir.name.startsWith('build') || libDir.name.startsWith('.')) continue; + projects.add("libs:${libDir.name}".toString()) +} + /* Create projects for building BWC snapshot distributions from the heads of other branches */ final List branches = ['5.6', '6.0', '6.1', '6.x'] for (final String branch : branches) { From 1adc24350f2313818417ca82b66de1c663288e81 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 19 Dec 2017 17:47:54 -0800 Subject: [PATCH 03/16] Remove reference to plugin classloader from core pom and fix transport client --- core/build.gradle | 3 +- .../plugins/PluginLoaderIndirection.java | 33 +++++++++++++++++++ .../elasticsearch/plugins/PluginsService.java | 2 +- 3 files changed, 35 insertions(+), 3 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/plugins/PluginLoaderIndirection.java diff --git a/core/build.gradle b/core/build.gradle index 4dc0b3075745a..32c7017acaae4 100644 --- a/core/build.gradle +++ b/core/build.gradle @@ -38,8 +38,7 @@ archivesBaseName = 'elasticsearch' dependencies { - // nocommit: should be provided? - provided project(':libs:plugin-classloader') + compileOnly project(':libs:plugin-classloader') testRuntime project(':libs:plugin-classloader') // lucene diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginLoaderIndirection.java b/core/src/main/java/org/elasticsearch/plugins/PluginLoaderIndirection.java new file mode 100644 index 0000000000000..7a07e4f08e849 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/plugins/PluginLoaderIndirection.java @@ -0,0 +1,33 @@ +/* + * 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. + */ + +package org.elasticsearch.plugins; + +import java.util.List; + +/** + * This class exists solely as an intermediate layer to avoid causing PluginsService + * to load ExtendedPluginsClassLoader when used in the transport client. + */ +class PluginLoaderIndirection { + + static ClassLoader createLoader(ClassLoader parent, List extendedLoaders) { + return ExtendedPluginsClassLoader.create(parent, extendedLoaders); + } +} diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index 5e8562324af34..4564f9c878671 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -482,7 +482,7 @@ private Plugin loadBundle(Bundle bundle, Map loaded) { } // create a child to load the plugin in this bundle - ExtendedPluginsClassLoader parentLoader = ExtendedPluginsClassLoader.create(getClass().getClassLoader(), extendedLoaders); + ClassLoader parentLoader = PluginLoaderIndirection.createLoader(getClass().getClassLoader(), extendedLoaders); ClassLoader loader = URLClassLoader.newInstance(bundle.urls.toArray(new URL[0]), parentLoader); // reload SPI with any new services from the plugin From 0f0e067a1ee567a11ebdd841f5519326e82bf748 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 20 Dec 2017 07:59:32 -0800 Subject: [PATCH 04/16] fixup test --- .../plugins/PluginsServiceTests.java | 15 +++++++-------- .../plugins/non-extensible-plugin.jar | Bin 0 -> 711 bytes 2 files changed, 7 insertions(+), 8 deletions(-) create mode 100644 core/src/test/resources/org/elasticsearch/plugins/non-extensible-plugin.jar diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index f8511a568e598..6679dcb5a9390 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -376,8 +376,6 @@ public static class DummyClass2 {} public static class DummyClass3 {} - public static class NonExtensible extends Plugin {} - void makeJar(Path jarFile, Class... classes) throws Exception { try (ZipOutputStream out = new ZipOutputStream(Files.newOutputStream(jarFile))) { for (Class clazz : classes) { @@ -527,7 +525,6 @@ public void testJarHellTransitiveMap() throws Exception { assertThat(deps, containsInAnyOrder(pluginJar.toUri().toURL(), dep1Jar.toUri().toURL(), dep2Jar.toUri().toURL())); } - /* nocommit: make this test work, need to create a jar with a real class that does not already exist on the test classpath public void testNonExtensibleDep() throws Exception { Path homeDir = createTempDir(); Settings settings = Settings.builder().put(Environment.PATH_HOME_SETTING.getKey(), homeDir).build(); @@ -542,17 +539,19 @@ public void testNonExtensibleDep() throws Exception { "java.version", System.getProperty("java.specification.version"), "extends", "nonextensible", "classname", "test.DummyClass1"); - Path notextensibleDir = pluginsDir.resolve("nonextensible"); + Path nonextensibleDir = pluginsDir.resolve("nonextensible"); PluginTestUtil.writeProperties( - notextensibleDir, + nonextensibleDir, "description", "whatever", "name", "nonextensible", "version", "1.0.0", "elasticsearch.version", Version.CURRENT.toString(), "java.version", System.getProperty("java.specification.version"), - "classname", "test.NonExtensible"); - makeJar(notextensibleDir.resolve("plugin.jar"), NonExtensible.class); + "classname", "test.NonExtensiblePlugin"); + try (InputStream jar = PluginsServiceTests.class.getResourceAsStream("non-extensible-plugin.jar")) { + Files.copy(jar, nonextensibleDir.resolve("plugin.jar")); + } IllegalStateException e = expectThrows(IllegalStateException.class, () -> newPluginsService(settings)); assertEquals("Plugin [myplugin] cannot extend non-extensible plugin [nonextensible]", e.getMessage()); - }*/ + } } diff --git a/core/src/test/resources/org/elasticsearch/plugins/non-extensible-plugin.jar b/core/src/test/resources/org/elasticsearch/plugins/non-extensible-plugin.jar new file mode 100644 index 0000000000000000000000000000000000000000..54d330349eda2a696fff9ba97c31af5ad2501716 GIT binary patch literal 711 zcmWIWW@Zs#;Nak3*xfhTn*j-MGO#fCx`sIFdiuHP|2xINz|0Wf&CUT*!30$nfK#&w zPz7AGucM!*n`>~0p0C?y-!rFuymj?1@_OrPojY@WbCAIm;|EWR^t^m^Jbf>gu43Vg zcp-U2de#h$j345vYR{KwSv(gPeIl+pjp1tcr=pKVj9^#Ht0@b10ovgT#0XbRL-T?h zP_`tsxJ2JCKhL$IBsH%%GbtxEAg44vGfyu$C$YG=)Zf;d*-@Z&a?HCgHoZ%@!fvf> zy?sN|Y|&LgCa+&-cd95qU-#x+XZ>lf4IkKlEIzWccj>tU$@h-)oVot}xOzkJo9Pls z+YiJw8Y=EjIsLKAz{W08U{m(@1dhkE*3K;T_kR5T>n73EpxtiUx!>-5zCTIqx(o+< zsfpsMnVZemU%YW3>D=wSqu-hW?(IBh{%qRs)|c`#H@p8`q@5zs`TxG+p@pBdQ=?>+ z%nxSVUvss1Q`0S%f|kabYvP`#Z4buG@RE_3yRZ3Pio^neGm|ZjPD|LznIN4Sb>DO4 zD|S#+`M=$^>?SY>Oc+6d#K Date: Wed, 20 Dec 2017 12:27:23 -0800 Subject: [PATCH 05/16] Add dummy plugin for plugin service tests, and add support in BootstrapForTesting to fill in libs codebases when testing --- .../org/elasticsearch/bootstrap/ESPolicy.java | 7 +-- .../org/elasticsearch/bootstrap/Security.java | 43 ++++++++++++++---- .../plugins/PluginsServiceTests.java | 20 +++++++- .../elasticsearch/plugins/dummy-plugin.jar | Bin 0 -> 689 bytes .../plugins/PluginSecurityTests.java | 6 +-- .../bootstrap/BootstrapForTesting.java | 21 +++++++-- 6 files changed, 76 insertions(+), 21 deletions(-) create mode 100644 core/src/test/resources/org/elasticsearch/plugins/dummy-plugin.jar diff --git a/core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java b/core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java index 74fa7e0c1d5ac..95de5ccca59ba 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java @@ -33,6 +33,7 @@ import java.security.ProtectionDomain; import java.util.Collections; import java.util.Map; +import java.util.Set; import java.util.function.Predicate; /** custom policy for union of static and dynamic permissions */ @@ -49,9 +50,9 @@ final class ESPolicy extends Policy { final PermissionCollection dynamic; final Map plugins; - ESPolicy(PermissionCollection dynamic, Map plugins, boolean filterBadDefaults) { - this.template = Security.readPolicy(getClass().getResource(POLICY_RESOURCE), JarHell.parseClassPath()); - this.untrusted = Security.readPolicy(getClass().getResource(UNTRUSTED_RESOURCE), Collections.emptySet()); + ESPolicy(Map codebases, PermissionCollection dynamic, Map plugins, boolean filterBadDefaults) { + this.template = Security.readPolicy(getClass().getResource(POLICY_RESOURCE), codebases); + this.untrusted = Security.readPolicy(getClass().getResource(UNTRUSTED_RESOURCE), Collections.emptyMap()); if (filterBadDefaults) { this.system = new SystemPolicy(Policy.getPolicy()); } else { diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index 3693f5cba58f0..92aecb8ecbe41 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -48,10 +48,13 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; import static org.elasticsearch.bootstrap.FilePermissionUtils.addDirectoryPath; import static org.elasticsearch.bootstrap.FilePermissionUtils.addSingleFilePath; @@ -116,7 +119,8 @@ private Security() {} static void configure(Environment environment, boolean filterBadDefaults) throws IOException, NoSuchAlgorithmException { // enable security policy: union of template and environment-based paths, and possibly plugin permissions - Policy.setPolicy(new ESPolicy(createPermissions(environment), getPluginPermissions(environment), filterBadDefaults)); + Map codebases = getCodebaseJarMap(JarHell.parseClassPath()); + Policy.setPolicy(new ESPolicy(codebases, createPermissions(environment), getPluginPermissions(environment), filterBadDefaults)); // enable security manager final String[] classesThatCanExit = @@ -130,6 +134,26 @@ static void configure(Environment environment, boolean filterBadDefaults) throws selfTest(); } + /** + * Return a map from codebase name to codebase url of jar codebases used by ES core. + */ + static Map getCodebaseJarMap(Set urls) { + Map codebases = new LinkedHashMap<>(); // maintain order + for (URL url : urls) { + try { + String fileName = PathUtils.get(url.toURI()).getFileName().toString(); + if (fileName.endsWith(".jar") == false) { + // tests :( + continue; + } + codebases.put(fileName, url); + } catch (URISyntaxException e) { + throw new RuntimeException(e); + } + } + return codebases; + } + /** * Sets properties (codebase URLs) for policy files. * we look for matching plugins and set URLs to fit @@ -174,7 +198,7 @@ static Map getPluginPermissions(Environment environment) throws I } // parse the plugin's policy file into a set of permissions - Policy policy = readPolicy(policyFile.toUri().toURL(), codebases); + Policy policy = readPolicy(policyFile.toUri().toURL(), getCodebaseJarMap(codebases)); // consult this policy for each of the plugin's jars: for (URL url : codebases) { @@ -197,21 +221,20 @@ static Map getPluginPermissions(Environment environment) throws I * would map to full URL. */ @SuppressForbidden(reason = "accesses fully qualified URLs to configure security") - static Policy readPolicy(URL policyFile, Set codebases) { + static Policy readPolicy(URL policyFile, Map codebases) { try { List propertiesSet = new ArrayList<>(); try { // set codebase properties - for (URL url : codebases) { - String fileName = PathUtils.get(url.toURI()).getFileName().toString(); - if (fileName.endsWith(".jar") == false) { - continue; // tests :( - } + for (Map.Entry codebase : codebases.entrySet()) { + String name = codebase.getKey(); + URL url = codebase.getValue(); + // We attempt to use a versionless identifier for each codebase. This assumes a specific version // format in the jar filename. While we cannot ensure all jars in all plugins use this format, nonconformity // only means policy grants would need to include the entire jar filename as they always have before. - String property = "codebase." + fileName; - String aliasProperty = "codebase." + fileName.replaceFirst("-\\d+\\.\\d+.*\\.jar", ""); + String property = "codebase." + name; + String aliasProperty = "codebase." + name.replaceFirst("-\\d+\\.\\d+.*\\.jar", ""); if (aliasProperty.equals(property) == false) { propertiesSet.add(aliasProperty); String previous = System.setProperty(aliasProperty, url.toString()); diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index 6679dcb5a9390..bab969cc3ec81 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -370,6 +370,19 @@ public void testSortBundlesCommonDep() throws Exception { assertThat(sortedBundles, Matchers.contains(bundle1, bundle4, bundle2, bundle3)); } + public void testSortBundlesAlreadyOrdered() throws Exception { + Path pluginDir = createTempDir(); + Set bundles = new LinkedHashSet<>(); // control iteration order + PluginInfo info1 = new PluginInfo("dep", "desc", "1.0", "MyPlugin", Collections.emptyList(), false, false); + PluginsService.Bundle bundle1 = new PluginsService.Bundle(info1, pluginDir); + bundles.add(bundle1); + PluginInfo info2 = new PluginInfo("myplugin", "desc", "1.0", "MyPlugin", Collections.singletonList("dep"), false, false); + PluginsService.Bundle bundle2 = new PluginsService.Bundle(info2, pluginDir); + bundles.add(bundle2); + List sortedBundles = PluginsService.sortBundles(bundles); + assertThat(sortedBundles, Matchers.contains(bundle1, bundle2)); + } + public static class DummyClass1 {} public static class DummyClass2 {} @@ -537,8 +550,11 @@ public void testNonExtensibleDep() throws Exception { "version", "1.0.0", "elasticsearch.version", Version.CURRENT.toString(), "java.version", System.getProperty("java.specification.version"), - "extends", "nonextensible", - "classname", "test.DummyClass1"); + "extends.plugins", "nonextensible", + "classname", "test.DummyPlugin"); + try (InputStream jar = PluginsServiceTests.class.getResourceAsStream("dummy-plugin.jar")) { + Files.copy(jar, mypluginDir.resolve("plugin.jar")); + } Path nonextensibleDir = pluginsDir.resolve("nonextensible"); PluginTestUtil.writeProperties( nonextensibleDir, diff --git a/core/src/test/resources/org/elasticsearch/plugins/dummy-plugin.jar b/core/src/test/resources/org/elasticsearch/plugins/dummy-plugin.jar new file mode 100644 index 0000000000000000000000000000000000000000..4f7d81df4fa30f28b2353c24265b4ebb1b278636 GIT binary patch literal 689 zcmWIWW@Zs#;Nak3$cdlg&42_r8CV#6T|*poJ^kGD|D9rBU}gyLX6FE@V1g&&<$(c8A{0n=pT>R>DL65AL1C*<6Kea-Z$e68xSY7CfN+OL0~O&jsEK$sg{A zEPP|_Tx@dBpki~8wq%ue=!t%<{-ys!3f)d!-sI!5O)%=}pZG6t{AS+Td?~_i{ii?a zQ$ts3E#rB(YfXug_$t0{`Fl=IztPPh7q#TRz7dc2+@hJB$)QP0E}ZPEw?6$Lo8^4Q z1@=8NYQw$6M6vD;h literal 0 HcmV?d00001 diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/PluginSecurityTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/PluginSecurityTests.java index 1c60e3264c72e..72092a53e080a 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/plugins/PluginSecurityTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/plugins/PluginSecurityTests.java @@ -48,7 +48,7 @@ public void testHasNativeController() throws IOException { "test cannot run with security manager enabled", System.getSecurityManager() == null); final PluginInfo info = - new PluginInfo("fake", "fake", Version.CURRENT.toString(), "Fake", true, false); + new PluginInfo("fake", "fake", Version.CURRENT.toString(), "Fake", Collections.emptyList(), true, false); final MockTerminal terminal = new MockTerminal(); terminal.addTextInput("y"); terminal.addTextInput("y"); @@ -63,7 +63,7 @@ public void testDeclineNativeController() throws IOException { "test cannot run with security manager enabled", System.getSecurityManager() == null); final PluginInfo info = - new PluginInfo("fake", "fake", Version.CURRENT.toString(), "Fake", true, false); + new PluginInfo("fake", "fake", Version.CURRENT.toString(), "Fake", Collections.emptyList(), true, false); final MockTerminal terminal = new MockTerminal(); terminal.addTextInput("y"); terminal.addTextInput("n"); @@ -79,7 +79,7 @@ public void testDoesNotHaveNativeController() throws IOException { "test cannot run with security manager enabled", System.getSecurityManager() == null); final PluginInfo info = - new PluginInfo("fake", "fake", Version.CURRENT.toString(), "Fake", false, false); + new PluginInfo("fake", "fake", Version.CURRENT.toString(), "Fake", Collections.emptyList(), false, false); final MockTerminal terminal = new MockTerminal(); terminal.addTextInput("y"); final Path policyFile = this.getDataPath("security/simple-plugin-security.policy"); diff --git a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java index e4ecd02615e91..1afe120f1a14a 100644 --- a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -131,8 +131,13 @@ public class BootstrapForTesting { perms.add(new SocketPermission("localhost:1024-", "listen,resolve")); // read test-framework permissions - final Policy testFramework = Security.readPolicy(Bootstrap.class.getResource("test-framework.policy"), JarHell.parseClassPath()); - final Policy esPolicy = new ESPolicy(perms, getPluginPermissions(), true); + Map codebases = Security.getCodebaseJarMap(JarHell.parseClassPath()); + if (System.getProperty("tests.gradle") == null) { + // intellij and eclipse don't package our internal libs, so we need to set the codebases for them manually + addClassCodebase(codebases,"plugin-classloader", "org.elasticsearch.plugins.ExtendedPluginsClassLoader"); + } + final Policy testFramework = Security.readPolicy(Bootstrap.class.getResource("test-framework.policy"), codebases); + final Policy esPolicy = new ESPolicy(codebases, perms, getPluginPermissions(), true); Policy.setPolicy(new Policy() { @Override public boolean implies(ProtectionDomain domain, Permission permission) { @@ -161,6 +166,16 @@ public boolean implies(ProtectionDomain domain, Permission permission) { } } + /** Add the codebase url of the given classname to the codebases map, if the class exists. */ + private static void addClassCodebase(Map codebases, String name, String classname) { + try { + Class clazz = BootstrapForTesting.class.getClassLoader().loadClass(classname); + codebases.put(name, clazz.getProtectionDomain().getCodeSource().getLocation()); + } catch (ClassNotFoundException e) { + // no class, fall through to not add + } + } + /** * we don't know which codesources belong to which plugin, so just remove the permission from key codebases * like core, test-framework, etc. this way tests fail if accesscontroller blocks are missing. @@ -191,7 +206,7 @@ static Map getPluginPermissions() throws Exception { // parse each policy file, with codebase substitution from the classpath final List policies = new ArrayList<>(pluginPolicies.size()); for (URL policyFile : pluginPolicies) { - policies.add(Security.readPolicy(policyFile, codebases)); + policies.add(Security.readPolicy(policyFile, Security.getCodebaseJarMap(codebases))); } // consult each policy file for those codebases From 083b922cf14961924767fcce9ce5d8c2e322fbd5 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 20 Dec 2017 12:34:04 -0800 Subject: [PATCH 06/16] add insurance --- .../java/org/elasticsearch/bootstrap/BootstrapForTesting.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java index 1afe120f1a14a..13aba14669051 100644 --- a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -170,7 +170,9 @@ public boolean implies(ProtectionDomain domain, Permission permission) { private static void addClassCodebase(Map codebases, String name, String classname) { try { Class clazz = BootstrapForTesting.class.getClassLoader().loadClass(classname); - codebases.put(name, clazz.getProtectionDomain().getCodeSource().getLocation()); + if (codebases.put(name, clazz.getProtectionDomain().getCodeSource().getLocation()) != null) { + throw new IllegalStateException("Already added " + name + " codebase for testing"); + } } catch (ClassNotFoundException e) { // no class, fall through to not add } From 319e88a8d1ff6da273ebebf9aaefc0a3d1241a81 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 20 Dec 2017 19:51:54 -0800 Subject: [PATCH 07/16] Address PR comments and add forbidden suppression --- .../plugin/PluginPropertiesExtension.groovy | 2 +- .../gradle/plugin/PluginPropertiesTask.groovy | 2 +- .../resources/plugin-descriptor.properties | 4 +-- .../org/elasticsearch/bootstrap/Security.java | 1 + .../org/elasticsearch/plugins/PluginInfo.java | 32 +++++++++---------- .../elasticsearch/plugins/PluginsService.java | 8 ++--- .../plugins/PluginInfoTests.java | 12 ++++--- .../plugins/RemovePluginCommand.java | 2 +- .../examples/painless-whitelist/build.gradle | 2 +- .../bootstrap/BootstrapForTesting.java | 3 +- 10 files changed, 37 insertions(+), 31 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy index 1fd7269237bfd..6cfe44c806833 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy @@ -41,7 +41,7 @@ class PluginPropertiesExtension { /** Other plugins this plugin extends through SPI */ @Input - List extendsPlugins = [] + List extendedPlugins = [] @Input boolean hasNativeController = false diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy index d3c2b059f0bcb..f5dbcfd8b0d48 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesTask.groovy @@ -80,7 +80,7 @@ class PluginPropertiesTask extends Copy { 'elasticsearchVersion': stringSnap(VersionProperties.elasticsearch), 'javaVersion': project.targetCompatibility as String, 'classname': extension.classname, - 'extendsPlugins': extension.extendsPlugins.join(','), + 'extendedPlugins': extension.extendedPlugins.join(','), 'hasNativeController': extension.hasNativeController, 'requiresKeystore': extension.requiresKeystore ] diff --git a/buildSrc/src/main/resources/plugin-descriptor.properties b/buildSrc/src/main/resources/plugin-descriptor.properties index 8a21fae2423f1..d9c51b3a73507 100644 --- a/buildSrc/src/main/resources/plugin-descriptor.properties +++ b/buildSrc/src/main/resources/plugin-descriptor.properties @@ -40,8 +40,8 @@ java.version=${javaVersion} elasticsearch.version=${elasticsearchVersion} ### optional elements for plugins: # -# 'extends.plugins': other plugins this plugin extends through SPI -extends.plugins=${extendsPlugins} +# 'extended.plugins': other plugins this plugin extends through SPI +extended.plugins=${extendedPlugins} # # 'has.native.controller': whether or not the plugin has a native controller has.native.controller=${hasNativeController} diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index 92aecb8ecbe41..89a1f794e89f8 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -137,6 +137,7 @@ static void configure(Environment environment, boolean filterBadDefaults) throws /** * Return a map from codebase name to codebase url of jar codebases used by ES core. */ + @SuppressForbidden(reason = "find URL path") static Map getCodebaseJarMap(Set urls) { Map codebases = new LinkedHashMap<>(); // maintain order for (URL url : urls) { diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java b/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java index 7937454260075..f9213b309f8a2 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java @@ -51,7 +51,7 @@ public class PluginInfo implements Writeable, ToXContentObject { private final String description; private final String version; private final String classname; - private final List extendsPlugins; + private final List extendedPlugins; private final boolean hasNativeController; private final boolean requiresKeystore; @@ -62,17 +62,17 @@ public class PluginInfo implements Writeable, ToXContentObject { * @param description a description of the plugin * @param version the version of Elasticsearch the plugin is built for * @param classname the entry point to the plugin - * @param extendsPlugins other plugins this plugin extends through SPI + * @param extendedPlugins other plugins this plugin extends through SPI * @param hasNativeController whether or not the plugin has a native controller * @param requiresKeystore whether or not the plugin requires the elasticsearch keystore to be created */ public PluginInfo(String name, String description, String version, String classname, - List extendsPlugins, boolean hasNativeController, boolean requiresKeystore) { + List extendedPlugins, boolean hasNativeController, boolean requiresKeystore) { this.name = name; this.description = description; this.version = version; this.classname = classname; - this.extendsPlugins = Collections.unmodifiableList(extendsPlugins); + this.extendedPlugins = Collections.unmodifiableList(extendedPlugins); this.hasNativeController = hasNativeController; this.requiresKeystore = requiresKeystore; } @@ -88,10 +88,10 @@ public PluginInfo(final StreamInput in) throws IOException { this.description = in.readString(); this.version = in.readString(); this.classname = in.readString(); - if (in.getVersion().onOrAfter(Version.V_6_1_0)) { - extendsPlugins = Collections.unmodifiableList(Arrays.asList(in.readStringArray())); + if (in.getVersion().onOrAfter(Version.V_6_2_0)) { + extendedPlugins = Collections.unmodifiableList(Arrays.asList(in.readStringArray())); } else { - extendsPlugins = Collections.emptyList(); + extendedPlugins = Collections.emptyList(); } if (in.getVersion().onOrAfter(Version.V_5_4_0)) { hasNativeController = in.readBoolean(); @@ -111,8 +111,8 @@ public void writeTo(final StreamOutput out) throws IOException { out.writeString(description); out.writeString(version); out.writeString(classname); - if (out.getVersion().onOrAfter(Version.V_6_1_0)) { - out.writeStringList(extendsPlugins); + if (out.getVersion().onOrAfter(Version.V_6_2_0)) { + out.writeStringList(extendedPlugins); } if (out.getVersion().onOrAfter(Version.V_5_4_0)) { out.writeBoolean(hasNativeController); @@ -181,8 +181,8 @@ public static PluginInfo readFromProperties(final Path path) throws IOException "property [classname] is missing for plugin [" + name + "]"); } - final String extendsString = props.getProperty("extends.plugins", ""); - final List extendsPlugins = Arrays.asList(Strings.delimitedListToStringArray(extendsString, ",")); + final String extendedString = props.getProperty("extended.plugins", ""); + final List extendedPlugins = Arrays.asList(Strings.delimitedListToStringArray(extendedString, ",")); final String hasNativeControllerValue = props.getProperty("has.native.controller"); final boolean hasNativeController; @@ -217,7 +217,7 @@ public static PluginInfo readFromProperties(final Path path) throws IOException " but was [" + requiresKeystoreValue + "]", e); } - return new PluginInfo(name, description, version, classname, extendsPlugins, hasNativeController, requiresKeystore); + return new PluginInfo(name, description, version, classname, extendedPlugins, hasNativeController, requiresKeystore); } /** @@ -252,8 +252,8 @@ public String getClassname() { * * @return the names of the plugins extended */ - public List getExtendsPlugins() { - return extendsPlugins; + public List getExtendedPlugins() { + return extendedPlugins; } /** @@ -291,7 +291,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field("version", version); builder.field("description", description); builder.field("classname", classname); - builder.array("extended_plugins", extendsPlugins); + builder.array("extended_plugins", extendedPlugins); builder.field("has_native_controller", hasNativeController); builder.field("requires_keystore", requiresKeystore); } @@ -328,7 +328,7 @@ public String toString() { .append("Native Controller: ").append(hasNativeController).append("\n") .append("Requires Keystore: ").append(requiresKeystore).append("\n") .append(" * Classname: ").append(classname).append("\n") - .append(" * Extended Plugins: ").append(extendsPlugins); + .append(" * Extended Plugins: ").append(extendedPlugins); return information.toString(); } diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index 4564f9c878671..34045e3e40370 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -388,7 +388,7 @@ private static void addSortedBundle(Bundle bundle, Map bundles, } dependencyStack.add(name); - for (String dependency : bundle.plugin.getExtendsPlugins()) { + for (String dependency : bundle.plugin.getExtendedPlugins()) { Bundle depBundle = bundles.get(dependency); if (depBundle == null) { throw new IllegalArgumentException("Missing plugin [" + dependency + "], dependency of [" + name + "]"); @@ -421,7 +421,7 @@ private List> loadBundles(Set bundles) { // the plugin cli does it, but we do it again, in case lusers mess with jar files manually static void checkBundleJarHell(Bundle bundle, Map> transitiveUrls) { // invariant: any plugins this plugin bundle extends have already been added to transitiveUrls - List exts = bundle.plugin.getExtendsPlugins(); + List exts = bundle.plugin.getExtendedPlugins(); try { Set urls = new HashSet<>(); @@ -472,7 +472,7 @@ private Plugin loadBundle(Bundle bundle, Map loaded) { // collect loaders of extended plugins List extendedLoaders = new ArrayList<>(); - for (String extendedPluginName : bundle.plugin.getExtendsPlugins()) { + for (String extendedPluginName : bundle.plugin.getExtendedPlugins()) { Plugin extendedPlugin = loaded.get(extendedPluginName); assert extendedPlugin != null; if (ExtensiblePlugin.class.isInstance(extendedPlugin) == false) { @@ -487,7 +487,7 @@ private Plugin loadBundle(Bundle bundle, Map loaded) { // reload SPI with any new services from the plugin reloadLuceneSPI(loader); - for (String extendedPluginName : bundle.plugin.getExtendsPlugins()) { + 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); } diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java index 2a96538e83970..1728a1f247aa6 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java @@ -25,11 +25,15 @@ import java.nio.file.Path; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.stream.Collectors; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; public class PluginInfoTests extends ESTestCase { @@ -47,7 +51,7 @@ public void testReadFromProperties() throws Exception { assertEquals("fake desc", info.getDescription()); assertEquals("1.0", info.getVersion()); assertEquals("FakePlugin", info.getClassname()); - assertTrue(info.getExtendsPlugins().isEmpty()); + assertThat(info.getExtendedPlugins(), empty()); } public void testReadFromPropertiesNameMissing() throws Exception { @@ -219,7 +223,7 @@ public void testExtendsPluginsSingleExtension() throws Exception { "classname", "FakePlugin", "extends.plugins", "foo"); PluginInfo info = PluginInfo.readFromProperties(pluginDir); - assertThat(info.getExtendsPlugins(), contains("foo")); + assertThat(info.getExtendedPlugins(), contains("foo")); } public void testExtendsPluginsMultipleExtensions() throws Exception { @@ -233,7 +237,7 @@ public void testExtendsPluginsMultipleExtensions() throws Exception { "classname", "FakePlugin", "extends.plugins", "foo,bar,baz"); PluginInfo info = PluginInfo.readFromProperties(pluginDir); - assertThat(info.getExtendsPlugins(), contains("foo", "bar", "baz")); + assertThat(info.getExtendedPlugins(), contains("foo", "bar", "baz")); } public void testExtendsPluginsEmpty() throws Exception { @@ -247,7 +251,7 @@ public void testExtendsPluginsEmpty() throws Exception { "classname", "FakePlugin", "extends.plugins", ""); PluginInfo info = PluginInfo.readFromProperties(pluginDir); - assertTrue(info.getExtendsPlugins().isEmpty()); + assertThat(info.getExtendedPlugins(), empty()); } public void testPluginListSorted() { diff --git a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java index 7c3e1f16d1d58..4cd83e329b158 100644 --- a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java +++ b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java @@ -88,7 +88,7 @@ void execute(Terminal terminal, Environment env, String pluginName, boolean purg List usedBy = new ArrayList<>(); Set bundles = PluginsService.getPluginBundles(env.pluginsFile()); for (PluginsService.Bundle bundle : bundles) { - for (String extendedPlugin : bundle.plugin.getExtendsPlugins()) { + for (String extendedPlugin : bundle.plugin.getExtendedPlugins()) { if (extendedPlugin.equals(pluginName)) { usedBy.add(bundle.plugin.getName()); } diff --git a/plugins/examples/painless-whitelist/build.gradle b/plugins/examples/painless-whitelist/build.gradle index 6e03a3c93c2bd..2213aea16f6cd 100644 --- a/plugins/examples/painless-whitelist/build.gradle +++ b/plugins/examples/painless-whitelist/build.gradle @@ -23,7 +23,7 @@ esplugin { name 'painless-whitelist' description 'An example whitelisting additional classes and methods in painless' classname 'org.elasticsearch.example.painlesswhitelist.MyWhitelistPlugin' - extendsPlugins = ['lang-painless'] + extendedPlugins = ['lang-painless'] } integTestCluster { diff --git a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java index 13aba14669051..c3d59b7590bea 100644 --- a/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -174,7 +174,8 @@ private static void addClassCodebase(Map codebases, String name, St throw new IllegalStateException("Already added " + name + " codebase for testing"); } } catch (ClassNotFoundException e) { - // no class, fall through to not add + // no class, fall through to not add. this can happen for any tests that do not include + // the given class. eg only core tests include plugin-classloader } } From 80dae085820bc63be09911cebbb1e1b4cfae30df Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Thu, 21 Dec 2017 11:35:59 -0800 Subject: [PATCH 08/16] fix precommit --- libs/plugin-classloader/build.gradle | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/libs/plugin-classloader/build.gradle b/libs/plugin-classloader/build.gradle index e421131de8520..1bc7fc9027237 100644 --- a/libs/plugin-classloader/build.gradle +++ b/libs/plugin-classloader/build.gradle @@ -1,3 +1,26 @@ - +/* + * 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. + */ apply plugin: 'elasticsearch.build' + +test.enabled = false + +// test depend on ES core... +forbiddenApisMain.enabled = false +jarHell.enabled = false From 01596bf1592a72c3a2402c8085124ae4cae75217 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Thu, 21 Dec 2017 13:07:51 -0800 Subject: [PATCH 09/16] fix evil tests --- .../java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java index f9931e9a182db..b53f817b69684 100644 --- a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java +++ b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java @@ -52,7 +52,7 @@ public void testNullCodeSource() throws Exception { Permission all = new AllPermission(); PermissionCollection allCollection = all.newPermissionCollection(); allCollection.add(all); - ESPolicy policy = new ESPolicy(allCollection, Collections.emptyMap(), true); + ESPolicy policy = new ESPolicy(Collections.emptyMap(), allCollection, Collections.emptyMap(), true); // restrict ourselves to NoPermission PermissionCollection noPermissions = new Permissions(); assertFalse(policy.implies(new ProtectionDomain(null, noPermissions), new FilePermission("foo", "read"))); @@ -67,7 +67,7 @@ public void testNullCodeSource() throws Exception { public void testNullLocation() throws Exception { assumeTrue("test cannot run with security manager", System.getSecurityManager() == null); PermissionCollection noPermissions = new Permissions(); - ESPolicy policy = new ESPolicy(noPermissions, Collections.emptyMap(), true); + ESPolicy policy = new ESPolicy(Collections.emptyMap(), noPermissions, Collections.emptyMap(), true); assertFalse(policy.implies(new ProtectionDomain(new CodeSource(null, (Certificate[]) null), noPermissions), new FilePermission("foo", "read"))); } @@ -75,7 +75,7 @@ public void testNullLocation() throws Exception { public void testListen() { assumeTrue("test cannot run with security manager", System.getSecurityManager() == null); final PermissionCollection noPermissions = new Permissions(); - final ESPolicy policy = new ESPolicy(noPermissions, Collections.emptyMap(), true); + final ESPolicy policy = new ESPolicy(Collections.emptyMap(), noPermissions, Collections.emptyMap(), true); assertFalse( policy.implies( new ProtectionDomain(ESPolicyUnitTests.class.getProtectionDomain().getCodeSource(), noPermissions), From da1f82e4c1de64c7acff43497ee0a13a06df001b Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Thu, 21 Dec 2017 13:53:02 -0800 Subject: [PATCH 10/16] fix more tests --- .../org/elasticsearch/plugins/PluginInfoTests.java | 12 ++++++------ .../elasticsearch/plugins/PluginsServiceTests.java | 10 +--------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java index 1728a1f247aa6..f917ef8bdc238 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java @@ -212,7 +212,7 @@ public void testReadFromPropertiesJvmMissingClassname() throws Exception { } } - public void testExtendsPluginsSingleExtension() throws Exception { + public void testExtendedPluginsSingleExtension() throws Exception { Path pluginDir = createTempDir().resolve("fake-plugin"); PluginTestUtil.writeProperties(pluginDir, "description", "fake desc", @@ -221,12 +221,12 @@ public void testExtendsPluginsSingleExtension() throws Exception { "elasticsearch.version", Version.CURRENT.toString(), "java.version", System.getProperty("java.specification.version"), "classname", "FakePlugin", - "extends.plugins", "foo"); + "extended.plugins", "foo"); PluginInfo info = PluginInfo.readFromProperties(pluginDir); assertThat(info.getExtendedPlugins(), contains("foo")); } - public void testExtendsPluginsMultipleExtensions() throws Exception { + public void testExtendedPluginsMultipleExtensions() throws Exception { Path pluginDir = createTempDir().resolve("fake-plugin"); PluginTestUtil.writeProperties(pluginDir, "description", "fake desc", @@ -235,12 +235,12 @@ public void testExtendsPluginsMultipleExtensions() throws Exception { "elasticsearch.version", Version.CURRENT.toString(), "java.version", System.getProperty("java.specification.version"), "classname", "FakePlugin", - "extends.plugins", "foo,bar,baz"); + "extended.plugins", "foo,bar,baz"); PluginInfo info = PluginInfo.readFromProperties(pluginDir); assertThat(info.getExtendedPlugins(), contains("foo", "bar", "baz")); } - public void testExtendsPluginsEmpty() throws Exception { + public void testExtendedPluginsEmpty() throws Exception { Path pluginDir = createTempDir().resolve("fake-plugin"); PluginTestUtil.writeProperties(pluginDir, "description", "fake desc", @@ -249,7 +249,7 @@ public void testExtendsPluginsEmpty() throws Exception { "elasticsearch.version", Version.CURRENT.toString(), "java.version", System.getProperty("java.specification.version"), "classname", "FakePlugin", - "extends.plugins", ""); + "extended.plugins", ""); PluginInfo info = PluginInfo.readFromProperties(pluginDir); assertThat(info.getExtendedPlugins(), empty()); } diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java index bab969cc3ec81..6d01ff14a399c 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java @@ -19,12 +19,6 @@ package org.elasticsearch.plugins; -import javax.annotation.processing.Processor; -import javax.tools.JavaCompiler; -import javax.tools.JavaFileObject; -import javax.tools.SimpleJavaFileObject; -import javax.tools.ToolProvider; - import org.apache.log4j.Level; import org.apache.lucene.util.Constants; import org.apache.lucene.util.LuceneTestCase; @@ -37,11 +31,9 @@ import org.elasticsearch.test.ESTestCase; import org.hamcrest.Matchers; -import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; import java.net.URL; -import java.nio.charset.StandardCharsets; import java.nio.file.FileSystemException; import java.nio.file.Files; import java.nio.file.NoSuchFileException; @@ -550,7 +542,7 @@ public void testNonExtensibleDep() throws Exception { "version", "1.0.0", "elasticsearch.version", Version.CURRENT.toString(), "java.version", System.getProperty("java.specification.version"), - "extends.plugins", "nonextensible", + "extended.plugins", "nonextensible", "classname", "test.DummyPlugin"); try (InputStream jar = PluginsServiceTests.class.getResourceAsStream("dummy-plugin.jar")) { Files.copy(jar, mypluginDir.resolve("plugin.jar")); From c71bc7c36c8bc45d332d30faa6c011370557eee7 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 22 Dec 2017 10:50:33 -0800 Subject: [PATCH 11/16] disable bwc tests until backport is done --- build.gradle | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 949ada13fd9dc..de1a9ec09f0e4 100644 --- a/build.gradle +++ b/build.gradle @@ -139,7 +139,8 @@ task verifyVersions { * after the backport of the backcompat code is complete. */ allprojects { - ext.bwc_tests_enabled = true + // TODO: re-enable after https://github.com/elastic/elasticsearch/pull/27881 is backported + ext.bwc_tests_enabled = false } task verifyBwcTestsEnabled { From 793344b8ea48aa237103c8a8fa086c55b8c6a1f8 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 2 Jan 2018 10:30:40 -0800 Subject: [PATCH 12/16] fix serialization and bwc testing --- .../java/org/elasticsearch/plugins/PluginInfo.java | 8 ++++---- .../org/elasticsearch/plugins/PluginInfoTests.java | 14 ++++++++++++++ qa/mixed-cluster/build.gradle | 6 ++++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java b/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java index 31a566e80ae5f..ef389493946ef 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java @@ -92,7 +92,7 @@ public PluginInfo(final StreamInput in) throws IOException { this.version = in.readString(); this.classname = in.readString(); if (in.getVersion().onOrAfter(Version.V_6_2_0)) { - extendedPlugins = Collections.unmodifiableList(Arrays.asList(in.readStringArray())); + extendedPlugins = in.readList(StreamInput::readString); } else { extendedPlugins = Collections.emptyList(); } @@ -312,7 +312,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field("version", version); builder.field("description", description); builder.field("classname", classname); - builder.array("extended_plugins", extendedPlugins); + builder.field("extended_plugins", extendedPlugins); builder.field("has_native_controller", hasNativeController); builder.field("requires_keystore", requiresKeystore); } @@ -348,8 +348,8 @@ public String toString() { .append("Version: ").append(version).append("\n") .append("Native Controller: ").append(hasNativeController).append("\n") .append("Requires Keystore: ").append(requiresKeystore).append("\n") - .append(" * Classname: ").append(classname).append("\n") - .append(" * Extended Plugins: ").append(extendedPlugins); + .append("Classname: ").append(classname).append("\n") + .append("Extended Plugins: ").append(extendedPlugins); return information.toString(); } diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java index d1393e9f4a30a..b79a005932829 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java @@ -21,8 +21,11 @@ import org.elasticsearch.Version; import org.elasticsearch.action.admin.cluster.node.info.PluginsAndModules; +import org.elasticsearch.common.io.stream.ByteBufferStreamInput; +import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.test.ESTestCase; +import java.nio.ByteBuffer; import java.nio.file.Path; import java.util.ArrayList; import java.util.Collections; @@ -205,6 +208,17 @@ public void testExtendedPluginsEmpty() throws Exception { assertThat(info.getExtendedPlugins(), empty()); } + public void testSerialize() throws Exception { + PluginInfo info = new PluginInfo("c", "foo", "dummy", "dummyclass", Collections.singletonList("foo"), randomBoolean(), randomBoolean()); + BytesStreamOutput output = new BytesStreamOutput(); + info.writeTo(output); + ByteBuffer buffer = ByteBuffer.wrap(output.bytes().toBytesRef().bytes); + ByteBufferStreamInput input = new ByteBufferStreamInput(buffer); + PluginInfo info2 = new PluginInfo(input); + assertThat(info2.toString(), equalTo(info.toString())); + + } + public void testPluginListSorted() { List plugins = new ArrayList<>(); plugins.add(new PluginInfo("c", "foo", "dummy", "dummyclass", Collections.emptyList(), randomBoolean(), randomBoolean())); diff --git a/qa/mixed-cluster/build.gradle b/qa/mixed-cluster/build.gradle index 71b9a4993f6d6..712fde94b7d31 100644 --- a/qa/mixed-cluster/build.gradle +++ b/qa/mixed-cluster/build.gradle @@ -65,8 +65,10 @@ test.enabled = false // no unit tests for rolling upgrades, only the rest integr // basic integ tests includes testing bwc against the most recent version task integTest { - for (final def version : versionCollection.basicIntegrationTestVersions) { - dependsOn "v${version}#bwcTest" + if (project.bwc_tests_enabled) { + for (final def version : versionCollection.basicIntegrationTestVersions) { + dependsOn "v${version}#bwcTest" + } } } From 56fb85d6d026392129a60e869ef57d97918df1ad Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 2 Jan 2018 12:46:16 -0800 Subject: [PATCH 13/16] fix checkstyle --- .../test/java/org/elasticsearch/plugins/PluginInfoTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java index b79a005932829..6d2b09f87eb70 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java @@ -209,7 +209,8 @@ public void testExtendedPluginsEmpty() throws Exception { } public void testSerialize() throws Exception { - PluginInfo info = new PluginInfo("c", "foo", "dummy", "dummyclass", Collections.singletonList("foo"), randomBoolean(), randomBoolean()); + PluginInfo info = new PluginInfo("c", "foo", "dummy", "dummyclass", + Collections.singletonList("foo"), randomBoolean(), randomBoolean()); BytesStreamOutput output = new BytesStreamOutput(); info.writeTo(output); ByteBuffer buffer = ByteBuffer.wrap(output.bytes().toBytesRef().bytes); From 768f3245cc14d1a5b37655199ab659922fe57ac5 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 2 Jan 2018 17:16:49 -0800 Subject: [PATCH 14/16] fix more tests --- .../org/elasticsearch/plugins/PluginInfo.java | 4 +-- .../elasticsearch/plugins/PluginsService.java | 6 ++-- .../plugins/ListPluginsCommandTests.java | 5 +++ .../plugins/RemovePluginCommandTests.java | 35 +++++++++++++------ 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java b/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java index ef389493946ef..01cc7ea65908b 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginInfo.java @@ -348,8 +348,8 @@ public String toString() { .append("Version: ").append(version).append("\n") .append("Native Controller: ").append(hasNativeController).append("\n") .append("Requires Keystore: ").append(requiresKeystore).append("\n") - .append("Classname: ").append(classname).append("\n") - .append("Extended Plugins: ").append(extendedPlugins); + .append("Extended Plugins: ").append(extendedPlugins).append("\n") + .append(" * Classname: ").append(classname); return information.toString(); } diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index 34045e3e40370..c781b8f1c495f 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -130,6 +130,7 @@ public PluginsService(Settings settings, Path configPath, Path modulesDirectory, // now, find all the ones that are in plugins/ if (pluginsDirectory != null) { try { + checkForFailedPluginRemovals(pluginsDirectory); Set plugins = getPluginBundles(pluginsDirectory); for (Bundle bundle : plugins) { pluginsList.add(bundle.plugin); @@ -324,13 +325,14 @@ static Set getPluginBundles(Path pluginsDirectory) throws IOException { Set bundles = new LinkedHashSet<>(); - checkForFailedPluginRemovals(pluginsDirectory); - try (DirectoryStream stream = Files.newDirectoryStream(pluginsDirectory)) { for (Path plugin : stream) { if (FileSystemUtils.isDesktopServicesStore(plugin)) { continue; } + if (plugin.getFileName().toString().startsWith(".removing-")) { + continue; + } logger.trace("--- adding plugin [{}]", plugin.toAbsolutePath()); final PluginInfo info; try { diff --git a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java index 0e82d5dd5c80f..4a243daf2ba70 100644 --- a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java +++ b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java @@ -154,6 +154,7 @@ public void testPluginWithVerbose() throws Exception { "Version: 1.0", "Native Controller: false", "Requires Keystore: false", + "Extended Plugins: []", " * Classname: org.fake"), terminal.getOutput()); } @@ -172,6 +173,7 @@ public void testPluginWithNativeController() throws Exception { "Version: 1.0", "Native Controller: true", "Requires Keystore: false", + "Extended Plugins: []", " * Classname: org.fake"), terminal.getOutput()); } @@ -190,6 +192,7 @@ public void testPluginWithRequiresKeystore() throws Exception { "Version: 1.0", "Native Controller: false", "Requires Keystore: true", + "Extended Plugins: []", " * Classname: org.fake"), terminal.getOutput()); } @@ -209,6 +212,7 @@ public void testPluginWithVerboseMultiplePlugins() throws Exception { "Version: 1.0", "Native Controller: false", "Requires Keystore: false", + "Extended Plugins: []", " * Classname: org.fake", "fake_plugin2", "- Plugin information:", @@ -217,6 +221,7 @@ public void testPluginWithVerboseMultiplePlugins() throws Exception { "Version: 1.0", "Native Controller: false", "Requires Keystore: false", + "Extended Plugins: []", " * Classname: org.fake2"), terminal.getOutput()); } diff --git a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java index 356aeff7260e6..c128a245cd2ec 100644 --- a/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java +++ b/distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/RemovePluginCommandTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.plugins; import org.apache.lucene.util.LuceneTestCase; +import org.elasticsearch.Version; import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.MockTerminal; import org.elasticsearch.cli.Terminal; @@ -77,6 +78,17 @@ public void setUp() throws Exception { env = TestEnvironment.newEnvironment(settings); } + void createPlugin(String name) throws Exception { + PluginTestUtil.writeProperties( + env.pluginsFile().resolve(name), + "description", "dummy", + "name", name, + "version", "1.0", + "elasticsearch.version", Version.CURRENT.toString(), + "java.version", System.getProperty("java.specification.version"), + "classname", "SomeClass"); + } + static MockTerminal removePlugin(String name, Path home, boolean purge) throws Exception { Environment env = TestEnvironment.newEnvironment(Settings.builder().put("path.home", home).build()); MockTerminal terminal = new MockTerminal(); @@ -101,10 +113,10 @@ public void testMissing() throws Exception { } public void testBasic() throws Exception { - Files.createDirectory(env.pluginsFile().resolve("fake")); + createPlugin("fake"); Files.createFile(env.pluginsFile().resolve("fake").resolve("plugin.jar")); Files.createDirectory(env.pluginsFile().resolve("fake").resolve("subdir")); - Files.createDirectory(env.pluginsFile().resolve("other")); + createPlugin("other"); removePlugin("fake", home, randomBoolean()); assertFalse(Files.exists(env.pluginsFile().resolve("fake"))); assertTrue(Files.exists(env.pluginsFile().resolve("other"))); @@ -112,7 +124,7 @@ public void testBasic() throws Exception { } public void testBin() throws Exception { - Files.createDirectories(env.pluginsFile().resolve("fake")); + createPlugin("fake"); Path binDir = env.binFile().resolve("fake"); Files.createDirectories(binDir); Files.createFile(binDir.resolve("somescript")); @@ -124,16 +136,17 @@ public void testBin() throws Exception { } public void testBinNotDir() throws Exception { - Files.createDirectories(env.pluginsFile().resolve("elasticsearch")); - UserException e = expectThrows(UserException.class, () -> removePlugin("elasticsearch", home, randomBoolean())); + createPlugin("fake"); + Files.createFile(env.binFile().resolve("fake")); + UserException e = expectThrows(UserException.class, () -> removePlugin("fake", home, randomBoolean())); assertTrue(e.getMessage(), e.getMessage().contains("not a directory")); - assertTrue(Files.exists(env.pluginsFile().resolve("elasticsearch"))); // did not remove - assertTrue(Files.exists(env.binFile().resolve("elasticsearch"))); + assertTrue(Files.exists(env.pluginsFile().resolve("fake"))); // did not remove + assertTrue(Files.exists(env.binFile().resolve("fake"))); assertRemoveCleaned(env); } public void testConfigDirPreserved() throws Exception { - Files.createDirectories(env.pluginsFile().resolve("fake")); + createPlugin("fake"); final Path configDir = env.configFile().resolve("fake"); Files.createDirectories(configDir); Files.createFile(configDir.resolve("fake.yml")); @@ -144,7 +157,7 @@ public void testConfigDirPreserved() throws Exception { } public void testPurgePluginExists() throws Exception { - Files.createDirectories(env.pluginsFile().resolve("fake")); + createPlugin("fake"); final Path configDir = env.configFile().resolve("fake"); if (randomBoolean()) { Files.createDirectories(configDir); @@ -181,7 +194,7 @@ public void testPurgeOnlyMarkerFileExists() throws Exception { } public void testNoConfigDirPreserved() throws Exception { - Files.createDirectories(env.pluginsFile().resolve("fake")); + createPlugin("fake"); final Path configDir = env.configFile().resolve("fake"); final MockTerminal terminal = removePlugin("fake", home, randomBoolean()); assertThat(terminal.getOutput(), not(containsString(expectedConfigDirPreservedMessage(configDir)))); @@ -214,7 +227,7 @@ public void testMissingPluginName() throws Exception { } public void testRemoveWhenRemovingMarker() throws Exception { - Files.createDirectory(env.pluginsFile().resolve("fake")); + createPlugin("fake"); Files.createFile(env.pluginsFile().resolve("fake").resolve("plugin.jar")); Files.createFile(env.pluginsFile().resolve(".removing-fake")); removePlugin("fake", home, randomBoolean()); From cba4379fdfa66313a9407b0ad3d0d9db112d1149 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 2 Jan 2018 20:55:31 -0800 Subject: [PATCH 15/16] rearrange test leniency --- .../elasticsearch/plugins/PluginsService.java | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java index c781b8f1c495f..445edec1c19c4 100644 --- a/core/src/main/java/org/elasticsearch/plugins/PluginsService.java +++ b/core/src/main/java/org/elasticsearch/plugins/PluginsService.java @@ -130,12 +130,15 @@ public PluginsService(Settings settings, Path configPath, Path modulesDirectory, // now, find all the ones that are in plugins/ if (pluginsDirectory != null) { try { - checkForFailedPluginRemovals(pluginsDirectory); - Set plugins = getPluginBundles(pluginsDirectory); - for (Bundle bundle : plugins) { - pluginsList.add(bundle.plugin); + // TODO: remove this leniency, but tests bogusly rely on it + if (isAccessibleDirectory(pluginsDirectory, logger)) { + checkForFailedPluginRemovals(pluginsDirectory); + Set plugins = getPluginBundles(pluginsDirectory); + for (Bundle bundle : plugins) { + pluginsList.add(bundle.plugin); + } + seenBundles.addAll(plugins); } - seenBundles.addAll(plugins); } catch (IOException ex) { throw new IllegalStateException("Unable to initialize plugins", ex); } @@ -317,12 +320,6 @@ static void checkForFailedPluginRemovals(final Path pluginsDirectory) throws IOE static Set getPluginBundles(Path pluginsDirectory) throws IOException { Logger logger = Loggers.getLogger(PluginsService.class); - - // TODO: remove this leniency, but tests bogusly rely on it - if (!isAccessibleDirectory(pluginsDirectory, logger)) { - return Collections.emptySet(); - } - Set bundles = new LinkedHashSet<>(); try (DirectoryStream stream = Files.newDirectoryStream(pluginsDirectory)) { From 4d83c6bd9dcd736ce0cab0830512f3d18dbc55a7 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 2 Jan 2018 22:22:30 -0800 Subject: [PATCH 16/16] use module getter --- .../java/org/elasticsearch/plugins/InstallPluginCommand.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java index accc8bd9da376..5fedb050ff2e1 100644 --- a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java +++ b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java @@ -575,7 +575,7 @@ void jarHellCheck(PluginInfo info, Path candidate, Path pluginsDir, Path modules // read existing bundles. this does some checks on the installation too. Set bundles = new HashSet<>(PluginsService.getPluginBundles(pluginsDir)); - bundles.addAll(PluginsService.getPluginBundles(modulesDir)); + bundles.addAll(PluginsService.getModuleBundles(modulesDir)); bundles.add(new PluginsService.Bundle(info, candidate)); List sortedBundles = PluginsService.sortBundles(bundles);