Map extension routes to permission-able action names by cwperks · Pull Request #622 · opensearch-project/opensearch-sdk-java

cwperks · 2023-03-29T18:22:06Z

Description

This PR relies on an an earlier PR (#619) to setup TLS for extensions.

This PR also has a companion core and security PR

Core - Create NamedRoute to map extension routes to a shortened name OpenSearch#6870
Security - Authorize rest requests security#2753 ~~Authorize Requests bound for extensions in the REST-Layer security#2601~~

Isolated changes can be viewed using the compare tool: cwperks/opensearch-sdk-java@setup-extension-tls...ssl-and-handler-naming

This change modifies the extension settings by adding a new shortName setting (an abbreviation for an extension) that is used when creating a name for a route. /hello of the HelloWorld extension is mapped to the permission-able name extensions:hw/greet. The RouteHandler moves to core as part of this change, its needed by the security plugin to determine if a RestRequest is destined for an extension.

Endpoint registration has been updated to send the name of the route to core upon registration.

Issues Resolved

Related to opensearch-project/security#2589

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

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

owaiskazi19 · 2023-03-31T22:24:07Z

Did a quick pass. The changes overall LGTM @cwperks! I'm still trying to figure out the best place for these changes. Should it be in OpenSearch like we have ExtensionRestRequest there or SDK?

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

vibrantvarun · 2023-05-16T19:48:19Z

src/main/java/org/opensearch/sdk/ExtensionRouteHandlerFactory.java

+    public static ExtensionRouteHandlerFactory getInstance() {
+        if(INSTANCE == null) {
+            INSTANCE = new ExtensionRouteHandlerFactory();
+        }


Are we trying to create SINGLETON object for INSTANCE here? if yes is it multithreaded safe?

Yes, the problem I was facing is that I needed the extensions shortName (abbreviation) inside the RestHandler classes like RestHelloAction in the sample extension. Instead of passing the value as a constructor arg or passing that extension runner into those classes, I thought it would be better to have singular place where its registered and can be used throughout.

Do you know how I can test if this is multi-threaded safe?

owaiskazi19 · 2023-05-16T22:19:00Z

src/main/java/org/opensearch/sdk/ExtensionRouteHandlerFactory.java

+import org.opensearch.OpenSearchException;
+
+public class ExtensionRouteHandlerFactory {
+    private static ExtensionRouteHandlerFactory INSTANCE;


Do you think we can use Guice here to achieve this?

That would be great. I'll look into it. The main problem I was facing was that I needed to have the shortName of the extension available in the REST Handler (i.e. inside of RestHelloAction for the sample extension). The way I went about doing that was to create a singleton class that could be fetched throughout the SDK but if there is a better way to do it then I will switch to using that.

if there is a better way to do it then I will switch to using that.

Extensions are always initialized with their settings. getExtensionSettings() must be implemented on the Extension interface. (The BaseExtension class offers the convenience of either a settings object or yml file.)

Any method on the mainExtension class ought to be able to call getExtensionSettings().getShortExtensionName() to access this value without any factory/singletons.

In the AD Extension we usually pass the ExtensionsRunner class to every REST handler, and we have a getter on the runner for the extension. So all the AD REST handlers can do runner.getExtension().getExtensionSettings().getShortExtensionName().

owaiskazi19 · 2023-05-16T22:19:27Z

src/main/java/org/opensearch/sdk/ExtensionSettings.java

-    public ExtensionSettings(String extensionName, String hostAddress, String hostPort, String opensearchAddress, String opensearchPort) {
+    public ExtensionSettings(
+        String extensionName,
+        String shortName,


Suggested change

String shortName,

String shortExtensionName,

I'll update it, I was torn between different names for this variable. One of the naming ideas was extensionAbbr, but I didn't know if was appropriate to use an abbreviation in a variable name signifying that this variable is an abbreviation. It felt like it was a crossword puzzle clue.

owaiskazi19 · 2023-05-16T22:24:35Z

src/main/java/org/opensearch/sdk/rest/ExtensionRestPathRegistry.java

+            (mHandlers, newMHandler) -> mHandlers.addMethods(extensionRestHandler, method)
+        );
+        String restPathWithName = restPathToString(method, path, name);
+        registeredPaths.add(restPathWithName);


We are missing adding paths to deprecated paths here. One way to handle this would be to merge both the registerHandler methods and add a check for name.

owaiskazi19 · 2023-05-16T22:26:11Z

src/main/java/org/opensearch/sdk/ExtensionSettings.java


+    public String getShortName() {
+        return shortName;
+    }


This getter should have a test in TestExtensionSettings.java

This has been added to a few different tests to verify its behavior

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

codecov · 2023-05-19T15:29:19Z

Codecov Report

Merging #622 (e0df300) into main (eff7635) will increase coverage by 0.03%.
The diff coverage is 65.85%.

❗ Current head e0df300 differs from pull request most recent head 6d2ee04. Consider uploading reports for the commit 6d2ee04 to get more accurate results

@@             Coverage Diff              @@
##               main     #622      +/-   ##
============================================
+ Coverage     43.57%   43.61%   +0.03%     
- Complexity      314      324      +10     
============================================
  Files            69       71       +2     
  Lines          1983     2011      +28     
  Branches        141      144       +3     
============================================
+ Hits            864      877      +13     
- Misses         1101     1113      +12     
- Partials         18       21       +3

Impacted Files	Coverage Δ
.../opensearch/sdk/rest/BaseExtensionRestHandler.java	`89.23% <ø> (-0.63%)`	⬇️
.../sample/helloworld/rest/RestRemoteHelloAction.java	`25.00% <ø> (ø)`
...main/java/org/opensearch/sdk/ExtensionsRunner.java	`72.85% <30.00%> (-2.88%)`	⬇️
...g/opensearch/sdk/ExtensionRouteHandlerFactory.java	`66.66% <66.66%> (ø)`
...opensearch/sdk/rest/ExtensionRestPathRegistry.java	`89.36% <73.33%> (-7.94%)`	⬇️
...java/org/opensearch/sdk/ExtensionRouteHandler.java	`100.00% <100.00%> (ø)`
...ain/java/org/opensearch/sdk/ExtensionSettings.java	`85.10% <100.00%> (+1.01%)`	⬆️
...ch/sdk/sample/helloworld/rest/RestHelloAction.java	`80.85% <100.00%> (ø)`

... and 2 files with indirect coverage changes

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

cwperks · 2023-06-07T14:59:59Z

Can the maintainers take another look at this PR?

dbwiddis

I think this is overly complex. Details below.

dbwiddis · 2023-06-10T22:14:10Z

src/main/java/org/opensearch/sdk/ExtensionRouteHandler.java

+ * compatible open source license.
+ */
+
+package org.opensearch.sdk;


If this class stays, it should not be in the base package. Probably would fit in the .rest subpackage.

dbwiddis · 2023-06-10T22:16:12Z

src/main/java/org/opensearch/sdk/ExtensionRouteHandler.java

+        String path,
+        Function<RestRequest, ExtensionRestResponse> handler
+    ) {
+        super(ExtensionRouteHandlerFactory.getInstance().generateRouteName(handlerName), method, path, handler);


I'm trying to understand why we need an entire (sub)class just to call a generator function for one parameter. I don't think this class is necessary; we should just stick in an appropriate (probably static) method that does this name generation whenever we call new RouteHandler() from an extension.

dbwiddis · 2023-06-10T22:21:16Z

src/main/java/org/opensearch/sdk/ExtensionRouteHandlerFactory.java

+     * @return Returns a name prepended with the extension's shortName
+     */
+    public String generateRouteName(String handlerName) {
+        return extensionShortName + ":" + handlerName;


This method call doesn't check whether extensionShortName has been initialized. ExtensionRouteHandlerFactory.getInstance().generateRouteName() will throw an exception.

There's no documentation in this method that it requires initialization first.

This seems like a whole lot of class overhead, initializations, and initialization checks to create a factory object whose only purpose seems to be to prepend one string to another, that seems to be possible with a simple static method somewhere.

dbwiddis · 2023-06-10T22:27:15Z

src/main/java/org/opensearch/sdk/ExtensionSettings.java

 public class ExtensionSettings {

    private String extensionName;
+    private String shortExtensionName;


Given that this is used for singleton security setup later, should it be final?

dbwiddis · 2023-06-10T22:27:29Z

src/main/java/org/opensearch/sdk/ExtensionSettings.java

    private String hostPort;
    private String opensearchAddress;
    private String opensearchPort;
+    private Map<String, String> securitySettings;


Should this also be final?

dbwiddis · 2023-06-10T23:59:08Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

        logger.info("SSL is " + sslText + " for transport");
        this.settings = settingsBuilder.build();

+        if (extensionSettings.getShortExtensionName() != null) {


When is it possible for this to be null? It looks like you've required it in all the ExtensionSettings constructors.

Assuming you allow someone to pass null to the constructor, then not including it means the factory is never initialized (assuming we need a factory) which means that all attempts to generate a route name without checking for the initialization will throw an exception.

dbwiddis · 2023-06-11T00:01:43Z

src/main/java/org/opensearch/sdk/ExtensionsRunner.java

+        if (extensionSettings.getShortExtensionName() != null) {
+            // initialize ExtensionRouteHandlerFactory
+            ExtensionRouteHandlerFactory factory = ExtensionRouteHandlerFactory.getInstance();
+            if (!factory.isInitialized()) {


I'm trying to think of a situation where this would ever be initialized before the ExtensionsRunner constructor was called. Possible but someone would have to go out of their way to do it.

Not sure we need a factory at all, and this is just one more reason this solution is overly complex.

dbwiddis · 2023-06-11T00:05:38Z

src/main/java/org/opensearch/sdk/rest/ExtensionRestPathRegistry.java

+            if (name.isPresent()) {
+                registeredPaths.add(restPathToString(method, path, name));
+            } else {
+                registeredPaths.add(restPathToString(method, path, name));


The method called in else is exactly the same as the method called in the isPresent() case. Why does this conditional exist?

Removed this conditional

dbwiddis · 2023-06-11T00:10:18Z

src/main/java/org/opensearch/sdk/rest/ExtensionRestPathRegistry.java

     * @return A string appending the method and path.
     */
-    public static String restPathToString(Method method, String path) {
+    public static String restPathToString(Method method, String path, Optional<String> name) {


Originally this was a more important method but I think it's been reduced to a cosmetic/logging/exception-message method as the rest paths are stored in the pathTrie. So I think the plain space-delimited text may not be useful or may be more confusing than using some other custom output here.

dbwiddis · 2023-06-11T00:19:29Z

src/main/java/org/opensearch/sdk/sample/helloworld/rest/RestHelloAction.java

-            new RouteHandler(POST, "/hello", handlePostRequest),
-            new RouteHandler(PUT, "/hello/{name}", handlePutRequest),
-            new RouteHandler(DELETE, "/goodbye", handleDeleteRequest)
+            new ExtensionRouteHandler("greet", GET, "/hello", handleGetRequest),


This appears to be the only case where we are actually using the new ExtensionRouteHandler class.

This method signature is an override that only exists if we are extending BaseExtensionRestHandler so it seems to me we could add all this logic in that superclass. The BaseExtensionRestHandler class could fetch the shortname from the settings and then just expose a protected routePrefix(String name) method that returns "shortname:name". Then this line would just be new RouteHandler(routePrefix("greet"), GET, ...) and you'd save a ton of intermediate objects and initialization checking, etc.

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

cwperks · 2023-06-14T19:52:10Z

@dbwiddis I removed the ExtensionRouteHandler and ExtensionRouteHandlerFactory in favor of your suggestion of including the routePrefix static method in the BaseExtensionRestHandler.

cwperks · 2023-07-05T17:22:32Z

Closing in favor of #827

cwperks added 17 commits March 27, 2023 18:46

WIP on Handler naming and SSL

daf3b2c

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

Add concept of extension shortname via settings

9c49d95

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

WIP on extension ssl

97d2a22

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

Merge branch 'main' into ssl-and-handler-naming

b5703ae

Get registry from runner

36b67cc

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

Read settings from extension config file

9238527

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

Update license headers

a9325d2

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

Run spotlessApply

0a1425b

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

Remove authz changes and only keep TLS

0b634be

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

Remove authz changes and only keep TLS

38baa81

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

Remove authz changes and only keep TLS

d504ed8

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

Remove authz changes and only keep TLS

6826fef

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

Remove authz changes and only keep TLS

0825b74

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

Update cert generation documents

5afe8c8

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

Merge branch 'setup-extension-tls' into ssl-and-handler-naming

ef085ce

Re-add authz changes for sample Hello world extension

4966514

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

Add ssl.transport.enabled in ExtensionsRunner

b287fe5

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

github-actions bot added first-time-contributor backport 1.x labels Mar 29, 2023

Name all HelloWorld extension routes

ff158f6

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

cwperks mentioned this pull request Mar 29, 2023

Authorize Requests bound for extensions in the REST-Layer opensearch-project/security#2601

Closed

3 tasks

cwperks added 4 commits March 29, 2023 15:04

Run spotlessApply

a80cae7

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

Merge branch 'main' into setup-extension-tls

636bcdc

Merge main into branch

b08d018

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

Merge branch 'setup-extension-tls' into ssl-and-handler-naming

1c23d9c

cwperks added 4 commits April 1, 2023 22:00

Add instructions for running in SSL only mode

e45ea0e

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

Merge branch 'main' into setup-extension-tls

10127c4

Add all SSL settings to extension settings

1eeafe6

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

Merge branch 'setup-extension-tls' into ssl-and-handler-naming

f4ab756

cwperks added 2 commits May 8, 2023 14:58

Remove extension: from action naming

a926392

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

Merge branch 'main' into ssl-and-handler-naming

f42db71

cwperks mentioned this pull request May 11, 2023

Create NamedRoute to map extension routes to a shortened name opensearch-project/OpenSearch#6870

Merged

6 tasks

vibrantvarun reviewed May 16, 2023

View reviewed changes

owaiskazi19 reviewed May 16, 2023

View reviewed changes

cwperks added 9 commits May 19, 2023 08:46

Merge branch 'main' into ssl-and-handler-naming

06f8e98

Add javadoc

024aace

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

Fix test compilation errors

9658efe

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

Consolidate registerHandler

7aeef67

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

Fix missed registerHandler usage

786f01b

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

Fix javadoc

70d9088

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

Add method to check if class is initialized

80ba90c

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

Fix failing tests

1e06d01

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

Update helloworld-settings

e0df300

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

cwperks mentioned this pull request Jun 1, 2023

Authorize rest requests opensearch-project/security#2753

Merged

3 tasks

cwperks added 2 commits June 7, 2023 09:44

Merge branch 'main' into ssl-and-handler-naming

058f12a

Run spotlessApply

433a42a

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

dbwiddis requested changes Jun 11, 2023

View reviewed changes

cwperks added 3 commits June 13, 2023 16:00

Merge branch 'main' into ssl-and-handler-naming

20885c0

Run spotlessApply

ba60cb7

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

Add shortExtensionName to BaseExtensionRouteHandler

f460699

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

DarshitChanpura mentioned this pull request Jun 20, 2023

Register new routes via SDK as named routes #827

Merged

Merge branch 'main' into ssl-and-handler-naming

6d2ee04

This was referenced Jul 1, 2023

Support transport action names when registering NamedRoutes opensearch-project/OpenSearch#7957

Merged

[FEATURE] Create an ExtensionRequestHandler class to pair with NamedRoutes #856

Open

cwperks closed this Jul 5, 2023

Conversation

cwperks commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Uh oh!

owaiskazi19 commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cwperks commented Jun 7, 2023

Uh oh!

dbwiddis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks commented Jun 14, 2023

Uh oh!

cwperks commented Jul 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cwperks commented Mar 29, 2023 •

edited

Loading

owaiskazi19 commented Mar 31, 2023 •

edited

Loading

cwperks May 19, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading