From 55dee530462cb077395033564b06dd36ac091f05 Mon Sep 17 00:00:00 2001
From: Jason Tedor <jason@tedor.me>
Date: Mon, 15 Oct 2018 19:49:58 -0400
Subject: [PATCH] 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.
---
 .../cluster/metadata/MetaData.java            | 12 ++++++++----
 .../cluster/routing/RoutingTable.java         | 12 ++++++++----
 .../cluster/routing/RoutingTableTests.java    |  2 +-
 .../allocation/InSyncAllocationIdTests.java   |  4 ++--
 .../PreferPrimaryAllocationTests.java         |  6 ++++--
 .../UpdateNumberOfReplicasTests.java          | 10 ++++++----
 .../settings/UpdateNumberOfReplicasIT.java    | 19 +++++++++++++++++++
 7 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
index 75869b54850..f4eafd05e15 100644
--- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
+++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
@@ -1034,10 +1034,14 @@ public class MetaData implements Iterable<IndexMetaData>, Diffable<MetaData>, To
             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) {
diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java b/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java
index 36c512c17aa..bab150fff12 100644
--- a/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java
+++ b/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java
@@ -457,13 +457,17 @@ public class RoutingTable implements Iterable<IndexRoutingTable>, Diffable<Routi
             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) {
diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java
index 349997d7793..0f552703543 100644
--- a/server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java
+++ b/server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java
@@ -275,7 +275,7 @@ public class RoutingTableTests extends ESAllocationTestCase {
             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"));
diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java
index 5f39336569f..8630d2cc5b6 100644
--- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java
+++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java
@@ -292,8 +292,8 @@ public class InSyncAllocationIdTests extends ESAllocationTestCase {
 
         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(
diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java
index d4e032f4761..35a9be017d5 100644
--- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java
+++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java
@@ -68,8 +68,10 @@ public class PreferPrimaryAllocationTests extends ESAllocationTestCase {
         }
 
         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");
diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java
index 167172ec9bd..3001a4ba9e4 100644
--- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java
+++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java
@@ -96,8 +96,10 @@ public class UpdateNumberOfReplicasTests extends ESAllocationTestCase {
 
         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 class UpdateNumberOfReplicasTests extends ESAllocationTestCase {
 
         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));
diff --git a/server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java b/server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java
index 9b759bff569..100cda1c03c 100644
--- a/server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java
+++ b/server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java
@@ -21,6 +21,7 @@ package org.elasticsearch.indices.settings;
 
 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;
@@ -28,6 +29,7 @@ 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 class UpdateNumberOfReplicasIT extends ESIntegTestCase {
             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));
+    }
+
 }