[FEATURE] Pass REST params to extensions

[dbwiddis]

Is your feature request related to a problem?

As part of #64, OS #4282 established a basic way of forwarding REST requests to extensions, passing the HTTP method and uri fields.

To expand functionality, the params() of the RestRequest should also be forwarded. This will be necessary for handling additional types of requests.

What solution would you like?

Update the RestExecuteOnExtensionRequest class to include params(). Update associated code in the RestSendToExtensionAction, ExtensionsRunner, ExtensionRestHandler interface, and the sample extension's RestHelloAction.

What alternatives have you considered?

Sending the entire RestRequest rather than extracting parts of it. This doesn't work from a security perspective as sensitive information can be included in headers, and these are not easily removed.

Do you have any additional context?

See first comment at #109.

[mukulsethi22]

Hey, can I work on this issue ?

[dbwiddis]

Hey, @mukulsethi22 thanks for your interest. We'd love for you to work on this. Some quick notes:

• This is a must-have capability to support [META] Migrate Anomaly Detector plugin to work as an Extension #24 and [META] APIs for Extension Point for creating a detector of AD #100, which we are targeting in the next few weeks. So we'd love if you can dive in promptly to work on this. If not, we might end up doing it first. :-)
• Note that this requires OpenSearch PR #4282 to be merged for the work needed on the OpenSearch repo side. That part should hopefully happen soon.
  • It will be on the feature/extensions branch after merging
  • If you want to get started now you can use my branch from that PR
• Note that it also requires Add Extension API interfaces and implement in sample Hello World extension #109 to be merged, which may take a little longer, and may have some name changes from the current state (and even this issue description).
  • Working off my branch on this PR is probably a good idea, then you can cherry-pick the commits over to a fresh branch off main once it's merged.
• Take a look at the DEVELOPER_GUIDE and DESIGN documents to orient yourself.

Let me know if you have any questions, happy to guide you along here.

[dbwiddis]

Hi @mukulsethi22, just a quick update. The PRs mentioned above have been merged, so the code is available for you to fork and start work if you're ready.

• You'll want to fork the OpenSearch project and create your branch of the latest code in the feature/extensions branch.
• You can fork this repo and branch off main.
• The main pieces of code you'll need to alter are the RestExecuteOnExtensionRequest and the RestSendToExtensionAction on the OpenSearch (feature/extensions) side, and ExtensionsRunner on the sdk side.
  • For the RestExecuteOnExtensionRequest, add a new Map<String, String> params field below the method and uri, and add it to the constructors and writeTo() method.
    • For the input stream you'll want to use in.readMap(StreamInput::readString, StreamInput::readString);.
    • For the output stream, out.writeMap(params) should work.
  • For the RestSendToExtensionAction you should just need to add the parameters which call the above request.
  • In the ExtensionsRunner look for the handleRestExecuteOnExtensionRequest(). You'll extract the additional field from the request and pass it to the extension action, which will be in the RestHelloAction in the sample project.

There may be a few other points you'll need to change, and the tests will need updating, but hopefully this is a good roadmap to get you started. Let me know if you have any questions!

[cwperks]

@dbwiddis In addition to forwarding the params, should we also be able to support routes with parameters?

i.e. new Route(POST, "/crud/update/{doc_id}")

Our current process for locating ExtensionRestHandlers looks for an exact match to the URI to find the respective handler (See https:/opensearch-project/opensearch-sdk-java/blob/main/src/main/java/org/opensearch/sdk/ExtensionsRunner.java#L228).

Are route parameters functionality we want to support?

[dbwiddis]

re route parameters functionality we want to support?

Yes.

We 100% want to match the route handling of plugins. My initial setup was just basic and as you've noted, the exact match is not the best... I will create a task to do this next Sprint.

[mukulsethi22]

Apart from passing params to RestHelloAction file's handleRequest method, any changes have to be done on the method?

[dbwiddis]

Hey @mukulsethi22, sorry for a slow reply. I was traveling the past few days.

I've been working down a parallel path with #122 which includes the "named wildcard" or "path parameters" which are part of the rest path, while leaving the "query parameters" to you for this issue. In doing so I realized we also need to handle consuming parameters.

So the steps in this comment still apply:

• Add the params() map to RestExecuteOnExtensionRequest
• Pass this map along to that request from RestSendToExtensionAction
• In handleRestExecuteOnExtensionRequest() pass the params to the extension (and update the interface)

There is one additional step required:

• Inside the extension's handler, create a Collection (set or list) of params that the request handles. These will need to be passed back to OpenSearch in the RestExecuteOnExtensionResponse. I'm working right now on updating that response to take that collection and pass it back. If my PR is merged first you'll have that argument available to populate in the response. If my PR hasn't been merged, just create a collection and add params you handle to it. For HelloWorld I'd suggest handling a GET request with an adjective to put before world, so the request URI would be /hello?worldtype=Brave+New and you would add the string worldtype to the list when you parsed it.

[dbwiddis]

So after implementing the consumed params, it turns out it's pretty easy, so no need for you to go to any extra effort. If you can get as far as changing the method signature in the ExtensionRestHandler interface and the code in the ExtensionsRunner that handles the requests, that'd be perfect.

[dbwiddis]

Hey @mukulsethi22, just checking in. Anything else you need? :)

[minalsha]

Hi @mukulsethi22 , just checking, is there any help you need for addressing the fix for this issue? Thanks

[dbwiddis]

Hey @mukulsethi22 we need to get this feature done in the next week or two. Are you still interested? Would love to help you complete this.

[dbwiddis]

@mukulsethi22 I'm going to go ahead and work on this myself as we need to get it done. If you'd like another issue to work on that's less time critical, let me know and we can find you another way to contribute.