Skip to content

[8.x] Forbid the removal of the write block if the index is read-only #120647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jan 28, 2025
Merged

[8.x] Forbid the removal of the write block if the index is read-only #120647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
effd5f8
[8.x] Forbid the removal of the `write` block if the index is read-only
tlrx Jan 22, 2025
86c33b9
Merge branch '8.x' into 2025/01/22/forbid-write-block-removal-8.x
tlrx Jan 23, 2025
da7ba9c
feedback
tlrx Jan 23, 2025
2c21630
false
tlrx Jan 23, 2025
c3904b3
reword
tlrx Jan 24, 2025
816f008
Merge branch '8.x' into 2025/01/22/forbid-write-block-removal-8.x
tlrx Jan 24, 2025
f5a75da
readd flag
tlrx Jan 24, 2025
7d57b16
remove assert
tlrx Jan 24, 2025
e84ed85
Merge branch '8.x' into 2025/01/22/forbid-write-block-removal-8.x
tlrx Jan 27, 2025
aa5e99c
[CI] Auto commit changes from spotless
Jan 27, 2025
7b004a2
Merge branch '8.x' into 2025/01/22/forbid-write-block-removal-8.x
tlrx Jan 28, 2025
66325bf
hasIndexBlockLevel
tlrx Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ClusterBlock> clusterBlocks = indicesBlocks.get(index);
if (clusterBlocks != null) {
Expand Down Expand Up @@ -398,6 +402,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;
Expand Down
67 changes: 61 additions & 6 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.Nullable;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexSettings;
Expand All @@ -51,7 +52,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;

/**
Expand Down Expand Up @@ -181,11 +184,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<Index> openIndices = new HashSet<>();
Set<Index> closedIndices = new HashSet<>();
Set<Index> readOnlyIndices = null;

final String[] actualIndices = new String[request.indices().length];
for (int i = 0; i < request.indices().length; i++) {
Index index = request.indices()[i];
Expand All @@ -197,6 +203,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) {
Expand Down Expand Up @@ -327,10 +339,23 @@ ClusterState execute(ClusterState currentState) {
}
}

// provides the value of VERIFIED_READ_ONLY_SETTING before block changes
final Function<String, Boolean> 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;

Expand Down Expand Up @@ -359,6 +384,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);
}
Expand Down Expand Up @@ -417,6 +443,18 @@ public static void updateIndexSettings(
}
}

private static void verifyReadOnlyIndices(@Nullable Set<Index> readOnlyIndices, ClusterBlocks blocks) {
if (readOnlyIndices != null) {
for (Index readOnlyIndex : readOnlyIndices) {
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)
);
}
}
}
}

/**
* Updates the cluster block only iff the setting exists in the given settings
*/
Expand All @@ -426,7 +464,8 @@ private static boolean maybeUpdateClusterBlock(
ClusterBlock block,
Setting<Boolean> setting,
Settings openSettings,
Metadata.Builder metadataBuilder
Metadata.Builder metadataBuilder,
Function<String, Boolean> verifiedReadOnlyBeforeBlockChanges
) {
boolean changed = false;
if (setting.exists(openSettings)) {
Expand All @@ -436,16 +475,32 @@ 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)) {
Copy link
Contributor

@henningandersen henningandersen Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In relation to my comment on the v9 PR, just noting that this should follow whatever we do on that PR.

Copy link
Member Author

@tlrx tlrx Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the behavior in the V9 PR and here, see this comment

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));
}
}
}
}
Expand Down