Forbid read-only-allow-delete block in blocks API (#58727)

* Forbid read-only-allow-delete block in blocks API

The read-only-allow-delete block is not really under the user's control
since Elasticsearch adds/removes it automatically. This commit removes
support for it from the new API for adding blocks to indices that was
introduced in #58094.

* Missing xref

* Reword paragraph on read-only-allow-delete block

Author: DaveCTurner

docs/reference/index-modules/blocks.asciidoc

@@ -22,16 +22,6 @@ index:
     Set to `true` to make the index and index metadata read only, `false` to
     allow writes and metadata changes.
 
-`index.blocks.read_only_allow_delete`::
-
-    Similar to `index.blocks.read_only`, but also allows deleting the index to
-    make more resources available. The <<disk-based-shard-allocation,disk-based shard
-    allocator>> may add and remove this block automatically.
-
-Deleting documents from an index to release resources - rather than deleting the index itself - can increase the index size over time. When `index.blocks.read_only_allow_delete` is set to `true`, deleting documents is not permitted. However, deleting the index itself releases the read-only index block and makes resources available almost immediately.
-
-IMPORTANT: {es} adds and removes the read-only index block automatically when the disk utilization falls below the high watermark, controlled by <<cluster-routing-flood_stage,cluster.routing.allocation.disk.watermark.flood_stage>>.
-
 `index.blocks.read`::
 
     Set to `true` to disable read operations against the index.
@@ -46,6 +36,26 @@ IMPORTANT: {es} adds and removes the read-only index block automatically when th
 
     Set to `true` to disable index metadata reads and writes.
 
+`index.blocks.read_only_allow_delete`::
+
+    Similar to `index.blocks.read_only`, but also allows deleting the index to
+    make more resources available. The <<disk-based-shard-allocation,disk-based shard
+    allocator>> adds and removes this block automatically.
+
+Deleting documents from an index - rather than deleting the index itself - can
+in fact increase the index size. When you are running out of disk space
+`index.blocks.read_only_allow_delete` is set to `true`, preventing you from
+consuming more disk space by deleting some documents. However, this block does
+permit you to delete the index itself since this does not require any extra
+disk space. When you delete an index the data is removed from disk almost
+immediately, freeing the space it consumes.
+
+IMPORTANT: {es} adds the read-only-allow-delete index block automatically when
+disk utilisation exceeds the <<cluster-routing-flood_stage,flood-stage
+watermark>> and removes it again when disk utilisation is below the
+<<cluster-routing-watermark-high,high watermark>>. You should not apply this
+block yourself.
+
 [discrete]
 [[add-index-block]]
 === Add index block API
@@ -94,11 +104,6 @@ Disable read operations.
 `read_only`::
 Disable write operations and metadata changes.
 
-`read_only_allow_delete`::
-Disable write operations and metadata changes.
-Document deletion is disabled.
-However, index deletion is still allowed.
-
 `write`::
 Disable write operations. However, metadata changes are still allowed.
 ====

docs/reference/modules/cluster/disk_allocator.asciidoc

@@ -13,6 +13,7 @@ file or updated dynamically on a live cluster with the
 
     Defaults to `true`.  Set to `false` to disable the disk allocation decider.
 
+[[cluster-routing-watermark-low]]
 `cluster.routing.allocation.disk.watermark.low`::
 
     Controls the low watermark for disk usage. It defaults to `85%`, meaning
@@ -22,6 +23,7 @@ file or updated dynamically on a live cluster with the
     amount of space is available. This setting has no effect on the primary
     shards of newly-created indices but will prevent their replicas from being allocated.
 
+[[cluster-routing-watermark-high]]
 `cluster.routing.allocation.disk.watermark.high`::
 
     Controls the high watermark. It defaults to `90%`, meaning that

rest-api-spec/src/main/resources/rest-api-spec/api/indices.add_block.json

@@ -19,7 +19,7 @@
             },
             "block":{
               "type":"string",
-              "description":"The block to add (one of read, write, read_only, metadata, read_only_allow_delete)"
+              "description":"The block to add (one of read, write, read_only or metadata)"
             }
           }
         }

server/src/internalClusterTest/java/org/elasticsearch/blocks/SimpleBlocksIT.java

@@ -22,6 +22,7 @@
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.admin.indices.create.CreateIndexResponse;
+import org.elasticsearch.action.admin.indices.readonly.AddIndexBlockRequestBuilder;
 import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsRequestBuilder;
 import org.elasticsearch.action.index.IndexRequestBuilder;
 import org.elasticsearch.action.index.IndexResponse;
@@ -177,6 +178,9 @@ public void testAddBlocksWhileExistingBlocks() {
         ensureGreen("test");
 
         for (APIBlock otherBlock : APIBlock.values()) {
+            if (otherBlock == APIBlock.READ_ONLY_ALLOW_DELETE) {
+                continue;
+            }
 
             for (APIBlock block : Arrays.asList(APIBlock.READ, APIBlock.WRITE)) {
                 try {
@@ -216,20 +220,20 @@ public void testAddBlocksWhileExistingBlocks() {
 
     public void testAddBlockToMissingIndex() {
         IndexNotFoundException e = expectThrows(IndexNotFoundException.class, () -> client().admin().indices()
-            .prepareAddBlock(randomFrom(APIBlock.values()),"test").get());
+            .prepareAddBlock(randomAddableBlock(), "test").get());
         assertThat(e.getMessage(), is("no such index [test]"));
     }
 
     public void testAddBlockToOneMissingIndex() {
         createIndex("test1");
         final IndexNotFoundException e = expectThrows(IndexNotFoundException.class,
-            () -> client().admin().indices().prepareAddBlock(randomFrom(APIBlock.values()),"test1", "test2").get());
+            () -> client().admin().indices().prepareAddBlock(randomAddableBlock(), "test1", "test2").get());
         assertThat(e.getMessage(), is("no such index [test2]"));
     }
 
     public void testCloseOneMissingIndexIgnoreMissing() throws Exception {
         createIndex("test1");
-        final APIBlock block = randomFrom(APIBlock.values());
+        final APIBlock block = randomAddableBlock();
         try {
             assertBusy(() -> assertAcked(client().admin().indices().prepareAddBlock(block, "test1", "test2")
                 .setIndicesOptions(lenientExpandOpen())));
@@ -241,13 +245,20 @@ public void testCloseOneMissingIndexIgnoreMissing() throws Exception {
 
     public void testAddBlockNoIndex() {
         final ActionRequestValidationException e = expectThrows(ActionRequestValidationException.class,
-            () -> client().admin().indices().prepareAddBlock(randomFrom(APIBlock.values())).get());
+            () -> client().admin().indices().prepareAddBlock(randomAddableBlock()).get());
         assertThat(e.getMessage(), containsString("index is missing"));
     }
 
     public void testAddBlockNullIndex() {
         expectThrows(NullPointerException.class,
-            () -> client().admin().indices().prepareAddBlock(randomFrom(APIBlock.values()), (String[])null));
+            () -> client().admin().indices().prepareAddBlock(randomAddableBlock(), (String[])null));
+    }
+
+    public void testCannotAddReadOnlyAllowDeleteBlock() {
+        createIndex("test1");
+        final AddIndexBlockRequestBuilder request = client().admin().indices().prepareAddBlock(APIBlock.READ_ONLY_ALLOW_DELETE, "test1");
+        final ActionRequestValidationException e = expectThrows(ActionRequestValidationException.class, request::get);
+        assertThat(e.getMessage(), containsString("read_only_allow_delete block is for internal use only"));
     }
 
     public void testAddIndexBlock() throws Exception {
@@ -258,7 +269,7 @@ public void testAddIndexBlock() throws Exception {
         indexRandom(randomBoolean(), false, randomBoolean(), IntStream.range(0, nbDocs)
             .mapToObj(i -> client().prepareIndex(indexName).setId(String.valueOf(i)).setSource("num", i)).collect(toList()));
 
-        final APIBlock block = randomFrom(APIBlock.values());
+        final APIBlock block = randomAddableBlock();
         try {
             assertAcked(client().admin().indices().prepareAddBlock(block, indexName));
             assertIndexHasBlock(block, indexName);
@@ -278,7 +289,7 @@ public void testSameBlockTwice() throws Exception {
             indexRandom(randomBoolean(), false, randomBoolean(), IntStream.range(0, randomIntBetween(1, 10))
                 .mapToObj(i -> client().prepareIndex(indexName).setId(String.valueOf(i)).setSource("num", i)).collect(toList()));
         }
-        final APIBlock block = randomFrom(APIBlock.values());
+        final APIBlock block = randomAddableBlock();
         try {
             assertAcked(client().admin().indices().prepareAddBlock(block, indexName));
             assertIndexHasBlock(block, indexName);
@@ -300,7 +311,7 @@ public void testAddBlockToUnassignedIndex() throws Exception {
         assertThat(clusterState.metadata().indices().get(indexName).getState(), is(IndexMetadata.State.OPEN));
         assertThat(clusterState.routingTable().allShards().stream().allMatch(ShardRouting::unassigned), is(true));
 
-        final APIBlock block = randomFrom(APIBlock.values());
+        final APIBlock block = randomAddableBlock();
         try {
             assertAcked(client().admin().indices().prepareAddBlock(block, indexName));
             assertIndexHasBlock(block, indexName);
@@ -321,7 +332,7 @@ public void testConcurrentAddBlock() throws InterruptedException {
         final CountDownLatch startClosing = new CountDownLatch(1);
         final Thread[] threads = new Thread[randomIntBetween(2, 5)];
 
-        final APIBlock block = randomFrom(APIBlock.values());
+        final APIBlock block = randomAddableBlock();
 
         try {
             for (int i = 0; i < threads.length; i++) {
@@ -356,7 +367,7 @@ public void testAddBlockWhileIndexingDocuments() throws Exception {
         final String indexName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT);
         createIndex(indexName);
 
-        final APIBlock block = randomFrom(APIBlock.values());
+        final APIBlock block = randomAddableBlock();
 
         int nbDocs = 0;
 
@@ -400,7 +411,7 @@ public void testAddBlockWhileDeletingIndices() throws Exception {
         final List<Thread> threads = new ArrayList<>();
         final CountDownLatch latch = new CountDownLatch(1);
 
-        final APIBlock block = randomFrom(APIBlock.values());
+        final APIBlock block = randomAddableBlock();
 
         Consumer<Exception> exceptionConsumer = t -> {
             Throwable cause = ExceptionsHelper.unwrapCause(t);
@@ -478,4 +489,12 @@ static void assertIndexHasBlock(APIBlock block, final String... indices) {
     public static void disableIndexBlock(String index, APIBlock block) {
         disableIndexBlock(index, block.settingName());
     }
+
+    /**
+     * The read-only-allow-delete block cannot be added via the add index block API; this method chooses randomly from the values that
+     * the add index block API does support.
+     */
+    private static APIBlock randomAddableBlock() {
+        return randomValueOtherThan(APIBlock.READ_ONLY_ALLOW_DELETE, () -> randomFrom(APIBlock.values()));
+    }
 }

server/src/main/java/org/elasticsearch/action/admin/indices/readonly/AddIndexBlockRequest.java

@@ -63,6 +63,9 @@ public ActionRequestValidationException validate() {
         if (CollectionUtils.isEmpty(indices)) {
             validationException = addValidationError("index is missing", validationException);
         }
+        if (block == APIBlock.READ_ONLY_ALLOW_DELETE) {
+            validationException = addValidationError("read_only_allow_delete block is for internal use only", validationException);
+        }
         return validationException;
     }
 

test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java

@@ -1470,7 +1470,8 @@ public static void disableIndexBlock(String index, String block) {
 
     /** Enables an index block for the specified index */
     public static void enableIndexBlock(String index, String block) {
-        if (randomBoolean()) {
+        if (IndexMetadata.APIBlock.fromSetting(block) == IndexMetadata.APIBlock.READ_ONLY_ALLOW_DELETE || randomBoolean()) {
+            // the read-only-allow-delete block isn't supported by the add block API so we must use the update settings API here.
             Settings settings = Settings.builder().put(block, true).build();
             client().admin().indices().prepareUpdateSettings(index).setSettings(settings).get();
         } else {