Wrap interactions with .opendistro-job-scheduler-lock in ThreadContext.stashContext to ensure JS can read and write to the index

[cwperks]

Description

This PR wraps every instance of JS interacting with .opendistro-job-scheduler-lock inside of stashContext which allows the plugin to interact with system indices that have special protections. System indices cannot be inadvertently created, deleted or modified by even the admin user of the cluster which protects the cluster from getting into a corrupted state.

This also makes JS extend SystemIndexPlugin which registers it with core a system index. Done separately in #474

Issues Resolved

#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.

[cwperks]

The failing check ./gradlew :opensearch-job-scheduler-sample-extension:integTest ran successfully on my local machine.

[saratvemulapalli]

@cwperks how does this work if the index exists in the system in older version of the distribution and now moving to system index in version? How does security/OpenSearch handle it?
I dont see problems with JS as the index was a normal index and now the newer version of plugin has a way to access the system index.

[cwperks]

@saratvemulapalli I will create the index as a normal index before installing JS and then install JS with this change and verify that it works as expected. I believe it can be converted without issue, but I'm not positive. Do you know where I can find information on how core treats system indices? I see logic that auto creates system indices if they do not exists, but is there anything else that is special about them in core? In the security plugin what is special about system indices is that no one can delete them unless you connect using admin cert and it matches the admin_dn.

I have not been able to replicate the build issues on my dev machine. I wanted to go through the plugin developer experience of working with security so this looked like a good issue to try.

[cwperks]

I see now, it may not be straightforward converting a regular index into a system index. I'm receiving this error after re-starting the node and designating an existing index as a system index.

[2023-03-23T14:30:42,053][INFO ][o.o.c.r.a.AllocationService] [88665a45db8d.ant.amazon.com] Cluster health status changed from [RED] to [YELLOW] (reason: [shards started [[.opendistro-job-scheduler-lock][0]]])

curl -XGET http://localhost:9200/_cluster/health
{"cluster_name":"opensearch","status":"yellow","timed_out":false,"number_of_nodes":1,"number_of_data_nodes":1,"discovered_master":true,"discovered_cluster_manager":true,"active_primary_shards":1,"active_shards":1,"relocating_shards":0,"initializing_shards":0,"unassigned_shards":1,"delayed_unassigned_shards":0,"number_of_pending_tasks":0,"number_of_in_flight_fetch":0,"task_max_waiting_in_queue_millis":0,"active_shards_percent_as_number":50.0}

Edit: The issue I was facing was due to watermark issues on my dev machine.

[cwperks]

@saratvemulapalli It's possible to give the index system index protection via the security plugin, and ensure that the plugin can write to it via a PR like this, but I can't currently find a way to make it a system index without steps to migrate like deleting the index and having it be recreated. Making it a formal system index involves extending SystemIndexPlugin and overwriting getSystemIndexDescriptors and that comes with some features like having the index automatically be created.

I will close this PR if its not a change that's desired. I wanted to take an issue to get more familiar with JS without taking one of the issues allocated for CCI.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.46%. Comparing base (bbb20a6) to head (21f3bb8).

❗ Current head 21f3bb8 differs from pull request most recent head 082996c

Please upload reports for the commit 082996c to get more accurate results.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #347      +/-   ##
============================================
- Coverage     37.37%   29.46%   -7.92%     
+ Complexity      133       99      -34     
============================================
  Files            22       23       +1     
  Lines          1188     1171      -17     
  Branches        109      109              
============================================
- Hits            444      345      -99     
- Misses          707      805      +98     
+ Partials         37       21      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cwperks]

Re-opening and rebasing this PR. The lock index was marked as a system index using the extension point SystemIndexPlugin.getSystemIndexDescriptor in #474 and maintained backward compatibility.

This PR surrounds calls to the lock index in a ThreadContext.stashContext block to ensure that job scheduler can interact with the index in a trusted context.

Resolves errors seen in #492 after the lock index was added in the security plugin's system index list.

[cwperks]

@prudhvigodithi Can you review this change?

[prudhvigodithi]

Hey @cwperks thanks for the contribution, Coming from this comment I understand its necessary to stash the thread context in JobScheduler for every CRUD operation on the lock index which is done part of this PR, after this PR I assume (please correct me if I'm wrong) the next step is also to update the the SYSTEM_INDICES list to add .opendistro-job-scheduler-lock which was reverted as part of this PR. Once this is merged is it required to modify the integration tests (with security) for other plugins that use the job-scheduler?
@getsaurabh02

[cwperks]

@prudhvigodithi That's correct. As part of opensearch-project/security#4439, I plan to make a change to feed the registry of system indices from core -> security plugin to avoid having to track them in a separate list in the security plugin. The job scheduler's lock index is one of the system indices that's already included in the registry and needs this change to ensure job-scheduler can interact with the index as desired.

This is actually a bug in job-scheduler atm where it will either use the current authenticated user or the roles that were injected into the threadcontext when determining whether a write operation can happen to the lock index. This PR will ensure that job-scheduler will always be able to interact with the index.

[prudhvigodithi]

Thanks @cwperks LGTM, since you are working on adding some unit tests, you can either go in a new PR and add them here.

[prudhvigodithi]

The job scheduler's lock index is one of the system indices that's already included in the registry and needs this change to ensure job-scheduler can interact with the index as desired.

Just curious are we referring to the NamedXContentRegistry and index .opendistro-job-scheduler-lock?

[cwperks]

Just curious are we referring to the NamedXContentRegistry and index .opendistro-job-scheduler-lock?

The registry of system indices meaning that this plugin implements SystemIndexPlugin.getSystemIndexDescriptors().

I'm working on a change to provide protection by default to all plugins that implement SystemIndexPlugin.getSystemIndexDescriptors(), without having to submit a PR to the security repo to add it in the list tracked here: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java#L49-L80