Make ThreadContext.markAsSystemContext package-private
#14988
Conversation
Signed-off-by: Craig Perkins <cwperx@amazon.com>
cwperks
requested review from
peternied,
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
Signed-off-by: Craig Perkins <cwperx@amazon.com>
|
❌ Gradle check result for bc19108: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for a62b147: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Craig Perkins <craig5008@gmail.com>
Signed-off-by: Craig Perkins <craig5008@gmail.com>
|
❌ Gradle check result for 39a7a0b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 7313e5f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 777a546: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Craig Perkins <craig5008@gmail.com>
|
❌ Gradle check result for 288d8f2: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14988 +/- ##
============================================
+ Coverage 71.78% 71.93% +0.14%
- Complexity 62694 62771 +77
============================================
Files 5160 5162 +2
Lines 294211 294374 +163
Branches 42553 42578 +25
============================================
+ Hits 211212 211769 +557
+ Misses 65599 65163 -436
- Partials 17400 17442 +42 ☔ View full report in Codecov by Sentry. |
| * | ||
| * @opensearch.internal | ||
| */ | ||
| public class InternalThreadContextWrapper { |
There was a problem hiding this comment.
@cwperks definitely +1 for the intent but I don't think the taking implementation path is correct:
- The usage of the
ThreadContextwithin core should be frictionless. TheInternalThreadContextWrapperdoes break this promise, plus the class exposespublic static fromso anyone could doInternalThreadContextWrapper.from(threadPool.getThreadContext());to gain the access - The usage of the
ThreadContextoutside of the core should not be possible. The long term solution for that is using JPMS but meanwhile, we could close it up using security permissions:
private static final Permission ACCESS_SYSTEM_THREAD_CONTEXT_PERMISSION = new RuntimePermission("markAsSystemContext");
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPermission(ACCESS_SYSTEM_THREAD_CONTEXT_PERMISSION);
}
Yes, the compile time checks won't be enforced but runtime checks will be.
There was a problem hiding this comment.
@reta That sounds like a good approach to grant/prohibit the usage of certain methods in this class in the plugin ecosystem.
I decided to take this approach initially to take advantage of the @opensearch.internal annotation so that this repo could use InternalThreadContextWrapper.from(threadPool.getThreadContext());, but plugins cannot. I will look into making this a permission that is granted through the JSM policy file.
There was a problem hiding this comment.
@reta I opened 2 other PRs similar to this one, but they don't make use of this class.
Do you think the other 2 PRs should be updated similarly, or would the changes in those PRs be ok?
There was a problem hiding this comment.
Do you think the other 2 PRs should be updated similarly, or would the changes in those PRs be ok?
Thanks @cwperks , I think if we cannot cleanly seal the ThreadContext methods, we would need same change there (plus, changing the method visibility for public API is breaking).
There was a problem hiding this comment.
I wasn't able to find any usages of those 2 methods outside of the core:
There was a problem hiding this comment.
I mean those are public, right? We sadly don't know who might have been using them (since we don't host all plugins)
There was a problem hiding this comment.
It would be safe to change the modifier on stashWithOrigin because a plugin would still have access to stashContext after that PR and could refactor to 2 lines.
final ThreadContext.StoredContext storedContext = threadContext.stashContext();
threadContext.putTransient(ACTION_ORIGIN_TRANSIENT_NAME, origin);
I think stashAndMergeHeaders would be safe to change based on the usages I see in the core repo, but yes generally changing an access modifier could be breaking.
There was a problem hiding this comment.
I think
stashAndMergeHeaderswould be safe to change based on the usages I see in the core repo, but yes generally changing an access modifier could be breaking.
That's the thing, thank you
|
Closing in favor of #15016 |
Description
The ThreadContext class overly exposes many methods as public. Currently,
markAsSystemContextis publicly exposed. This PR changes the access modifier on this method to package-private to prevent this internally used method from being exposed publicly.This PR creates an Internal wrapper around the ThreadContext called InternalThreadContextWrapper which is marked as
@opensearch.internal. The purpose of this class is to allow the core repo to still have public access to certain methods while preventing plugins from having access to the same methods.Related Issues
Related to #14931
Check List
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.