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

Converts .opendistro-job-scheduler-lock index into a system index #474

Merged
merged 5 commits into from
Aug 30, 2023
Merged

Converts .opendistro-job-scheduler-lock index into a system index #474

merged 5 commits into from
Aug 30, 2023

Conversation

joshpalis
Copy link
Member

@joshpalis joshpalis commented Aug 24, 2023

Description

Actioning two issues to convert the .opendistro-job-scheduler-lock index into a system index.
This PR makes the following modifications :

  • makes JobSchedulerPlugin implement the SystemIndexPlugin and overrides the getSystemIndexDescriptors extension point
  • wraps each instance in which the plugin interacts with the system index within the LockService to stash the threadcontext prior to executing a CRUD operation. From a security perspective this allows the plugin to go into a local trusted mode in which they are allowed to interact with the system index

Next steps :

Issues Resolved

#466 - System Index Usage Verification
#305

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

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.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #474 (fac7d7d) into main (2db686b) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main     #474      +/-   ##
============================================
- Coverage     29.19%   29.17%   -0.03%     
  Complexity       98       98              
============================================
  Files            22       22              
  Lines          1185     1186       +1     
  Branches        109      109              
============================================
  Hits            346      346              
- Misses          818      819       +1     
  Partials         21       21              
Files Changed Coverage Δ
...rg/opensearch/jobscheduler/JobSchedulerPlugin.java 31.14% <0.00%> (-0.52%) ⬇️

@joshpalis
Copy link
Member Author

Interestingly the SampleJobRunnerIT tests are failing consistently, but pass locally. I will continue to look into this

Signed-off-by: Joshua Palis <[email protected]>
@vibrantvarun
Copy link
Member

Tests are failing

@cwperks
Copy link
Member

cwperks commented Aug 29, 2023

It may be the case that JobScheduler does not need to wrap calls to interact with the lock system index inside of a threadContext.stashContext { ... } block, but I don't think it would hurt either.

Typically stashContext is used within plugins when handling authenticated requests. (i.e. API Endpoint Handling) which JS does not do to my knowledge.

A good example is Anomaly Detection create detector.

  1. User calls on endpoint to create detector and passes credentials with the request
  2. The security plugin wraps the REST handler, verifies the credentials and authenticates the request. Authentication here means that the security plugin injects the User into the threadcontext
  3. When AD wants to write a document to the detector system index it will stash the threadcontext (effectively switching from the context of the authenticated user into a new trusted context where it can do anything) and index the document into its system index containing the detector definitions.

If JS is always outside of an authenticated user context than stashing the threadcontext could be considered redundant, however, there are other avenues for plugins to inject roles/user into the threadcontext so stashing the context ensures that the block executing code has a clean slate.

@joshpalis
Copy link
Member Author

It may be the case that JobScheduler does not need to wrap calls to interact with the lock system index inside of a threadContext.stashContext { ... } block, but I don't think it would hurt either.

I've observed that the integration tests would fail consistently after this change, at first I considered it might be a transitive error but after a few test runs it hasn't been resolved. I'll opt to revert the changes to stash the thread context

@joshpalis joshpalis merged commit 0cec350 into opensearch-project:main Aug 30, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 30, 2023
* Converts .opendistro-job-scheduler-lock into a system index

Signed-off-by: Joshua Palis <[email protected]>

* testing

Signed-off-by: Joshua Palis <[email protected]>

* setting fail-fast to false

Signed-off-by: Joshua Palis <[email protected]>

* reverting CI/thread context modifications to the lockservice

Signed-off-by: Joshua Palis <[email protected]>

---------

Signed-off-by: Joshua Palis <[email protected]>
(cherry picked from commit 0cec350)
vibrantvarun pushed a commit that referenced this pull request Aug 30, 2023
…) (#478)

* Converts .opendistro-job-scheduler-lock into a system index

Signed-off-by: Joshua Palis <[email protected]>

* testing

Signed-off-by: Joshua Palis <[email protected]>

* setting fail-fast to false

Signed-off-by: Joshua Palis <[email protected]>

* reverting CI/thread context modifications to the lockservice

Signed-off-by: Joshua Palis <[email protected]>

---------

Signed-off-by: Joshua Palis <[email protected]>
(cherry picked from commit 0cec350)

Co-authored-by: Joshua Palis <[email protected]>
joshpalis added a commit to joshpalis/job-scheduler that referenced this pull request Sep 12, 2023
dbwiddis pushed a commit that referenced this pull request Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants