Address review comments and rebase

Signed-off-by: Suraj Singh <surajrider@gmail.com>
opensearch-project · Dec 20, 2023 · 39d47be · 39d47be
1 parent 20cf129
commit 39d47be
Show file tree

Hide file tree

Showing 6 changed files with 115 additions and 112 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -122,7 +122,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [BWC and API enforcement] Introduce checks for enforcing the API restrictions ([#11175](https://github.com/opensearch-project/OpenSearch/pull/11175))
 - Create separate transport action for render search template action ([#11170](https://github.com/opensearch-project/OpenSearch/pull/11170))
 - Add additional handling in SearchTemplateRequest when simulate is set to true ([#11591](https://github.com/opensearch-project/OpenSearch/pull/11591))
-- Introduce cluster level setting `cluster.force.index.replication_type` that prevents new create index requests when replication type mis-matches with cluster level replication `cluster.indices.replication.strategy`([#11583](https://github.com/opensearch-project/OpenSearch/pull/11583))
+- Introduce cluster level setting `cluster.force.index.replication.type` to prevent replication type setting override during index creations([#11583](https://github.com/opensearch-project/OpenSearch/pull/11583))
 
 ### Dependencies
 - Bump Lucene from 9.7.0 to 9.8.0 ([10276](https://github.com/opensearch-project/OpenSearch/pull/10276))

diff --git a/...usterTest/java/org/opensearch/indices/replication/SegmentReplicationClusterSettingIT.java b/...usterTest/java/org/opensearch/indices/replication/SegmentReplicationClusterSettingIT.java
@@ -45,7 +45,7 @@ public class SegmentReplicationClusterSettingIT extends OpenSearchIntegTestCase
     protected static final int REPLICA_COUNT = 1;
 
     protected static final String REPLICATION_MISMATCH_VALIDATION_ERROR =
-        "Validation Failed: 1: index setting [index.replication.type] is not allowed to be set as [cluster.force.index.replication_type=true];";
+        "Validation Failed: 1: index setting [index.replication.type] is not allowed to be set as [cluster.force.index.replication.type=true];";
 
     @Override
     public Settings indexSettings() {
@@ -142,18 +142,6 @@ public void testDifferentReplicationTypes_CreateIndex_StrictMode() throws Throwa
         executeTest(true, consumer, INDEX_NAME, documentCount);
     }
 
-    public void testDifferentReplicationTypes_CreateIndex_NonStrictMode() throws Throwable {
-        final int documentCount = scaledRandomIntBetween(1, 10);
-        BiConsumer<List<ReplicationType>, List<String>> consumer = (replicationList, dataNodesList) -> {
-            Settings indexSettings = Settings.builder().put(indexSettings()).put(SETTING_REPLICATION_TYPE, replicationList.get(1)).build();
-            createIndex(INDEX_NAME, indexSettings);
-            for (int i = 0; i < documentCount; i++) {
-                client().prepareIndex(INDEX_NAME).setId(String.valueOf(i)).setSource("foo", "bar").get();
-            }
-        };
-        executeTest(false, consumer, INDEX_NAME, documentCount);
-    }
-
     public void testDifferentReplicationTypes_IndexTemplates_StrictMode() throws Throwable {
         final int documentCount = scaledRandomIntBetween(1, 10);
 
@@ -174,30 +162,6 @@ public void testDifferentReplicationTypes_IndexTemplates_StrictMode() throws Thr
         executeTest(true, consumer, INDEX_NAME, documentCount);
     }
 
-    public void testDifferentReplicationTypes_IndexTemplates_NonStrictMode() throws Throwable {
-        final int documentCount = scaledRandomIntBetween(1, 10);
-
-        // Create an index using current cluster level settings
-        BiConsumer<List<ReplicationType>, List<String>> consumer = (replicationList, dataNodesList) -> {
-            client().admin()
-                .indices()
-                .preparePutTemplate("template_1")
-                .setPatterns(Collections.singletonList("test-idx*"))
-                .setSettings(Settings.builder().put(indexSettings()).put(SETTING_REPLICATION_TYPE, replicationList.get(1)).build())
-                .setOrder(0)
-                .get();
-
-            GetIndexTemplatesResponse response = client().admin().indices().prepareGetTemplates().get();
-            assertThat(response.getIndexTemplates(), hasSize(1));
-
-            createIndex(INDEX_NAME);
-            for (int i = 0; i < documentCount; i++) {
-                client().prepareIndex(INDEX_NAME).setId(String.valueOf(i)).setSource("foo", "bar").get();
-            }
-        };
-        executeTest(false, consumer, INDEX_NAME, documentCount);
-    }
-
     public void testMismatchingReplicationType_ResizeAction_StrictMode() throws Throwable {
         // Define resize action and target shard count.
         List<Tuple<ResizeType, Integer>> resizeActionsList = new ArrayList<>();
@@ -264,74 +228,8 @@ public void testMismatchingReplicationType_ResizeAction_StrictMode() throws Thro
         executeTest(true, consumer, targetIndexName, documentCount);
     }
 
-    public void testMismatchingReplicationType_ResizeAction_NonStrictMode() throws Throwable {
-        final int initialShardCount = 2;
-        // Define resize action and target shard count.
-        List<Tuple<ResizeType, Integer>> resizeActionsList = new ArrayList<>();
-        resizeActionsList.add(new Tuple<>(ResizeType.SPLIT, 2 * initialShardCount));
-        resizeActionsList.add(new Tuple<>(ResizeType.SHRINK, SHARD_COUNT));
-        resizeActionsList.add(new Tuple<>(ResizeType.CLONE, initialShardCount));
-
-        Tuple<ResizeType, Integer> resizeActionTuple = resizeActionsList.get(random().nextInt(resizeActionsList.size()));
-        final String targetIndexName = resizeActionTuple.v1().name().toLowerCase(Locale.ROOT) + "-target";
-        final int documentCount = scaledRandomIntBetween(1, 10);
-
-        logger.info("--> Performing resize action {} with shard count {}", resizeActionTuple.v1(), resizeActionTuple.v2());
-
-        // Create an index using current cluster level settings
-        BiConsumer<List<ReplicationType>, List<String>> consumer = (replicationList, dataNodesList) -> {
-            Settings indexSettings = Settings.builder()
-                .put(indexSettings())
-                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, initialShardCount)
-                .put(SETTING_REPLICATION_TYPE, replicationList.get(0))
-                .build();
-
-            createIndex(INDEX_NAME, indexSettings);
-
-            for (int i = 0; i < documentCount; i++) {
-                client().prepareIndex(INDEX_NAME).setId(String.valueOf(i)).setSource("foo", "bar").get();
-            }
-
-            // Block writes
-            client().admin()
-                .indices()
-                .prepareUpdateSettings(INDEX_NAME)
-                .setSettings(Settings.builder().put("index.blocks.write", true))
-                .get();
-            ensureGreen();
-
-            try {
-                for (String node : dataNodesList) {
-                    assertBusy(() -> {
-                        assertHitCount(
-                            client(node).prepareSearch(INDEX_NAME).setSize(100).setQuery(new TermsQueryBuilder("foo", "bar")).get(),
-                            documentCount
-                        );
-                    }, 30, TimeUnit.SECONDS);
-                }
-            } catch (Exception e) {
-                throw new RuntimeException(e);
-            }
-
-            client().admin()
-                .indices()
-                .prepareResizeIndex(INDEX_NAME, targetIndexName)
-                .setResizeType(resizeActionTuple.v1())
-                .setSettings(
-                    Settings.builder()
-                        .put("index.number_of_replicas", 0)
-                        .put("index.number_of_shards", resizeActionTuple.v2())
-                        .putNull("index.blocks.write")
-                        .put(SETTING_REPLICATION_TYPE, replicationList.get(1))
-                        .build()
-                )
-                .get();
-        };
-        executeTest(false, consumer, targetIndexName, documentCount);
-    }
-
     // Creates a cluster with mis-matching cluster level and index level replication strategies and validates that index
-    // creation fails when cluster level setting `cluster.force.index.replication_type` is set to true and creation goes
+    // creation fails when cluster level setting `cluster.force.index.replication.type` is set to true and creation goes
     // through when it is false.
     private void executeTest(
         boolean restrictIndexLevelReplicationTypeSetting,

diff --git a/...r/src/internalClusterTest/java/org/opensearch/snapshots/SegmentReplicationSnapshotIT.java b/...r/src/internalClusterTest/java/org/opensearch/snapshots/SegmentReplicationSnapshotIT.java
@@ -49,7 +49,7 @@ public class SegmentReplicationSnapshotIT extends AbstractSnapshotIntegTestCase
     private static final String SNAPSHOT_NAME = "test-segrep-snapshot";
 
     protected static final String REPLICATION_MISMATCH_VALIDATION_ERROR =
-        "Validation Failed: 1: index setting [index.replication.type] is not allowed to be set as [cluster.force.index.replication_type=true];";
+        "Validation Failed: 1: index setting [index.replication.type] is not allowed to be set as [cluster.force.index.replication.type=true];";
 
     public Settings segRepEnableIndexSettings() {
         return getShardSettings().put(IndexMetadata.SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT).build();
@@ -314,7 +314,7 @@ public void testSnapshotRestoreOnIndexWithSegRepClusterSetting() throws Exceptio
     /**
     * 1. Create index in DOCUMENT replication type
     * 2. Snapshot index
-    * 3. Add new set of nodes with `cluster.indices.replication.strategy` set to SEGMENT and `cluster.force.index.replication_type`
+    * 3. Add new set of nodes with `cluster.indices.replication.strategy` set to SEGMENT and `cluster.force.index.replication.type`
     *    set to true.
     * 4. Perform restore on new set of nodes to validate restored index has `DOCUMENT` replication.
     */

diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
@@ -1332,7 +1332,7 @@ private static List<String> validateIndexCustomPath(Settings settings, @Nullable
 
     /**
      * Validates {@code index.replication.type} is matches with cluster level setting {@code cluster.indices.replication.strategy}
-     * when {@code cluster.force.index.replication_type} is set to true.
+     * when {@code cluster.force.index.replication.type} is set to true.
      *
      * @param requestSettings settings passed in during index create request
      * @param clusterSettings cluster setting

diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java
@@ -302,12 +302,13 @@ public class IndicesService extends AbstractLifecycleComponent
     );
 
     /**
-     * This setting is used to prevents creation of indices where the 'index.replication.type' setting does not match
-     * with the replication type index setting, set at cluster level i.e. 'cluster.indices.replication.strategy'.
-     * If disabled, the replication type can be specified.
+     * If enabled, this setting enforces that indexes will be created with a replication type matching the cluster setting
+     * defined in cluster.indices.replication.strategy by rejecting any request that specifies a replication type that
+     * does not match the cluster setting. If disabled, a user can choose a replication type on a per-index basis using
+     * the index.replication.type setting.
      */
     public static final Setting<Boolean> CLUSTER_FORCE_INDEX_REPLICATION_TYPE_SETTING = Setting.boolSetting(
-        "cluster.force.index.replication_type",
+        "cluster.force.index.replication.type",
         false,
         Property.NodeScope,
         Property.Final

diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java
@@ -71,6 +71,7 @@
 import org.opensearch.index.mapper.MapperService;
 import org.opensearch.index.query.QueryShardContext;
 import org.opensearch.index.translog.Translog;
+import org.opensearch.indices.IndexCreationException;
 import org.opensearch.indices.IndicesService;
 import org.opensearch.indices.InvalidAliasNameException;
 import org.opensearch.indices.InvalidIndexNameException;
@@ -137,6 +138,7 @@
 import static org.opensearch.index.IndexSettings.INDEX_SOFT_DELETES_SETTING;
 import static org.opensearch.index.IndexSettings.INDEX_TRANSLOG_DURABILITY_SETTING;
 import static org.opensearch.indices.IndicesService.CLUSTER_DEFAULT_INDEX_REFRESH_INTERVAL_SETTING;
+import static org.opensearch.indices.IndicesService.CLUSTER_FORCE_INDEX_REPLICATION_TYPE_SETTING;
 import static org.opensearch.indices.IndicesService.CLUSTER_MINIMUM_INDEX_REFRESH_INTERVAL_SETTING;
 import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_INDEX_RESTRICT_ASYNC_DURABILITY_SETTING;
 import static org.opensearch.indices.IndicesService.CLUSTER_REPLICATION_TYPE_SETTING;
@@ -166,6 +168,9 @@ public class MetadataCreateIndexServiceTests extends OpenSearchTestCase {
     private static final String translogRepositoryNameAttributeKey = NODE_ATTRIBUTES.getKey()
         + REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY;
 
+    final String REPLICATION_MISMATCH_VALIDATION_ERROR =
+        "Validation Failed: 1: index setting [index.replication.type] is not allowed to be set as [cluster.force.index.replication.type=true];";
+
     @Before
     public void setup() throws Exception {
         super.setUp();
@@ -1239,6 +1244,105 @@ public void testIndexTemplateReplicationType() {
         assertEquals(ReplicationType.SEGMENT.toString(), indexSettings.get(INDEX_REPLICATION_TYPE_SETTING.getKey()));
     }
 
+    public void testClusterForceReplicationTypeInAggregateSettings() {
+        Settings settings = Settings.builder()
+            .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT)
+            .put(CLUSTER_FORCE_INDEX_REPLICATION_TYPE_SETTING.getKey(), true)
+            .build();
+        ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
+        Settings matchingReplicationIndexSettings = Settings.builder()
+            .put(INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.DOCUMENT)
+            .build();
+        request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test");
+        request.settings(matchingReplicationIndexSettings);
+        IndexCreationException exception = expectThrows(
+            IndexCreationException.class,
+            () -> aggregateIndexSettings(
+                ClusterState.EMPTY_STATE,
+                request,
+                Settings.EMPTY,
+                null,
+                Settings.EMPTY,
+                IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
+                randomShardLimitService(),
+                Collections.emptySet(),
+                clusterSettings
+            )
+        );
+        assertEquals(REPLICATION_MISMATCH_VALIDATION_ERROR, exception.getCause().getMessage());
+
+        Settings nonMatchingReplicationIndexSettings = Settings.builder()
+            .put(INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT)
+            .build();
+        request.settings(nonMatchingReplicationIndexSettings);
+        Settings aggregateIndexSettings = aggregateIndexSettings(
+            ClusterState.EMPTY_STATE,
+            request,
+            Settings.EMPTY,
+            null,
+            Settings.EMPTY,
+            IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
+            randomShardLimitService(),
+            Collections.emptySet(),
+            clusterSettings
+        );
+        assertEquals(ReplicationType.SEGMENT.toString(), aggregateIndexSettings.get(INDEX_REPLICATION_TYPE_SETTING.getKey()));
+    }
+
+    public void testClusterForceReplicationTypeInValidateIndexSettings() {
+        ClusterService clusterService = mock(ClusterService.class);
+        Metadata metadata = Metadata.builder()
+            .transientSettings(Settings.builder().put(Metadata.DEFAULT_REPLICA_COUNT_SETTING.getKey(), 1).build())
+            .build();
+        ClusterState clusterState = ClusterState.builder(org.opensearch.cluster.ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY))
+            .metadata(metadata)
+            .build();
+        ThreadPool threadPool = new TestThreadPool(getTestName());
+        // Enforce cluster level replication type setting
+        final Settings forceClusterSettingEnabled = Settings.builder()
+            .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT)
+            .put(CLUSTER_FORCE_INDEX_REPLICATION_TYPE_SETTING.getKey(), true)
+            .build();
+        ClusterSettings clusterSettings = new ClusterSettings(forceClusterSettingEnabled, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
+        when(clusterService.getSettings()).thenReturn(forceClusterSettingEnabled);
+        when(clusterService.getClusterSettings()).thenReturn(clusterSettings);
+        when(clusterService.state()).thenReturn(clusterState);
+
+        final MetadataCreateIndexService checkerService = new MetadataCreateIndexService(
+            forceClusterSettingEnabled,
+            clusterService,
+            null,
+            null,
+            null,
+            createTestShardLimitService(randomIntBetween(1, 1000), false, clusterService),
+            new Environment(Settings.builder().put("path.home", "dummy").build(), null),
+            IndexScopedSettings.DEFAULT_SCOPED_SETTINGS,
+            threadPool,
+            null,
+            new SystemIndices(Collections.emptyMap()),
+            true,
+            new AwarenessReplicaBalance(forceClusterSettingEnabled, clusterService.getClusterSettings())
+        );
+        // Use DOCUMENT replication type setting for index creation
+        final Settings indexSettings = Settings.builder().put(INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.DOCUMENT).build();
+
+        IndexCreationException exception = expectThrows(
+            IndexCreationException.class,
+            () -> checkerService.validateIndexSettings("test", indexSettings, false)
+        );
+        assertEquals(REPLICATION_MISMATCH_VALIDATION_ERROR, exception.getCause().getMessage());
+
+        // Cluster level replication type setting not enforced
+        final Settings forceClusterSettingDisabled = Settings.builder()
+            .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT)
+            .put(CLUSTER_FORCE_INDEX_REPLICATION_TYPE_SETTING.getKey(), false)
+            .build();
+        clusterSettings = new ClusterSettings(forceClusterSettingDisabled, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
+        when(clusterService.getClusterSettings()).thenReturn(clusterSettings);
+        checkerService.validateIndexSettings("test", indexSettings, false);
+        threadPool.shutdown();
+    }
+
     public void testRemoteStoreNoUserOverrideExceptReplicationTypeSegmentIndexSettings() {
         Settings settings = Settings.builder()
             .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.DOCUMENT)