Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create NamedRoute to map extension routes to a shortened name #6870

Merged
merged 59 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
a688fd0
WIP on rest layer authz
cwperks Mar 27, 2023
88955ff
Create PermissibleRoute
cwperks Mar 28, 2023
a7655e5
Update extension handshake
cwperks Mar 28, 2023
de8b8b8
Add connectToNodeAsExtension in TransportService
cwperks Mar 29, 2023
ad68321
Merge branch 'extension-handshake' into move-ext-rest-request
cwperks Mar 29, 2023
0003dcc
Add to CHANGELOG
cwperks Mar 31, 2023
4d87934
Merge branch 'main' into extension-handshake
cwperks Mar 31, 2023
6c41e96
Merge branch 'extension-handshake' into move-ext-rest-request
cwperks Mar 31, 2023
f1d9c71
Add to CHANGELOG
cwperks Mar 31, 2023
64e8667
Update RouteHandler
cwperks Mar 31, 2023
98b1af0
Update java docstrings
cwperks Apr 3, 2023
a7f83b5
Merge branch 'extension-handshake' into move-ext-rest-request
cwperks Apr 3, 2023
e1c754b
Merge branch 'main' into extension-handshake
cwperks Apr 10, 2023
4268341
Merge branch 'extension-handshake' into move-ext-rest-request
cwperks Apr 10, 2023
1ef93fc
Merge branch 'main' into extension-handshake
cwperks Apr 22, 2023
e041cf6
Merge branch 'extension-handshake' into move-ext-rest-request
cwperks Apr 22, 2023
3b4b91b
Run spotlessApply
cwperks Apr 22, 2023
d90d065
Fix merge conflicts
cwperks Apr 22, 2023
38ca9da
Merge branch 'main' into extension-handshake
cwperks Apr 27, 2023
8913f9f
Merge branch 'extension-handshake' into move-ext-rest-request
cwperks Apr 27, 2023
c202da9
Merge branch 'main' into move-ext-rest-request
cwperks Apr 28, 2023
dace95e
Rename to ProtectedRoute
cwperks Apr 28, 2023
bff0ea2
Merge branch 'main' into move-ext-rest-request
cwperks May 1, 2023
13a00a0
Merge branch 'main' into move-ext-rest-request
cwperks May 4, 2023
29284a1
Merge branch 'main' into move-ext-rest-request
cwperks May 5, 2023
634e0b8
Merge branch 'main' into move-ext-rest-request
cwperks May 8, 2023
b2c60d9
Create method to get extension settings from extensions.yml
cwperks May 8, 2023
7d38fee
Add ExtensionsManager.lookupExtensionSettings
cwperks May 8, 2023
f188c34
Small change to name
cwperks May 8, 2023
140440d
Add to CHANGELOG
cwperks May 8, 2023
48264c2
Move extensionSettingsMap.put
cwperks May 9, 2023
00f3ece
Re-run CI
cwperks May 9, 2023
5fc95e4
Merge branch 'lookup-extension-settings' into move-ext-rest-request
cwperks May 9, 2023
955a1de
Merge branch 'main' into move-ext-rest-request
cwperks May 11, 2023
b305e9a
Address review feedback
cwperks May 11, 2023
9f53b37
Add test for ProtectedRoute
cwperks May 11, 2023
91b2753
spotlessApply
cwperks May 11, 2023
097e8d2
Merge branch 'main' into move-ext-rest-request
cwperks May 11, 2023
8155630
Merge branch 'main' into move-ext-rest-request
cwperks May 16, 2023
5e1239a
Add RouteHandlerTests
cwperks May 16, 2023
271b6f6
Switch to NamedRoute and add validation for action naming
cwperks May 16, 2023
8e087b3
Merge branch 'main' into move-ext-rest-request
cwperks May 16, 2023
9e453ec
Avoid magic numbers
cwperks May 16, 2023
1fbde26
Remove @Test annotation
cwperks May 16, 2023
c3f3a0e
Address code review feedback
cwperks May 16, 2023
6cb42fe
Update error message
cwperks May 16, 2023
eceda9c
Check for REST Action name uniqueness across all registered actions
cwperks May 17, 2023
6095f8d
minimize code in the test
cwperks May 17, 2023
52c3f1e
Merge branch 'check-rest-action-name-in-registry' into move-ext-rest-…
cwperks May 17, 2023
300b6a9
Update changelog
cwperks May 17, 2023
2f35272
Add DynamicRouteRegistry
cwperks May 17, 2023
cb1244f
Address code review feedback
cwperks May 17, 2023
5a9c363
Add mock DynamicRouteRegistry.class
cwperks May 17, 2023
9ca3c90
Add RouteRegistry to DynamicActionModule
cwperks May 18, 2023
c7d7c15
Pass around dynamicActionRegistry instead of ActionModule
cwperks May 18, 2023
fd53943
Only pass dynamic action registry
cwperks May 18, 2023
ccbe4b5
Add DynamicActionRegistryTests for tests of dynamic registry
cwperks May 18, 2023
0ad12ee
Move CHANGELOG entry
cwperks May 18, 2023
98a9d44
Merge branch 'main' into move-ext-rest-request
cwperks May 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 11 additions & 39 deletions server/src/main/java/org/opensearch/action/ActionModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -498,8 +498,6 @@ public class ActionModule extends AbstractModule {
// associated with remote action execution on extensions, possibly in
// a different JVM and possibly on a different server.
private final DynamicActionRegistry dynamicActionRegistry;
// A dynamic route registry associated with registered extension routes
private final DynamicRouteRegistry dynamicRouteRegistry;
private final ActionFilters actionFilters;
private final AutoCreateIndex autoCreateIndex;
private final DestructiveOperations destructiveOperations;
Expand Down Expand Up @@ -532,7 +530,6 @@ public ActionModule(
actions = setupActions(actionPlugins);
actionFilters = setupActionFilters(actionPlugins);
dynamicActionRegistry = new DynamicActionRegistry();
dynamicRouteRegistry = new DynamicRouteRegistry(dynamicActionRegistry);
autoCreateIndex = new AutoCreateIndex(settings, clusterSettings, indexNameExpressionResolver, systemIndices);
destructiveOperations = new DestructiveOperations(settings, clusterSettings);
Set<RestHeaderDefinition> headers = Stream.concat(
Expand Down Expand Up @@ -1010,10 +1007,6 @@ public DynamicActionRegistry getDynamicActionRegistry() {
return dynamicActionRegistry;
}

public DynamicRouteRegistry getDynamicRouteRegistry() {
return dynamicRouteRegistry;
}

public RestController getRestController() {
return restController;
}
Expand All @@ -1034,6 +1027,10 @@ 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<>();

/**
Expand Down Expand Up @@ -1098,31 +1095,6 @@ public boolean isActionRegistered(String actionName) {
}
return registry.get(action);
}
}

/**
* The DynamicRouteRegistry maintains a registry mapping {@link org.opensearch.rest.RestHandler.Route} instances to {@link org.opensearch.rest.extensions.RestSendToExtensionAction} instances.
* <p>
* This class is modeled after {@link NamedRegistry} but provides both register and unregister capabilities.
*
* @opensearch.internal
*/
public class DynamicRouteRegistry {
// This is an instance of a DynamicActionRegistry containing all registered transport actions
private DynamicActionRegistry dynamicActionRegistry;
// A dynamic registry to add or remove Route / RestSendToExtensionAction pairs
// at times other than node bootstrap.
private final Map<RestHandler.Route, RestSendToExtensionAction> registry = new ConcurrentHashMap<>();

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

/**
*
* @param dynamicActionRegistry A registry of all transport actions - native, plugins and dynamic
*/
public DynamicRouteRegistry(DynamicActionRegistry dynamicActionRegistry) {
this.dynamicActionRegistry = dynamicActionRegistry;
}

/**
* Add a dynamic action to the registry.
Expand All @@ -1136,15 +1108,15 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct
Optional<String> routeName = Optional.empty();
if (route instanceof NamedRoute) {
routeName = Optional.of(((NamedRoute) route).name());
if (dynamicActionRegistry.isActionRegistered(routeName.get()) || registeredRestActionNames.contains(routeName.get())) {
if (isActionRegistered(routeName.get()) || registeredActionNames.contains(routeName.get())) {
throw new IllegalArgumentException("route [" + route + "] already registered");
}
}
if (registry.containsKey(route)) {
if (routeRegistry.containsKey(route)) {
throw new IllegalArgumentException("route [" + route + "] already registered");
}
registry.put(route, action);
routeName.ifPresent(registeredRestActionNames::add);
routeRegistry.put(route, action);
routeName.ifPresent(registeredActionNames::add);
}

/**
Expand All @@ -1154,11 +1126,11 @@ public void registerDynamicRoute(RestHandler.Route route, RestSendToExtensionAct
*/
public void unregisterDynamicRoute(RestHandler.Route route) {
owaiskazi19 marked this conversation as resolved.
Show resolved Hide resolved
requireNonNull(route, "route is required");
if (registry.remove(route) == null) {
if (routeRegistry.remove(route) == null) {
throw new IllegalArgumentException("action [" + route + "] was not registered");
}
if (route instanceof NamedRoute) {
registeredRestActionNames.remove(((NamedRoute) route).name());
registeredActionNames.remove(((NamedRoute) route).name());
}
}
owaiskazi19 marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -1170,7 +1142,7 @@ public void unregisterDynamicRoute(RestHandler.Route route) {
*/
@SuppressWarnings("unchecked")
public RestSendToExtensionAction get(RestHandler.Route route) {
return registry.get(route);
return routeRegistry.get(route);
}
}
}
10 changes: 8 additions & 2 deletions server/src/main/java/org/opensearch/rest/RestHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,18 @@ public String getPath() {
return path;
}

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

public Method getMethod() {
return method;
}

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

@Override
Expand All @@ -219,7 +224,8 @@ public boolean equals(Object o) {
return false;
}
Route that = (Route) o;
return Objects.equals(method, that.method) && Objects.equals(path, that.path);
return Objects.equals(method, that.method)
&& Objects.equals(getPathWithPathParamsReplaced(), that.getPathWithPathParamsReplaced());
cwperks marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ public RestSendToExtensionAction(
if (name.isPresent()) {
reta marked this conversation as resolved.
Show resolved Hide resolved
NamedRoute nr = new NamedRoute(method, path, name.get());
restActionsAsRoutes.add(nr);
actionModule.getDynamicRouteRegistry().registerDynamicRoute(nr, this);
actionModule.getDynamicActionRegistry().registerDynamicRoute(nr, this);
} else {
Route r = new Route(method, path);
restActionsAsRoutes.add(r);
actionModule.getDynamicRouteRegistry().registerDynamicRoute(r, this);
actionModule.getDynamicActionRegistry().registerDynamicRoute(r, this);
}
}
this.routes = unmodifiableList(restActionsAsRoutes);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
import org.junit.Before;
import org.opensearch.Version;
import org.opensearch.action.ActionModule;
import org.opensearch.action.ActionModule.DynamicRouteRegistry;
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 @@ -169,7 +169,7 @@ public void setup() throws Exception {
new UsageService(),
new IdentityService(Settings.EMPTY, List.of())
);
when(actionModule.getDynamicRouteRegistry()).thenReturn(mock(DynamicRouteRegistry.class));
when(actionModule.getDynamicActionRegistry()).thenReturn(mock(DynamicActionRegistry.class));
when(actionModule.getRestController()).thenReturn(restController);
settingsModule = new SettingsModule(Settings.EMPTY, emptyList(), emptyList(), emptySet());
clusterService = createClusterService(threadPool);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,31 @@ public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPath() throws
);
}

public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithDifferentPathParams() throws Exception {
RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
"uniqueid1",
List.of("GET /foo/{path_param1}", "GET /foo/{path_param2}"),
List.of()
);
expectThrows(
IllegalArgumentException.class,
() -> new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, actionModule)
);
}

public void testRestSendToExtensionMultipleRoutesWithSameMethodAndPathWithPathParams() throws Exception {
RegisterRestActionsRequest registerRestActionRequest = new RegisterRestActionsRequest(
"uniqueid1",
List.of("GET /foo/{path_param}", "GET /foo/{path_param}/list"),
List.of()
);
try {
new RestSendToExtensionAction(registerRestActionRequest, discoveryExtensionNode, transportService, actionModule);
} catch (IllegalArgumentException e) {
fail("IllegalArgumentException should not be thrown for different paths");
}
}

public void testRestSendToExtensionWithNamedRouteCollidingWithDynamicTransportAction() throws Exception {
DynamicActionRegistry dynamicActionRegistry = actionModule.getDynamicActionRegistry();
ActionFilters emptyFilters = new ActionFilters(Collections.emptySet());
Expand Down