Plugins: Separate plugin semantic validation from properties format validation (#28581)

This commit moves the semantic validation (like which version a plugin
was built for or which java version it is compatible with) from reading
a plugin descriptor, leaving the checks on the format of the descriptor
intact.

relates #28540

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

@@ -565,6 +565,7 @@ class InstallPluginCommand extends EnvironmentAwareCommand {
     /** Load information about the plugin, and verify it can be installed with no errors. */
     private PluginInfo loadPluginInfo(Terminal terminal, Path pluginRoot, boolean isBatch, Environment env) throws Exception {
         final PluginInfo info = PluginInfo.readFromProperties(pluginRoot);
+        PluginsService.verifyCompatibility(info);
 
         // checking for existing version of the plugin
         verifyPluginName(env.pluginsFile(), info.getName(), pluginRoot);
@@ -649,6 +650,7 @@ class InstallPluginCommand extends EnvironmentAwareCommand {
                     continue;
                 }
                 final PluginInfo info = PluginInfo.readFromProperties(plugin);
+                PluginsService.verifyCompatibility(info);
                 verifyPluginName(env.pluginsFile(), info.getName(), plugin);
                 pluginPaths.add(plugin);
             }

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/ListPluginsCommand.java

@@ -20,6 +20,7 @@
 package org.elasticsearch.plugins;
 
 import joptsimple.OptionSet;
+import org.elasticsearch.Version;
 import org.elasticsearch.cli.EnvironmentAwareCommand;
 import org.elasticsearch.cli.Terminal;
 import org.elasticsearch.common.Nullable;
@@ -84,15 +85,11 @@ class ListPluginsCommand extends EnvironmentAwareCommand {
 
     private void printPlugin(Environment env, Terminal terminal, Path plugin, String prefix) throws IOException {
         terminal.println(Terminal.Verbosity.SILENT, prefix + plugin.getFileName().toString());
-        try {
-            PluginInfo info = PluginInfo.readFromProperties(env.pluginsFile().resolve(plugin.toAbsolutePath()));
-            terminal.println(Terminal.Verbosity.VERBOSE, info.toString(prefix));
-        } catch (IllegalArgumentException e) {
-            if (e.getMessage().contains("incompatible with version")) {
-                terminal.println("WARNING: " + e.getMessage());
-            } else {
-                throw e;
-            }
+        PluginInfo info = PluginInfo.readFromProperties(env.pluginsFile().resolve(plugin.toAbsolutePath()));
+        terminal.println(Terminal.Verbosity.VERBOSE, info.toString(prefix));
+        if (info.getElasticsearchVersion().equals(Version.CURRENT) == false) {
+            terminal.println("WARNING: plugin [" + info.getName() + "] was built for Elasticsearch version " + info.getVersion() +
+                " but version " + Version.CURRENT + " is required");
         }
     }
 }

distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/RemovePluginCommand.java

@@ -86,7 +86,7 @@ class RemovePluginCommand extends EnvironmentAwareCommand {
 
         // first make sure nothing extends this plugin
         List<String> usedBy = new ArrayList<>();
-        Set<PluginsService.Bundle> bundles = PluginsService.getPluginBundles(env.pluginsFile(), false);
+        Set<PluginsService.Bundle> bundles = PluginsService.getPluginBundles(env.pluginsFile());
         for (PluginsService.Bundle bundle : bundles) {
             for (String extendedPlugin : bundle.plugin.getExtendedPlugins()) {
                 if (extendedPlugin.equals(pluginName)) {

distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java

@@ -362,11 +362,7 @@ public class ListPluginsCommandTests extends ESTestCase {
         buildFakePlugin(env, "fake desc 2", "fake_plugin2", "org.fake2");
 
         MockTerminal terminal = listPlugins(home);
-        final String message = String.format(Locale.ROOT,
-                "plugin [%s] is incompatible with version [%s]; was designed for version [%s]",
-                "fake_plugin1",
-                Version.CURRENT.toString(),
-                "1.0.0");
+        String message = "plugin [fake_plugin1] was built for Elasticsearch version 1.0 but version " + Version.CURRENT + " is required";
         assertEquals(
                 "fake_plugin1\n" + "WARNING: " + message + "\n" + "fake_plugin2\n",
                 terminal.getOutput());
@@ -388,11 +384,7 @@ public class ListPluginsCommandTests extends ESTestCase {
         buildFakePlugin(env, "fake desc 2", "fake_plugin2", "org.fake2");
 
         MockTerminal terminal = listPlugins(home);
-        final String message = String.format(Locale.ROOT,
-            "plugin [%s] is incompatible with version [%s]; was designed for version [%s]",
-            "fake_plugin1",
-            Version.CURRENT.toString(),
-            "1.0.0");
+        String message = "plugin [fake_plugin1] was built for Elasticsearch version 1.0 but version " + Version.CURRENT + " is required";
         assertEquals(
             "fake_plugin2\nmeta_plugin\n\tfake_plugin1\n" + "WARNING: " + message + "\n",
             terminal.getOutput());

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

@@ -27,6 +27,7 @@ import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.http.HttpTransportSettings;
 import org.elasticsearch.plugins.PluginInfo;
+import org.elasticsearch.plugins.PluginsService;
 import org.elasticsearch.secure_sm.SecureSM;
 import org.elasticsearch.transport.TcpTransport;
 
@@ -161,7 +162,7 @@ final class Security {
     static Map<String,Policy> getPluginPermissions(Environment environment) throws IOException, NoSuchAlgorithmException {
         Map<String,Policy> map = new HashMap<>();
         // collect up set of plugins and modules by listing directories.
-        Set<Path> pluginsAndModules = new LinkedHashSet<>(PluginInfo.extractAllPlugins(environment.pluginsFile()));
+        Set<Path> pluginsAndModules = new LinkedHashSet<>(PluginsService.findPluginDirs(environment.pluginsFile()));
 
         if (Files.exists(environment.modulesFile())) {
             try (DirectoryStream<Path> stream = Files.newDirectoryStream(environment.modulesFile())) {

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

@@ -24,6 +24,7 @@ import org.apache.lucene.util.IOUtils;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.plugins.Platforms;
 import org.elasticsearch.plugins.PluginInfo;
+import org.elasticsearch.plugins.PluginsService;
 
 import java.io.Closeable;
 import java.io.IOException;
@@ -70,7 +71,7 @@ final class Spawner implements Closeable {
          * For each plugin, attempt to spawn the controller daemon. Silently ignore any plugin that
          * don't include a controller for the correct platform.
          */
-        List<Path> paths = PluginInfo.extractAllPlugins(pluginsFile);
+        List<Path> paths = PluginsService.findPluginDirs(pluginsFile);
         for (Path plugin : paths) {
             final PluginInfo info = PluginInfo.readFromProperties(plugin);
             final Path spawnPath = Platforms.nativeControllerPath(plugin);

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

@@ -150,67 +150,13 @@ public class PluginInfo implements Writeable, ToXContentObject {
     }
 
     /**
-     * Extracts all {@link PluginInfo} from the provided {@code rootPath} expanding meta plugins if needed.
-     * @param rootPath the path where the plugins are installed
-     * @return A list of all plugin paths installed in the {@code rootPath}
-     * @throws IOException if an I/O exception occurred reading the plugin descriptors
-     */
-    public static List<Path> extractAllPlugins(final Path rootPath) throws IOException {
-        final List<Path> plugins = new LinkedList<>();  // order is already lost, but some filesystems have it
-        final Set<String> seen = new HashSet<>();
-        if (Files.exists(rootPath)) {
-            try (DirectoryStream<Path> stream = Files.newDirectoryStream(rootPath)) {
-                for (Path plugin : stream) {
-                    if (FileSystemUtils.isDesktopServicesStore(plugin) ||
-                            plugin.getFileName().toString().startsWith(".removing-")) {
-                        continue;
-                    }
-                    if (seen.add(plugin.getFileName().toString()) == false) {
-                        throw new IllegalStateException("duplicate plugin: " + plugin);
-                    }
-                    if (MetaPluginInfo.isMetaPlugin(plugin)) {
-                        try (DirectoryStream<Path> subStream = Files.newDirectoryStream(plugin)) {
-                            for (Path subPlugin : subStream) {
-                                if (MetaPluginInfo.isPropertiesFile(subPlugin) ||
-                                        FileSystemUtils.isDesktopServicesStore(subPlugin)) {
-                                    continue;
-                                }
-                                if (seen.add(subPlugin.getFileName().toString()) == false) {
-                                    throw new IllegalStateException("duplicate plugin: " + subPlugin);
-                                }
-                                plugins.add(subPlugin);
-                            }
-                        }
-                    } else {
-                        plugins.add(plugin);
-                    }
-                }
-            }
-        }
-        return plugins;
-    }
-
-    /**
-     * Reads and validates the plugin descriptor file.
-     *
-     * @param path the path to the root directory for the plugin
-     * @return the plugin info
-     * @throws IOException if an I/O exception occurred reading the plugin descriptor
-     */
-    public static PluginInfo readFromProperties(final Path path) throws IOException {
-        return readFromProperties(path, true);
-    }
-
-    /**
-     * Reads and validates the plugin descriptor file. If {@code enforceVersion} is false then version enforcement for the plugin descriptor
-     * is skipped.
+     * Reads the plugin descriptor file.
      *
      * @param path           the path to the root directory for the plugin
-     * @param enforceVersion whether or not to enforce the version when reading plugin descriptors
      * @return the plugin info
      * @throws IOException if an I/O exception occurred reading the plugin descriptor
      */
-    static PluginInfo readFromProperties(final Path path, final boolean enforceVersion) throws IOException {
+    public static PluginInfo readFromProperties(final Path path) throws IOException {
         final Path descriptor = path.resolve(ES_PLUGIN_PROPERTIES);
 
         final Map<String, String> propsMap;
@@ -244,22 +190,12 @@ public class PluginInfo implements Writeable, ToXContentObject {
                     "property [elasticsearch.version] is missing for plugin [" + name + "]");
         }
         final Version esVersion = Version.fromString(esVersionString);
-        if (enforceVersion && esVersion.equals(Version.CURRENT) == false) {
-            final String message = String.format(
-                    Locale.ROOT,
-                    "plugin [%s] is incompatible with version [%s]; was designed for version [%s]",
-                    name,
-                    Version.CURRENT.toString(),
-                    esVersionString);
-            throw new IllegalArgumentException(message);
-        }
         final String javaVersionString = propsMap.remove("java.version");
         if (javaVersionString == null) {
             throw new IllegalArgumentException(
                     "property [java.version] is missing for plugin [" + name + "]");
         }
         JarHell.checkVersionFormat(javaVersionString);
-        JarHell.checkJavaVersion(name, javaVersionString);
         final String classname = propsMap.remove("classname");
         if (classname == null) {
             throw new IllegalArgumentException(

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

@@ -35,6 +35,7 @@ import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.component.LifecycleComponent;
 import org.elasticsearch.common.inject.Module;
+import org.elasticsearch.common.io.FileSystemUtils;
 import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Setting.Property;
@@ -56,6 +57,7 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -278,6 +280,59 @@ public class PluginsService extends AbstractComponent {
         }
     }
 
+    /**
+     * Extracts all installed plugin directories from the provided {@code rootPath} expanding meta plugins if needed.
+     * @param rootPath the path where the plugins are installed
+     * @return A list of all plugin paths installed in the {@code rootPath}
+     * @throws IOException if an I/O exception occurred reading the directories
+     */
+    public static List<Path> findPluginDirs(final Path rootPath) throws IOException {
+        final List<Path> plugins = new ArrayList<>();
+        final Set<String> seen = new HashSet<>();
+        if (Files.exists(rootPath)) {
+            try (DirectoryStream<Path> stream = Files.newDirectoryStream(rootPath)) {
+                for (Path plugin : stream) {
+                    if (FileSystemUtils.isDesktopServicesStore(plugin) ||
+                        plugin.getFileName().toString().startsWith(".removing-") ||
+                        plugin.getFileName().toString().startsWith(".installing-")) {
+                        continue;
+                    }
+                    if (seen.add(plugin.getFileName().toString()) == false) {
+                        throw new IllegalStateException("duplicate plugin: " + plugin);
+                    }
+                    if (MetaPluginInfo.isMetaPlugin(plugin)) {
+                        try (DirectoryStream<Path> subStream = Files.newDirectoryStream(plugin)) {
+                            for (Path subPlugin : subStream) {
+                                if (MetaPluginInfo.isPropertiesFile(subPlugin) ||
+                                    FileSystemUtils.isDesktopServicesStore(subPlugin)) {
+                                    continue;
+                                }
+                                if (seen.add(subPlugin.getFileName().toString()) == false) {
+                                    throw new IllegalStateException("duplicate plugin: " + subPlugin);
+                                }
+                                plugins.add(subPlugin);
+                            }
+                        }
+                    } else {
+                        plugins.add(plugin);
+                    }
+                }
+            }
+        }
+        return plugins;
+    }
+
+    /**
+     * Verify the given plugin is compatible with the current Elasticsearch installation.
+     */
+    static void verifyCompatibility(PluginInfo info) {
+        if (info.getElasticsearchVersion().equals(Version.CURRENT) == false) {
+            throw new IllegalArgumentException("Plugin [" + info.getName() + "] was built for Elasticsearch version "
+                + info.getElasticsearchVersion() + " but version " + Version.CURRENT + " is running");
+        }
+        JarHell.checkJavaVersion(info.getName(), info.getJavaVersion());
+    }
+
     // 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<Bundle> getModuleBundles(Path modulesDirectory) throws IOException {
@@ -326,28 +381,15 @@ public class PluginsService extends AbstractComponent {
      * @throws IOException if an I/O exception occurs reading the plugin bundles
      */
     static Set<Bundle> getPluginBundles(final Path pluginsDirectory) throws IOException {
-        return getPluginBundles(pluginsDirectory, true);
-    }
-
-    /**
-     * Get the plugin bundles from the specified directory. If {@code enforceVersion} is true, then the version in each plugin descriptor
-     * must match the current version.
-     *
-     * @param pluginsDirectory the directory
-     * @param enforceVersion   whether or not to enforce the version when reading plugin descriptors
-     * @return the set of plugin bundles in the specified directory
-     * @throws IOException if an I/O exception occurs reading the plugin bundles
-     */
-    static Set<Bundle> getPluginBundles(final Path pluginsDirectory, final boolean enforceVersion) throws IOException {
         Logger logger = Loggers.getLogger(PluginsService.class);
         Set<Bundle> bundles = new LinkedHashSet<>();
 
-        List<Path> infos = PluginInfo.extractAllPlugins(pluginsDirectory);
+        List<Path> infos = findPluginDirs(pluginsDirectory);
         for (Path plugin : infos) {
             logger.trace("--- adding plugin [{}]", plugin.toAbsolutePath());
             final PluginInfo info;
             try {
-                info = PluginInfo.readFromProperties(plugin, enforceVersion);
+                info = PluginInfo.readFromProperties(plugin);
             } catch (IOException e) {
                 throw new IllegalStateException("Could not load plugin descriptor for existing plugin ["
                         + plugin.getFileName() + "]. Was the plugin built before 2.0?", e);
@@ -480,6 +522,8 @@ public class PluginsService extends AbstractComponent {
     private Plugin loadBundle(Bundle bundle, Map<String, Plugin> loaded) {
         String name = bundle.plugin.getName();
 
+        verifyCompatibility(bundle.plugin);
+
         // collect loaders of extended plugins
         List<ClassLoader> extendedLoaders = new ArrayList<>();
         for (String extendedPluginName : bundle.plugin.getExtendedPlugins()) {

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

@@ -113,7 +113,7 @@ public class MetaPluginInfoTests extends ESTestCase {
             "classname", "FakePlugin");
 
         IllegalStateException exc =
-            expectThrows(IllegalStateException.class, () -> PluginInfo.extractAllPlugins(pluginDir));
+            expectThrows(IllegalStateException.class, () -> PluginsService.findPluginDirs(pluginDir));
         assertThat(exc.getMessage(), containsString("duplicate plugin"));
         assertThat(exc.getMessage(), endsWith("plugin1"));
     }

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

@@ -103,20 +103,6 @@ public class PluginInfoTests extends ESTestCase {
         assertThat(e.getMessage(), containsString("[java.version] is missing"));
     }
 
-    public void testReadFromPropertiesJavaVersionIncompatible() throws Exception {
-        String pluginName = "fake-plugin";
-        Path pluginDir = createTempDir().resolve(pluginName);
-        PluginTestUtil.writePluginProperties(pluginDir,
-            "description", "fake desc",
-            "name", pluginName,
-            "elasticsearch.version", Version.CURRENT.toString(),
-            "java.version", "1000000.0",
-            "classname", "FakePlugin",
-            "version", "1.0");
-        IllegalStateException e = expectThrows(IllegalStateException.class, () -> PluginInfo.readFromProperties(pluginDir));
-        assertThat(e.getMessage(), containsString(pluginName + " requires Java"));
-    }
-
     public void testReadFromPropertiesBadJavaVersionFormat() throws Exception {
         String pluginName = "fake-plugin";
         Path pluginDir = createTempDir().resolve(pluginName);
@@ -143,17 +129,6 @@ public class PluginInfoTests extends ESTestCase {
         assertThat(e.getMessage(), containsString("version needs to contain major, minor, and revision"));
     }
 
-    public void testReadFromPropertiesOldElasticsearchVersion() throws Exception {
-        Path pluginDir = createTempDir().resolve("fake-plugin");
-        PluginTestUtil.writePluginProperties(pluginDir,
-            "description", "fake desc",
-            "name", "my_plugin",
-            "version", "1.0",
-            "elasticsearch.version", Version.V_5_0_0.toString());
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> PluginInfo.readFromProperties(pluginDir));
-        assertThat(e.getMessage(), containsString("was designed for version [5.0.0]"));
-    }
-
     public void testReadFromPropertiesJvmMissingClassname() throws Exception {
         Path pluginDir = createTempDir().resolve("fake-plugin");
         PluginTestUtil.writePluginProperties(pluginDir,

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

@@ -590,4 +590,18 @@ public class PluginsServiceTests extends ESTestCase {
         IllegalStateException e = expectThrows(IllegalStateException.class, () -> newPluginsService(settings));
         assertEquals("Plugin [myplugin] cannot extend non-extensible plugin [nonextensible]", e.getMessage());
     }
+
+    public void testIncompatibleElasticsearchVersion() throws Exception {
+        PluginInfo info = new PluginInfo("my_plugin", "desc", "1.0", Version.V_5_0_0,
+            "1.8", "FakePlugin", Collections.emptyList(), false, false);
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> PluginsService.verifyCompatibility(info));
+        assertThat(e.getMessage(), containsString("was built for Elasticsearch version 5.0.0"));
+    }
+
+    public void testIncompatibleJavaVersion() throws Exception {
+        PluginInfo info = new PluginInfo("my_plugin", "desc", "1.0", Version.CURRENT,
+            "1000000.0", "FakePlugin", Collections.emptyList(), false, false);
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> PluginsService.verifyCompatibility(info));
+        assertThat(e.getMessage(), containsString("my_plugin requires Java"));
+    }
 }