Forbid the removal of the write block if the index is read-only

qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java

@@ -12,8 +12,12 @@
 import org.apache.http.entity.ContentType;
 import org.apache.http.entity.InputStreamEntity;
 import org.elasticsearch.client.Request;
+import org.elasticsearch.client.ResponseException;
+import org.elasticsearch.cluster.block.ClusterBlock;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
+import org.elasticsearch.cluster.metadata.MetadataIndexStateService;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.mapper.MapperService;
@@ -24,6 +28,7 @@
 import org.elasticsearch.test.cluster.util.Version;
 import org.elasticsearch.test.rest.ESRestTestCase;
 import org.elasticsearch.xcontent.XContentType;
+import org.hamcrest.Matcher;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.ClassRule;
@@ -32,16 +37,18 @@
 import org.junit.rules.TestRule;
 
 import java.io.IOException;
+import java.util.Comparator;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.stream.IntStream;
 
-import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING;
 import static org.elasticsearch.test.cluster.util.Version.CURRENT;
 import static org.elasticsearch.test.cluster.util.Version.fromString;
 import static org.elasticsearch.test.rest.ObjectPath.createFromResponse;
 import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 import static org.hamcrest.Matchers.is;
@@ -272,9 +279,51 @@ protected void addIndexBlock(String indexName, IndexMetadata.APIBlock apiBlock)
         assertAcknowledged(client().performRequest(request));
     }
 
-    protected void assertThatIndexBlock(String indexName, IndexMetadata.APIBlock apiBlock) throws Exception {
+    private static ClusterBlock toIndexBlock(String blockId) {
+        int block = Integer.parseInt(blockId);
+        for (var indexBlock : List.of(
+            IndexMetadata.INDEX_READ_ONLY_BLOCK,
+            IndexMetadata.INDEX_READ_BLOCK,
+            IndexMetadata.INDEX_WRITE_BLOCK,
+            IndexMetadata.INDEX_METADATA_BLOCK,
+            IndexMetadata.INDEX_READ_ONLY_ALLOW_DELETE_BLOCK,
+            IndexMetadata.INDEX_REFRESH_BLOCK,
+            MetadataIndexStateService.INDEX_CLOSED_BLOCK
+        )) {
+            if (block == indexBlock.id()) {
+                return indexBlock;
+            }
+        }
+        throw new AssertionError("No index block found with id [" + blockId + ']');
+    }
+
+    @SuppressWarnings("unchecked")
+    protected static List<ClusterBlock> indexBlocks(String indexName) throws Exception {
+        var responseBody = createFromResponse(client().performRequest(new Request("GET", "_cluster/state/blocks/" + indexName)));
+        var blocks = (Map<String, ?>) responseBody.evaluate("blocks.indices." + indexName);
+        if (blocks == null || blocks.isEmpty()) {
+            return List.of();
+        }
+        return blocks.keySet()
+            .stream()
+            .map(AbstractIndexCompatibilityTestCase::toIndexBlock)
+            .sorted(Comparator.comparing(ClusterBlock::id))
+            .toList();
+    }
+
+    @SuppressWarnings("unchecked")
+    protected static void assertIndexSetting(String indexName, Setting<?> setting, Matcher<Boolean> matcher) throws Exception {
         var indexSettings = getIndexSettingsAsMap(indexName);
-        assertThat(indexSettings.get(VERIFIED_READ_ONLY_SETTING.getKey()), equalTo(Boolean.TRUE.toString()));
-        assertThat(indexSettings.get(apiBlock.settingName()), equalTo(Boolean.TRUE.toString()));
+        assertThat(Boolean.parseBoolean((String) indexSettings.get(setting.getKey())), matcher);
+    }
+
+    protected static ResponseException expectUpdateIndexSettingsThrows(String indexName, Settings.Builder settings) {
+        var exception = expectThrows(ResponseException.class, () -> updateIndexSettings(indexName, settings));
+        assertThat(exception.getResponse().getStatusLine().getStatusCode(), equalTo(400));
+        return exception;
+    }
+
+    protected static Matcher<String> containsStringCannotRemoveBlockOnReadOnlyIndex(String indexName) {
+        return allOf(containsString("Can't remove the write block on read-only compatible index"), containsString(indexName));
     }
 }

qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java

@@ -9,13 +9,23 @@
 
 package org.elasticsearch.lucene;
 
+import org.elasticsearch.client.ResponseException;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.repositories.fs.FsRepository;
 import org.elasticsearch.test.cluster.util.Version;
 
+import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_READ_ONLY_BLOCK;
+import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_WRITE_BLOCK;
+import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.INDEX_CLOSED_BLOCK;
+import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_BEFORE_CLOSE_SETTING;
+import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.either;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
 
 public class FullClusterRestartLuceneIndexCompatibilityIT extends FullClusterRestartIndexCompatibilityTestCase {
 
@@ -28,14 +38,13 @@ public FullClusterRestartLuceneIndexCompatibilityIT(Version version) {
     }
 
     /**
-     * Creates an index on N-2, upgrades to N -1 and marks as read-only, then upgrades to N.
+     * Creates an index on N-2, upgrades to N-1 and marks as read-only, then upgrades to N.
      */
     public void testIndexUpgrade() throws Exception {
         final String index = suffix("index");
         final int numDocs = 2431;
 
         if (isFullyUpgradedTo(VERSION_MINUS_2)) {
-            logger.debug("--> creating index [{}]", index);
             createIndex(
                 client(),
                 index,
@@ -45,29 +54,85 @@ public void testIndexUpgrade() throws Exception {
                     .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true)
                     .build()
             );
-
-            logger.debug("--> indexing [{}] docs in [{}]", numDocs, index);
             indexDocs(index, numDocs);
             return;
         }
 
-        if (isFullyUpgradedTo(VERSION_MINUS_1)) {
-            ensureGreen(index);
+        assertThat(indexVersion(index), equalTo(VERSION_MINUS_2));
+        ensureGreen(index);
 
-            assertThat(indexVersion(index), equalTo(VERSION_MINUS_2));
+        if (isIndexClosed(index) == false) {
             assertDocCount(client(), index, numDocs);
+        }
 
-            addIndexBlock(index, IndexMetadata.APIBlock.WRITE);
+        if (isFullyUpgradedTo(VERSION_MINUS_1)) {
+            final boolean maybeClose = randomBoolean();
+            if (maybeClose) {
+                logger.debug("--> closing index [{}] before upgrade", index);
+                closeIndex(index);
+            }
+
+            final var block = randomFrom(IndexMetadata.APIBlock.WRITE, IndexMetadata.APIBlock.READ_ONLY);
+            addIndexBlock(index, block);
+
+            assertThat(indexBlocks(index), maybeClose ? contains(INDEX_CLOSED_BLOCK, block.getBlock()) : contains(block.getBlock()));
+            assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(maybeClose));
+            assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true));
             return;
         }
 
         if (isFullyUpgradedTo(VERSION_CURRENT)) {
-            ensureGreen(index);
+            final var isClosed = isIndexClosed(index);
+            logger.debug("--> upgraded index [{}] is in [{}] state", index, isClosed ? "closed" : "open");
+            assertThat(
+                indexBlocks(index),
+                isClosed
+                    ? either(contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK)).or(contains(INDEX_CLOSED_BLOCK, INDEX_READ_ONLY_BLOCK))
+                    : either(contains(INDEX_WRITE_BLOCK)).or(contains(INDEX_READ_ONLY_BLOCK))
+            );
+            assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed));
+            assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true));
+
+            if (isClosed == false) {
+                logger.debug("--> write/read_only API blocks cannot be removed on an opened index");
+                var ex = expectUpdateIndexSettingsThrows(
+                    index,
+                    Settings.builder()
+                        .putNull(IndexMetadata.APIBlock.WRITE.settingName())
+                        .putNull(IndexMetadata.APIBlock.READ_ONLY.settingName())
+                );
+                assertThat(ex.getMessage(), containsStringCannotRemoveBlockOnReadOnlyIndex(index));
+
+            } else if (randomBoolean()) {
+                logger.debug("--> write/read_only API blocks can be removed on a closed index: INDEX_CLOSED_BLOCK already blocks writes");
+                updateIndexSettings(
+                    index,
+                    Settings.builder()
+                        .putNull(IndexMetadata.APIBlock.WRITE.settingName())
+                        .putNull(IndexMetadata.APIBlock.READ_ONLY.settingName())
+                );
+                logger.debug("--> but attempts to re-opening [{}] should fail due to the missing block", index);
+                var ex = expectThrows(ResponseException.class, () -> openIndex(index));
+                assertThat(ex.getMessage(), containsString("must be marked as read-only"));
+
+                // TODO this could be randomized once we support recovering verified-before-close closed indices with no write/ro block
+                addIndexBlock(index, IndexMetadata.APIBlock.WRITE);
+            }
 
-            assertThat(indexVersion(index), equalTo(VERSION_MINUS_2));
-            assertDocCount(client(), index, numDocs);
+            var block = indexBlocks(index).stream().filter(c -> c.equals(INDEX_WRITE_BLOCK) || c.equals(INDEX_READ_ONLY_BLOCK)).findFirst();
+            if (block.isPresent() && block.get().equals(INDEX_READ_ONLY_BLOCK)) {
+                logger.debug("--> read_only API block can be replaced by a write block (required for the remaining tests)");
+                updateIndexSettings(
+                    index,
+                    Settings.builder()
+                        .putNull(IndexMetadata.APIBlock.READ_ONLY.settingName())
+                        .put(IndexMetadata.APIBlock.WRITE.settingName(), true)
+                );
+            }
 
-            assertThatIndexBlock(index, IndexMetadata.APIBlock.WRITE);
+            assertThat(indexBlocks(index), isClosed ? contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK) : contains(INDEX_WRITE_BLOCK));
+            assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed));
+            assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true));
 
             var numberOfReplicas = getNumberOfReplicas(index);
             if (0 < numberOfReplicas) {
@@ -82,66 +147,29 @@ public void testIndexUpgrade() throws Exception {
             updateIndexSettings(index, Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1));
             ensureGreen(index);
 
-            logger.debug("--> closing restored index [{}]", index);
-            closeIndex(index);
-            ensureGreen(index);
+            if (isClosed) {
+                logger.debug("--> re-opening index [{}]", index);
+                openIndex(index);
+                ensureGreen(index);
 
-            logger.debug("--> adding replica to test peer-recovery for closed shards");
-            updateIndexSettings(index, Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2));
-            ensureGreen(index);
+                assertDocCount(client(), index, numDocs);
+            } else {
+                logger.debug("--> closing index [{}]", index);
+                closeIndex(index);
+                ensureGreen(index);
+            }
 
-            logger.debug("--> re-opening restored index [{}]", index);
-            openIndex(index);
+            logger.debug("--> adding more replicas to test peer-recovery");
+            updateIndexSettings(index, Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2));
             ensureGreen(index);
 
-            assertDocCount(client(), index, numDocs);
-
-            logger.debug("--> deleting index [{}]", index);
-            deleteIndex(index);
-        }
-    }
-
-    /**
-     * Similar to {@link #testIndexUpgrade()} but with a read_only block.
-     */
-    public void testIndexUpgradeReadOnlyBlock() throws Exception {
-        final String index = suffix("index");
-        final int numDocs = 2531;
-
-        if (isFullyUpgradedTo(VERSION_MINUS_2)) {
-            logger.debug("--> creating index [{}]", index);
-            createIndex(
-                client(),
-                index,
-                Settings.builder()
-                    .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
-                    .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, randomInt(2))
-                    .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true)
-                    .build()
+            assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true));
+            assertThat(
+                indexBlocks(index),
+                isIndexClosed(index) ? contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK) : contains(INDEX_WRITE_BLOCK)
             );
 
-            logger.debug("--> indexing [{}] docs in [{}]", numDocs, index);
-            indexDocs(index, numDocs);
-            return;
-        }
-
-        if (isFullyUpgradedTo(VERSION_MINUS_1)) {
-            ensureGreen(index);
-
-            assertThat(indexVersion(index), equalTo(VERSION_MINUS_2));
-            assertDocCount(client(), index, numDocs);
-
-            addIndexBlock(index, IndexMetadata.APIBlock.READ_ONLY);
-            return;
-        }
-
-        if (isFullyUpgradedTo(VERSION_CURRENT)) {
-            ensureGreen(index);
-
-            assertThat(indexVersion(index), equalTo(VERSION_MINUS_2));
-            assertDocCount(client(), index, numDocs);
-
-            assertThatIndexBlock(index, IndexMetadata.APIBlock.READ_ONLY);
+            deleteIndex(index);
         }
     }
 
@@ -196,7 +224,8 @@ public void testRestoreIndex() throws Exception {
             restoreIndex(repository, snapshot, index, restoredIndex);
             ensureGreen(restoredIndex);
 
-            assertThatIndexBlock(restoredIndex, IndexMetadata.APIBlock.WRITE);
+            assertIndexSetting(restoredIndex, VERIFIED_READ_ONLY_SETTING, is(true));
+            assertThat(indexBlocks(restoredIndex), contains(INDEX_WRITE_BLOCK));
             assertThat(indexVersion(restoredIndex), equalTo(VERSION_MINUS_2));
             assertDocCount(client(), restoredIndex, numDocs);
 
@@ -277,7 +306,8 @@ public void testRestoreIndexOverClosedIndex() throws Exception {
 
         if (isFullyUpgradedTo(VERSION_CURRENT)) {
             assertThat(isIndexClosed(index), equalTo(true));
-            assertThatIndexBlock(index, IndexMetadata.APIBlock.WRITE);
+            assertThat(indexBlocks(index), contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK));
+            assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true));
 
             logger.debug("--> restoring index [{}] over existing closed index", index);
             restoreIndex(repository, snapshot, index, index);