Do not allow unload/restart plugins with ApiModule

The ApiModule from plugins is used for creating the
base injectors for all other plugins and cannot be
unloaded without potentially breaking all the other
plugins that depend on them.

Block the start of the new plugin for avoiding
leaving the overall Guice stack in an inconsistent
state. Plugins with ApiModule need to be effectively
considered mandatory.

Release-Notes: Prevent reload of plugins with ApiModule
Bug: Issue 324462734
Change-Id: Iac2851022ea34e52014c519eeef70ce6028f4fda

4 files changed, 46 insertions, 9 deletions

diff --git a/java/com/google/gerrit/server/plugins/DisablePlugin.java b/java/com/google/gerrit/server/plugins/DisablePlugin.java
index 9e238f897a..d845ec17f8 100644
--- a/java/com/google/gerrit/server/plugins/DisablePlugin.java
+++ b/java/com/google/gerrit/server/plugins/DisablePlugin.java
@@ -53,7 +53,12 @@ public class DisablePlugin implements RestModifyView<PluginResource, Input> {
     if (mandatoryPluginsCollection.contains(name)) {
       throw new MethodNotAllowedException("Plugin " + name + " is mandatory");
     }
-    loader.disablePlugins(ImmutableSet.of(name));
+    try {
+      loader.disablePlugins(ImmutableSet.of(name));
+    } catch (PluginInstallException e) {
+      throw new MethodNotAllowedException("Plugin " + name + " cannot be disabled", e);
+    }
+
     return Response.ok(ListPlugins.toPluginInfo(loader.get(name)));
   }
 }
diff --git a/java/com/google/gerrit/server/plugins/PluginLoader.java b/java/com/google/gerrit/server/plugins/PluginLoader.java
index 3574dc3d00..8260104018 100644
--- a/java/com/google/gerrit/server/plugins/PluginLoader.java
+++ b/java/com/google/gerrit/server/plugins/PluginLoader.java
@@ -220,7 +220,7 @@ public class PluginLoader implements LifecycleListener {
     toCleanup.add(plugin);
   }
 
-  public void disablePlugins(Set<String> names) {
+  public void disablePlugins(Set<String> names) throws PluginInstallException {
     if (!isRemoteAdminEnabled()) {
       logger.atWarning().log(
           "Remote plugin administration is disabled, ignoring disablePlugins(%s)", names);
@@ -234,6 +234,12 @@ public class PluginLoader implements LifecycleListener {
           continue;
         }
 
+        if (active.getApiModule().isPresent()) {
+          throw new PluginInstallException(
+              String.format(
+                  "Plugin %s has registered an ApiModule therefore it cannot be disabled", name));
+        }
+
         if (mandatoryPlugins.contains(name)) {
           logger.atWarning().log("Mandatory plugin %s cannot be disabled", name);
           continue;
@@ -380,11 +386,14 @@ public class PluginLoader implements LifecycleListener {
         try {
           logger.atInfo().log("Reloading plugin %s", name);
           Plugin newPlugin = runPlugin(name, active.getSrcFile(), active);
-          logger.atInfo().log(
-              "Reloaded plugin %s%s, version %s",
-              newPlugin.getName(),
-              newPlugin.getApiModule().isPresent() ? " (w/ ApiModule)" : "",
-              newPlugin.getVersion());
+
+          if (newPlugin != active) {
+            logger.atInfo().log(
+                "Reloaded plugin %s%s, version %s",
+                newPlugin.getName(),
+                newPlugin.getApiModule().isPresent() ? " (w/ ApiModule)" : "",
+                newPlugin.getVersion());
+          }
         } catch (PluginInstallException e) {
           logger.atWarning().withCause(e.getCause()).log("Cannot reload plugin %s", name);
           throw e;
@@ -520,6 +529,13 @@ public class PluginLoader implements LifecycleListener {
         return oldPlugin;
       }
 
+      if (oldPlugin != null && oldPlugin.getApiModule().isPresent()) {
+        throw new PluginInstallException(
+            String.format(
+                "Plugin %s has registered an ApiModule therefore its restart/reload is not allowed",
+                name));
+      }
+
       Plugin newPlugin = loadPlugin(name, plugin, snapshot);
       if (newPlugin.getCleanupHandle() != null) {
         cleanupHandles.put(newPlugin, newPlugin.getCleanupHandle());
@@ -571,7 +587,13 @@ public class PluginLoader implements LifecycleListener {
       }
     }
     for (String name : unload) {
-      unloadPlugin(running.get(name));
+      Plugin runningPlugin = running.get(name);
+
+      if (runningPlugin.getApiModule().isPresent()) {
+        logger.atWarning().log("Cannot remove plugin %s as it has registered an ApiModule", name);
+      } else {
+        unloadPlugin(running.get(name));
+      }
     }
   }
 
diff --git a/java/com/google/gerrit/server/plugins/ServerPlugin.java b/java/com/google/gerrit/server/plugins/ServerPlugin.java
index c275ff977f..036285e330 100644
--- a/java/com/google/gerrit/server/plugins/ServerPlugin.java
+++ b/java/com/google/gerrit/server/plugins/ServerPlugin.java
@@ -172,6 +172,11 @@ public class ServerPlugin extends Plugin {
   @Override
   protected boolean canReload() {
     Attributes main = manifest.getMainAttributes();
+    String apiModule = main.getValue("Gerrit-ApiModule");
+    if (apiModule != null) {
+      return false;
+    }
+
     String v = main.getValue("Gerrit-ReloadMode");
     if (Strings.isNullOrEmpty(v) || "reload".equalsIgnoreCase(v)) {
       return true;
diff --git a/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java b/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
index 0119349b20..d478a9da9b 100644
--- a/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
+++ b/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
@@ -17,6 +17,7 @@ package com.google.gerrit.sshd.commands;
 import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE;
 
 import com.google.common.collect.Sets;
+import com.google.gerrit.server.plugins.PluginInstallException;
 import com.google.gerrit.sshd.CommandMetaData;
 import java.util.List;
 import org.kohsuke.args4j.Argument;
@@ -29,7 +30,11 @@ final class PluginRemoveCommand extends PluginAdminSshCommand {
   @Override
   protected void doRun() throws UnloggedFailure {
     if (names != null && !names.isEmpty()) {
-      loader.disablePlugins(Sets.newHashSet(names));
+      try {
+        loader.disablePlugins(Sets.newHashSet(names));
+      } catch (PluginInstallException e) {
+        throw die(e);
+      }
     }
   }
 }