Skip to content

20517: Fix for security plugin not working - #5989

Closed
Deepti24 wants to merge 9 commits into
opensearch-project:mainfrom
Deepti24:main
Closed

20517: Fix for security plugin not working#5989
Deepti24 wants to merge 9 commits into
opensearch-project:mainfrom
Deepti24:main

Conversation

@Deepti24

@Deepti24 Deepti24 commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Description

This is the fix for the security plugin issue mentioned here: https://github.com/opensearch-project/opensearch/issues/20517
Problem was that security plugin was using restore method from thread context which is overriding the transients (which store span id as well)

Issues Resolved

https://github.com/opensearch-project/opensearch/issues/20517

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

UNIT TESTS:
Added a unit to see if I set span id in transients and call restore and try to set it again later, I will able to generate traces successfully. To avoid static mocking in junits, I have tested required functionality by creating a mocked scenario. Do let me know if I should use static mocking for the same

MANUAL TESTS:
Steps:

  • Built security plugin for 3.5 OS version with main branch in local using command: ./gradlew assemble -Dopensearch.version=3.5.0 -Dbuild.snapshot=false -x test
  • Downloaded Open Search version 3.5 from the internet
  • After unzipping it, moved security plugin jar built in local to its plugin folder. Sample command: unzip -q ~/Documents/CodeBase/OpenSource/opensearch-security/build/distributions/opensearch-security-3.5.0.0-SNAPSHOT.zip -d "$CLUSTER_DIR/$node/opensearch-$OS_VERSION/plugins/opensearch-security"
  • Started Open search in local
  • Added debugger to this part of the code and was able to see trace being propagated
  • Also tried pushing e2e trace to jaegar from open search, was able to see entire trace

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.

@cwperks

cwperks commented Mar 5, 2026

Copy link
Copy Markdown
Member

@Deepti24 I think it makes more sense to add this into the existing allowlist in core here: https://github.com/opensearch-project/OpenSearch/blob/521cef17225014e717e16a414aa50b5753fe043b/server/src/main/java/org/opensearch/tasks/Task.java#L85

@sgup432 iirc did you have a similar PR open to allowlist additional headers related to tracing?

@cwperks

cwperks commented Mar 5, 2026

Copy link
Copy Markdown
Member

@Deepti24 we can actually do this from a plugin as well ActionPlugin.getRestHeaders extension point.

@Deepti24

Deepti24 commented Mar 10, 2026

Copy link
Copy Markdown
Contributor Author

@cwperks I have added relevant changes here as well: opensearch-project/OpenSearch#20819

A new method getTransients is added to ActionPlugin class. We could have reused headers but felt like it would be unclear which fields are transients to recover in security plugin within headers map. Also, responsibility wise both looked different. Do let me know if my understanding is incorrect
Also, have created it to be of type Set transientsToCopy. Do let me know if different object type is supposed to be used

Also, I am signed some unwanted commits in this PR. Have raised new one with only my signed commits
#6000

closing this one

dchauhan3-blip and others added 9 commits March 10, 2026 15:12
Signed-off-by: Deepti Chauhan <dchauhan3@atlassian.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
…-project#5995)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
Signed-off-by: Deepti24 <chauhan.deepti24@gmail.com>
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.

3 participants