From effd5f8342ef3fa593fdbf7e198245fcb8b4ef10 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Wed, 22 Jan 2025 15:58:33 +0100 Subject: [PATCH 1/8] [8.x] Forbid the removal of the `write` block if the index is read-only --- .../MetadataUpdateSettingsService.java | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index e984768277d27..c4643b2727218 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -39,6 +39,7 @@ import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexSettings; +import org.elasticsearch.index.IndexVersion; import org.elasticsearch.indices.IndicesService; import org.elasticsearch.indices.ShardLimitValidator; import org.elasticsearch.threadpool.ThreadPool; @@ -330,7 +331,15 @@ ClusterState execute(ClusterState currentState) { final ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); boolean changedBlocks = false; for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) { - changedBlocks |= maybeUpdateClusterBlock(actualIndices, blocks, block.block, block.setting, openSettings, metadataBuilder); + changedBlocks |= maybeUpdateClusterBlock( + actualIndices, + blocks, + block.block, + block.setting, + openSettings, + metadataBuilder, + currentState.nodes().getMinSupportedIndexVersion() + ); } changed |= changedBlocks; @@ -426,7 +435,8 @@ private static boolean maybeUpdateClusterBlock( ClusterBlock block, Setting setting, Settings openSettings, - Metadata.Builder metadataBuilder + Metadata.Builder metadataBuilder, + IndexVersion minSupportedIndexVersion ) { boolean changed = false; if (setting.exists(openSettings)) { @@ -443,6 +453,17 @@ private static boolean maybeUpdateClusterBlock( changed = true; if (block.contains(ClusterBlockLevel.WRITE)) { IndexMetadata indexMetadata = metadataBuilder.get(index); + if (indexMetadata.getCompatibilityVersion().before(minSupportedIndexVersion)) { + // Forbid the removal of the block if the index cannot be written by one or more nodes of the cluster + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "Can't update setting [%s] for read-only compatible index %s", + setting.getKey(), + indexMetadata.getIndex() + ) + ); + } Settings.Builder indexSettings = Settings.builder().put(indexMetadata.getSettings()); indexSettings.remove(MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING.getKey()); metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(indexSettings)); From da7ba9cb157ef58890a0d6400f0c022fd2c075a5 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Thu, 23 Jan 2025 14:23:25 +0100 Subject: [PATCH 2/8] feedback --- .../MetadataUpdateSettingsService.java | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index c4643b2727218..26da9a1c0ad20 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -36,10 +36,10 @@ import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexSettings; -import org.elasticsearch.index.IndexVersion; import org.elasticsearch.indices.IndicesService; import org.elasticsearch.indices.ShardLimitValidator; import org.elasticsearch.threadpool.ThreadPool; @@ -182,11 +182,14 @@ ClusterState execute(ClusterState currentState) { RoutingTable.Builder routingTableBuilder = null; Metadata.Builder metadataBuilder = Metadata.builder(currentState.metadata()); + final var minSupportedIndexVersion = currentState.nodes().getMinSupportedIndexVersion(); // allow to change any settings to a closed index, and only allow dynamic settings to be changed // on an open index Set openIndices = new HashSet<>(); Set closedIndices = new HashSet<>(); + Set readOnlyIndices = null; + final String[] actualIndices = new String[request.indices().length]; for (int i = 0; i < request.indices().length; i++) { Index index = request.indices()[i]; @@ -198,6 +201,12 @@ ClusterState execute(ClusterState currentState) { } else { closedIndices.add(index); } + if (metadata.getCompatibilityVersion().before(minSupportedIndexVersion)) { + if (readOnlyIndices == null) { + readOnlyIndices = new HashSet<>(); + } + readOnlyIndices.add(index); + } } if (skippedSettings.isEmpty() == false && openIndices.isEmpty() == false) { @@ -331,15 +340,7 @@ ClusterState execute(ClusterState currentState) { final ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); boolean changedBlocks = false; for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) { - changedBlocks |= maybeUpdateClusterBlock( - actualIndices, - blocks, - block.block, - block.setting, - openSettings, - metadataBuilder, - currentState.nodes().getMinSupportedIndexVersion() - ); + changedBlocks |= maybeUpdateClusterBlock(actualIndices, blocks, block.block, block.setting, openSettings, metadataBuilder); } changed |= changedBlocks; @@ -368,6 +369,7 @@ ClusterState execute(ClusterState currentState) { // This step is mandatory since we allow to update non-dynamic settings on closed indices. indicesService.verifyIndexMetadata(updatedMetadata, updatedMetadata); } + verifyReadOnlyIndices(readOnlyIndices, updatedState.blocks()); } catch (IOException ex) { throw ExceptionsHelper.convertToElastic(ex); } @@ -426,6 +428,18 @@ public static void updateIndexSettings( } } + private static void verifyReadOnlyIndices(@Nullable Set readOnlyIndices, ClusterBlocks blocks) { + if (readOnlyIndices != null) { + for (Index readOnlyIndex : readOnlyIndices) { + if (blocks.indexBlocked(ClusterBlockLevel.WRITE, readOnlyIndex.getName())) { + throw new IllegalArgumentException( + String.format(Locale.ROOT, "Can't remove the `write` level block for read-only compatible index %s", readOnlyIndex) + ); + } + } + } + } + /** * Updates the cluster block only iff the setting exists in the given settings */ @@ -435,8 +449,7 @@ private static boolean maybeUpdateClusterBlock( ClusterBlock block, Setting setting, Settings openSettings, - Metadata.Builder metadataBuilder, - IndexVersion minSupportedIndexVersion + Metadata.Builder metadataBuilder ) { boolean changed = false; if (setting.exists(openSettings)) { @@ -453,17 +466,6 @@ private static boolean maybeUpdateClusterBlock( changed = true; if (block.contains(ClusterBlockLevel.WRITE)) { IndexMetadata indexMetadata = metadataBuilder.get(index); - if (indexMetadata.getCompatibilityVersion().before(minSupportedIndexVersion)) { - // Forbid the removal of the block if the index cannot be written by one or more nodes of the cluster - throw new IllegalArgumentException( - String.format( - Locale.ROOT, - "Can't update setting [%s] for read-only compatible index %s", - setting.getKey(), - indexMetadata.getIndex() - ) - ); - } Settings.Builder indexSettings = Settings.builder().put(indexMetadata.getSettings()); indexSettings.remove(MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING.getKey()); metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(indexSettings)); From 2c21630d734c4ee8bdbcfd09a29a7c8bea45ca3f Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Thu, 23 Jan 2025 14:30:05 +0100 Subject: [PATCH 3/8] false --- .../cluster/metadata/MetadataUpdateSettingsService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index 26da9a1c0ad20..55783f607b67f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -431,7 +431,7 @@ public static void updateIndexSettings( private static void verifyReadOnlyIndices(@Nullable Set readOnlyIndices, ClusterBlocks blocks) { if (readOnlyIndices != null) { for (Index readOnlyIndex : readOnlyIndices) { - if (blocks.indexBlocked(ClusterBlockLevel.WRITE, readOnlyIndex.getName())) { + if (blocks.indexBlocked(ClusterBlockLevel.WRITE, readOnlyIndex.getName()) == false) { throw new IllegalArgumentException( String.format(Locale.ROOT, "Can't remove the `write` level block for read-only compatible index %s", readOnlyIndex) ); From c3904b34ccbe06465eca8fb377c38d9ee514b82f Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Fri, 24 Jan 2025 10:34:45 +0100 Subject: [PATCH 4/8] reword --- .../cluster/metadata/MetadataUpdateSettingsService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index 55783f607b67f..b9c9656902ce4 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -433,7 +433,7 @@ private static void verifyReadOnlyIndices(@Nullable Set readOnlyIndices, for (Index readOnlyIndex : readOnlyIndices) { if (blocks.indexBlocked(ClusterBlockLevel.WRITE, readOnlyIndex.getName()) == false) { throw new IllegalArgumentException( - String.format(Locale.ROOT, "Can't remove the `write` level block for read-only compatible index %s", readOnlyIndex) + String.format(Locale.ROOT, "Can't remove the write block on read-only compatible index %s", readOnlyIndex) ); } } From f5a75da3428f43a76a6edf979efb6018b234a23e Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Fri, 24 Jan 2025 12:44:36 +0100 Subject: [PATCH 5/8] readd flag --- .../cluster/block/ClusterBlocks.java | 4 ++ .../MetadataUpdateSettingsService.java | 46 ++++++++++++++++--- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java index aa3a6a201eac4..660ce9f4d0b51 100644 --- a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java +++ b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java @@ -398,6 +398,10 @@ public boolean hasIndexBlock(String index, ClusterBlock block) { return indices.getOrDefault(index, Set.of()).contains(block); } + public boolean hasIndexBlockLevel(String index, ClusterBlockLevel level) { + return indices.getOrDefault(index, Set.of()).stream().anyMatch(clusterBlock -> clusterBlock.contains(level)); + } + public Builder removeIndexBlock(String index, ClusterBlock block) { if (indices.containsKey(index) == false) { return this; diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index b9c9656902ce4..d120b08972e76 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -36,6 +36,7 @@ import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.Assertions; import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.Index; @@ -52,7 +53,9 @@ import java.util.Objects; import java.util.Set; import java.util.function.BiFunction; +import java.util.function.Function; +import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING; import static org.elasticsearch.index.IndexSettings.same; /** @@ -337,10 +340,22 @@ ClusterState execute(ClusterState currentState) { } } + // provides the value of VERIFIED_READ_ONLY_SETTING before block changes + final Function verifiedReadOnly = indexName -> + VERIFIED_READ_ONLY_SETTING.get(currentState.metadata().index(indexName).getSettings()); + final ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); boolean changedBlocks = false; for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) { - changedBlocks |= maybeUpdateClusterBlock(actualIndices, blocks, block.block, block.setting, openSettings, metadataBuilder); + changedBlocks |= maybeUpdateClusterBlock( + actualIndices, + blocks, + block.block, + block.setting, + openSettings, + metadataBuilder, + verifiedReadOnly + ); } changed |= changedBlocks; @@ -449,7 +464,8 @@ private static boolean maybeUpdateClusterBlock( ClusterBlock block, Setting setting, Settings openSettings, - Metadata.Builder metadataBuilder + Metadata.Builder metadataBuilder, + Function verifiedReadOnlyBeforeBlockChanges ) { boolean changed = false; if (setting.exists(openSettings)) { @@ -459,16 +475,34 @@ private static boolean maybeUpdateClusterBlock( if (blocks.hasIndexBlock(index, block) == false) { blocks.addIndexBlock(index, block); changed = true; + if (block.contains(ClusterBlockLevel.WRITE)) { + var isVerifiedReadOnly = verifiedReadOnlyBeforeBlockChanges.apply(index); + if (isVerifiedReadOnly) { + var indexMetadata = metadataBuilder.get(index); + metadataBuilder.put( + IndexMetadata.builder(indexMetadata) + .settings( + Settings.builder() + .put(indexMetadata.getSettings()) + .put(VERIFIED_READ_ONLY_SETTING.getKey(), true) + ) + ); + } + } } } else { if (blocks.hasIndexBlock(index, block)) { blocks.removeIndexBlock(index, block); changed = true; if (block.contains(ClusterBlockLevel.WRITE)) { - IndexMetadata indexMetadata = metadataBuilder.get(index); - Settings.Builder indexSettings = Settings.builder().put(indexMetadata.getSettings()); - indexSettings.remove(MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING.getKey()); - metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(indexSettings)); + if (blocks.hasIndexBlockLevel(index, ClusterBlockLevel.WRITE) == false) { + var indexMetadata = metadataBuilder.get(index); + var indexSettings = Settings.builder().put(indexMetadata.getSettings()); + indexSettings.remove(VERIFIED_READ_ONLY_SETTING.getKey()); + metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(indexSettings)); + } else if (Assertions.ENABLED) { + assert VERIFIED_READ_ONLY_SETTING.get(metadataBuilder.get(index).getSettings()); + } } } } From 7d57b1610ef1a2ac1507270d8eeff34730090253 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Fri, 24 Jan 2025 17:25:34 +0100 Subject: [PATCH 6/8] remove assert --- .../cluster/metadata/MetadataUpdateSettingsService.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index d120b08972e76..4fd852a210228 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -36,7 +36,6 @@ import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.core.Assertions; import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.Index; @@ -500,8 +499,6 @@ private static boolean maybeUpdateClusterBlock( var indexSettings = Settings.builder().put(indexMetadata.getSettings()); indexSettings.remove(VERIFIED_READ_ONLY_SETTING.getKey()); metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(indexSettings)); - } else if (Assertions.ENABLED) { - assert VERIFIED_READ_ONLY_SETTING.get(metadataBuilder.get(index).getSettings()); } } } From aa5e99c41207049b3b490daf4cdcb26c8fbe62f6 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Mon, 27 Jan 2025 07:56:31 +0000 Subject: [PATCH 7/8] [CI] Auto commit changes from spotless --- .../cluster/metadata/MetadataUpdateSettingsService.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index 4fd852a210228..c857917827589 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -340,8 +340,9 @@ ClusterState execute(ClusterState currentState) { } // provides the value of VERIFIED_READ_ONLY_SETTING before block changes - final Function verifiedReadOnly = indexName -> - VERIFIED_READ_ONLY_SETTING.get(currentState.metadata().index(indexName).getSettings()); + final Function verifiedReadOnly = indexName -> VERIFIED_READ_ONLY_SETTING.get( + currentState.metadata().index(indexName).getSettings() + ); final ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); boolean changedBlocks = false; From 66325bf4bd08e1fb91f29640f24c9a7127d712ce Mon Sep 17 00:00:00 2001 From: tlrx Date: Tue, 28 Jan 2025 10:19:45 +0100 Subject: [PATCH 8/8] hasIndexBlockLevel --- .../java/org/elasticsearch/cluster/block/ClusterBlocks.java | 4 ++++ .../cluster/metadata/MetadataUpdateSettingsService.java | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java index 660ce9f4d0b51..659e78f99c21a 100644 --- a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java +++ b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java @@ -146,6 +146,10 @@ public boolean hasIndexBlock(String index, ClusterBlock block) { return indicesBlocks.containsKey(index) && indicesBlocks.get(index).contains(block); } + public boolean hasIndexBlockLevel(String index, ClusterBlockLevel level) { + return blocksForIndex(level, index).isEmpty() == false; + } + public boolean hasIndexBlockWithId(String index, int blockId) { final Set clusterBlocks = indicesBlocks.get(index); if (clusterBlocks != null) { diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index c857917827589..ddc6037777b41 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -446,7 +446,7 @@ public static void updateIndexSettings( private static void verifyReadOnlyIndices(@Nullable Set readOnlyIndices, ClusterBlocks blocks) { if (readOnlyIndices != null) { for (Index readOnlyIndex : readOnlyIndices) { - if (blocks.indexBlocked(ClusterBlockLevel.WRITE, readOnlyIndex.getName()) == false) { + if (blocks.hasIndexBlockLevel(readOnlyIndex.getName(), ClusterBlockLevel.WRITE) == false) { throw new IllegalArgumentException( String.format(Locale.ROOT, "Can't remove the write block on read-only compatible index %s", readOnlyIndex) );