diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java index 2d2533500bf9d..24882b03ef721 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/CreateRemoteIndexIT.java @@ -115,7 +115,17 @@ public void testRemoteStoreDisabledByUser() throws Exception { IllegalArgumentException.class, () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() ); - assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); + assertThat( + exc.getMessage(), + containsString( + String.format( + Locale.ROOT, + "Cannot override [%s] settings when [%s] is set to [true].", + SETTING_REMOTE_STORE_ENABLED, + CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey() + ) + ) + ); } public void testRemoteStoreEnabledByUserWithoutRemoteRepoAndSegmentReplicationIllegalArgumentException() throws Exception { @@ -147,7 +157,17 @@ public void testRemoteStoreEnabledByUserWithoutRemoteRepoIllegalArgumentExceptio IllegalArgumentException.class, () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() ); - assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); + assertThat( + exc.getMessage(), + containsString( + String.format( + Locale.ROOT, + "Cannot override [%s] settings when [%s] is set to [true].", + SETTING_REMOTE_STORE_ENABLED, + CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey() + ) + ) + ); } public void testReplicationTypeDocumentByUser() throws Exception { @@ -197,7 +217,7 @@ public void testRemoteStoreSegmentRepoWithoutRemoteEnabledAndSegmentReplicationI ); } - public void testRemoteStoreEnabledByUserWithRemoteRepo() throws Exception { + public void testRemoteStoreEnabledByUserWithRemoteRepoIllegalArgumentException() throws Exception { Settings settings = Settings.builder() .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) @@ -210,7 +230,18 @@ public void testRemoteStoreEnabledByUserWithRemoteRepo() throws Exception { IllegalArgumentException.class, () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() ); - assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); + assertThat( + exc.getMessage(), + containsString( + String.format( + Locale.ROOT, + "Cannot override [%s][%s] settings when [%s] is set to [true].", + SETTING_REMOTE_STORE_ENABLED, + SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, + CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey() + ) + ) + ); } public void testRemoteStoreOverrideOnlyTranslogRepoIllegalArgumentException() throws Exception { @@ -249,7 +280,19 @@ public void testRemoteStoreOverrideTranslogRepoCorrectly() throws Exception { IllegalArgumentException.class, () -> client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get() ); - assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); + assertThat( + exc.getMessage(), + containsString( + String.format( + Locale.ROOT, + "Cannot override [%s][%s][%s] settings when [%s] is set to [true].", + SETTING_REMOTE_STORE_ENABLED, + SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, + SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, + CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey() + ) + ) + ); } public void testRemoteStoreOverrideReplicationTypeIndexSettings() throws Exception { diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index 1ba38daa40566..238a118e06758 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -334,7 +334,7 @@ public Iterator> settings() { /** * Used to specify remote store repository to use for this index. */ - public static final Setting INDEX_REMOTE_STORE_REPOSITORY_SETTING = Setting.simpleString( + public static final Setting INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING = Setting.simpleString( SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, new Setting.Validator<>() { @@ -345,10 +345,12 @@ public void validate(final String value) {} public void validate(final String value, final Map, Object> settings) { if (value == null || value.isEmpty()) { throw new IllegalArgumentException( - "Setting " + INDEX_REMOTE_STORE_REPOSITORY_SETTING.getKey() + " should be provided with non-empty repository ID" + "Setting " + + INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.getKey() + + " should be provided with non-empty repository ID" ); } else { - validateRemoteStoreSettingEnabled(settings, INDEX_REMOTE_STORE_REPOSITORY_SETTING); + validateRemoteStoreSettingEnabled(settings, INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING); } } diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index 3a10f7ad177c2..6d46b8c2aac68 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -119,7 +119,7 @@ import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING; -import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING; +import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_REPLICATION_TYPE_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS; @@ -949,27 +949,39 @@ private static void updateReplicationStrategy(Settings.Builder settingsBuilder, * @param clusterSettings cluster level settings */ private static void updateRemoteStoreSettings(Settings.Builder settingsBuilder, Settings requestSettings, Settings clusterSettings) { - if (CLUSTER_REMOTE_STORE_ENABLED_SETTING.get(clusterSettings) == true) { - // User should not be able to override remote store index settings - if (canCreateRemoteStoreIndex(requestSettings) == false) { - throw new IllegalArgumentException("Cannot override settings related to remote store."); - } + boolean isRemoteStoreClusterEnabled = CLUSTER_REMOTE_STORE_ENABLED_SETTING.get(clusterSettings); + List overriddenSettings = getRemoteStoreOverriddenSetting(requestSettings); + if (overriddenSettings.isEmpty() == false) { + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "Cannot override [%s] settings when [%s] is set to [%s].", + String.join("][", overriddenSettings), + CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey(), + isRemoteStoreClusterEnabled + ) + ); + } + if (isRemoteStoreClusterEnabled == true) { 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)); - } else { - // User should not be able to create remote indices - if (canCreateRemoteStoreIndex(requestSettings) == false) { - throw new IllegalArgumentException(String.format(Locale.ROOT, "Cannot create remote store indices where [%s] is not set", CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey())); - } } } - private static boolean canCreateRemoteStoreIndex(Settings 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; + private static List getRemoteStoreOverriddenSetting(Settings requestSettings) { + List overriddenSettings = new ArrayList<>(); + if (INDEX_REMOTE_STORE_ENABLED_SETTING.exists(requestSettings) == true) { + overriddenSettings.add(SETTING_REMOTE_STORE_ENABLED); + } + if (INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.exists(requestSettings) == true) { + overriddenSettings.add(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY); + } + if (INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.exists(requestSettings) == true) { + overriddenSettings.add(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY); + } + return overriddenSettings; } public static void validateStoreTypeSettings(Settings settings) { diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 3cc7c351fe1bf..be2b5f00bc0ec 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -235,7 +235,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings { FeatureFlags.REMOTE_STORE, List.of( IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING, - IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING, + IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING, IndexMetadata.INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING ), FeatureFlags.CONCURRENT_SEGMENT_SEARCH, diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java index 0e6c1bf8717f5..fe1330b004510 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -1280,7 +1280,16 @@ public void testRemoteStoreDisabledByUserIndexSettings() { Collections.emptySet() ) ); - assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); + assertThat( + exc.getMessage(), + containsString( + String.format( + "Cannot override [%s] settings when [%s] is set to [true].", + SETTING_REMOTE_STORE_ENABLED, + CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey() + ) + ) + ); } public void testRemoteStoreOverrideSegmentRepoIndexSettings() { @@ -1311,7 +1320,17 @@ public void testRemoteStoreOverrideSegmentRepoIndexSettings() { Collections.emptySet() ) ); - assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); + assertThat( + exc.getMessage(), + containsString( + String.format( + "Cannot override [%s][%s] settings when [%s] is set to [true].", + SETTING_REMOTE_STORE_ENABLED, + SETTING_REMOTE_SEGMENT_STORE_REPOSITORY, + CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey() + ) + ) + ); } public void testRemoteStoreOverrideTranslogRepoIndexSettings() { @@ -1340,7 +1359,16 @@ public void testRemoteStoreOverrideTranslogRepoIndexSettings() { Collections.emptySet() ) ); - assertThat(exc.getMessage(), containsString("Cannot override settings related to remote store.")); + assertThat( + exc.getMessage(), + containsString( + String.format( + "Cannot override [%s] settings when [%s] is set to [true].", + SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY, + CLUSTER_REMOTE_STORE_ENABLED_SETTING.getKey() + ) + ) + ); } public void testBuildIndexMetadata() { diff --git a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java index e42e9b4970081..6207d2f2725ad 100644 --- a/server/src/test/java/org/opensearch/index/IndexSettingsTests.java +++ b/server/src/test/java/org/opensearch/index/IndexSettingsTests.java @@ -856,7 +856,7 @@ public void testRemoteRepositoryExplicitSetting() { public void testUpdateRemoteRepositoryFails() { Set> remoteStoreSettingSet = new HashSet<>(); - remoteStoreSettingSet.add(IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING); + remoteStoreSettingSet.add(IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING); IndexScopedSettings settings = new IndexScopedSettings(Settings.EMPTY, remoteStoreSettingSet); SettingsException error = expectThrows( SettingsException.class, @@ -881,7 +881,7 @@ public void testSetRemoteRepositoryFailsWhenRemoteStoreIsNotEnabled() { .build(); IllegalArgumentException iae = expectThrows( IllegalArgumentException.class, - () -> IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING.get(indexSettings) + () -> IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.get(indexSettings) ); assertEquals( String.format( @@ -902,7 +902,7 @@ public void testSetRemoteRepositoryFailsWhenEmptyString() { .build(); IllegalArgumentException iae = expectThrows( IllegalArgumentException.class, - () -> IndexMetadata.INDEX_REMOTE_STORE_REPOSITORY_SETTING.get(indexSettings) + () -> IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.get(indexSettings) ); assertEquals( String.format(