Settings: validate number_of_shards/number_of_replicas without index setting prefix

Move the validation logic to MetaDataCreateIndexService
Add ShardClusterSnapshotRestoreTests
Add the validation to RestoreService

Closes #10693

src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java

@@ -106,14 +106,6 @@ public class CreateIndexRequest extends AcknowledgedRequest<CreateIndexRequest>
         if (index == null) {
             validationException = addValidationError("index is missing", validationException);
         }
-        Integer number_of_primaries = settings.getAsInt(IndexMetaData.SETTING_NUMBER_OF_SHARDS, null);
-        Integer number_of_replicas = settings.getAsInt(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, null);
-        if (number_of_primaries != null && number_of_primaries <= 0) {
-            validationException = addValidationError("index must have 1 or more primary shards", validationException);
-        }
-        if (number_of_replicas != null && number_of_replicas < 0) {
-            validationException = addValidationError("index must have 0 or more replica shards", validationException);
-        }
         return validationException;
     }
 

src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java

@@ -338,8 +338,7 @@ public class MetaDataCreateIndexService extends AbstractComponent {
                     if (request.index().equals(ScriptService.SCRIPT_INDEX)) {
                         indexSettingsBuilder.put(SETTING_NUMBER_OF_REPLICAS, settings.getAsInt(SETTING_NUMBER_OF_REPLICAS, 0));
                         indexSettingsBuilder.put(SETTING_AUTO_EXPAND_REPLICAS, "0-all");
-                    }
-                    else {
+                    } else {
                         if (indexSettingsBuilder.get(SETTING_NUMBER_OF_REPLICAS) == null) {
                             if (request.index().equals(riverIndexName)) {
                                 indexSettingsBuilder.put(SETTING_NUMBER_OF_REPLICAS, settings.getAsInt(SETTING_NUMBER_OF_REPLICAS, 1));
@@ -426,7 +425,7 @@ public class MetaDataCreateIndexService extends AbstractComponent {
                     }
                     for (Alias alias : request.aliases()) {
                         AliasMetaData aliasMetaData = AliasMetaData.builder(alias.name()).filter(alias.filter())
-                                .indexRouting(alias.indexRouting()).searchRouting(alias.searchRouting()).build();
+                            .indexRouting(alias.indexRouting()).searchRouting(alias.searchRouting()).build();
                         indexMetaDataBuilder.putAlias(aliasMetaData);
                     }
 
@@ -445,11 +444,11 @@ public class MetaDataCreateIndexService extends AbstractComponent {
                     }
 
                     indexService.indicesLifecycle().beforeIndexAddedToCluster(new Index(request.index()),
-                            indexMetaData.settings());
+                        indexMetaData.settings());
 
                     MetaData newMetaData = MetaData.builder(currentState.metaData())
-                            .put(indexMetaData, false)
-                            .build();
+                        .put(indexMetaData, false)
+                        .build();
 
                     logger.info("[{}] creating index, cause [{}], templates {}, shards [{}]/[{}], mappings {}", request.index(), request.cause(), templateNames, indexMetaData.numberOfShards(), indexMetaData.numberOfReplicas(), mappings.keySet());
 
@@ -467,7 +466,7 @@ public class MetaDataCreateIndexService extends AbstractComponent {
 
                     if (request.state() == State.OPEN) {
                         RoutingTable.Builder routingTableBuilder = RoutingTable.builder(updatedState.routingTable())
-                                .addAsNew(updatedState.metaData().index(request.index()));
+                            .addAsNew(updatedState.metaData().index(request.index()));
                         RoutingAllocation.Result routingResult = allocationService.reroute(ClusterState.builder(updatedState).routingTable(routingTableBuilder).build());
                         updatedState = ClusterState.builder(updatedState).routingResult(routingResult).build();
                     }
@@ -554,11 +553,37 @@ public class MetaDataCreateIndexService extends AbstractComponent {
 
     private void validate(CreateIndexClusterStateUpdateRequest request, ClusterState state) throws ElasticsearchException {
         validateIndexName(request.index(), state);
-        String customPath = request.settings().get(IndexMetaData.SETTING_DATA_PATH, null);
+        validateIndexSettings(request.index(), request.settings());
+    }
+
+    public void validateIndexSettings(String indexName, Settings settings) throws IndexCreationException {
+        String customPath = settings.get(IndexMetaData.SETTING_DATA_PATH, null);
+        List<String> validationErrors = Lists.newArrayList();
         if (customPath != null && nodeEnv.isCustomPathsEnabled() == false) {
-            throw new IndexCreationException(new Index(request.index()),
-                    new ElasticsearchIllegalArgumentException("custom data_paths for indices is disabled"));
+            validationErrors.add("custom data_paths for indices is disabled");
+        }
+        Integer number_of_primaries = settings.getAsInt(IndexMetaData.SETTING_NUMBER_OF_SHARDS, null);
+        Integer number_of_replicas = settings.getAsInt(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, null);
+        if (number_of_primaries != null && number_of_primaries <= 0) {
+            validationErrors.add("index must have 1 or more primary shards");
+        }
+        if (number_of_replicas != null && number_of_replicas < 0) {
+           validationErrors.add("index must have 0 or more replica shards");
+        }
+        if (validationErrors.isEmpty() == false) {
+            throw new IndexCreationException(new Index(indexName),
+                new ElasticsearchIllegalArgumentException(getMessage(validationErrors)));
+        }
+    }
+
+    private String getMessage(List<String> validationErrors) {
+        StringBuilder sb = new StringBuilder();
+        sb.append("Validation Failed: ");
+        int index = 0;
+        for (String error : validationErrors) {
+            sb.append(++index).append(": ").append(error).append(";");
         }
+        return sb.toString();
     }
 
     private static class DefaultIndexTemplateFilter implements IndexTemplateFilter {

src/main/java/org/elasticsearch/snapshots/RestoreService.java

@@ -190,6 +190,7 @@ public class RestoreService extends AbstractComponent implements ClusterStateLis
                                 // Index doesn't exist - create it and start recovery
                                 // Make sure that the index we are about to create has a validate name
                                 createIndexService.validateIndexName(renamedIndex, currentState);
+                                createIndexService.validateIndexSettings(renamedIndex, snapshotIndexMetaData.settings());
                                 IndexMetaData.Builder indexMdBuilder = IndexMetaData.builder(snapshotIndexMetaData).state(IndexMetaData.State.OPEN).index(renamedIndex);
                                 indexMdBuilder.settings(ImmutableSettings.settingsBuilder().put(snapshotIndexMetaData.settings()).put(IndexMetaData.SETTING_UUID, Strings.randomBase64UUID()));
                                 if (!request.includeAliases() && !snapshotIndexMetaData.aliases().isEmpty()) {

src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexTests.java

@@ -19,7 +19,7 @@
 
 package org.elasticsearch.action.admin.indices.create;
 
-import org.elasticsearch.action.ActionRequestValidationException;
+import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
@@ -107,34 +107,34 @@ public class CreateIndexTests extends ElasticsearchIntegrationTest{
     public void testInvalidShardCountSettings() throws Exception {
         try {
             prepareCreate("test").setSettings(ImmutableSettings.builder()
-                    .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, randomIntBetween(-10, 0))
-                    .build())
+                .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, randomIntBetween(-10, 0))
+                .build())
             .get();
             fail("should have thrown an exception about the primary shard count");
-        } catch (ActionRequestValidationException e) {
+        } catch (ElasticsearchIllegalArgumentException e) {
             assertThat("message contains error about shard count: " + e.getMessage(),
                     e.getMessage().contains("index must have 1 or more primary shards"), equalTo(true));
         }
 
         try {
             prepareCreate("test").setSettings(ImmutableSettings.builder()
-                    .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, randomIntBetween(-10, -1))
-                    .build())
+                .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, randomIntBetween(-10, -1))
+                .build())
                     .get();
             fail("should have thrown an exception about the replica shard count");
-        } catch (ActionRequestValidationException e) {
+        } catch (ElasticsearchIllegalArgumentException e) {
             assertThat("message contains error about shard count: " + e.getMessage(),
                     e.getMessage().contains("index must have 0 or more replica shards"), equalTo(true));
         }
 
         try {
             prepareCreate("test").setSettings(ImmutableSettings.builder()
-                    .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, randomIntBetween(-10, 0))
-                    .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, randomIntBetween(-10, -1))
-                    .build())
+                .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, randomIntBetween(-10, 0))
+                .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, randomIntBetween(-10, -1))
+                .build())
                     .get();
             fail("should have thrown an exception about the shard count");
-        } catch (ActionRequestValidationException e) {
+        } catch (ElasticsearchIllegalArgumentException e) {
             assertThat("message contains error about shard count: " + e.getMessage(),
                     e.getMessage().contains("index must have 1 or more primary shards"), equalTo(true));
             assertThat("message contains error about shard count: " + e.getMessage(),
@@ -151,4 +151,42 @@ public class CreateIndexTests extends ElasticsearchIntegrationTest{
             setClusterReadOnly(false);
         }
     }
+
+    @Test
+    public void testInvalidShardCountSettingsWithoutPrefix() throws Exception {
+        try {
+            prepareCreate("test").setSettings(ImmutableSettings.builder()
+                .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS.substring(IndexMetaData.INDEX_SETTING_PREFIX.length()), randomIntBetween(-10, 0))
+                .build())
+                .get();
+            fail("should have thrown an exception about the shard count");
+        } catch (ElasticsearchIllegalArgumentException e) {
+            assertThat("message contains error about shard count: " + e.getMessage(),
+                e.getMessage().contains("index must have 1 or more primary shards"), equalTo(true));
+        }
+        try {
+            prepareCreate("test").setSettings(ImmutableSettings.builder()
+                .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS.substring(IndexMetaData.INDEX_SETTING_PREFIX.length()), randomIntBetween(-10, -1))
+                .build())
+                .get();
+            fail("should have thrown an exception about the shard count");
+        } catch (ElasticsearchIllegalArgumentException e) {
+            assertThat("message contains error about shard count: " + e.getMessage(),
+                e.getMessage().contains("index must have 0 or more replica shards"), equalTo(true));
+        }
+        try {
+            prepareCreate("test").setSettings(ImmutableSettings.builder()
+                .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS.substring(IndexMetaData.INDEX_SETTING_PREFIX.length()), randomIntBetween(-10, 0))
+                .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS.substring(IndexMetaData.INDEX_SETTING_PREFIX.length()), randomIntBetween(-10, -1))
+                .build())
+                .get();
+            fail("should have thrown an exception about the shard count");
+        } catch (ElasticsearchIllegalArgumentException e) {
+            assertThat("message contains error about shard count: " + e.getMessage(),
+                e.getMessage().contains("index must have 1 or more primary shards"), equalTo(true));
+            assertThat("message contains error about shard count: " + e.getMessage(),
+                e.getMessage().contains("index must have 0 or more replica shards"), equalTo(true));
+        }
+    }
+
 }

src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasTests.java

@@ -19,10 +19,13 @@
 
 package org.elasticsearch.indices.settings;
 
+import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthStatus;
 import org.elasticsearch.action.count.CountResponse;
+import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.Priority;
+import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.test.ElasticsearchIntegrationTest;
 import org.junit.Test;
 
@@ -263,4 +266,20 @@ public class UpdateNumberOfReplicasTests extends ElasticsearchIntegrationTest {
         assertThat(clusterHealth.getIndices().get("test").getNumberOfReplicas(), equalTo(3));
         assertThat(clusterHealth.getIndices().get("test").getActiveShards(), equalTo(numShards.numPrimaries * 4));
     }
+
+    @Test
+    public void testUpdateWithInvalidNumberOfReplicas() {
+        createIndex("test");
+        try {
+            client().admin().indices().prepareUpdateSettings("test")
+                .setSettings(ImmutableSettings.settingsBuilder()
+                        .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, randomIntBetween(-10, -1))
+                )
+                .execute().actionGet();
+            fail("should have thrown an exception about the replica shard count");
+        } catch (ElasticsearchIllegalArgumentException e) {
+            assertThat("message contains error about shard count: " + e.getMessage(),
+                e.getMessage().contains("the value of the setting index.number_of_replicas must be a non negative integer"), equalTo(true));
+        }
+    }
 }

src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreTests.java

@@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import org.apache.lucene.util.IOUtils;
 import org.apache.lucene.util.LuceneTestCase.Slow;
+import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.ListenableActionFuture;
 import org.elasticsearch.action.admin.cluster.repositories.put.PutRepositoryResponse;
@@ -1636,6 +1637,17 @@ public class SharedClusterSnapshotRestoreTests extends AbstractSnapshotTests {
                 .setIndexSettings(newIncorrectIndexSettings)
                 .setWaitForCompletion(true), SnapshotRestoreException.class);
 
+        logger.info("--> try restoring while changing the number of replicas to a negative number - should fail");
+        Settings newIncorrectReplicasIndexSettings = ImmutableSettings.builder()
+            .put(newIndexSettings)
+            .put(SETTING_NUMBER_OF_REPLICAS.substring(IndexMetaData.INDEX_SETTING_PREFIX.length()), randomIntBetween(-10, -1))
+            .build();
+        assertThrows(client.admin().cluster()
+            .prepareRestoreSnapshot("test-repo", "test-snap")
+            .setIgnoreIndexSettings("index.analysis.*")
+            .setIndexSettings(newIncorrectReplicasIndexSettings)
+            .setWaitForCompletion(true), ElasticsearchIllegalArgumentException.class);
+
         logger.info("--> restore index with correct settings from the snapshot");
         RestoreSnapshotResponse restoreSnapshotResponse = client.admin().cluster()
                 .prepareRestoreSnapshot("test-repo", "test-snap")