[Extensions] Renames AD extension routes to NamedRoutes by DarshitChanpura · Pull Request #942 · opensearch-project/anomaly-detection

DarshitChanpura · 2023-07-06T18:39:45Z

Description

As part of an ongoing effort to introduce authorization in REST layer, a concept of NamedRoute was introduced in core. A NamedRoute provides cluster admins an ability to grant/revoke access to specific endpoints in a cluster. This is introduced as an optional feature for plugins, and a mandatory feature for extensions since they don't interact with the cluster at transport layer and so the only way to authorize these requests would be at REST layer. To enforce all routes to be NamedRoute, a change was made in SDK: opensearch-project/opensearch-sdk-java#827 to enforce declaring new routes as NamedRoute.

This PR introduces changes that conform to the changes introduced in SDK.

Example

Consider this url for example: POST /detectors/{detectorId}/profile
This route will now be registered with a route named: ad:detectors/profile considering routeNamePrefix value in settings.yml is set to ad.
Consider this role:

ad_role:
  reserved: true
  cluster_permissions:
    - 'ad:detectors/profile'

And says user X is mapped to this role. Now there are two possible use cases:

Without Security: No change.
With Security:
a. Once [Backport 2.x] Authorize rest requests (#2753) and Enhance RestLayerPrivilegesEvaluator Code Coverage (#3218) security#2956 is merged and released for 2.9, cluster admins can now permission these routes as if they were granting cluster_permission to a transport action. This will be evaluated at REST layer inside security plugin. For the given example, user X will be evaluated against the route /detectors/{detectorId}/profile which is mapped to name ad:detectors/profile, and since user X is mapped to a role ad_role which contains this permission, the user will be successfully evaluated and the request chain will proceed. On other hand, if user was not mapped to this role or if there was a typo mapping the role, the evaluation would fail with 401.

b. If [Backport 2.x] Authorize rest requests (#2753) and Enhance RestLayerPrivilegesEvaluator Code Coverage (#3218) security#2956 does not get released in 2.9 due to internal reviews not being complete on time, then there will not be any change in the way authorization works. It will be business as usual.

Issues Resolved

Related to [META] Authorization in the REST layer security#2590

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.

…lers to use new handler signature from SDK Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2023-07-06T18:42:25Z

CI will be unblocked once opensearch-project/opensearch-sdk-java#827 is merged and an artifact is available.

dbwiddis

LGTM except we don't use star imports (if spotless isn't failing we should make it do so!) and the base URI is "detectors" so the named route should match.

src/main/java/org/opensearch/ad/rest/RestPreviewAnomalyDetectorAction.java

src/main/java/org/opensearch/ad/rest/RestDeleteAnomalyResultsAction.java

src/main/java/org/opensearch/ad/rest/AbstractSearchAction.java

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2023-07-10T17:22:48Z

@dbwiddis The terms "detector/" and "detectors/" seem to be used interchangeably. I've updated the changes to reflect uri for base detectors, but it could be standardized if needed.

codecov · 2023-07-10T17:22:58Z

Codecov Report

Merging #942 (47bdc95) into feature/extensions (766fd6e) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@                   Coverage Diff                    @@
##             feature/extensions     #942      +/-   ##
========================================================
- Coverage                 42.86%   42.79%   -0.07%     
- Complexity                 2420     2421       +1     
========================================================
  Files                       301      301              
  Lines                     18169    18203      +34     
  Branches                   1873     1873              
========================================================
+ Hits                       7788     7790       +2     
- Misses                     9806     9839      +33     
+ Partials                    575      574       -1

Flag	Coverage Δ
plugin	`42.79% <0.00%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...a/org/opensearch/ad/rest/AbstractSearchAction.java	`0.00% <0.00%> (ø)`
...ensearch/ad/rest/RestAnomalyDetectorJobAction.java	`0.00% <0.00%> (ø)`
...earch/ad/rest/RestDeleteAnomalyDetectorAction.java	`0.00% <0.00%> (ø)`
...search/ad/rest/RestDeleteAnomalyResultsAction.java	`0.00% <0.00%> (ø)`
...arch/ad/rest/RestExecuteAnomalyDetectorAction.java	`0.00% <0.00%> (ø)`
...ensearch/ad/rest/RestGetAnomalyDetectorAction.java	`0.00% <0.00%> (ø)`
...search/ad/rest/RestIndexAnomalyDetectorAction.java	`0.00% <0.00%> (ø)`
...arch/ad/rest/RestPreviewAnomalyDetectorAction.java	`0.00% <0.00%> (ø)`
...h/ad/rest/RestSearchAnomalyDetectorInfoAction.java	`0.00% <0.00%> (ø)`
...arch/ad/rest/RestSearchTopAnomalyResultAction.java	`0.00% <0.00%> (ø)`
... and 2 more

... and 1 file with indirect coverage changes

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2023-07-11T16:15:58Z

CI will pass once opensearch-project/opensearch-sdk-java#868 is merged and an artifact is available.

owaiskazi19 · 2023-07-11T18:49:08Z

@DarshitChanpura can you provide an example of the new URl for ad-extension for let's say create detector? Let's add it in the PR description.

owaiskazi19 · 2023-07-11T18:52:26Z

src/main/java/org/opensearch/ad/rest/RestDeleteAnomalyDetectorAction.java

    }

-    private Function<RestRequest, ExtensionRestResponse> handleRequest = (request) -> {
+    private Function<RestRequest, RestResponse> handleRequest = (request) -> {


Trying to understand why are we changing the response type from ExtensionRestResponse to RestResponse?
If this is coming from NameRoute then we should change the return type of prepareRequest to RestResponse as well?

NamedRoute is available to both plugins and extensions, and hence RestResponse instead of ExtensionRestResponse (which inherits RestResponse transitively). prepareRequest should still return ExtensionRestResponse as it is one of the concrete implementation of RestResponse

prepareRequest should still return ExtensionRestResponse as it is one of the concrete implementations of RestResponse and will be interpreted as such when send the response back to user. However, the only use case I see for ExtensionRestResponse is for communication between OS and extension to mark that the content was consumed and a list of consumed content.

@dbwiddis had some thoughts on whether we should keep ExtensionRestResponse or just use BytesRestReponse class

DarshitChanpura · 2023-07-11T19:38:22Z

@DarshitChanpura can you provide an example of the new URl for ad-extension for let's say create detector? Let's add it in the PR description.

I've updated the description.

owaiskazi19

LGTM! Thanks for addressing comments

DarshitChanpura · 2023-08-23T14:10:30Z

Closing this until the work is resumed.

Renames AD extension routes to NamedRoutes and modifies replaced hand…

45c30fd

…lers to use new handler signature from SDK Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura requested review from VijayanB, amitgalitz, dbwiddis, jackiehanyang, jmazanec15, jngz-es, joshpalis, kaituo, ohltyler, owaiskazi19, saratvemulapalli, sean-zheng-amazon, sudiptoguha, vamshin and ylwu-amzn as code owners July 6, 2023 18:39

dbwiddis requested changes Jul 10, 2023

View reviewed changes

Renames route names to conform to uris and avoids star imports

713b2af

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Changes the routePrefix method call to reflect correct name

47bdc95

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura added the v2.9.0 label Jul 11, 2023

DarshitChanpura requested a review from dbwiddis July 11, 2023 18:18

owaiskazi19 reviewed Jul 11, 2023

View reviewed changes

owaiskazi19 approved these changes Jul 11, 2023

View reviewed changes

DarshitChanpura removed the v2.9.0 label Aug 23, 2023

DarshitChanpura closed this Aug 23, 2023

Conversation

DarshitChanpura commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Example

Issues Resolved

Uh oh!

DarshitChanpura commented Jul 6, 2023

Uh oh!

dbwiddis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DarshitChanpura commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

DarshitChanpura commented Jul 11, 2023

Uh oh!

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

Uh oh!

Uh oh!

owaiskazi19 Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura Jul 11, 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!

owaiskazi19 Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owaiskazi19 left a comment

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura commented Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DarshitChanpura commented Jul 6, 2023 •

edited

Loading

DarshitChanpura commented Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 10, 2023 •

edited

Loading

owaiskazi19 commented Jul 11, 2023 •

edited

Loading

DarshitChanpura Jul 11, 2023 •

edited

Loading

DarshitChanpura commented Jul 11, 2023 •

edited

Loading