Skip to content

Feat: Make system index auto-expand replicas setting configurable#1746

Open
theundefined wants to merge 1 commit into
opensearch-project:mainfrom
theundefined:feature/configurable-system-index-replicas
Open

Feat: Make system index auto-expand replicas setting configurable#1746
theundefined wants to merge 1 commit into
opensearch-project:mainfrom
theundefined:feature/configurable-system-index-replicas

Conversation

@theundefined

Copy link
Copy Markdown

Description

This PR makes the replica configuration of system indices created by the Security Analytics plugin configurable.

System indices created by the Security Analytics plugin have had hardcoded 'index.auto_expand_replicas' values (such as '0-20' or '0-all'). In large production clusters, this causes indices to replicate across many or all nodes, consuming excessive cluster resources (disk space and JVM heap), while overriding matching index templates configured by administrators.

This commit replaces the hardcoded replica settings with three new dynamic cluster settings:

  • plugins.security_analytics.auto_expand_system_index_replicas (defaults to true)
  • plugins.security_analytics.min_system_index_replicas (defaults to 0)
  • plugins.security_analytics.max_system_index_replicas (defaults to 20)

By setting auto_expand_system_index_replicas to false, administrators can disable programmatic auto-expansion on index creation and allow custom index templates (such as those overriding number_of_replicas or auto_expand_replicas) to be fully respected.

Related Issues

Resolves # [Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

System indices created by the Security Analytics plugin have had hardcoded 'index.auto_expand_replicas' values (such as '0-20' or '0-all'). In large production clusters, this causes indices to replicate across many or all nodes, consuming excessive cluster resources (disk space and JVM heap), while overriding matching index templates configured by administrators.

This commit replaces the hardcoded replica settings with three new dynamic cluster settings:
- plugins.security_analytics.auto_expand_system_index_replicas (defaults to true)
- plugins.security_analytics.min_system_index_replicas (defaults to 0)
- plugins.security_analytics.max_system_index_replicas (defaults to 20)

By setting auto_expand_system_index_replicas to false, administrators can disable programmatic auto-expansion on index creation and allow custom index templates (such as those overriding number_of_replicas or auto_expand_replicas) to be fully respected.

Signed-off-by: theundefined <undefine@aramin.net>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

The .settings(...) call now passes the result of getSystemIndexAutoExpandReplicas(clusterService) for both branches of the ternary, but the surrounding method previously used instance fields minSystemIndexReplicas/maxSystemIndexReplicas that were likely populated from cluster settings updates. If those instance fields are still referenced elsewhere in the class but no longer used to populate the settings, dynamic updates to MIN_SYSTEM_INDEX_REPLICAS/MAX_SYSTEM_INDEX_REPLICAS will only take effect via getSystemIndexAutoExpandReplicas. Confirm that clusterService is non-null in this context (not visible in the diff) — a null clusterService would cause an NPE on every rollover.

        .put("index.auto_expand_replicas", SecurityAnalyticsSettings.getSystemIndexAutoExpandReplicas(clusterService))
        .build():
Settings.builder()
        .put("index.hidden", true)
        .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
        .put("index.auto_expand_replicas", SecurityAnalyticsSettings.getSystemIndexAutoExpandReplicas(clusterService))
Setting Validation

MIN_SYSTEM_INDEX_REPLICAS and MAX_SYSTEM_INDEX_REPLICAS are defined as unbounded intSetting with no validators. A user can set min > max, or negative values, producing an invalid auto_expand_replicas string like "5-0" or "-1-20" that will cause index creation to fail at runtime. Consider adding minValue/maxValue bounds and cross-validation between min and max.

public static final Setting<Integer> MIN_SYSTEM_INDEX_REPLICAS = Setting.intSetting(
        "plugins.security_analytics.min_system_index_replicas",
        0,
        Setting.Property.NodeScope, Setting.Property.Dynamic
);

public static final Setting<Integer> MAX_SYSTEM_INDEX_REPLICAS = Setting.intSetting(
        "plugins.security_analytics.max_system_index_replicas",
        20,
        Setting.Property.NodeScope, Setting.Property.Dynamic
);
Behavior Change

Previously the job index used SecurityAnalyticsPlugin.TIF_JOB_INDEX_SETTING, which may have included additional settings beyond shards/replicas/hidden (e.g., number_of_replicas explicit, or other tuning). The new inline Settings only sets shards, auto_expand_replicas, and hidden. Verify no settings from the original TIF_JOB_INDEX_SETTING constant have been lost.

Settings tifJobIndexSettings = Settings.builder()
        .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
        .put("index.auto_expand_replicas", SecurityAnalyticsSettings.getSystemIndexAutoExpandReplicas(clusterService))
        .put("index.hidden", true)
        .build();
final CreateIndexRequest createIndexRequest = new CreateIndexRequest(SecurityAnalyticsPlugin.JOB_INDEX_NAME).mapping(getIndexMapping())
        .settings(tifJobIndexSettings);
Test Assertion vs. Prior Behavior

The test now asserts "0-20" where it previously asserted "0-all". This confirms a functional behavior change: the TIF job index will now replicate across at most 20 nodes instead of all nodes. On clusters with more than 20 data nodes that relied on 0-all, this reduces replication and could be an unintended regression for existing users after upgrade. Consider documenting this as a breaking change or preserving prior default via migration.

assertEquals("0-20", request.settings().get("index.auto_expand_replicas"));

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add bounds validation to replica settings

The MIN_SYSTEM_INDEX_REPLICAS and MAX_SYSTEM_INDEX_REPLICAS settings lack bounds
validation. Users could set negative values or a min greater than max, causing
invalid auto_expand_replicas values like "5-2" or "-1-20" which will cause index
creation failures. Add minValue and validators to ensure min >= 0 and min <= max.

src/main/java/org/opensearch/securityanalytics/settings/SecurityAnalyticsSettings.java [25-35]

 public static final Setting<Integer> MIN_SYSTEM_INDEX_REPLICAS = Setting.intSetting(
         "plugins.security_analytics.min_system_index_replicas",
-        0,
+        0, 0,
         Setting.Property.NodeScope, Setting.Property.Dynamic
 );
 
 public static final Setting<Integer> MAX_SYSTEM_INDEX_REPLICAS = Setting.intSetting(
         "plugins.security_analytics.max_system_index_replicas",
-        20,
+        20, 0,
         Setting.Property.NodeScope, Setting.Property.Dynamic
 );
Suggestion importance[1-10]: 6

__

Why: Adding a minimum bound of 0 to the replica settings is a reasonable validation that prevents negative values which would cause invalid auto_expand_replicas strings and index creation failures.

Low
Possible issue
Prevent invalid replica range when min>max

If a user sets MIN_SYSTEM_INDEX_REPLICAS greater than MAX_SYSTEM_INDEX_REPLICAS, the
produced string (e.g. "10-5") will be rejected by OpenSearch and index creation will
fail. Guard against this by swapping or clamping the values, or by validating and
falling back to safe defaults.

src/main/java/org/opensearch/securityanalytics/settings/SecurityAnalyticsSettings.java [37-45]

 public static String getSystemIndexAutoExpandReplicas(ClusterService clusterService) {
     if (clusterService.getClusterSettings().get(AUTO_EXPAND_SYSTEM_INDEX_REPLICAS) == true) {
         int min = clusterService.getClusterSettings().get(MIN_SYSTEM_INDEX_REPLICAS);
         int max = clusterService.getClusterSettings().get(MAX_SYSTEM_INDEX_REPLICAS);
+        if (min > max) {
+            max = min;
+        }
         return min + "-" + max;
     } else {
         return "false";
     }
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential issue where min > max would produce an invalid string. However, silently clamping may hide user misconfiguration; validation at setting-time might be preferable.

Low

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.

1 participant