-
Notifications
You must be signed in to change notification settings - Fork 141
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
Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors #2772
Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors #2772
Conversation
…IndexDescriptors Signed-off-by: Craig Perkins <[email protected]>
@cwperks The Flint Request Index is another system index. Will it be covered in this PR? |
@noCharger Can you link me to where the index is defined? I created this PR based off of the indices currently tracked by the security repo: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java#L72 |
@noCharger This index? https://github.com/opensearch-project/sql/blob/main/async-query-core/src/main/java/org/opensearch/sql/spark/execution/statestore/OpenSearchStateStoreUtil.java#L18 Should I add |
Yes looks like that should be added |
Yes please, also could you point me if any additional change needed for CI? |
Signed-off-by: Craig Perkins <[email protected]>
Added |
@noCharger On the main branch, security is targeting Java 21. The CI failure is related to the Java version used for CI checks.
|
|
@noCharger I rebased, but there are still CI Check failures. Have you seen these failures before?
ref: https://github.com/opensearch-project/sql/actions/runs/9810713201/job/27092980700?pr=2772 |
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
This reverts commit b35573b.
Signed-off-by: Derek Ho <[email protected]>
@derek-ho @noCharger All CI checks have passed. Can you re-review this PR? |
OpenSearchDataSourceMetadataStorage.DATASOURCE_INDEX_NAME, "SQL DataSources index")); | ||
systemIndexDescriptors.add( | ||
new SystemIndexDescriptor( | ||
SPARK_REQUEST_BUFFER_INDEX_NAME + "*", "SQL Spark Request Buffer index pattern")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noCharger does each datasource create its own request index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it is the case based on:
Line 18 in f241f34
"%s_%s", SPARK_REQUEST_BUFFER_INDEX_NAME, datasourceName.toLowerCase(Locale.ROOT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes each datasource creates an index based on its name. Is it enough to define the pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this registration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opensearch-project/security#4535
Security currently maintains a list of system indices through an OpenSearch setting. This change will allow security to rely on a registry of these indices from the core instead of relying on a setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of this greater effort: opensearch-project/security#4439
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but let's confirm we need the * pattern @noCharger can you take a look here?
…IndexDescriptors (#2772) * Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors Signed-off-by: Craig Perkins <[email protected]> * Add Spark Buffer index pattern Signed-off-by: Craig Perkins <[email protected]> * Upgrade to checkout v4 Signed-off-by: Craig Perkins <[email protected]> * Fix CI issues Signed-off-by: Craig Perkins <[email protected]> * Move env Signed-off-by: Craig Perkins <[email protected]> * Revert "Upgrade to checkout v4" This reverts commit b35573b. --------- Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Derek Ho <[email protected]> Co-authored-by: Derek Ho <[email protected]> (cherry picked from commit d3ecb2d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…IndexDescriptors (#2772) (#2817) * Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors * Add Spark Buffer index pattern * Upgrade to checkout v4 * Fix CI issues * Move env * Revert "Upgrade to checkout v4" This reverts commit b35573b. --------- (cherry picked from commit d3ecb2d) Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Derek Ho <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Derek Ho <[email protected]>
…IndexDescriptors (opensearch-project#2772) * Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors Signed-off-by: Craig Perkins <[email protected]> * Add Spark Buffer index pattern Signed-off-by: Craig Perkins <[email protected]> * Upgrade to checkout v4 Signed-off-by: Craig Perkins <[email protected]> * Fix CI issues Signed-off-by: Craig Perkins <[email protected]> * Move env Signed-off-by: Craig Perkins <[email protected]> * Revert "Upgrade to checkout v4" This reverts commit b35573b. --------- Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Derek Ho <[email protected]> Co-authored-by: Derek Ho <[email protected]>
…IndexDescriptors (opensearch-project#2772) * Register system index descriptors through SystemIndexPlugin.getSystemIndexDescriptors Signed-off-by: Craig Perkins <[email protected]> * Add Spark Buffer index pattern Signed-off-by: Craig Perkins <[email protected]> * Upgrade to checkout v4 Signed-off-by: Craig Perkins <[email protected]> * Fix CI issues Signed-off-by: Craig Perkins <[email protected]> * Move env Signed-off-by: Craig Perkins <[email protected]> * Revert "Upgrade to checkout v4" This reverts commit b35573b. --------- Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Derek Ho <[email protected]> Co-authored-by: Derek Ho <[email protected]>
Description
This PR registers the system indices in this plugin through the SystemIndexPlugin extension point in core. These indices will not be functionally different than they are today, its just a formal registration as a system index.
Issues Resolved
Related to opensearch-project/security#4439
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.