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

Add a flag to set whether a system index is readable on SystemIndexDescriptor #17296

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Feb 7, 2025

Description

I'm opening up this PR to start a discussion on this approach to a problem that I am seeing while trying to change over plugins to use the replacement for ThreadContext.stashContext for System index access.

This PR introduces the notion of a readable system index. readable means that GET and SEARCH operations would be permitted on the index, but write operations would be reserved for the system.

This PR is a start to some much needed code hygiene cleanup to start removing the this request accesses system indices: [{}], but in a future major version, direct access to system indices will be prevented by default which have been present since the fork.

From the core's perspective, the only behavior this PR will change is that the message above will not be logged for search operations that cover readable system indices since that would be officially supported.

When the security plugin is installed, there would be a more noticeable difference in behavior between readable system indices and non-readable ones.

  • non-readable system indices - This is the current behavior where security scrubs the results of GET and SEARCH operations on these indices. The reason for this is that the security index can contain secrets and this logic is used to scrub any results
  • readable system index - This would be new behavior that would be officially supported where a plugin dev can specify whether a system index is readable when defining system indices using SystemIndexPlugin.getSystemIndexDescriptors. A couple example use-cases:
  1. The security auditlog when using an internal opensearch index. This is an index that the security plugin writes to, but users should be permitted to query it
  2. Anomaly Detection has a notion of a custom results index where they store results information and users use these indices to make dashboards

This PR is an alternative to #15778

Related Issues

Resolves opensearch-project/security#2487

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

@github-actions github-actions bot added >upgrade Label used when upgrading library dependencies (e.g., Lucene) non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues v2.0.0 Version 2.0.0 labels Feb 7, 2025
@cwperks
Copy link
Member Author

cwperks commented Feb 7, 2025

@reta I wanted to run this idea by you. Opening this in draft because I haven't written tests yet, but would like to know what you think about this approach as an alternative to #15778

@cwperks cwperks removed v2.0.0 Version 2.0.0 non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues >upgrade Label used when upgrading library dependencies (e.g., Lucene) labels Feb 7, 2025
@github-actions github-actions bot added >upgrade Label used when upgrading library dependencies (e.g., Lucene) non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues v2.0.0 Version 2.0.0 labels Feb 7, 2025
Copy link
Contributor

github-actions bot commented Feb 7, 2025

✅ Gradle check result for 777fe07: SUCCESS

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 74.28571% with 9 lines in your changes missing coverage. Please review.

Project coverage is 72.40%. Comparing base (77e4112) to head (00ef4ba).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
.../org/opensearch/indices/SystemIndexDescriptor.java 61.53% 3 Missing and 2 partials ⚠️
.../cluster/metadata/IndexNameExpressionResolver.java 83.33% 2 Missing and 1 partial ⚠️
.../example/resthandler/ExampleRestHandlerPlugin.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #17296      +/-   ##
============================================
- Coverage     72.40%   72.40%   -0.01%     
- Complexity    65554    65583      +29     
============================================
  Files          5292     5292              
  Lines        304493   304548      +55     
  Branches      44218    44229      +11     
============================================
+ Hits         220463   220497      +34     
- Misses        65975    66004      +29     
+ Partials      18055    18047       -8     

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

Signed-off-by: Craig Perkins <[email protected]>
Copy link
Contributor

github-actions bot commented Feb 7, 2025

❌ Gradle check result for 9a54a4e: 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 <[email protected]>
@cwperks cwperks removed v2.0.0 Version 2.0.0 non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues >upgrade Label used when upgrading library dependencies (e.g., Lucene) labels Feb 7, 2025
@cwperks cwperks marked this pull request as ready for review February 7, 2025 17:57
@github-actions github-actions bot added >upgrade Label used when upgrading library dependencies (e.g., Lucene) non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues v2.0.0 Version 2.0.0 labels Feb 7, 2025
Copy link
Contributor

github-actions bot commented Feb 7, 2025

❌ Gradle check result for 40f2afc: 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?

Copy link
Contributor

github-actions bot commented Feb 7, 2025

❌ Gradle check result for 00ef4ba: 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?

Copy link
Contributor

github-actions bot commented Feb 8, 2025

❕ Gradle check result for 00ef4ba: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues >upgrade Label used when upgrading library dependencies (e.g., Lucene) v2.0.0 Version 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Deprecate the usage of ThreadContext.stashContext in plugins
1 participant