Do not update number of replicas on no indices (#34481)

Today when submitting an update settings request to update the number of
replicas with a wildcard that does not match any indices and allow no
indices is set to true, the request ends up being interpreted as
updating the number of replicas for all indices. That is, consider the
following sequence:

PUT /test-index
{
  "settings": {
    "index.number_of_replicas": 0
  }
}

PUT /non-existent-*/_settings?expand_wildcards=open&allow_no_indices=true
{
  "settings": {
    "index.number_of_replicas": 1
  }
}

GET /test-index/_settings

The latter will show that the number of replicas on test-index is now
one. This is surprising, and should be considered a bug.

The underlying problem here is treating no indices in the underlying
methods used to update the routing table and the metadata as meaning all
indices. This commit takes away this assumption. Tests that relied on
this behavior have been changed to no longer rely on this.

A test for this situation is added in UpdateNumberOfReplicasIT.

Author: jasontedor

server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java

@@ -1034,10 +1034,14 @@ public Builder updateSettings(Settings settings, String... indices) {
             return this;
         }
 
-        public Builder updateNumberOfReplicas(int numberOfReplicas, String... indices) {
-            if (indices == null || indices.length == 0) {
-                indices = this.indices.keys().toArray(String.class);
-            }
+        /**
+         * Update the number of replicas for the specified indices.
+         *
+         * @param numberOfReplicas the number of replicas
+         * @param indices          the indices to update the number of replicas for
+         * @return the builder
+         */
+        public Builder updateNumberOfReplicas(final int numberOfReplicas, final String[] indices) {
             for (String index : indices) {
                 IndexMetaData indexMetaData = this.indices.get(index);
                 if (indexMetaData == null) {

server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java

@@ -457,13 +457,17 @@ public Builder updateNodes(long version, RoutingNodes routingNodes) {
             return this;
         }
 
-        public Builder updateNumberOfReplicas(int numberOfReplicas, String... indices) {
+        /**
+         * Update the number of replicas for the specified indices.
+         *
+         * @param numberOfReplicas the number of replicas
+         * @param indices          the indices to update the number of replicas for
+         * @return the builder
+         */
+        public Builder updateNumberOfReplicas(final int numberOfReplicas, final String[] indices) {
             if (indicesRouting == null) {
                 throw new IllegalStateException("once build is called the builder cannot be reused");
             }
-            if (indices == null || indices.length == 0) {
-                indices = indicesRouting.keys().toArray(String.class);
-            }
             for (String index : indices) {
                 IndexRoutingTable indexRoutingTable = indicesRouting.get(index);
                 if (indexRoutingTable == null) {

server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java

@@ -275,7 +275,7 @@ public void testRoutingTableBuiltMoreThanOnce() {
             assertThat(e.getMessage(), containsString("cannot be reused"));
         }
         try {
-            b.updateNumberOfReplicas(1, "foo");
+            b.updateNumberOfReplicas(1, new String[]{"foo"});
             fail("expected exception");
         } catch (IllegalStateException e) {
             assertThat(e.getMessage(), containsString("cannot be reused"));

server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java

@@ -292,8 +292,8 @@ public void testInSyncIdsNotTrimmedWhenNotGrowing() throws Exception {
 
         logger.info("decrease number of replicas to 0");
         clusterState = ClusterState.builder(clusterState)
-            .routingTable(RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(0, "test").build())
-            .metaData(MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(0, "test")).build();
+            .routingTable(RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(0, new String[]{"test"}).build())
+            .metaData(MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(0, new String[]{"test"})).build();
 
         logger.info("add back node 1");
         clusterState = ClusterState.builder(clusterState).nodes(DiscoveryNodes.builder().add(

server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java

@@ -68,8 +68,10 @@ public void testPreferPrimaryAllocationOverReplicas() {
         }
 
         logger.info("increasing the number of replicas to 1, and perform a reroute (to get the replicas allocation going)");
-        RoutingTable updatedRoutingTable = RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(1).build();
-        metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(1).build();
+        final String[] indices = {"test1", "test2"};
+        RoutingTable updatedRoutingTable =
+                RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(1, indices).build();
+        metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(1, indices).build();
         clusterState = ClusterState.builder(clusterState).routingTable(updatedRoutingTable).metaData(metaData).build();
 
         clusterState = strategy.reroute(clusterState, "reroute");

server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java

@@ -96,8 +96,10 @@ public void testUpdateNumberOfReplicas() {
 
         logger.info("add another replica");
         routingNodes = clusterState.getRoutingNodes();
-        RoutingTable updatedRoutingTable = RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(2).build();
-        metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(2).build();
+        final String[] indices = {"test"};
+        RoutingTable updatedRoutingTable =
+                RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(2, indices).build();
+        metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(2, indices).build();
         clusterState = ClusterState.builder(clusterState).routingTable(updatedRoutingTable).metaData(metaData).build();
 
         assertThat(clusterState.metaData().index("test").getNumberOfReplicas(), equalTo(2));
@@ -143,8 +145,8 @@ public void testUpdateNumberOfReplicas() {
 
         logger.info("now remove a replica");
         routingNodes = clusterState.getRoutingNodes();
-        updatedRoutingTable = RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(1).build();
-        metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(1).build();
+        updatedRoutingTable = RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(1, indices).build();
+        metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(1, indices).build();
         clusterState = ClusterState.builder(clusterState).routingTable(updatedRoutingTable).metaData(metaData).build();
 
         assertThat(clusterState.metaData().index("test").getNumberOfReplicas(), equalTo(1));

server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java

@@ -21,13 +21,15 @@
 
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
 import org.elasticsearch.action.search.SearchResponse;
+import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.cluster.health.ClusterHealthStatus;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.test.ESIntegTestCase;
 
 import java.io.IOException;
+import java.util.EnumSet;
 
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
@@ -274,4 +276,21 @@ public void testUpdateWithInvalidNumberOfReplicas() {
             assertEquals("Failed to parse value [" + value + "] for setting [index.number_of_replicas] must be >= 0", e.getMessage());
         }
     }
+
+    public void testUpdateNumberOfReplicasAllowNoIndices() {
+        createIndex("test-index", Settings.builder().put("index.number_of_replicas", 0).build());
+        final IndicesOptions options =
+                new IndicesOptions(EnumSet.of(IndicesOptions.Option.ALLOW_NO_INDICES), EnumSet.of(IndicesOptions.WildcardStates.OPEN));
+        assertAcked(client()
+                .admin()
+                .indices()
+                .prepareUpdateSettings("non-existent-*")
+                .setSettings(Settings.builder().put("index.number_of_replicas", 1))
+                .setIndicesOptions(options)
+                .get());
+        final int numberOfReplicas = Integer.parseInt(
+                client().admin().indices().prepareGetSettings("test-index").get().getSetting("test-index", "index.number_of_replicas"));
+        assertThat(numberOfReplicas, equalTo(0));
+    }
+
 }