Retrict remote only indices in a remote enabled cluster

Signed-off-by: bansvaru <bansvaru@amazon.com>
linuxpi · Jul 21, 2023 · 8e73972 · 8e73972
1 parent c47ec1b
commit 8e73972
Show file tree

Hide file tree

Showing 4 changed files with 117 additions and 180 deletions.
diff --git a/...nalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexClusterDefaultDocRep.java b/...nalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexClusterDefaultDocRep.java
@@ -16,7 +16,11 @@
 import org.opensearch.indices.replication.common.ReplicationType;
 import org.opensearch.test.OpenSearchIntegTestCase;
 
+import java.util.Locale;
+
 import static org.hamcrest.Matchers.containsString;
+import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED;
+import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE;
 import static org.opensearch.indices.IndicesService.CLUSTER_REPLICATION_TYPE_SETTING;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
 
@@ -44,7 +48,15 @@ public void testDefaultRemoteStoreNoUserOverride() throws Exception {
         );
         assertThat(
             exc.getMessage(),
-            containsString("Cannot enable [index.remote_store.enabled] when [index.replication.type] is DOCUMENT")
+            containsString(
+                String.format(
+                    Locale.ROOT,
+                    "To enable %s, %s should be set to %s",
+                    SETTING_REMOTE_STORE_ENABLED,
+                    SETTING_REPLICATION_TYPE,
+                    ReplicationType.SEGMENT
+                )
+            )
         );
     }
 

diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java
@@ -26,14 +26,13 @@
 import static org.hamcrest.Matchers.containsString;
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED;
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY;
-import static org.opensearch.index.IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING;
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY;
 import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE;
+import static org.opensearch.index.IndexSettings.INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING;
 import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_STORE_REPOSITORY_SETTING;
 import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING;
 import static org.opensearch.indices.IndicesService.CLUSTER_REMOTE_STORE_ENABLED_SETTING;
 import static org.opensearch.indices.IndicesService.CLUSTER_REPLICATION_TYPE_SETTING;
-import static org.opensearch.indices.IndicesService.CLUSTER_SETTING_REPLICATION_TYPE;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
 
 @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST)
@@ -111,20 +110,12 @@ public void testRemoteStoreDisabledByUser() throws Exception {
             .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
             .put(SETTING_REMOTE_STORE_ENABLED, false)
             .build();
-        assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get());
-        GetIndexResponse getIndexResponse = client().admin()
-            .indices()
-            .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true))
-            .get();
-        Settings indexSettings = getIndexResponse.settings().get("test-idx-1");
-        verifyRemoteStoreIndexSettings(
-            indexSettings,
-            "false",
-            null,
-            null,
-            client().settings().get(CLUSTER_SETTING_REPLICATION_TYPE),
-            IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
+
+        IllegalArgumentException exc = expectThrows(
+            IllegalArgumentException.class,
+            () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
         );
+        assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
     }
 
     public void testRemoteStoreEnabledByUserWithoutRemoteRepoAndSegmentReplicationIllegalArgumentException() throws Exception {
@@ -156,16 +147,7 @@ public void testRemoteStoreEnabledByUserWithoutRemoteRepoIllegalArgumentExceptio
             IllegalArgumentException.class,
             () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
         );
-        assertThat(
-            exc.getMessage(),
-            containsString(
-                String.format(
-                    Locale.ROOT,
-                    "Setting %s should be provided with non-empty repository ID",
-                    SETTING_REMOTE_SEGMENT_STORE_REPOSITORY
-                )
-            )
-        );
+        assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
     }
 
     public void testReplicationTypeDocumentByUser() throws Exception {
@@ -174,19 +156,21 @@ public void testReplicationTypeDocumentByUser() throws Exception {
             .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
             .put(SETTING_REPLICATION_TYPE, ReplicationType.DOCUMENT)
             .build();
-        assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get());
-        GetIndexResponse getIndexResponse = client().admin()
-            .indices()
-            .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true))
-            .get();
-        Settings indexSettings = getIndexResponse.settings().get("test-idx-1");
-        verifyRemoteStoreIndexSettings(
-            indexSettings,
-            null,
-            null,
-            null,
-            ReplicationType.DOCUMENT.toString(),
-            IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
+        IllegalArgumentException exc = expectThrows(
+            IllegalArgumentException.class,
+            () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
+        );
+        assertThat(
+            exc.getMessage(),
+            containsString(
+                String.format(
+                    Locale.ROOT,
+                    "To enable %s, %s should be set to %s",
+                    SETTING_REMOTE_STORE_ENABLED,
+                    SETTING_REPLICATION_TYPE,
+                    ReplicationType.SEGMENT
+                )
+            )
         );
     }
 
@@ -222,20 +206,11 @@ public void testRemoteStoreEnabledByUserWithRemoteRepo() throws Exception {
             .put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, "my-custom-repo")
             .build();
 
-        assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get());
-        GetIndexResponse getIndexResponse = client().admin()
-            .indices()
-            .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true))
-            .get();
-        Settings indexSettings = getIndexResponse.settings().get("test-idx-1");
-        verifyRemoteStoreIndexSettings(
-            indexSettings,
-            "true",
-            "my-custom-repo",
-            "my-translog-repo-1",
-            ReplicationType.SEGMENT.toString(),
-            IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
+        IllegalArgumentException exc = expectThrows(
+            IllegalArgumentException.class,
+            () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
         );
+        assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
     }
 
     public void testRemoteStoreOverrideOnlyTranslogRepoIllegalArgumentException() throws Exception {
@@ -270,20 +245,11 @@ public void testRemoteStoreOverrideTranslogRepoCorrectly() throws Exception {
             .put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, "my-custom-repo")
             .put(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, "my-custom-repo")
             .build();
-        assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get());
-        GetIndexResponse getIndexResponse = client().admin()
-            .indices()
-            .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true))
-            .get();
-        Settings indexSettings = getIndexResponse.settings().get("test-idx-1");
-        verifyRemoteStoreIndexSettings(
-            indexSettings,
-            "true",
-            "my-custom-repo",
-            "my-custom-repo",
-            ReplicationType.SEGMENT.toString(),
-            IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
+        IllegalArgumentException exc = expectThrows(
+            IllegalArgumentException.class,
+            () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
         );
+        assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store."));
     }
 
     public void testRemoteStoreOverrideReplicationTypeIndexSettings() throws Exception {
@@ -292,19 +258,21 @@ public void testRemoteStoreOverrideReplicationTypeIndexSettings() throws Excepti
             .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
             .put(SETTING_REPLICATION_TYPE, ReplicationType.DOCUMENT)
             .build();
-        assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get());
-        GetIndexResponse getIndexResponse = client().admin()
-            .indices()
-            .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true))
-            .get();
-        Settings indexSettings = getIndexResponse.settings().get("test-idx-1");
-        verifyRemoteStoreIndexSettings(
-            indexSettings,
-            null,
-            null,
-            null,
-            ReplicationType.DOCUMENT.toString(),
-            IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL
+        IllegalArgumentException exc = expectThrows(
+            IllegalArgumentException.class,
+            () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()
+        );
+        assertThat(
+            exc.getMessage(),
+            containsString(
+                String.format(
+                    Locale.ROOT,
+                    "To enable %s, %s should be set to %s",
+                    SETTING_REMOTE_STORE_ENABLED,
+                    SETTING_REPLICATION_TYPE,
+                    ReplicationType.SEGMENT
+                )
+            )
         );
     }
 

diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
@@ -91,7 +91,6 @@
 import org.opensearch.indices.InvalidIndexNameException;
 import org.opensearch.indices.ShardLimitValidator;
 import org.opensearch.indices.SystemIndices;
-import org.opensearch.indices.replication.common.ReplicationType;
 import org.opensearch.threadpool.ThreadPool;
 
 import java.io.IOException;
@@ -105,7 +104,6 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -954,44 +952,19 @@ private static void updateRemoteStoreSettings(Settings.Builder settingsBuilder,
         if (CLUSTER_REMOTE_STORE_ENABLED_SETTING.get(clusterSettings)) {
             // Verify if we can create a remote store based index based on user provided settings
             if (canCreateRemoteStoreIndex(requestSettings) == false) {
-                return;
+                throw new IllegalArgumentException("Cannot override settings related to remote store.");
             }
 
-            // Verify index has replication type as SEGMENT
-            if (ReplicationType.DOCUMENT.equals(ReplicationType.parseString(settingsBuilder.get(SETTING_REPLICATION_TYPE)))) {
-                throw new IllegalArgumentException(
-                    "Cannot enable ["
-                        + SETTING_REMOTE_STORE_ENABLED
-                        + "] when ["
-                        + SETTING_REPLICATION_TYPE
-                        + "] is "
-                        + ReplicationType.DOCUMENT
-                );
-            }
-
-            settingsBuilder.put(SETTING_REMOTE_STORE_ENABLED, true);
-            String remoteStoreRepo;
-            if (Objects.equals(requestSettings.get(INDEX_REMOTE_STORE_ENABLED_SETTING.getKey()), "true")) {
-                remoteStoreRepo = requestSettings.get(INDEX_REMOTE_STORE_REPOSITORY_SETTING.getKey());
-            } else {
-                remoteStoreRepo = CLUSTER_REMOTE_STORE_REPOSITORY_SETTING.get(clusterSettings);
-            }
-            settingsBuilder.put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, remoteStoreRepo)
-                .put(
-                    SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY,
-                    requestSettings.get(
-                        INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.getKey(),
-                        CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING.get(clusterSettings)
-                    )
-                );
+            settingsBuilder.put(SETTING_REMOTE_STORE_ENABLED, true)
+                .put(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, CLUSTER_REMOTE_STORE_REPOSITORY_SETTING.get(clusterSettings))
+                .put(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING.get(clusterSettings));
         }
     }
 
     private static boolean canCreateRemoteStoreIndex(Settings requestSettings) {
-        return (INDEX_REPLICATION_TYPE_SETTING.exists(requestSettings) == false
-            || INDEX_REPLICATION_TYPE_SETTING.get(requestSettings).equals(ReplicationType.SEGMENT))
-            && (INDEX_REMOTE_STORE_ENABLED_SETTING.exists(requestSettings) == false
-                || INDEX_REMOTE_STORE_ENABLED_SETTING.get(requestSettings));
+        return INDEX_REMOTE_STORE_ENABLED_SETTING.exists(requestSettings) == false
+            && INDEX_REMOTE_STORE_REPOSITORY_SETTING.exists(requestSettings) == false
+            && INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.exists(requestSettings) == false;
     }
 
     public static void validateStoreTypeSettings(Settings settings) {