Allow action prefixes to be registered from plugins

CHANGELOG.md

@@ -23,6 +23,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add intra segment support for single-value metric aggregations ([#20503](https://github.com/opensearch-project/OpenSearch/pull/20503))
 - Add ref_path support for package-based hunspell dictionary loading ([#20840](https://github.com/opensearch-project/OpenSearch/pull/20840))
 - Add support for enabling pluggable data formats, starting with phase-1 of decoupling shard from engine, and introducing basic abstractions ([#20675](https://github.com/opensearch-project/OpenSearch/pull/20675))
+- Allow action prefixes to be registered from plugins ([#20913](https://github.com/opensearch-project/OpenSearch/pull/20913))
 
 - Add warmup phase to wait for lag to catch up in pull-based ingestion before serving ([#20526](https://github.com/opensearch-project/OpenSearch/pull/20526))
 ### Changed

server/src/main/java/org/opensearch/action/ActionModule.java

@@ -34,6 +34,7 @@
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.opensearch.OpenSearchException;
 import org.opensearch.action.admin.cluster.allocation.ClusterAllocationExplainAction;
 import org.opensearch.action.admin.cluster.allocation.TransportClusterAllocationExplainAction;
 import org.opensearch.action.admin.cluster.configuration.AddVotingConfigExclusionsAction;
@@ -342,6 +343,7 @@
 import org.opensearch.rest.NamedRoute;
 import org.opensearch.rest.RestController;
 import org.opensearch.rest.RestHandler;
+import org.opensearch.rest.RestHandler.Route;
 import org.opensearch.rest.RestHeaderDefinition;
 import org.opensearch.rest.action.RestFieldCapabilitiesAction;
 import org.opensearch.rest.action.RestMainAction;
@@ -501,6 +503,7 @@
 import org.opensearch.rest.action.search.RestSearchScrollAction;
 import org.opensearch.tasks.Task;
 import org.opensearch.threadpool.ThreadPool;
+import org.opensearch.transport.TransportService;
 import org.opensearch.transport.client.node.NodeClient;
 import org.opensearch.usage.UsageService;
 import org.opensearch.wlm.WorkloadGroupTask;
@@ -620,6 +623,21 @@ public ActionModule(
         return actions;
     }
 
+    /**
+     * Returns the set of action name prefixes (the segment before the first {@code :}) contributed
+     * by plugins that are not already covered by {@link org.opensearch.transport.TransportService#VALID_ACTION_PREFIXES}.
+     * Used to extend the valid action prefix set for plugin-defined {@link org.opensearch.action.DocRequest} types.
+     */
+    public Set<String> getPluginActionPrefixes() {
+        return actionPlugins.stream()
+            .flatMap(p -> p.getActions().stream())
+            .map(h -> h.getAction().name())
+            .filter(name -> name.contains(":"))
+            .map(name -> name.substring(0, name.indexOf(':')))
+            .filter(prefix -> TransportService.VALID_ACTION_PREFIXES.stream().noneMatch(v -> v.startsWith(prefix)))
+            .collect(Collectors.toSet());
+    }
+
     static Map<String, ActionHandler<?, ?>> setupActions(List<ActionPlugin> actionPlugins) {
         // Subclass NamedRegistry for easy registration
         class ActionRegistry extends NamedRegistry<ActionHandler<?, ?>> {
@@ -855,6 +873,20 @@ public void initRestHandlers(Supplier<DiscoveryNodes> nodesInCluster) {
                     catActions.add(abstractCatAction);
                 }
             }
+            for (Route route : handler.routes()) {
+                if (route instanceof NamedRoute namedRoute) {
+                    for (String actionName : namedRoute.actionNames()) {
+                        if (!TransportService.isValidActionName(actionName)) {
+                            throw new OpenSearchException(
+                                "Invalid action name ["
+                                    + actionName
+                                    + "]. It must start with one of: "
+                                    + TransportService.VALID_ACTION_PREFIXES
+                            );
+                        }
+                    }
+                }
+            }
             restController.registerHandler(handler);
         };
         registerHandler.accept(new RestAddVotingConfigExclusionAction());

server/src/main/java/org/opensearch/rest/NamedRoute.java

@@ -10,7 +10,6 @@
 
 import org.opensearch.OpenSearchException;
 import org.opensearch.common.annotation.PublicApi;
-import org.opensearch.transport.TransportService;
 
 import java.util.HashSet;
 import java.util.Set;
@@ -87,7 +86,9 @@ public Builder uniqueName(String name) {
          * @return the builder instance
          */
         public Builder legacyActionNames(Set<String> legacyActionNames) {
-            this.legacyActionNames.addAll(validateLegacyActionNames(legacyActionNames));
+            if (legacyActionNames != null) {
+                this.legacyActionNames.addAll(legacyActionNames);
+            }
             return this;
         }
 
@@ -123,20 +124,6 @@ private void checkIfFieldsAreSet() {
             }
         }
 
-        private Set<String> validateLegacyActionNames(Set<String> legacyActionNames) {
-            if (legacyActionNames == null) {
-                return new HashSet<>();
-            }
-            for (String actionName : legacyActionNames) {
-                if (!TransportService.isValidActionName(actionName)) {
-                    throw new OpenSearchException(
-                        "Invalid action name [" + actionName + "]. It must start with one of: " + TransportService.VALID_ACTION_PREFIXES
-                    );
-                }
-            }
-            return legacyActionNames;
-        }
-
     }
 
     private NamedRoute(Builder builder) {

server/src/main/java/org/opensearch/transport/TransportService.java

@@ -80,6 +80,7 @@
 import java.io.UncheckedIOException;
 import java.net.UnknownHostException;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
@@ -294,6 +295,7 @@ public TransportService(
         this.tracer = tracer;
         this.remoteClusterService = remoteClusterService;
         responseHandlers = streamTransport.getResponseHandlers();
+        this.additionalActionPrefixes = new HashSet<>();
     }
 
     public TransportService(
@@ -325,6 +327,7 @@ public TransportService(
         this.tracer = tracer;
         remoteClusterService = new RemoteClusterService(settings, this);
         responseHandlers = transport.getResponseHandlers();
+        this.additionalActionPrefixes = new HashSet<>();
         if (clusterSettings != null) {
             clusterSettings.addSettingsUpdateConsumer(TransportSettings.TRACE_LOG_INCLUDE_SETTING, this::setTracerLogInclude);
             clusterSettings.addSettingsUpdateConsumer(TransportSettings.TRACE_LOG_EXCLUDE_SETTING, this::setTracerLogExclude);
@@ -1215,11 +1218,21 @@ public TransportAddress[] addressesFromString(String address) throws UnknownHost
         )
     );
 
+    private final Set<String> additionalActionPrefixes;
+
+    /**
+     * Registers additional valid action name prefixes contributed by plugins.
+     */
+    public void registerAdditionalActionPrefixes(Collection<String> prefixes) {
+        additionalActionPrefixes.addAll(prefixes);
+    }
+
     protected void validateActionName(String actionName) {
         // TODO we should makes this a hard validation and throw an exception but we need a good way to add backwards layer
         // for it. Maybe start with a deprecation layer
-        if (isValidActionName(actionName) == false) {
-            logger.warn("invalid action name [" + actionName + "] must start with one of: " + TransportService.VALID_ACTION_PREFIXES);
+        if (isValidActionName(actionName, additionalActionPrefixes) == false) {
+            String extra = additionalActionPrefixes.isEmpty() ? "" : ", " + additionalActionPrefixes;
+            logger.warn("invalid action name [" + actionName + "] must start with one of: " + VALID_ACTION_PREFIXES + extra);
         }
     }
 
@@ -1229,11 +1242,24 @@ protected void validateActionName(String actionName) {
      * @see #VALID_ACTION_PREFIXES
      */
     public static boolean isValidActionName(String actionName) {
+        return isValidActionName(actionName, Set.of());
+    }
+
+    /**
+     * Returns <code>true</code> iff the action name starts with a valid prefix from {@link #VALID_ACTION_PREFIXES}
+     * or the supplied set of additional prefixes.
+     */
+    public static boolean isValidActionName(String actionName, Set<String> additionalPrefixes) {
         for (String prefix : VALID_ACTION_PREFIXES) {
             if (actionName.startsWith(prefix)) {
                 return true;
             }
         }
+        for (String prefix : additionalPrefixes) {
+            if (actionName.startsWith(prefix)) {
+                return true;
+            }
+        }
         return false;
     }
 

server/src/test/java/org/opensearch/action/ActionModuleTests.java

@@ -72,6 +72,8 @@
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.hasEntry;
 import static org.hamcrest.Matchers.startsWith;
 import static org.mockito.Mockito.mock;
@@ -274,4 +276,116 @@ public List<Route> routes() {
             threadPool.shutdown();
         }
     }
+
+    public void testGetPluginActionPrefixesIsEmptyWithNoPlugins() throws IOException {
+        SettingsModule settings = new SettingsModule(Settings.EMPTY);
+        ActionModule actionModule = new ActionModule(
+            settings.getSettings(),
+            new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)),
+            settings.getIndexScopedSettings(),
+            settings.getClusterSettings(),
+            settings.getSettingsFilter(),
+            null,
+            emptyList(),
+            null,
+            null,
+            new UsageService(),
+            null,
+            new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()),
+            new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()))
+        );
+        assertThat(actionModule.getPluginActionPrefixes(), empty());
+    }
+
+    public void testGetPluginActionPrefixesExtractsNovelPrefix() throws IOException {
+        class FakeRequest extends ActionRequest {
+            @Override
+            public ActionRequestValidationException validate() {
+                return null;
+            }
+        }
+        class FakeTransportAction extends TransportAction<FakeRequest, ActionResponse> {
+            protected FakeTransportAction(String actionName, ActionFilters actionFilters, TaskManager taskManager) {
+                super(actionName, actionFilters, taskManager);
+            }
+
+            @Override
+            protected void doExecute(Task task, FakeRequest request, ActionListener<ActionResponse> listener) {}
+        }
+        ActionPlugin pluginWithNovelPrefix = new ActionPlugin() {
+            @Override
+            public List<ActionHandler<? extends ActionRequest, ? extends ActionResponse>> getActions() {
+                return singletonList(new ActionHandler<>(new ActionType<ActionResponse>("report_definition:data/read/get", null) {
+                }, FakeTransportAction.class));
+            }
+        };
+        SettingsModule settings = new SettingsModule(Settings.EMPTY);
+        ThreadPool threadPool = new TestThreadPool(getTestName());
+        try {
+            ActionModule actionModule = new ActionModule(
+                settings.getSettings(),
+                new IndexNameExpressionResolver(threadPool.getThreadContext()),
+                settings.getIndexScopedSettings(),
+                settings.getClusterSettings(),
+                settings.getSettingsFilter(),
+                threadPool,
+                singletonList(pluginWithNovelPrefix),
+                null,
+                null,
+                new UsageService(),
+                null,
+                new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()),
+                new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()))
+            );
+            assertThat(actionModule.getPluginActionPrefixes(), containsInAnyOrder("report_definition"));
+        } finally {
+            threadPool.shutdown();
+        }
+    }
+
+    public void testGetPluginActionPrefixesExcludesKnownPrefixes() throws IOException {
+        class FakeRequest extends ActionRequest {
+            @Override
+            public ActionRequestValidationException validate() {
+                return null;
+            }
+        }
+        class FakeTransportAction extends TransportAction<FakeRequest, ActionResponse> {
+            protected FakeTransportAction(String actionName, ActionFilters actionFilters, TaskManager taskManager) {
+                super(actionName, actionFilters, taskManager);
+            }
+
+            @Override
+            protected void doExecute(Task task, FakeRequest request, ActionListener<ActionResponse> listener) {}
+        }
+        ActionPlugin pluginWithKnownPrefix = new ActionPlugin() {
+            @Override
+            public List<ActionHandler<? extends ActionRequest, ? extends ActionResponse>> getActions() {
+                return singletonList(new ActionHandler<>(new ActionType<ActionResponse>("indices:data/read/custom", null) {
+                }, FakeTransportAction.class));
+            }
+        };
+        SettingsModule settings = new SettingsModule(Settings.EMPTY);
+        ThreadPool threadPool = new TestThreadPool(getTestName());
+        try {
+            ActionModule actionModule = new ActionModule(
+                settings.getSettings(),
+                new IndexNameExpressionResolver(threadPool.getThreadContext()),
+                settings.getIndexScopedSettings(),
+                settings.getClusterSettings(),
+                settings.getSettingsFilter(),
+                threadPool,
+                singletonList(pluginWithKnownPrefix),
+                null,
+                null,
+                new UsageService(),
+                null,
+                new IdentityService(Settings.EMPTY, mock(ThreadPool.class), new ArrayList<>()),
+                new ExtensionsManager(Set.of(), new IdentityService(Settings.EMPTY, mock(ThreadPool.class), List.of()))
+            );
+            assertThat(actionModule.getPluginActionPrefixes(), empty());
+        } finally {
+            threadPool.shutdown();
+        }
+    }
 }

server/src/test/java/org/opensearch/rest/NamedRouteTests.java

@@ -11,7 +11,6 @@
 import org.opensearch.OpenSearchException;
 import org.opensearch.test.OpenSearchTestCase;
 
-import java.util.Set;
 import java.util.function.Function;
 
 import static org.opensearch.rest.NamedRoute.MAX_LENGTH_OF_ACTION_NAME;
@@ -97,19 +96,6 @@ public void testNamedRouteWithNullLegacyActionNames() {
         }
     }
 
-    public void testNamedRouteWithInvalidLegacyActionNames() {
-        try {
-            NamedRoute r = new NamedRoute.Builder().method(GET)
-                .path("foo/bar")
-                .uniqueName("foo:bar")
-                .legacyActionNames(Set.of("foo:bar-legacy"))
-                .build();
-            fail("Did not expect NamedRoute to pass with an invalid legacy action name");
-        } catch (OpenSearchException e) {
-            assertTrue(e.getMessage().contains("Invalid action name [foo:bar-legacy]. It must start with one of:"));
-        }
-    }
-
     public void testNamedRouteWithHandler() {
         Function<RestRequest, RestResponse> fooHandler = restRequest -> null;
         try {