Skip to content

Commit 955f92e

Browse files
DaveCTurnerDaveCTurner
authored
Tighten up readonly invariants on repositories (#128296)
Today there are various mechanisms to prevent writes to readonly repositories, but they are scattered across the snapshot codebase and do not obviously prevent writes in all possible circumstances; it'd be easy to add a new operation on a repository that does not check the readonly flag in quite the right way. This commit adds much tighter checks which cannot be circumvented: - Do not allow to start an update of the root `index-N` blob if the repository is marked as readonly in the cluster state. - Conversely, do not allow the readonly flag to be set if an update of the root `index-N` blob is in progress. - Establish the invariant that we never create a `SnapshotsInProgress$Entry`, `SnapshotDeletionsInProgress$Entry`, or `RepositoryCleanupInProgress$Entry` if the repository is marked as readonly in the cluster state. Closes #93575 Backport of #127964 to `8.19`
1 parent 01650e9 commit 955f92e

File tree

6 files changed

+267
-4
lines changed

6 files changed

+267
-4
lines changed

‎server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ private void cleanupRepo(String repositoryName, ActionListener<RepositoryCleanup
171171
@Override
172172
public ClusterState execute(ClusterState currentState) {
173173
SnapshotsService.ensureRepositoryExists(repositoryName, currentState);
174+
SnapshotsService.ensureNotReadOnly(currentState, repositoryName);
174175
final RepositoryCleanupInProgress repositoryCleanupInProgress = RepositoryCleanupInProgress.get(currentState);
175176
if (repositoryCleanupInProgress.hasCleanupInProgress()) {
176177
throw new IllegalStateException(

‎server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import org.elasticsearch.index.Index;
5050
import org.elasticsearch.index.IndexVersion;
5151
import org.elasticsearch.repositories.VerifyNodeRepositoryAction.Request;
52+
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
5253
import org.elasticsearch.repositories.blobstore.MeteredBlobStoreRepository;
5354
import org.elasticsearch.snapshots.Snapshot;
5455
import org.elasticsearch.threadpool.ThreadPool;
@@ -288,6 +289,7 @@ public ClusterState execute(ClusterState currentState) {
288289
List<RepositoryMetadata> repositoriesMetadata = new ArrayList<>(repositories.repositories().size() + 1);
289290
for (RepositoryMetadata repositoryMetadata : repositories.repositories()) {
290291
if (repositoryMetadata.name().equals(request.name())) {
292+
rejectInvalidReadonlyFlagChange(repositoryMetadata, request.settings());
291293
final RepositoryMetadata newRepositoryMetadata = new RepositoryMetadata(
292294
request.name(),
293295
// Copy the UUID from the existing instance rather than resetting it back to MISSING_UUID which would force us to
@@ -600,6 +602,7 @@ public static boolean isDedicatedVotingOnlyNode(Set<DiscoveryNodeRole> roles) {
600602
public void applyClusterState(ClusterChangedEvent event) {
601603
try {
602604
final ClusterState state = event.state();
605+
assert assertReadonlyRepositoriesNotInUseForWrites(state);
603606
final RepositoriesMetadata oldMetadata = RepositoriesMetadata.get(event.previousState());
604607
final RepositoriesMetadata newMetadata = RepositoriesMetadata.get(state);
605608

@@ -884,7 +887,7 @@ public static void validateRepositoryName(final String repositoryName) {
884887
}
885888
}
886889

887-
private static void ensureRepositoryNotInUse(ClusterState clusterState, String repository) {
890+
private static void ensureRepositoryNotInUseForWrites(ClusterState clusterState, String repository) {
888891
if (SnapshotsInProgress.get(clusterState).forRepo(repository).isEmpty() == false) {
889892
throw newRepositoryConflictException(repository, "snapshot is in progress");
890893
}
@@ -898,13 +901,57 @@ private static void ensureRepositoryNotInUse(ClusterState clusterState, String r
898901
throw newRepositoryConflictException(repository, "repository clean up is in progress");
899902
}
900903
}
904+
}
905+
906+
private static void ensureRepositoryNotInUse(ClusterState clusterState, String repository) {
907+
ensureRepositoryNotInUseForWrites(clusterState, repository);
901908
for (RestoreInProgress.Entry entry : RestoreInProgress.get(clusterState)) {
902909
if (repository.equals(entry.snapshot().getRepository())) {
903910
throw newRepositoryConflictException(repository, "snapshot restore is in progress");
904911
}
905912
}
906913
}
907914

915+
public static boolean isReadOnly(Settings repositorySettings) {
916+
return Boolean.TRUE.equals(repositorySettings.getAsBoolean(BlobStoreRepository.READONLY_SETTING_KEY, null));
917+
}
918+
919+
/**
920+
* Test-only check for the invariant that read-only repositories never have any write activities.
921+
*/
922+
private static boolean assertReadonlyRepositoriesNotInUseForWrites(ClusterState clusterState) {
923+
for (final var repositoryMetadata : RepositoriesMetadata.get(clusterState).repositories()) {
924+
if (isReadOnly(repositoryMetadata.settings())) {
925+
try {
926+
ensureRepositoryNotInUseForWrites(clusterState, repositoryMetadata.name());
927+
} catch (Exception e) {
928+
throw new AssertionError("repository [" + repositoryMetadata + "] is readonly but still in use", e);
929+
}
930+
}
931+
}
932+
return true;
933+
}
934+
935+
/**
936+
* Reject a change to the {@code readonly} setting if there is a pending generation change in progress, i.e. some node somewhere is
937+
* updating the root {@link RepositoryData} blob.
938+
*/
939+
private static void rejectInvalidReadonlyFlagChange(RepositoryMetadata existingRepositoryMetadata, Settings newSettings) {
940+
if (isReadOnly(newSettings)
941+
&& isReadOnly(existingRepositoryMetadata.settings()) == false
942+
&& existingRepositoryMetadata.generation() >= RepositoryData.EMPTY_REPO_GEN
943+
&& existingRepositoryMetadata.generation() != existingRepositoryMetadata.pendingGeneration()) {
944+
throw newRepositoryConflictException(
945+
existingRepositoryMetadata.name(),
946+
Strings.format(
947+
"currently updating root blob generation from [%d] to [%d], cannot update readonly flag",
948+
existingRepositoryMetadata.generation(),
949+
existingRepositoryMetadata.pendingGeneration()
950+
)
951+
);
952+
}
953+
}
954+
908955
private static void ensureNoSearchableSnapshotsIndicesInUse(ClusterState clusterState, RepositoryMetadata repositoryMetadata) {
909956
long count = 0L;
910957
List<Index> indices = null;

‎server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2732,6 +2732,14 @@ protected void writeIndexGen(
27322732
public ClusterState execute(ClusterState currentState) {
27332733
final RepositoryMetadata meta = getRepoMetadata(currentState);
27342734
final String repoName = metadata.name();
2735+
2736+
if (RepositoriesService.isReadOnly(meta.settings())) {
2737+
// Last resort check: we shouldn't have been able to mark the repository as readonly while the operation that led to
2738+
// this writeIndexGen() call was in progress, and conversely shouldn't have started any such operation if the repo
2739+
// was already readonly, but these invariants are not obviously true and it is disastrous to proceed here.
2740+
throw new RepositoryException(meta.name(), "repository is readonly, cannot update root blob");
2741+
}
2742+
27352743
final long genInState = meta.generation();
27362744
final boolean uninitializedMeta = meta.generation() == RepositoryData.UNKNOWN_REPO_GEN || bestEffortConsistency;
27372745
if (uninitializedMeta == false && meta.pendingGeneration() != genInState) {

‎server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,7 @@ public ClusterState execute(ClusterState currentState) {
337337
ensureRepositoryExists(repositoryName, currentState);
338338
ensureSnapshotNameAvailableInRepo(repositoryData, snapshotName, repository);
339339
ensureNoCleanupInProgress(currentState, repositoryName, snapshotName, "clone snapshot");
340+
ensureNotReadOnly(currentState, repositoryName);
340341
final SnapshotsInProgress snapshots = SnapshotsInProgress.get(currentState);
341342
ensureSnapshotNameNotRunning(snapshots, repositoryName, snapshotName);
342343
validate(repositoryName, snapshotName, currentState);
@@ -432,6 +433,13 @@ private static void ensureNoCleanupInProgress(
432433
}
433434
}
434435

436+
public static void ensureNotReadOnly(final ClusterState currentState, final String repositoryName) {
437+
final var repositoryMetadata = RepositoriesMetadata.get(currentState).repository(repositoryName);
438+
if (RepositoriesService.isReadOnly(repositoryMetadata.settings())) {
439+
throw new RepositoryException(repositoryMetadata.name(), "repository is readonly");
440+
}
441+
}
442+
435443
private static void ensureSnapshotNameAvailableInRepo(RepositoryData repositoryData, String snapshotName, Repository repository) {
436444
// check if the snapshot name already exists in the repository
437445
if (repositoryData.getSnapshotIds().stream().anyMatch(s -> s.getName().equals(snapshotName))) {
@@ -2152,6 +2160,8 @@ public ClusterState execute(ClusterState currentState) {
21522160
"delete snapshot"
21532161
);
21542162

2163+
ensureNotReadOnly(currentState, repositoryName);
2164+
21552165
final SnapshotDeletionsInProgress deletionsInProgress = SnapshotDeletionsInProgress.get(currentState);
21562166

21572167
final RestoreInProgress restoreInProgress = RestoreInProgress.get(currentState);
@@ -3991,8 +4001,13 @@ public ClusterState execute(BatchExecutionContext<SnapshotTask> batchExecutionCo
39914001
for (final var taskContext : batchExecutionContext.taskContexts()) {
39924002
if (taskContext.getTask() instanceof CreateSnapshotTask task) {
39934003
try {
3994-
registeredPolicySnapshots.maybeAdd(task.createSnapshotRequest.userMetadata(), task.snapshot.getSnapshotId());
39954004
final var repoMeta = RepositoriesMetadata.get(state).repository(task.snapshot.getRepository());
4005+
if (RepositoriesService.isReadOnly(repoMeta.settings())) {
4006+
taskContext.onFailure(new RepositoryException(repoMeta.name(), "repository is readonly"));
4007+
continue;
4008+
}
4009+
4010+
registeredPolicySnapshots.maybeAdd(task.createSnapshotRequest.userMetadata(), task.snapshot.getSnapshotId());
39964011
if (Objects.equals(task.initialRepositoryMetadata, repoMeta)) {
39974012
snapshotsInProgress = createSnapshot(task, taskContext, state, snapshotsInProgress);
39984013
} else {

‎server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java‎

Lines changed: 105 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,14 @@
1212
import org.elasticsearch.action.ActionListener;
1313
import org.elasticsearch.action.admin.cluster.repositories.put.PutRepositoryRequest;
1414
import org.elasticsearch.action.support.ActionFilters;
15+
import org.elasticsearch.action.support.ActionTestUtils;
1516
import org.elasticsearch.action.support.SubscribableListener;
1617
import org.elasticsearch.action.support.master.AcknowledgedResponse;
1718
import org.elasticsearch.client.internal.node.NodeClient;
1819
import org.elasticsearch.cluster.ClusterChangedEvent;
1920
import org.elasticsearch.cluster.ClusterName;
2021
import org.elasticsearch.cluster.ClusterState;
22+
import org.elasticsearch.cluster.ClusterStateUpdateTask;
2123
import org.elasticsearch.cluster.metadata.IndexMetadata;
2224
import org.elasticsearch.cluster.metadata.Metadata;
2325
import org.elasticsearch.cluster.metadata.RepositoriesMetadata;
@@ -26,6 +28,7 @@
2628
import org.elasticsearch.cluster.node.DiscoveryNodeRole;
2729
import org.elasticsearch.cluster.node.DiscoveryNodeUtils;
2830
import org.elasticsearch.cluster.service.ClusterService;
31+
import org.elasticsearch.common.Strings;
2932
import org.elasticsearch.common.UUIDs;
3033
import org.elasticsearch.common.blobstore.BlobPath;
3134
import org.elasticsearch.common.blobstore.BlobStore;
@@ -41,6 +44,7 @@
4144
import org.elasticsearch.index.store.Store;
4245
import org.elasticsearch.indices.recovery.RecoverySettings;
4346
import org.elasticsearch.indices.recovery.RecoveryState;
47+
import org.elasticsearch.repositories.blobstore.BlobStoreRepository;
4448
import org.elasticsearch.repositories.blobstore.MeteredBlobStoreRepository;
4549
import org.elasticsearch.snapshots.SnapshotId;
4650
import org.elasticsearch.snapshots.SnapshotInfo;
@@ -376,6 +380,103 @@ public void testRegisterRepositorySuccessAfterCreationFailed() {
376380
assertThat(repositoriesService.repository(repoName), isA(TestRepository.class));
377381
}
378382

383+
public void testCannotSetRepositoryReadonlyFlagDuringGenerationChange() {
384+
final var repoName = randomAlphaOfLengthBetween(10, 25);
385+
final long originalGeneration = randomFrom(RepositoryData.EMPTY_REPO_GEN, 0L, 1L, randomLongBetween(2, Long.MAX_VALUE - 1));
386+
final long newGeneration = originalGeneration + 1;
387+
388+
safeAwait(
389+
SubscribableListener
390+
391+
.newForked(
392+
l -> repositoriesService.registerRepository(
393+
new PutRepositoryRequest(TEST_REQUEST_TIMEOUT, TEST_REQUEST_TIMEOUT, repoName).type(TestRepository.TYPE),
394+
l.map(ignored -> null)
395+
)
396+
)
397+
.andThen(l -> updateGenerations(repoName, originalGeneration, newGeneration, l))
398+
.andThenAccept(ignored -> {
399+
final var metadata = repositoriesService.repository(repoName).getMetadata();
400+
assertEquals(originalGeneration, metadata.generation());
401+
assertEquals(newGeneration, metadata.pendingGeneration());
402+
assertNull(metadata.settings().getAsBoolean(BlobStoreRepository.READONLY_SETTING_KEY, null));
403+
})
404+
.andThen(
405+
l -> repositoriesService.registerRepository(
406+
new PutRepositoryRequest(TEST_REQUEST_TIMEOUT, TEST_REQUEST_TIMEOUT, repoName).type(TestRepository.TYPE)
407+
.settings(Settings.builder().put(BlobStoreRepository.READONLY_SETTING_KEY, true)),
408+
ActionTestUtils.assertNoSuccessListener(e -> {
409+
assertEquals(
410+
Strings.format(
411+
"""
412+
[%s] trying to modify or unregister repository that is currently used \
413+
(currently updating root blob generation from [%d] to [%d], cannot update readonly flag)""",
414+
repoName,
415+
originalGeneration,
416+
newGeneration
417+
),
418+
asInstanceOf(RepositoryConflictException.class, e).getMessage()
419+
);
420+
l.onResponse(null);
421+
})
422+
)
423+
)
424+
.andThenAccept(ignored -> {
425+
final var metadata = repositoriesService.repository(repoName).getMetadata();
426+
assertEquals(originalGeneration, metadata.generation());
427+
assertEquals(newGeneration, metadata.pendingGeneration());
428+
assertNull(metadata.settings().getAsBoolean(BlobStoreRepository.READONLY_SETTING_KEY, null));
429+
})
430+
.andThen(l -> updateGenerations(repoName, newGeneration, newGeneration, l))
431+
.andThenAccept(ignored -> {
432+
final var metadata = repositoriesService.repository(repoName).getMetadata();
433+
assertEquals(newGeneration, metadata.generation());
434+
assertEquals(newGeneration, metadata.pendingGeneration());
435+
assertNull(metadata.settings().getAsBoolean(BlobStoreRepository.READONLY_SETTING_KEY, null));
436+
})
437+
.andThen(
438+
l -> repositoriesService.registerRepository(
439+
new PutRepositoryRequest(TEST_REQUEST_TIMEOUT, TEST_REQUEST_TIMEOUT, repoName).type(TestRepository.TYPE)
440+
.settings(Settings.builder().put(BlobStoreRepository.READONLY_SETTING_KEY, true)),
441+
l.map(ignored -> null)
442+
)
443+
)
444+
.andThenAccept(
445+
ignored -> assertTrue(
446+
repositoriesService.repository(repoName)
447+
.getMetadata()
448+
.settings()
449+
.getAsBoolean(BlobStoreRepository.READONLY_SETTING_KEY, null)
450+
)
451+
)
452+
);
453+
}
454+
455+
private void updateGenerations(String repositoryName, long safeGeneration, long pendingGeneration, ActionListener<?> listener) {
456+
clusterService.submitUnbatchedStateUpdateTask("update repo generations", new ClusterStateUpdateTask() {
457+
@Override
458+
public ClusterState execute(ClusterState currentState) {
459+
return new ClusterState.Builder(currentState).metadata(
460+
Metadata.builder(currentState.metadata())
461+
.putCustom(
462+
RepositoriesMetadata.TYPE,
463+
RepositoriesMetadata.get(currentState).withUpdatedGeneration(repositoryName, safeGeneration, pendingGeneration)
464+
)
465+
).build();
466+
}
467+
468+
@Override
469+
public void onFailure(Exception e) {
470+
listener.onFailure(e);
471+
}
472+
473+
@Override
474+
public void clusterStateProcessed(ClusterState initialState, ClusterState newState) {
475+
listener.onResponse(null);
476+
}
477+
});
478+
}
479+
379480
private ClusterState createClusterStateWithRepo(String repoName, String repoType) {
380481
ClusterState.Builder state = ClusterState.builder(new ClusterName("test"));
381482
Metadata.Builder mdBuilder = Metadata.builder();
@@ -405,7 +506,7 @@ private void assertThrowsOnRegister(String repoName) {
405506
private static class TestRepository implements Repository {
406507

407508
private static final String TYPE = "internal";
408-
private final RepositoryMetadata metadata;
509+
private RepositoryMetadata metadata;
409510
private boolean isClosed;
410511
private boolean isStarted;
411512

@@ -513,7 +614,9 @@ public IndexShardSnapshotStatus.Copy getShardSnapshotStatus(SnapshotId snapshotI
513614
}
514615

515616
@Override
516-
public void updateState(final ClusterState state) {}
617+
public void updateState(final ClusterState state) {
618+
metadata = RepositoriesMetadata.get(state).repository(metadata.name());
619+
}
517620

518621
@Override
519622
public void cloneShardSnapshot(

0 commit comments

Comments
 (0)