Fix the ability to remove old plugin

We now read the plugin descriptor when removing an old plugin. This is
to check if we are removing a plugin that is extended by another
plugin. However, when reading the descriptor we enforce that it is of
the same version that we are. This is not the case when a user has
upgraded Elasticsearch and is now trying to remove an old plugin. This
commit fixes this by skipping the version enforcement when reading the
plugin descriptor only when removing a plugin.

Relates #28540

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

@@ -86,7 +86,7 @@ void execute(Terminal terminal, Environment env, String pluginName, boolean purg
 
         // first make sure nothing extends this plugin
         List<String> usedBy = new ArrayList<>();
-        Set<PluginsService.Bundle> bundles = PluginsService.getPluginBundles(env.pluginsFile());
+        Set<PluginsService.Bundle> bundles = PluginsService.getPluginBundles(env.pluginsFile(), false);
         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/RemovePluginCommandTests.java

@@ -29,6 +29,7 @@
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.env.TestEnvironment;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.VersionUtils;
 import org.junit.Before;
 
 import java.io.BufferedReader;
@@ -41,6 +42,7 @@
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasToString;
 
 @LuceneTestCase.SuppressFileSystems("*")
@@ -78,19 +80,27 @@ public void setUp() throws Exception {
         env = TestEnvironment.newEnvironment(settings);
     }
 
-    void createPlugin(String name) throws Exception {
+    void createPlugin(String name) throws IOException {
         createPlugin(env.pluginsFile(), name);
     }
 
-    void createPlugin(Path path, String name) throws Exception {
+    void createPlugin(String name, Version version) throws IOException {
+        createPlugin(env.pluginsFile(), name, version);
+    }
+
+    void createPlugin(Path path, String name) throws IOException {
+        createPlugin(path, name, Version.CURRENT);
+    }
+
+    void createPlugin(Path path, String name, Version version) throws IOException {
         PluginTestUtil.writePluginProperties(
-            path.resolve(name),
-            "description", "dummy",
-            "name", name,
-            "version", "1.0",
-            "elasticsearch.version", Version.CURRENT.toString(),
-            "java.version", System.getProperty("java.specification.version"),
-            "classname", "SomeClass");
+                path.resolve(name),
+                "description", "dummy",
+                "name", name,
+                "version", "1.0",
+                "elasticsearch.version", version.toString(),
+                "java.version", System.getProperty("java.specification.version"),
+                "classname", "SomeClass");
     }
 
     void createMetaPlugin(String name, String... plugins) throws Exception {
@@ -137,6 +147,18 @@ public void testBasic() throws Exception {
         assertRemoveCleaned(env);
     }
 
+    public void testRemoveOldVersion() throws Exception {
+        createPlugin(
+                "fake",
+                VersionUtils.randomVersionBetween(
+                        random(),
+                        Version.CURRENT.minimumIndexCompatibilityVersion(),
+                        VersionUtils.getPreviousVersion()));
+        removePlugin("fake", home, randomBoolean());
+        assertThat(Files.exists(env.pluginsFile().resolve("fake")), equalTo(false));
+        assertRemoveCleaned(env);
+    }
+
     public void testBasicMeta() throws Exception {
         createMetaPlugin("meta", "fake1");
         createPlugin("other");

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

@@ -181,6 +181,19 @@ public static List<Path> extractAllPlugins(final Path rootPath) throws IOExcepti
      * @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.
+     *
+     * @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 {
         final Path descriptor = path.resolve(ES_PLUGIN_PROPERTIES);
 
         final Map<String, String> propsMap;
@@ -214,7 +227,7 @@ public static PluginInfo readFromProperties(final Path path) throws IOException
                     "property [elasticsearch.version] is missing for plugin [" + name + "]");
         }
         final Version esVersion = Version.fromString(esVersionString);
-        if (esVersion.equals(Version.CURRENT) == false) {
+        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]",
@@ -258,12 +271,12 @@ public static PluginInfo readFromProperties(final Path path) throws IOException
                     break;
                 default:
                     final String message = String.format(
-                        Locale.ROOT,
-                        "property [%s] must be [%s], [%s], or unspecified but was [%s]",
-                        "has_native_controller",
-                        "true",
-                        "false",
-                        hasNativeControllerValue);
+                            Locale.ROOT,
+                            "property [%s] must be [%s], [%s], or unspecified but was [%s]",
+                            "has_native_controller",
+                            "true",
+                            "false",
+                            hasNativeControllerValue);
                     throw new IllegalArgumentException(message);
             }
         }
@@ -277,7 +290,7 @@ public static PluginInfo readFromProperties(final Path path) throws IOException
             requiresKeystore = Booleans.parseBoolean(requiresKeystoreValue);
         } catch (IllegalArgumentException e) {
             throw new IllegalArgumentException("property [requires.keystore] must be [true] or [false]," +
-                                               " but was [" + requiresKeystoreValue + "]", e);
+                    " but was [" + requiresKeystoreValue + "]", e);
         }
 
         if (propsMap.isEmpty() == false) {

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

@@ -317,7 +317,27 @@ static void checkForFailedPluginRemovals(final Path pluginsDirectory) throws IOE
         }
     }
 
-    static Set<Bundle> getPluginBundles(Path pluginsDirectory) throws IOException {
+    /**
+     * Get the plugin bundles from the specified directory.
+     *
+     * @param pluginsDirectory the directory
+     * @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) 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<>();
 
@@ -326,10 +346,10 @@ static Set<Bundle> getPluginBundles(Path pluginsDirectory) throws IOException {
             logger.trace("--- adding plugin [{}]", plugin.toAbsolutePath());
             final PluginInfo info;
             try {
-                info = PluginInfo.readFromProperties(plugin);
+                info = PluginInfo.readFromProperties(plugin, enforceVersion);
             } catch (IOException e) {
                 throw new IllegalStateException("Could not load plugin descriptor for existing plugin ["
-                    + plugin.getFileName() + "]. Was the plugin built before 2.0?", e);
+                        + plugin.getFileName() + "]. Was the plugin built before 2.0?", e);
             }
             if (bundles.add(new Bundle(info, plugin)) == false) {
                 throw new IllegalStateException("duplicate plugin: " + info);