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

[FEATURE] Register REST API and map to associated extension in ExtensionsOrchestrator #69

Closed
dbwiddis opened this issue Jul 30, 2022 · 3 comments · Fixed by #101
Closed
Assignees
Labels
enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

Is your feature request related to a problem?

See steps 4, 6, and 7 described in #64.

What solution would you like?

Upon receipt of REST API listing provided in #68, The ExtensionsOrchestrator should:

  • Register the APIs with the RestHandler
  • Store the response from the RestHandler in a map with the API as the key and the requesting extension as the value.

What alternatives have you considered?

The best location for the map and API redirects to be stored is still an open question.

Do you have any additional context?

See also the DESIGN.md file (draft in #60)

@dbwiddis dbwiddis added enhancement New feature or request untriaged labels Jul 30, 2022
@dbwiddis dbwiddis self-assigned this Aug 2, 2022
@dbwiddis dbwiddis changed the title [FEATURE] Store map of REST API to associated extension in ExtensionsOrchestrator [FEATURE] Register REST API and map to associated extension in ExtensionsOrchestrator Aug 16, 2022
@dbwiddis
Copy link
Member Author

I think we might be able to re-use the vast majority of existing Plugin code for the REST actions. However, we need to carefully address the question of "which JVM is the code executed in". Directly applying the code may end up with OpenSearch's JVM processing the actions, so the registered "actions" need to prompt network communication to a separate request and associated handler.

Details below.


Current Plugin setup uses a base URI containing _plugins:

public static final String AD_BASE_URI = "/_plugins/_anomaly_detection";

So we can follow this model and use an _extensions base URI:

String restApiBaseString = "/_extensions/_" + extensionNameGoesHere;
String restActionStringToRegister = restApiBaseString + restActionString;

Only concern here might be conflicting/duplicate extension names so we may consider using a uniqueID, or an expanded "fully qualified name" (e.g., similar to JPMS module names which are supposed to be globally unique) to deconflict. I'll go ahead with the extension name for now.


REST Handlers correspond to Action classes which extend BaseRestHandler. For the AD plugin, these are returned by this method:

public List<RestHandler> getRestHandlers( ... ) {
  // ...
  RestGetAnomalyDetectorAction restGetAnomalyDetectorAction = new RestGetAnomalyDetectorAction();
  // ...
  return ImmutableList.of(
    restGetAnomalyDetectorAction,
  // ...
  )
}
}

These are eventually registered with OpenSearch (the end goal of this issue) in the ActionModule class

for (ActionPlugin plugin : actionPlugins) {
    for (RestHandler handler : plugin.getRestHandlers(
        settings,
        restController,
        clusterSettings,
        indexScopedSettings,
        settingsFilter,
        indexNameExpressionResolver,
        nodesInCluster
    )) {
        registerHandler.accept(handler);
    }
}

This accept() method calls restController.registerHandler()

We have the ability to send these Action classes back and forth and already do so in the current SDK code for a handful of REST actions, so we'll just send that list similar to (or along with) how the current PR #74 is sending a list of strings. The ExtensionsOrchestrator can easily create a loop inside the ExtensionRestApiRequestHandler (which will probably be renamed) to take the collection of API strings and associated Action classes and call registerHandler.accept(). We just need to be able to access the RestController from within the Orchestrator, and that can be passed in Node.java (it's initialized on line 729 and then some ExtensionsOrchestrator setters are called on lines 777 and 778 where we can add another setter for setRestController().

this.extensionsOrchestrator.setTransportService(transportService);
this.extensionsOrchestrator.setClusterService(clusterService);
// add this
this.extensionsOrchestrator.setRestController(restController);

Way forward:

We can either merge #74 (and its OS counterpart) and I can build on it or I can put it into draft and start making the following changes to address this issue:

  • Add a setter for the RestController to ExtensionsOrchestrator
  • Tightly couple the Extension API strings with Action class strings and send them together where the current "registerRestApi" code is.
  • Using the RestController, do the registerHandler.accept(handler) equivalent
  • Optionally, store some of this information in hashmaps. Not sure if this is needed.

And finally, to be sure the Action code executes on the extension's node:

  • Make sure the executed Action is to establish a connection and send a request to a separate RequestHandler in the extension. (This will look similar to how we are currently sending the API as a request, in the "response" code in the extension.)
    • This will probably require some parent classes extending RestHandler and type safety to enforce their use in extensions, which will include most of the "boilerplate" request/response stuff and make the actual extension code mostly configuring the corresponding handler class (which will have its own parent).

@owaiskazi19 and @saratvemulapalli I'd appreciate your thoughts on this approach before I head down this road.

@dbwiddis
Copy link
Member Author

Further progress from tonight's explorations:

  • Created a new class RestActionsRequestHandler
  • During Node startup, initialize this class, passing the RestController to the constructor
  • moved the handleRegisterRestActionsRequest() method into it.
  • this method receives the register API request, presently a list of Strings
  • Next step to implement is to turn the strings into a RestHandler object and then call controller.register(handler)

Plan going forward is:

  • Create a new class RestExtensionAction extends BaseRestHandler
  • Use the values in the RegisterRestActionsRequest being processed to create this object
    • The current "API" strings correlate directly to routes()
    • Will need to update the YAML (eventually a spec file) to have multiple "action names" followed by the list of API strings for each action name (easy), one Action Object per action name
    • The last bit to override is the prepareRequest() method. In our case the action we're going to have is to send the request to the appropriate extension node
      • Inside the Extension code we'll have a request handler for these requests that parses them properly

@dbwiddis
Copy link
Member Author

Current sequence (will eventually find its way to a DESIGN doc):

  • From Node during bootstrap, calls initializeRestController to instantiate the RestActionsRequestHandler object
    • passes the extensionIdMap to cross-reference full extension information from the uniqueId
      • note this is not yet initialized, just an object!
    • this also initializes the transport service and request handlers in the correct sequence
  • From Node (later) during bootstrap, calls extensionsInitialize which sends initialize requests to extensions
  • Extensions respond to initialization, populating the extensionIdMap
  • Extensions then send register API request
  • The request handler for rest API forwards to the RestActionsRequestHandler
    • This adds the API routes to the rest controller with a RestSendToExtensionAction

Next steps:

  • Test that above code works and that a URI appropriately triggers the SendToExtensionAction
  • Write unit tests
  • Write new Request/Response objects so that when the SendToExtensionAction is triggered it sends the RestRequest to the extension and executes it there and ultimately responds (this is part of [FEATURE] Redirect REST API request to appropriate extensions #70 so may be deferred or done in the same PR, depending on review status)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants