Skip to content

Conversation

vamsimanohar
Copy link
Member

Description

[Describe what this change achieves]

  • Category : Enhancement
  • Why these changes are required?
    PPL started using create/delete PIT APIs for execution and ppl_full_access is not reflecting those permissions. This PR address the gap.

Testing

Manual testing done with above permissions and requests are working fine.

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • Commits are signed per the DCO using --signoff

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.

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

@vamsimanohar I'm approving this PR, but I think we should hide the internal implementation from a user by executing these actions in the plugin's system context.

i.e. stash the threadContext then execute the actions

try (ThreadContext.StoredContext ctx = threadContext.stashContext()) {
    ...run actions here...
}

For example, see my core PR to hide the internal scroll implementation of _update_by_query: opensearch-project/OpenSearch#17250

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

@vamsimanohar qq, shouldn't manage_point_in_time action-group cover these? Or do we not want readll and segments permission added as part of refresh?

Also, would you mind adding a CHANGELOG entry for this PR under maintenance section.

Copy link

codecov bot commented Jul 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.73%. Comparing base (5dc83be) to head (e68a26d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5510      +/-   ##
==========================================
- Coverage   72.78%   72.73%   -0.05%     
==========================================
  Files         398      398              
  Lines       24641    24641              
  Branches     3747     3747              
==========================================
- Hits        17934    17923      -11     
- Misses       4878     4891      +13     
+ Partials     1829     1827       -2     

see 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vamsimanohar
Copy link
Member Author

vamsimanohar commented Jul 28, 2025

@vamsimanohar I'm approving this PR, but I think we should hide the internal implementation from a user by executing these actions in the plugin's system context.

i.e. stash the threadContext then execute the actions

try (ThreadContext.StoredContext ctx = threadContext.stashContext()) {
    ...run actions here...
}

For example, see my core PR to hide the internal scroll implementation of _update_by_query: opensearch-project/OpenSearch#17250

@cwperks Yeah, we did this in our plugins for accessing system indices.. Honestly I didn't give a thought on why is PPL plugin using user's permissions to create and delete PITs. Lets keep this PR on hold and see if I can make changes to SQL plugin to use plugin's permissions instead of user's

@vamsimanohar
Copy link
Member Author

@cwperks SQL Plugin has few use cases where PIT management within plugin is not feasible right now.

  • We have support for cross cluster search queries where we are creating PIT across cluster where plugin permissions are not working.
  • We are additionally working on making sql plugin as a library which works on top of RestClient without plugin environment.

These two use cases are making the changes much more involved. So, we still want to continue with existing permission model and revisit at a later time.

@DarshitChanpura ppl_full_access is a default role not an action group?. I didn't get your concern actually? Are you suggesting to use point_in_time action group inside the default role?

@shikharj05
Copy link
Collaborator

@vamsimanohar is this required?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants