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.

Sign up for GitHub

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

Add changes for Create PIT and Delete PIT rest layer and rest high level client #4064

Merged
merged 23 commits into from
Sep 1, 2022
Merged

Add changes for Create PIT and Delete PIT rest layer and rest high level client #4064

Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
49b3f01
Create and delete pit rest layer changes
bharath-techie Aug 1, 2022
6746775
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 9, 2022
1626cfb
addressing review comments
bharath-techie Aug 9, 2022
3bdf1cc
Marking APIs stable
bharath-techie Aug 10, 2022
6ce972e
Addressing comments
bharath-techie Aug 12, 2022
03785b9
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 16, 2022
60b5552
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 17, 2022
dce02c2
changing action name for delete
bharath-techie Aug 17, 2022
ce306c1
addressing comments
bharath-techie Aug 17, 2022
94b8a6a
Changing client due to change in security action and also fixing conc…
bharath-techie Aug 18, 2022
5203a3c
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 18, 2022
90bdb5f
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 19, 2022
1be4e8b
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 23, 2022
611a43b
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 23, 2022
f1b408c
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 24, 2022
4ec23ca
Fixing rest client tests
bharath-techie Aug 24, 2022
66b0ca7
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 26, 2022
2e97a58
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 28, 2022
7796ace
Changes to make security granular for PIT Ids for delete and get pits…
bharath-techie Aug 28, 2022
40ac831
Fixing build
bharath-techie Aug 29, 2022
fab586c
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 30, 2022
1c714c7
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
bharath-techie Aug 31, 2022
6412c08
addressing review comment
bharath-techie Aug 31, 2022
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
Prev Previous commit
Next Next commit
changing action name for delete 
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
  • Loading branch information
@bharath-techie
bharath-techie committed Aug 17, 2022
commit dce02c22b855b6b06835e007166b89607d3eebff
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/action/search/DeletePitAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
public class DeletePitAction extends ActionType<DeletePitResponse> {

public static final DeletePitAction INSTANCE = new DeletePitAction();
public static final String NAME = "indices:data/read/point_in_time/delete";
public static final String NAME = "cluster:admin/point_in_time/delete";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bharath-techie why it was moved to cluster:admin? It does not look right, the semantics of PIT should be kept under "indices:data/read", like scroll fe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we initially went with "indices:read" , but then during testing, we found out that the user needs " all indices : read" permission to delete PITs. Same case with list all PITs. This is because the API does not have indices as part of request.

We initially went with a scroll model but scroll does not have 'clear scroll' permission assigned to any role other than kibana_server.

For more details : please refer to opensearch-project/security#2033

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be great if you can review the overall security model as well @reta @Bukhtawar :
#3960 --- please review "Security model" section and the examples provided.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bharath-techie why all indices : read" permission is a problem? it does make sense because PIT could be created for any set of indices (same for delete). Having "indices:data/read/point_in_time/create" for create and "cluster:admin/point_in_time/delete" for delete does not look logical to me :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay i understand. but also on the flipside, thing to note is that 'pit search' and 'pit create' has indices as part of the request. So user only needs permission to that particular index.

But if user wants to delete pits or list all pits, user will need 'all indices read' permission.

Are we okay with that ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta @Bukhtawar
I have made the changes in security plugin and opensearch to make PIT security model more granular as discussed which filters PITs based on underlying indices and associated permissions.

https://github.com/opensearch-project/security/pull/2053/files

TransportNodesGetAllPITsAction - this will be the parent action ( not user / rest facing - transport only ) which will belong to 'cluster/admin' action type which initially gets all the active PITs from the cluster.

For Delete all , Delete list of PITs and Get All PITs APIs, security plugin will intercept the request and validate and permit the PITs based on underlying indices permission.

We have a 'custom privilege evaluator' to handle above point in time requests.

Please refer to security PR https://github.com/opensearch-project/security/pull/2053/files to know more about how each API filters the PITs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @bharath-techie , this approach looks much better and cleaner (to me at least)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta Here is my understanding, the current implementation does
REST layer -> security plugin -> Transport(cluster:admin/point_in_time/read_from_nodes), where the security plugin evaluates what PITs should be accessible to the current user, with the caveat that security plugin doesn't have the original request's response listener as a result it requires to block on a latch/interrupt ultimately blocking the request handler thread. While I am not sure how could we implement a callback without blocking threads with security plugin specifically.

Note : the cluster:admin is still a proxy just that security plugin kicks in earlier before cluster:admin is hit with the earlier approach.

An alternative that was also discussed was somewhat similar(with few minor modification), which avoids the blocking nature of call made from the security plugin altogether.

1. User makes Delete all / List all API call
2. Rest Layer makes a call to Transport layer to get all PITs ('cluster:admin/getAllPits') -> 
     2.b. With this call, we will get initial PIT IDs 
3. Transport layer now passes the PIT IDs to Transport delete / get all ('indices:data/read/<action-name>')
    3.b. which can get decoded into indices and can be resolved in security plugin.
4. Only PIT IDs belonging to indices for which the user has permission will be returned back to user (or) deleted in case of delete all.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to this commit for latch usage , we've made changes to ensure that this is avoided , so latest changes don't have it @reta

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to this commit for latch usage , we've made changes to ensure that this is avoided , so latest changes don't have it @reta

Yeah, already saw it, thanks @bharath-techie


private DeletePitAction() {
super(NAME, DeletePitResponse::new);
Expand Down