Skip to content

Commit

Permalink
Create NamedRoute to map extension routes to a shortened name (opense…
Browse files Browse the repository at this point in the history
…arch-project#6870)

* WIP on rest layer authz

Signed-off-by: Craig Perkins <[email protected]>

* Create PermissibleRoute

Signed-off-by: Craig Perkins <[email protected]>

* Update extension handshake

Signed-off-by: Craig Perkins <[email protected]>

* Add connectToNodeAsExtension in TransportService

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

* Update RouteHandler

Signed-off-by: Craig Perkins <[email protected]>

* Update java docstrings

Signed-off-by: Craig Perkins <[email protected]>

* Run spotlessApply

Signed-off-by: Craig Perkins <[email protected]>

* Fix merge conflicts

Signed-off-by: Craig Perkins <[email protected]>

* Rename to ProtectedRoute

Signed-off-by: Craig Perkins <[email protected]>

* Create method to get extension settings from extensions.yml

Signed-off-by: Craig Perkins <[email protected]>

* Add ExtensionsManager.lookupExtensionSettings

Signed-off-by: Craig Perkins <[email protected]>

* Small change to name

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

* Move extensionSettingsMap.put

Signed-off-by: Craig Perkins <[email protected]>

* Re-run CI

Signed-off-by: Craig Perkins <[email protected]>

* Address review feedback

Signed-off-by: Craig Perkins <[email protected]>

* Add test for ProtectedRoute

Signed-off-by: Craig Perkins <[email protected]>

* spotlessApply

Signed-off-by: Craig Perkins <[email protected]>

* Add RouteHandlerTests

Signed-off-by: Craig Perkins <[email protected]>

* Switch to NamedRoute and add validation for action naming

Signed-off-by: Craig Perkins <[email protected]>

* Avoid magic numbers

Signed-off-by: Craig Perkins <[email protected]>

* Remove @test annotation

Signed-off-by: Craig Perkins <[email protected]>

* Address code review feedback

Signed-off-by: Craig Perkins <[email protected]>

* Update error message

Signed-off-by: Craig Perkins <[email protected]>

* Check for REST Action name uniqueness across all registered actions

Signed-off-by: Craig Perkins <[email protected]>

* minimize code in the test

Signed-off-by: Craig Perkins <[email protected]>

* Update changelog

Signed-off-by: Craig Perkins <[email protected]>

* Add DynamicRouteRegistry

Signed-off-by: Craig Perkins <[email protected]>

* Address code review feedback

Signed-off-by: Craig Perkins <[email protected]>

* Add mock DynamicRouteRegistry.class

Signed-off-by: Craig Perkins <[email protected]>

* Add RouteRegistry to DynamicActionModule

Signed-off-by: Craig Perkins <[email protected]>

* Pass around dynamicActionRegistry instead of ActionModule

Signed-off-by: Craig Perkins <[email protected]>

* Only pass dynamic action registry

Signed-off-by: Craig Perkins <[email protected]>

* Add DynamicActionRegistryTests for tests of dynamic registry

Signed-off-by: Craig Perkins <[email protected]>

* Move CHANGELOG entry

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Bharathwaj G <[email protected]>
  • Loading branch information
cwperks authored and bharath-techie committed May 23, 2023
1 parent d919e48 commit 8915128
Show file tree
Hide file tree
Showing 15 changed files with 720 additions and 100 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- SegRep with Remote: Add hook for publishing checkpoint notifications after segment upload to remote store ([#7394](https://github.com/opensearch-project/OpenSearch/pull/7394))
- Provide mechanism to configure XContent parsing constraints (after update to Jackson 2.15.0 and above) ([#7550](https://github.com/opensearch-project/OpenSearch/pull/7550))
- Support to clear filecache using clear indices cache API ([#7498](https://github.com/opensearch-project/OpenSearch/pull/7498))
- Create NamedRoute to map extension routes to a shortened name ([#6870](https://github.com/opensearch-project/OpenSearch/pull/6870))

### Dependencies
- Bump `com.netflix.nebula:gradle-info-plugin` from 12.0.0 to 12.1.3 (#7564)
Expand Down
73 changes: 73 additions & 0 deletions server/src/main/java/org/opensearch/action/ActionModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@
import org.opensearch.persistent.UpdatePersistentTaskStatusAction;
import org.opensearch.plugins.ActionPlugin;
import org.opensearch.plugins.ActionPlugin.ActionHandler;
import org.opensearch.rest.NamedRoute;
import org.opensearch.rest.RestController;
import org.opensearch.rest.RestHandler;
import org.opensearch.rest.RestHeaderDefinition;
Expand Down Expand Up @@ -452,6 +453,7 @@
import org.opensearch.rest.action.search.RestPutSearchPipelineAction;
import org.opensearch.rest.action.search.RestSearchAction;
import org.opensearch.rest.action.search.RestSearchScrollAction;
import org.opensearch.rest.extensions.RestSendToExtensionAction;
import org.opensearch.tasks.Task;
import org.opensearch.threadpool.ThreadPool;
import org.opensearch.usage.UsageService;
Expand All @@ -460,8 +462,10 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.function.Consumer;
import java.util.function.Supplier;
import java.util.function.UnaryOperator;
Expand Down Expand Up @@ -1028,13 +1032,22 @@ public static class DynamicActionRegistry {
// at times other than node bootstrap.
private final Map<ActionType<?>, TransportAction<?, ?>> registry = new ConcurrentHashMap<>();

// A dynamic registry to add or remove Route / RestSendToExtensionAction pairs
// at times other than node bootstrap.
private final Map<RestHandler.Route, RestSendToExtensionAction> routeRegistry = new ConcurrentHashMap<>();

private final Set<String> registeredActionNames = new ConcurrentSkipListSet<>();

/**
* Register the immutable actions in the registry.
*
* @param actions The injected map of {@link ActionType} to {@link TransportAction}
*/
public void registerUnmodifiableActionMap(Map<ActionType, TransportAction> actions) {
this.actions = actions;
for (ActionType action : actions.keySet()) {
registeredActionNames.add(action.name());
}
}

/**
Expand All @@ -1049,6 +1062,7 @@ public void registerDynamicAction(ActionType<?> action, TransportAction<?, ?> tr
if (actions.containsKey(action) || registry.putIfAbsent(action, transportAction) != null) {
throw new IllegalArgumentException("action [" + action.name() + "] already registered");
}
registeredActionNames.add(action.name());
}

/**
Expand All @@ -1061,6 +1075,16 @@ public void unregisterDynamicAction(ActionType<?> action) {
if (registry.remove(action) == null) {
throw new IllegalArgumentException("action [" + action.name() + "] was not registered");
}
registeredActionNames.remove(action.name());
}

/**
* Checks to see if an action is registered provided an action name
*
* @param actionName The name of the action to check
*/
public boolean isActionRegistered(String actionName) {
return registeredActionNames.contains(actionName);
}

/**
Expand All @@ -1076,5 +1100,54 @@ public void unregisterDynamicAction(ActionType<?> action) {
}
return registry.get(action);
}

/**
* Add a dynamic action to the registry.
*
* @param route The route instance to add
* @param action The corresponding instance of RestSendToExtensionAction to execute
*/
public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAction action) {
requireNonNull(route, "route is required");
requireNonNull(action, "action is required");
Optional<String> routeName = Optional.empty();
if (route instanceof NamedRoute) {
routeName = Optional.of(((NamedRoute) route).name());
if (isActionRegistered(routeName.get()) || registeredActionNames.contains(routeName.get())) {
throw new IllegalArgumentException("route [" + route + "] already registered");
}
}
if (routeRegistry.containsKey(route)) {
throw new IllegalArgumentException("route [" + route + "] already registered");
}
routeRegistry.put(route, action);
routeName.ifPresent(registeredActionNames::add);
}

/**
* Remove a dynamic route from the registry.
*
* @param route The route to remove
*/
public void unregisterDynamicRoute(RestHandler.Route route) {
requireNonNull(route, "route is required");
if (routeRegistry.remove(route) == null) {
throw new IllegalArgumentException("action [" + route + "] was not registered");
}
if (route instanceof NamedRoute) {
registeredActionNames.remove(((NamedRoute) route).name());
}
}

/**
* Gets the {@link RestSendToExtensionAction} instance corresponding to the {@link RestHandler.Route} instance.
*
* @param route The {@link RestHandler.Route}.
* @return the corresponding {@link RestSendToExtensionAction} if it is registered, null otherwise.
*/
@SuppressWarnings("unchecked")
public RestSendToExtensionAction get(RestHandler.Route route) {
return routeRegistry.get(route);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.opensearch.OpenSearchException;
import org.opensearch.Version;
import org.opensearch.action.ActionModule;
import org.opensearch.action.ActionModule.DynamicActionRegistry;
import org.opensearch.action.admin.cluster.state.ClusterStateResponse;
import org.opensearch.client.node.NodeClient;
import org.opensearch.cluster.ClusterSettingsResponse;
Expand Down Expand Up @@ -108,7 +109,6 @@ public static enum OpenSearchRequestType {

private final Path extensionsPath;
private ExtensionTransportActionsHandler extensionTransportActionsHandler;

private Map<String, Extension> extensionSettingsMap;
private Map<String, DiscoveryExtensionNode> initializedExtensions;
private Map<String, DiscoveryExtensionNode> extensionIdMap;
Expand Down Expand Up @@ -182,7 +182,7 @@ public void initializeServicesAndRestHandler(
actionModule,
this
);
registerRequestHandler();
registerRequestHandler(actionModule.getDynamicActionRegistry());
}

/**
Expand Down Expand Up @@ -223,14 +223,16 @@ public ExtensionActionResponse handleTransportRequest(ExtensionActionRequest req
return extensionTransportActionsHandler.sendTransportRequestToExtension(request);
}

private void registerRequestHandler() {
private void registerRequestHandler(DynamicActionRegistry dynamicActionRegistry) {
transportService.registerRequestHandler(
REQUEST_EXTENSION_REGISTER_REST_ACTIONS,
ThreadPool.Names.GENERIC,
false,
false,
RegisterRestActionsRequest::new,
((request, channel, task) -> channel.sendResponse(restActionsRequestHandler.handleRegisterRestActionsRequest(request)))
((request, channel, task) -> channel.sendResponse(
restActionsRequestHandler.handleRegisterRestActionsRequest(request, dynamicActionRegistry)
))
);
transportService.registerRequestHandler(
REQUEST_EXTENSION_REGISTER_CUSTOM_SETTINGS,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@

package org.opensearch.extensions.rest;

import org.opensearch.action.ActionModule.DynamicActionRegistry;
import org.opensearch.extensions.AcknowledgedResponse;
import org.opensearch.extensions.DiscoveryExtensionNode;
import org.opensearch.rest.RestController;
import org.opensearch.rest.RestHandler;
import org.opensearch.rest.extensions.RestSendToExtensionAction;
import org.opensearch.transport.TransportResponse;
import org.opensearch.transport.TransportService;

Expand Down Expand Up @@ -52,9 +54,17 @@ public RestActionsRequestHandler(
* @return A {@link AcknowledgedResponse} indicating success.
* @throws Exception if the request is not handled properly.
*/
public TransportResponse handleRegisterRestActionsRequest(RegisterRestActionsRequest restActionsRequest) throws Exception {
public TransportResponse handleRegisterRestActionsRequest(
RegisterRestActionsRequest restActionsRequest,
DynamicActionRegistry dynamicActionRegistry
) throws Exception {
DiscoveryExtensionNode discoveryExtensionNode = extensionIdMap.get(restActionsRequest.getUniqueId());
RestHandler handler = new RestSendToExtensionAction(restActionsRequest, discoveryExtensionNode, transportService);
RestHandler handler = new RestSendToExtensionAction(
restActionsRequest,
discoveryExtensionNode,
transportService,
dynamicActionRegistry
);
restController.registerHandler(handler);
return new AcknowledgedResponse(true);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.extensions.rest;

import java.util.function.Function;

import org.opensearch.rest.RestHandler.Route;
import org.opensearch.rest.RestRequest;
import org.opensearch.rest.RestRequest.Method;

/**
* A subclass of {@link Route} that includes a handler method for that route.
*/
public class RouteHandler extends Route {

private final String name;

private final Function<RestRequest, ExtensionRestResponse> responseHandler;

/**
* Handle the method and path with the specified handler.
*
* @param method The {@link Method} to handle.
* @param path The path to handle.
* @param handler The method which handles the method and path.
*/
public RouteHandler(Method method, String path, Function<RestRequest, ExtensionRestResponse> handler) {
super(method, path);
this.responseHandler = handler;
this.name = null;
}

/**
* Handle the method and path with the specified handler.
*
* @param name The name of the handler.
* @param method The {@link Method} to handle.
* @param path The path to handle.
* @param handler The method which handles the method and path.
*/
public RouteHandler(String name, Method method, String path, Function<RestRequest, ExtensionRestResponse> handler) {
super(method, path);
this.responseHandler = handler;
this.name = name;
}

/**
* Executes the handler for this route.
*
* @param request The request to handle
* @return the {@link ExtensionRestResponse} result from the handler for this route.
*/
public ExtensionRestResponse handleRequest(RestRequest request) {
return responseHandler.apply(request);
}

/**
* The name of the RouteHandler. Must be unique across route handlers.
*/
public String name() {
return this.name;
}
}
55 changes: 55 additions & 0 deletions server/src/main/java/org/opensearch/rest/NamedRoute.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.rest;

import org.opensearch.OpenSearchException;

/**
* 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;

public boolean isValidRouteName(String routeName) {
if (routeName == null || routeName.isBlank() || routeName.length() > MAX_LENGTH_OF_ACTION_NAME) {
return false;
}
return routeName.matches(VALID_ACTION_NAME_PATTERN);
}

public NamedRoute(RestRequest.Method method, String path, String name) {
super(method, path);
if (!isValidRouteName(name)) {
throw new OpenSearchException(
"Invalid route name specified. The route name may include the following characters"
+ " 'a-z', 'A-Z', '0-9', ':', '/', '*', '_' and be less than "
+ MAX_LENGTH_OF_ACTION_NAME
+ " characters"
);
}
this.name = name;
}

/**
* The name of the Route. Must be unique across Route.
*/
public String name() {
return this.name;
}

@Override
public String toString() {
return "NamedRoute [method=" + method + ", path=" + path + ", name=" + name + "]";
}
}
32 changes: 30 additions & 2 deletions server/src/main/java/org/opensearch/rest/RestHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,8 @@ public boolean allowSystemIndexAccessByDefault() {
*/
class Route {

private final String path;
private final Method method;
protected final String path;
protected final Method method;

public Route(Method method, String path) {
this.path = path;
Expand All @@ -196,9 +196,37 @@ public String getPath() {
return path;
}

public String getPathWithPathParamsReplaced() {
return path.replaceAll("(?<=\\{).*?(?=\\})", "path_param");
}

public Method getMethod() {
return method;
}

@Override
public int hashCode() {
String routeStr = "Route [method=" + method + ", path=" + getPathWithPathParamsReplaced() + "]";
return routeStr.hashCode();
}

@Override
public String toString() {
return "Route [method=" + method + ", path=" + path + "]";
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
Route that = (Route) o;
return Objects.equals(method, that.method)
&& Objects.equals(getPathWithPathParamsReplaced(), that.getPathWithPathParamsReplaced());
}
}

/**
Expand Down
Loading

0 comments on commit 8915128

Please sign in to comment.