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.

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

Create concept of persistent ThreadContext headers that are unstashable #8291

Merged
merged 14 commits into from
Jul 6, 2023

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Jun 27, 2023

Description

This PR introduces the concept of "persistent" ThreadContext headers modeled after transient headers. These headers perform exactly the same as ThreadContext headers, but are not stashable and will always be available even inside a block where the ThreadContext has been stashed.

Some additional context on the ThreadContext:

The values in the ThreadContext are immutable after they have already been written to once. If a caller tries to overwrite an existing ThreadContext header than the caller will receive an IllegalArgumentException.

Related Issues

Resolves opensearch-project/security#2909

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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 added 2 commits June 20, 2023 16:54
…nd get durable headers and transient headers

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      2 org.opensearch.index.shard.RemoteStoreRefreshListenerTests.classMethod
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication
      1 org.opensearch.index.shard.RemoteStoreRefreshListenerTests.testRefreshSuccessAfterFailureInFirstAttemptAfterSnapshotAndMetadataUpload

@reta
Copy link
Collaborator

reta commented Jun 29, 2023

@dblock @andrross any concerns folks?

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

This PR is building up features around stashing context, what about eliminating the need for plugins to access this API, what would that take?

This feature adds overhead in latency/bandwidth to every transport request without providing a better security implementation in the short term.

@cwperks
Copy link
Member Author

cwperks commented Jun 30, 2023

This PR is building up features around stashing context, what about eliminating the need for plugins to access this API, what would that take?

@peternied One assurance that plugins cannot read from the persistent headers is that if an object is stored there that is only known to the plugin and no other plugins (i.e. the Security User object), then other plugins cannot read it since their classloaders don't have access to the Security User object.

I am also looking at a more JSM style approach, or similar, that would let only the IdentityPlugin utilize the ThreadContext and no other plugins. Or making it more targeted and forbidding accessing certain headers or areas of the ThreadContext.

This feature adds overhead in latency/bandwidth to every transport request without providing a better security implementation in the short term.

I have not measured that, but I don't anticipate a hit to performance by storing the authenticated user as a persistent header (not currently implemented yet in the security plugin). I will try to gather data on that, but the impact should be marginal.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I don't see any problems here. @peternied do you have strong objections to merging this?

dblock and others added 2 commits July 6, 2023 13:51
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.RemoteStoreIT.testStaleCommitDeletionWithoutInvokeFlush
      1 org.opensearch.index.shard.RemoteStoreRefreshListenerTests.testRefreshSuccessOnThirdAttemptAttempt
      1 org.opensearch.gateway.RecoveryFromGatewayIT.testReuseInFileBasedPeerRecovery
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness
      1 org.opensearch.aliases.IndexAliasesIT.testSameAlias

@peternied
Copy link
Member

End of the day we've got the performance testing requirements pre-release and we can use that to detect issues, lets lean on those instead of trying to micro-optimize/analyze in this scenario.

Lets merge this and keep iteration - thanks for the contribution @cwperks

@dblock dblock merged commit 2f9728e into opensearch-project:main Jul 6, 2023
@dblock dblock added the backport 2.x Backport to 2.x branch label Jul 6, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-8291-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2f9728e8398e923cbbb93e36dd52c22e02612a8d
# Push it to GitHub
git push --set-upstream origin backport/backport-8291-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-8291-to-2.x.

@dblock
Copy link
Member

dblock commented Jul 6, 2023

will need a manual backport, as usual

@cwperks
Copy link
Member Author

cwperks commented Jul 6, 2023

Creating a manual backport now

cwperks added a commit to cwperks/OpenSearch that referenced this pull request Jul 6, 2023
…le (opensearch-project#8291)

* Add unstashable section of a threadcontext by adding ability to put and get durable headers and transient headers

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add CHANGELOG entry

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove new lines from top of CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Only one threadLocal

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Single putPersistent and getPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove unnecessary line

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Chain putPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
(cherry picked from commit 2f9728e)
@cwperks
Copy link
Member Author

cwperks commented Jul 6, 2023

@dblock Created backport PR: #8507

reta pushed a commit that referenced this pull request Jul 6, 2023
…le (#8291) (#8507)

* Add unstashable section of a threadcontext by adding ability to put and get durable headers and transient headers

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add CHANGELOG entry

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove new lines from top of CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Only one threadLocal

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Single putPersistent and getPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove unnecessary line

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Chain putPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
(cherry picked from commit 2f9728e)
vikasvb90 pushed a commit to raghuvanshraj/OpenSearch that referenced this pull request Jul 12, 2023
…le (opensearch-project#8291)

* Add unstashable section of a threadcontext by adding ability to put and get durable headers and transient headers

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add CHANGELOG entry

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove new lines from top of CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Only one threadLocal

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Single putPersistent and getPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove unnecessary line

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Chain putPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
raghuvanshraj pushed a commit to raghuvanshraj/OpenSearch that referenced this pull request Jul 12, 2023
…le (opensearch-project#8291)

* Add unstashable section of a threadcontext by adding ability to put and get durable headers and transient headers

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add CHANGELOG entry

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove new lines from top of CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Only one threadLocal

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Single putPersistent and getPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove unnecessary line

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Chain putPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
buddharajusahil pushed a commit to buddharajusahil/OpenSearch that referenced this pull request Jul 18, 2023
…le (opensearch-project#8291)

* Add unstashable section of a threadcontext by adding ability to put and get durable headers and transient headers

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add CHANGELOG entry

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove new lines from top of CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Only one threadLocal

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Single putPersistent and getPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove unnecessary line

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Chain putPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Signed-off-by: sahil buddharaju <sahilbud@amazon.com>
baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
…le (opensearch-project#8291)

* Add unstashable section of a threadcontext by adding ability to put and get durable headers and transient headers

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add CHANGELOG entry

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove new lines from top of CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Only one threadLocal

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Single putPersistent and getPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove unnecessary line

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Chain putPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…le (opensearch-project#8291)

* Add unstashable section of a threadcontext by adding ability to put and get durable headers and transient headers

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Add CHANGELOG entry

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove new lines from top of CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Only one threadLocal

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Update CHANGELOG

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Single putPersistent and getPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Remove unnecessary line

Signed-off-by: Craig Perkins <cwperx@amazon.com>

* Chain putPersistent

Signed-off-by: Craig Perkins <cwperx@amazon.com>

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security Information is removed from the TheadContext when plugins call on ThreadContext.stashContext
5 participants