Support transport action names when registering NamedRoutes

CHANGELOG.md

@@ -97,7 +97,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add TokenManager Interface ([#7452](https://github.com/opensearch-project/OpenSearch/pull/7452))
 - Add Remote store as a segment replication source ([#7653](https://github.com/opensearch-project/OpenSearch/pull/7653))
 - Add descending order search optimization through reverse segment read. ([#7967](https://github.com/opensearch-project/OpenSearch/pull/7967))
-
+- Support legacy action names when registering NamedRoutes ([#7957](https://github.com/opensearch-project/OpenSearch/pull/7957))
 
 ### Dependencies
 - Bump `jackson` from 2.15.1 to 2.15.2 ([#7897](https://github.com/opensearch-project/OpenSearch/pull/7897))
@@ -124,4 +124,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Security
 
 [Unreleased 3.0]: https://github.com/opensearch-project/OpenSearch/compare/2.x...HEAD
-[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.8...2.x
+[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.8...2.x

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

@@ -460,6 +460,7 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -1111,17 +1112,26 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct
             requireNonNull(route, "route is required");
             requireNonNull(action, "action is required");
             Optional<String> routeName = Optional.empty();
+            Set<String> actionNames = new HashSet<>();
             if (route instanceof NamedRoute) {
-                routeName = Optional.of(((NamedRoute) route).name());
+                NamedRoute nr = (NamedRoute) route;
+                routeName = Optional.of(nr.name());
                 if (isActionRegistered(routeName.get()) || registeredActionNames.contains(routeName.get())) {
                     throw new IllegalArgumentException("route [" + route + "] already registered");
                 }
+                actionNames = nr.actionNames();
+                actionNames.forEach(act -> {
+                    if (isActionRegistered(act) || registeredActionNames.contains(act)) {
+                        throw new IllegalArgumentException("action [" + act + "] already registered");
+                    }
+                });
             }
             if (routeRegistry.containsKey(route)) {
                 throw new IllegalArgumentException("route [" + route + "] already registered");
             }
             routeRegistry.put(route, action);
             routeName.ifPresent(registeredActionNames::add);
+            registeredActionNames.addAll(actionNames);
         }
 
         /**
@@ -1136,6 +1146,7 @@ public void unregisterDynamicRoute(RestHandler.Route route) {
             }
             if (route instanceof NamedRoute) {
                 registeredActionNames.remove(((NamedRoute) route).name());
+                registeredActionNames.remove(((NamedRoute) route).actionNames());
             }
         }
 

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

@@ -9,17 +9,23 @@
 package org.opensearch.rest;
 
 import org.opensearch.OpenSearchException;
+import org.opensearch.transport.TransportService;
+
+import java.util.Set;
 
 /**
  * A named Route
  *
  * @opensearch.internal
  */
 public class NamedRoute extends RestHandler.Route {
+
     private static final String VALID_ACTION_NAME_PATTERN = "^[a-zA-Z0-9:/*_]*$";
     static final int MAX_LENGTH_OF_ACTION_NAME = 250;
 
-    private final String name;
+    private String name;
+
+    private Set<String> actionNames;
 
     public boolean isValidRouteName(String routeName) {
         if (routeName == null || routeName.isBlank() || routeName.length() > MAX_LENGTH_OF_ACTION_NAME) {
@@ -41,15 +47,42 @@ public NamedRoute(RestRequest.Method method, String path, String name) {
         this.name = name;
     }
 
+    /**
+     * Allows registering a legacyName to match against transport action
+     * @param method - The REST method for this route
+     * @param path - the URL for this route
+     * @param name - the shortname for this route
+     * @param legacyActionNames - list of names of the transport action this route will be matched against
+     */
+    public NamedRoute(RestRequest.Method method, String path, String name, Set<String> legacyActionNames) {
+        this(method, path, name);
+        for (String actionName : actionNames) {
+            if (!TransportService.isValidActionName(actionName)) {
+                throw new OpenSearchException(
+                    "invalid action name [" + actionName + "] must start with one of: " + TransportService.VALID_ACTION_PREFIXES
+                );
+            }
+        }
+        this.actionNames = legacyActionNames;
+    }
+
     /**
      * The name of the Route. Must be unique across Route.
      */
     public String name() {
         return this.name;
     }
 
+    /**
+     * The legacy transport Action name to match against this route to support authorization in REST layer.
+     * MUST be unique across all Routes
+     */
+    public Set<String> actionNames() {
+        return this.actionNames;
+    }
+
     @Override
     public String toString() {
-        return "NamedRoute [method=" + method + ", path=" + path + ", name=" + name + "]";
+        return "NamedRoute [method=" + method + ", path=" + path + ", name=" + name + ", actionNames=" + actionNames + "]";
     }
 }

server/src/main/java/org/opensearch/rest/extensions/RestSendToExtensionAction.java

@@ -36,6 +36,8 @@
 import java.nio.charset.StandardCharsets;
 import java.security.Principal;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -94,6 +96,7 @@ public RestSendToExtensionAction(
         List<Route> restActionsAsRoutes = new ArrayList<>();
         for (String restAction : restActionsRequest.getRestActions()) {
             Optional<String> name = Optional.empty();
+            Set<String> actionNames = new HashSet<>();
             String[] parts = restAction.split(" ");
             if (parts.length < 2) {
                 throw new IllegalArgumentException("REST action must contain at least a REST method and route");
@@ -104,12 +107,20 @@ public RestSendToExtensionAction(
                 if (parts.length > 2) {
                     name = Optional.of(parts[2].trim());
                 }
+                if (parts.length > 3) {
+                    actionNames = Collections.singleton(parts[3].trim());
+                }
             } catch (IndexOutOfBoundsException | IllegalArgumentException e) {
                 throw new IllegalArgumentException(restAction + " does not begin with a valid REST method");
             }
             logger.info("Registering: " + method + " " + path);
             if (name.isPresent()) {
-                NamedRoute nr = new NamedRoute(method, path, name.get());
+                NamedRoute nr;
+                if (!actionNames.isEmpty()) {
+                    nr = new NamedRoute(method, path, name.get(), actionNames);
+                } else {
+                    nr = new NamedRoute(method, path, name.get());
+                }
                 restActionsAsRoutes.add(nr);
                 dynamicActionRegistry.registerDynamicRoute(nr, this);
             } else {

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

@@ -26,6 +26,7 @@
 import java.io.IOException;
 import java.util.Collections;
 import java.util.Map;
+import java.util.Set;
 
 import static org.mockito.Mockito.mock;
 
@@ -89,22 +90,30 @@ public void testDynamicActionRegistryWithNamedRoutes() {
 
         assertTrue(registry.isActionRegistered("foo"));
         assertTrue(registry.isActionRegistered("bar"));
+
+        registry.unregisterDynamicRoute(r2);
+
+        assertTrue(registry.isActionRegistered("foo"));
+        assertFalse(registry.isActionRegistered("bar"));
     }
 
-    public void testDynamicActionRegistryRegisterAndUnregisterWithNamedRoutes() {
+    public void testDynamicActionRegistryWithNamedRoutesAndLegacyActionNames() {
         RestSendToExtensionAction action = mock(RestSendToExtensionAction.class);
         RestSendToExtensionAction action2 = mock(RestSendToExtensionAction.class);
-        NamedRoute r1 = new NamedRoute(RestRequest.Method.GET, "/foo", "foo");
-        NamedRoute r2 = new NamedRoute(RestRequest.Method.GET, "/bar", "bar");
+        NamedRoute r1 = new NamedRoute(RestRequest.Method.GET, "/foo", "foo", Set.of("cluster:admin/opensearch/abc/foo"));
+        NamedRoute r2 = new NamedRoute(RestRequest.Method.GET, "/bar", "bar", Set.of("cluster:admin/opensearch/xyz/bar"));
 
         DynamicActionRegistry registry = new DynamicActionRegistry();
         registry.registerDynamicRoute(r1, action);
         registry.registerDynamicRoute(r2, action2);
 
+        assertTrue(registry.isActionRegistered("cluster:admin/opensearch/abc/foo"));
+        assertTrue(registry.isActionRegistered("cluster:admin/opensearch/xyz/bar"));
+
         registry.unregisterDynamicRoute(r2);
 
-        assertTrue(registry.isActionRegistered("foo"));
-        assertFalse(registry.isActionRegistered("bar"));
+        assertTrue(registry.isActionRegistered("cluster:admin/opensearch/abc/foo"));
+        assertFalse(registry.isActionRegistered("cluster:admin/opensearch/xyz/bar"));
     }
 
     private static final class TestAction extends ActionType<ActionResponse> {

server/src/test/java/org/opensearch/extensions/rest/RestSendToExtensionActionTests.java

@@ -198,6 +198,48 @@ public void testRestSendToExtensionActionWithNamedRoute() throws Exception {
         assertTrue(expectedNames.containsAll(names));
     }
 
+    public void testRestSendToExtensionActionWithNamedRouteAndLegacyActionName() throws Exception {
+        RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
+            "uniqueid1",
+            List.of(
+                "GET /foo foo cluster:admin/opensearch/abc/foo",
+                "PUT /bar bar cluster:admin/opensearch/jkl/bar",
+                "POST /baz baz cluster:admin/opensearch/xyz/baz"
+            ),
+            List.of("GET /deprecated/foo foo_deprecated cluster:admin/opensearch/abc/foo_deprecated", "It's deprecated!")
+        );
+        RestSendToExtensionAction restSendToExtensionAction = new RestSendToExtensionAction(
+            registerRestActionRequest,
+            discoveryExtensionNode,
+            transportService,
+            dynamicActionRegistry
+        );
+
+        assertEquals("send_to_extension_action", restSendToExtensionAction.getName());
+        List<NamedRoute> expected = new ArrayList<>();
+        String uriPrefix = "/_extensions/_uniqueid1";
+        expected.add(new NamedRoute(Method.GET, uriPrefix + "/foo", "foo", Set.of("cluster:admin/opensearch/abc/foo")));
+        expected.add(new NamedRoute(Method.PUT, uriPrefix + "/bar", "bar", Set.of("cluster:admin/opensearch/jkl/bar")));
+        expected.add(new NamedRoute(Method.POST, uriPrefix + "/baz", "baz", Set.of("cluster:admin/opensearch/xyz/baz")));
+
+        List<Route> routes = restSendToExtensionAction.routes();
+        assertEquals(expected.size(), routes.size());
+        List<String> expectedPaths = expected.stream().map(Route::getPath).collect(Collectors.toList());
+        List<String> paths = routes.stream().map(Route::getPath).collect(Collectors.toList());
+        List<Method> expectedMethods = expected.stream().map(Route::getMethod).collect(Collectors.toList());
+        List<Method> methods = routes.stream().map(Route::getMethod).collect(Collectors.toList());
+        List<String> expectedNames = expected.stream().map(NamedRoute::name).collect(Collectors.toList());
+        List<String> names = routes.stream().map(r -> ((NamedRoute) r).name()).collect(Collectors.toList());
+        Set<String> expectedActionNames = expected.stream().flatMap(nr -> nr.actionNames().stream()).collect(Collectors.toSet());
+        Set<String> actionNames = routes.stream().flatMap(nr -> ((NamedRoute) nr).actionNames().stream()).collect(Collectors.toSet());
+        assertTrue(paths.containsAll(expectedPaths));
+        assertTrue(expectedPaths.containsAll(paths));
+        assertTrue(methods.containsAll(expectedMethods));
+        assertTrue(expectedMethods.containsAll(methods));
+        assertTrue(expectedNames.containsAll(names));
+        assertTrue(expectedActionNames.containsAll(actionNames));
+    }
+
     public void testRestSendToExtensionMultipleNamedRoutesWithSameName() throws Exception {
         RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
             "uniqueid1",
@@ -210,6 +252,18 @@ public void testRestSendToExtensionMultipleNamedRoutesWithSameName() throws Exce
         );
     }
 
+    public void testRestSendToExtensionMultipleNamedRoutesWithSameLegacyActionName() throws Exception {
+        RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
+            "uniqueid1",
+            List.of("GET /foo foo cluster:admin/opensearch/abc/foo", "PUT /bar bar cluster:admin/opensearch/abc/foo"),
+            List.of()
+        );
+        expectThrows(
+            IllegalArgumentException.class,
+            () -> new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, dynamicActionRegistry)
+        );
+    }
+
     public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPath() throws Exception {
         RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
             "uniqueid1",
@@ -234,7 +288,7 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDiffer
         );
     }
 
-    public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() throws Exception {
+    public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() {
         RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
             "uniqueid1",
             List.of("GET /foo/{path_param}", "GET /foo/{path_param}/list"),