Skip to content

Add plugin system indices to the default restorable list - #6691

Merged
DarshitChanpura merged 3 commits into
opensearch-project:mainfrom
sharathkanaka:dr-restore-default-allowlist
Oct 30, 2026
Merged

DarshitChanpura merged 3 commits into
opensearch-project:mainfrom
sharathkanaka:dr-restore-default-allowlist

Conversation

@sharathkanaka

@sharathkanaka sharathkanaka commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Description

Adds system indices that restore correctly onto a new cluster to the default of plugins.security.system_indices.restore.indices, so a REST admin can restore them without the operator configuring the list. Each one is covered by restore tests in its plugin's repo (same OpenSearch version).

  • alerting: .opendistro-alerting-config, .opendistro-alerting-alerts, .opendistro-alerting-alert-history-*
  • ISM: .opendistro-ism-config
  • LTR: .ltrstore*
  • other system indices of these plugins stay off the list
  • a configured value still replaces the default; the Security configuration index stays excluded
  • document in the README how to see the default list, and that a configured value replaces it

This list is a starting point. Longer term, plugins could declare restore eligibility on their own system index descriptors, so adding a plugin would not need a change in the security plugin.

Issues Resolved

Related to #6553

Testing

  • Unit: SystemIndexRestoreEligibilityHelperTest checks each default entry is restorable and that other system indices are not
  • Integration: SystemIndexRestoreDefaultIntTests (legacy, legacy with system_index permission, V4): with the default list a REST admin restores a listed index (200) and is denied a system index outside it (403)

Check List

  • New functionality includes testing
  • New functionality has been documented
  • 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.

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 4e633d5.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

PathLineSeverityDescription
src/main/java/org/opensearch/security/support/ConfigConstants.java418mediumThe default for SECURITY_SYSTEM_INDICES_RESTORE_INDICES_DEFAULT is changed from an empty list (deny-by-default) to a non-empty list that includes broad wildcard patterns (.opendistro-alerting-alert-history-* and .ltrstore*). Existing clusters that upgrade without explicitly setting this key will silently gain restore permissions for these indices. The change is consistent with the documented intent and tests, but it is a default-permission expansion that operators should consciously accept rather than inherit passively.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 4e633d5)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 Security concerns

Privilege expansion on upgrade:
Users whose roles are listed in plugins.security.restapi.roles_enabled will, after upgrade, gain the ability to restore the newly defaulted system indices (.opendistro-alerting-config, .opendistro-alerting-alerts, .opendistro-alerting-alert-history-*, .opendistro-ism-config, .ltrstore*) without any explicit opt-in by the operator. Operators relying on the previous empty-default behavior should be clearly notified; otherwise this is a silent broadening of restore privileges.

✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Behavior Change

The default of plugins.security.system_indices.restore.indices changes from an empty list to a non-empty list containing alerting, ISM, and LTR system indices. Existing clusters that upgrade and did not explicitly configure this setting will suddenly allow REST admins to restore these indices where they previously could not. This is a user-visible privilege expansion on upgrade; confirm this is intended and that upgrade notes/release notes call it out.

public static final List<String> SECURITY_SYSTEM_INDICES_RESTORE_INDICES_DEFAULT = List.of(
    ".opendistro-alerting-config",
    ".opendistro-alerting-alerts",
    ".opendistro-alerting-alert-history-*",
    ".opendistro-ism-config",
    ".ltrstore*"
);

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5823d21

Comment thread README.md Resolved
@codecov

codecov Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.34%. Comparing base (0f07289) to head (4e633d5).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6691      +/-   ##
==========================================
- Coverage   77.36%   77.34%   -0.03%     
==========================================
  Files         487      487              
  Lines       32331    32331              
  Branches     4886     4886              
==========================================
- Hits        25012    25005       -7     
- Misses       5090     5098       +8     
+ Partials     2229     2228       -1     
Files with missing lines Coverage Δ
...g/opensearch/security/support/ConfigConstants.java 95.65% <100.00%> (ø)

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit e03569d

Sharath Kanaka added 3 commits October 30, 2026 16:00
Signed-off-by: Sharath Kanaka <sharatcr@amazon.com>
Signed-off-by: Sharath Kanaka <sharatcr@amazon.com>
Signed-off-by: Sharath Kanaka <sharatcr@amazon.com>
@sharathkanaka
sharathkanaka force-pushed the dr-restore-default-allowlist branch from e03569d to 4e633d5 Compare October 30, 2026 23:03
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4e633d5

@DarshitChanpura
DarshitChanpura merged commit 3e461cc into opensearch-project:main Oct 30, 2026
68 of 69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants