Protect shard splitting from illegal target shards (#27468)

While we have an assertion that checks if the number of routing shards is a multiple
of the number of shards we need a real hard exception that checks this way earlier.
This change adds a check and test that is executed before we create the index.

Relates to #26931

Author: s1monw

core/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java

@@ -1343,6 +1343,12 @@ public static ShardId selectSplitShard(int shardId, IndexMetaData sourceIndexMet
                  + "] must be less that the number of target shards [" + numTargetShards + "]");
         }
         int routingFactor = getRoutingFactor(numSourceShards, numTargetShards);
+        // now we verify that the numRoutingShards is valid in the source index
+        int routingNumShards = sourceIndexMetadata.getRoutingNumShards();
+        if (routingNumShards % numTargetShards != 0) {
+            throw new IllegalStateException("the number of routing shards ["
+                + routingNumShards + "] must be a multiple of the target shards [" + numTargetShards + "]");
+        }
         // this is just an additional assertion that ensures we are a factor of the routing num shards.
         assert getRoutingFactor(numTargetShards, sourceIndexMetadata.getRoutingNumShards()) >= 0;
         return new ShardId(sourceIndexMetadata.getIndex(), shardId/routingFactor);

core/src/test/java/org/elasticsearch/cluster/metadata/IndexMetaDataTests.java

@@ -118,13 +118,16 @@ public void testSelectShrinkShards() {
     }
 
     public void testSelectResizeShards() {
+        int numTargetShards = randomFrom(4, 6, 8, 12);
+
         IndexMetaData split = IndexMetaData.builder("foo")
             .settings(Settings.builder()
                 .put("index.version.created", 1)
                 .put("index.number_of_shards", 2)
                 .put("index.number_of_replicas", 0)
                 .build())
             .creationDate(randomLong())
+            .setRoutingNumShards(numTargetShards * 2)
             .build();
 
         IndexMetaData shrink = IndexMetaData.builder("foo")
@@ -135,7 +138,6 @@ public void testSelectResizeShards() {
                 .build())
             .creationDate(randomLong())
             .build();
-        int numTargetShards = randomFrom(4, 6, 8, 12);
         int shard = randomIntBetween(0, numTargetShards-1);
         assertEquals(Collections.singleton(IndexMetaData.selectSplitShard(shard, split, numTargetShards)),
             IndexMetaData.selectRecoverFromShards(shard, split, numTargetShards));
@@ -173,6 +175,9 @@ public void testSelectSplitShard() {
 
         assertEquals("the number of source shards [2] must be a must be a factor of [3]",
             expectThrows(IllegalArgumentException.class, () -> IndexMetaData.selectSplitShard(0, metaData, 3)).getMessage());
+
+        assertEquals("the number of routing shards [4] must be a multiple of the target shards [8]",
+            expectThrows(IllegalStateException.class, () -> IndexMetaData.selectSplitShard(0, metaData, 8)).getMessage());
     }
 
     public void testIndexFormat() {

core/src/test/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexServiceTests.java

@@ -56,10 +56,12 @@
 public class MetaDataCreateIndexServiceTests extends ESTestCase {
 
     private ClusterState createClusterState(String name, int numShards, int numReplicas, Settings settings) {
+        int numRoutingShards = settings.getAsInt(IndexMetaData.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING.getKey(), numShards);
         MetaData.Builder metaBuilder = MetaData.builder();
         IndexMetaData indexMetaData = IndexMetaData.builder(name).settings(settings(Version.CURRENT)
             .put(settings))
-            .numberOfShards(numShards).numberOfReplicas(numReplicas).build();
+            .numberOfShards(numShards).numberOfReplicas(numReplicas)
+            .setRoutingNumShards(numRoutingShards).build();
         metaBuilder.put(indexMetaData, false);
         MetaData metaData = metaBuilder.build();
         RoutingTable.Builder routingTableBuilder = RoutingTable.builder();
@@ -204,10 +206,13 @@ public void testValidateSplitIndex() {
                 }
             ).getMessage());
 
-
+        int targetShards;
+        do {
+            targetShards = randomIntBetween(numShards+1, 100);
+        } while (isSplitable(numShards, targetShards) == false);
         ClusterState clusterState = ClusterState.builder(createClusterState("source", numShards, 0,
-            Settings.builder().put("index.blocks.write", true).build())).nodes(DiscoveryNodes.builder().add(newNode("node1")))
-            .build();
+            Settings.builder().put("index.blocks.write", true).put("index.number_of_routing_shards", targetShards).build()))
+            .nodes(DiscoveryNodes.builder().add(newNode("node1"))).build();
         AllocationService service = new AllocationService(Settings.builder().build(), new AllocationDeciders(Settings.EMPTY,
             Collections.singleton(new MaxRetryAllocationDecider(Settings.EMPTY))),
             new TestGatewayAllocator(), new BalancedShardsAllocator(Settings.EMPTY), EmptyClusterInfoService.INSTANCE);
@@ -218,10 +223,7 @@ public void testValidateSplitIndex() {
         routingTable = service.applyStartedShards(clusterState,
             routingTable.index("source").shardsWithState(ShardRoutingState.INITIALIZING)).routingTable();
         clusterState = ClusterState.builder(clusterState).routingTable(routingTable).build();
-        int targetShards;
-        do {
-            targetShards = randomIntBetween(numShards+1, 100);
-        } while (isSplitable(numShards, targetShards) == false);
+
         MetaDataCreateIndexService.validateSplitIndex(clusterState, "source", Collections.emptySet(), "target",
             Settings.builder().put("index.number_of_shards", targetShards).build());
     }

rest-api-spec/src/main/resources/rest-api-spec/test/indices.split/10_basic.yml

@@ -1,8 +1,5 @@
 ---
-"Split index via API":
-  - skip:
-      version: " - 6.0.99"
-      reason: Added in 6.1.0
+setup:
   - do:
       indices.create:
         index: source
@@ -33,6 +30,12 @@
         id:    "3"
         body:  { "foo": "hello world 3" }
 
+---
+"Split index via API":
+  - skip:
+      version: " - 6.0.99"
+      reason: Added in 6.1.0
+
   # make it read-only
   - do:
       indices.put_settings:
@@ -97,5 +100,38 @@
   - match: { _id:      "3"     }
   - match: { _source:  { foo: "hello world 3" } }
 
+---
+"Create illegal split indices":
+  - skip:
+      version: " - 6.99.99"
+      reason: fixed in 7.0.0
+
+  # try to do an illegal split with number_of_routing_shards set
+  - do:
+      catch: /illegal_argument_exception/
+      indices.split:
+        index: "source"
+        target: "target"
+        wait_for_active_shards: 1
+        master_timeout: 10s
+        body:
+          settings:
+            index.number_of_replicas: 0
+            index.number_of_shards: 2
+            index.number_of_routing_shards: 4
+
+  # try to do an illegal split with illegal number_of_shards
+  - do:
+      catch: /illegal_state_exception/
+      indices.split:
+        index: "source"
+        target: "target"
+        wait_for_active_shards: 1
+        master_timeout: 10s
+        body:
+          settings:
+            index.number_of_replicas: 0
+            index.number_of_shards: 3
+