Skip to content

Commit 1e79dc1

Browse files
psychbotpsychbot
committed
[Backport 2.x][Remote Store] Add support to restrict creation & deletion if system repository and mutation of immutable settings of system repository (opensearch-project#9839)
--------- Signed-off-by: Dharmesh 💤 <sdharms@amazon.com> (cherry picked from commit 699d235) Signed-off-by: Dharmesh 💤 <sdharms@amazon.com>
1 parent 689adc1 commit 1e79dc1

File tree

29 files changed

+452
-61
lines changed

29 files changed

+452
-61
lines changed

‎CHANGELOG.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
1515
- Add Doc Status Counter for Indexing Engine ([#4562](https://github.com/opensearch-project/OpenSearch/issues/4562))
1616
- Add unreferenced file cleanup count to merge stats ([#10204](https://github.com/opensearch-project/OpenSearch/pull/10204))
1717
- Configurable merge policy for index with an option to choose from LogByteSize and Tiered merge policy ([#9992](https://github.com/opensearch-project/OpenSearch/pull/9992))
18-
18+
- [Remote Store] Add support to restrict creation & deletion if system repository and mutation of immutable settings of system repository ([#9839](https://github.com/opensearch-project/OpenSearch/pull/9839))
1919
### Dependencies
2020
- Bump JNA version from 5.5 to 5.13 ([#9963](https://github.com/opensearch-project/OpenSearch/pull/9963))
2121
- Bump `peter-evans/create-or-update-comment` from 2 to 3 ([#9575](https://github.com/opensearch-project/OpenSearch/pull/9575))

‎plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureRepository.java

+11
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@
4747
import org.opensearch.indices.recovery.RecoverySettings;
4848
import org.opensearch.repositories.blobstore.MeteredBlobStoreRepository;
4949

50+
import java.util.ArrayList;
51+
import java.util.List;
5052
import java.util.Locale;
5153
import java.util.Map;
5254
import java.util.function.Function;
@@ -192,4 +194,13 @@ protected ByteSizeValue chunkSize() {
192194
public boolean isReadOnly() {
193195
return readonly;
194196
}
197+
198+
@Override
199+
public List<Setting<?>> getRestrictedSystemRepositorySettings() {
200+
List<Setting<?>> restrictedSettings = new ArrayList<>();
201+
restrictedSettings.addAll(super.getRestrictedSystemRepositorySettings());
202+
restrictedSettings.add(Repository.BASE_PATH_SETTING);
203+
restrictedSettings.add(Repository.LOCATION_MODE_SETTING);
204+
return restrictedSettings;
205+
}
195206
}

‎plugins/repository-azure/src/test/java/org/opensearch/repositories/azure/AzureRepositorySettingsTests.java

+21
Original file line numberDiff line numberDiff line change
@@ -34,16 +34,20 @@
3434

3535
import org.opensearch.cluster.metadata.RepositoryMetadata;
3636
import org.opensearch.common.settings.ClusterSettings;
37+
import org.opensearch.common.settings.Setting;
3738
import org.opensearch.common.settings.Settings;
3839
import org.opensearch.core.common.unit.ByteSizeUnit;
3940
import org.opensearch.core.common.unit.ByteSizeValue;
4041
import org.opensearch.core.xcontent.NamedXContentRegistry;
4142
import org.opensearch.env.Environment;
4243
import org.opensearch.indices.recovery.RecoverySettings;
44+
import org.opensearch.repositories.blobstore.BlobStoreRepository;
4345
import org.opensearch.repositories.blobstore.BlobStoreTestUtil;
4446
import org.opensearch.test.OpenSearchTestCase;
4547
import org.junit.AfterClass;
4648

49+
import java.util.List;
50+
4751
import reactor.core.scheduler.Schedulers;
4852

4953
import static org.hamcrest.Matchers.is;
@@ -179,4 +183,21 @@ public void testChunkSize() {
179183
);
180184
}
181185

186+
public void testSystemRepositoryDefault() {
187+
assertThat(azureRepository(Settings.EMPTY).isSystemRepository(), is(false));
188+
}
189+
190+
public void testSystemRepositoryOn() {
191+
assertThat(azureRepository(Settings.builder().put("system_repository", true).build()).isSystemRepository(), is(true));
192+
}
193+
194+
public void testRestrictedSettingsDefault() {
195+
List<Setting<?>> restrictedSettings = azureRepository(Settings.EMPTY).getRestrictedSystemRepositorySettings();
196+
assertThat(restrictedSettings.size(), is(5));
197+
assertTrue(restrictedSettings.contains(BlobStoreRepository.SYSTEM_REPOSITORY_SETTING));
198+
assertTrue(restrictedSettings.contains(BlobStoreRepository.READONLY_SETTING));
199+
assertTrue(restrictedSettings.contains(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY));
200+
assertTrue(restrictedSettings.contains(AzureRepository.Repository.BASE_PATH_SETTING));
201+
assertTrue(restrictedSettings.contains(AzureRepository.Repository.LOCATION_MODE_SETTING));
202+
}
182203
}

‎plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageRepository.java

+11
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@
4646
import org.opensearch.repositories.RepositoryException;
4747
import org.opensearch.repositories.blobstore.MeteredBlobStoreRepository;
4848

49+
import java.util.ArrayList;
50+
import java.util.List;
4951
import java.util.Map;
5052
import java.util.function.Function;
5153

@@ -138,6 +140,15 @@ protected ByteSizeValue chunkSize() {
138140
return chunkSize;
139141
}
140142

143+
@Override
144+
public List<Setting<?>> getRestrictedSystemRepositorySettings() {
145+
List<Setting<?>> restrictedSettings = new ArrayList<>();
146+
restrictedSettings.addAll(super.getRestrictedSystemRepositorySettings());
147+
restrictedSettings.add(BUCKET);
148+
restrictedSettings.add(BASE_PATH);
149+
return restrictedSettings;
150+
}
151+
141152
/**
142153
* Get a given setting from the repository settings, throwing a {@link RepositoryException} if the setting does not exist or is empty.
143154
*/

‎plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java

+11
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,9 @@
6767
import org.opensearch.threadpool.Scheduler;
6868
import org.opensearch.threadpool.ThreadPool;
6969

70+
import java.util.ArrayList;
7071
import java.util.Collection;
72+
import java.util.List;
7173
import java.util.Map;
7274
import java.util.concurrent.TimeUnit;
7375
import java.util.concurrent.atomic.AtomicReference;
@@ -473,6 +475,15 @@ protected ByteSizeValue chunkSize() {
473475
return chunkSize;
474476
}
475477

478+
@Override
479+
public List<Setting<?>> getRestrictedSystemRepositorySettings() {
480+
List<Setting<?>> restrictedSettings = new ArrayList<>();
481+
restrictedSettings.addAll(super.getRestrictedSystemRepositorySettings());
482+
restrictedSettings.add(BUCKET_SETTING);
483+
restrictedSettings.add(BASE_PATH_SETTING);
484+
return restrictedSettings;
485+
}
486+
476487
@Override
477488
protected void doClose() {
478489
final Scheduler.Cancellable cancellable = finalizationFuture.getAndSet(null);

‎plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java

+16
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,20 @@
3636

3737
import org.opensearch.cluster.metadata.RepositoryMetadata;
3838
import org.opensearch.common.settings.ClusterSettings;
39+
import org.opensearch.common.settings.Setting;
3940
import org.opensearch.common.settings.Settings;
4041
import org.opensearch.core.common.unit.ByteSizeUnit;
4142
import org.opensearch.core.common.unit.ByteSizeValue;
4243
import org.opensearch.core.xcontent.NamedXContentRegistry;
4344
import org.opensearch.indices.recovery.RecoverySettings;
4445
import org.opensearch.repositories.RepositoryException;
46+
import org.opensearch.repositories.blobstore.BlobStoreRepository;
4547
import org.opensearch.repositories.blobstore.BlobStoreTestUtil;
4648
import org.opensearch.test.OpenSearchTestCase;
4749
import org.hamcrest.Matchers;
4850

4951
import java.nio.file.Path;
52+
import java.util.List;
5053
import java.util.Map;
5154

5255
import static org.hamcrest.Matchers.containsString;
@@ -133,6 +136,19 @@ public void testDefaultBufferSize() {
133136
}
134137
}
135138

139+
public void testRestrictedSettingsDefault() {
140+
final RepositoryMetadata metadata = new RepositoryMetadata("dummy-repo", "mock", Settings.EMPTY);
141+
try (S3Repository s3repo = createS3Repo(metadata)) {
142+
List<Setting<?>> restrictedSettings = s3repo.getRestrictedSystemRepositorySettings();
143+
assertThat(restrictedSettings.size(), is(5));
144+
assertTrue(restrictedSettings.contains(BlobStoreRepository.SYSTEM_REPOSITORY_SETTING));
145+
assertTrue(restrictedSettings.contains(BlobStoreRepository.READONLY_SETTING));
146+
assertTrue(restrictedSettings.contains(BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY));
147+
assertTrue(restrictedSettings.contains(S3Repository.BUCKET_SETTING));
148+
assertTrue(restrictedSettings.contains(S3Repository.BASE_PATH_SETTING));
149+
}
150+
}
151+
136152
private S3Repository createS3Repo(RepositoryMetadata metadata) {
137153
return new S3Repository(
138154
metadata,

‎server/src/internalClusterTest/java/org/opensearch/remotestore/AbstractRemoteStoreMockRepositoryIntegTestCase.java

+7-6
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT;
3434
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY;
3535
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY;
36-
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
3736

3837
public abstract class AbstractRemoteStoreMockRepositoryIntegTestCase extends AbstractSnapshotIntegTestCase {
3938

@@ -107,11 +106,11 @@ public Settings buildRemoteStoreNodeAttributes(Path repoLocation, double ioFailu
107106
.build();
108107
}
109108

110-
protected void deleteRepo() {
111-
logger.info("--> Deleting the repository={}", REPOSITORY_NAME);
112-
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME));
113-
logger.info("--> Deleting the repository={}", TRANSLOG_REPOSITORY_NAME);
114-
assertAcked(clusterAdmin().prepareDeleteRepository(TRANSLOG_REPOSITORY_NAME));
109+
protected void cleanupRepo() {
110+
logger.info("--> Cleanup the repository={}", REPOSITORY_NAME);
111+
clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).execute().actionGet();
112+
logger.info("--> Cleanup the repository={}", TRANSLOG_REPOSITORY_NAME);
113+
clusterAdmin().prepareCleanupRepository(TRANSLOG_REPOSITORY_NAME).execute().actionGet();
115114
}
116115

117116
protected String setup(Path repoLocation, double ioFailureRate, String skipExceptionBlobList, long maxFailure) {
@@ -125,6 +124,8 @@ protected String setup(Path repoLocation, double ioFailureRate, String skipExcep
125124
settings.put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT);
126125
}
127126

127+
disableRepoConsistencyCheck("Remote Store Creates System Repository");
128+
128129
internalCluster().startClusterManagerOnlyNode(settings.build());
129130
String dataNodeName = internalCluster().startDataOnlyNode(settings.build());
130131
createIndex(INDEX_NAME);

‎server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteIndexRecoveryIT.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import java.nio.file.Path;
2424

2525
import static org.opensearch.remotestore.RemoteStoreBaseIntegTestCase.remoteStoreClusterSettings;
26-
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
2726

2827
@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0)
2928
public class RemoteIndexRecoveryIT extends IndexRecoveryIT {
@@ -57,7 +56,7 @@ public Settings indexSettings() {
5756

5857
@After
5958
public void teardown() {
60-
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME));
59+
clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get();
6160
}
6261

6362
@Override

‎server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java

+3-18
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public void setup() {
5555

5656
@After
5757
public void teardown() {
58-
assertAcked(clusterAdmin().prepareDeleteRepository(BASE_REMOTE_REPO));
58+
clusterAdmin().prepareCleanupRepository(BASE_REMOTE_REPO).get();
5959
}
6060

6161
@Override
@@ -440,7 +440,7 @@ public void testRestoreShallowCopySnapshotWithDifferentRepo() throws IOException
440440
assertDocsPresentInIndex(client, restoredIndexName1, numDocsInIndex1 + 2);
441441
}
442442

443-
public void testRestoreShallowSnapshotRepositoryOverriden() throws ExecutionException, InterruptedException {
443+
public void testRestoreShallowSnapshotRepository() throws ExecutionException, InterruptedException {
444444
String indexName1 = "testindex1";
445445
String snapshotRepoName = "test-restore-snapshot-repo";
446446
String remoteStoreRepoNameUpdated = "test-rs-repo-updated" + TEST_REMOTE_STORE_REPO_SUFFIX;
@@ -490,22 +490,7 @@ public void testRestoreShallowSnapshotRepositoryOverriden() throws ExecutionExce
490490
);
491491
assertThat(createSnapshotResponse.getSnapshotInfo().state(), equalTo(SnapshotState.SUCCESS));
492492

493-
createRepository(BASE_REMOTE_REPO, "fs", absolutePath2);
494-
495-
RestoreSnapshotResponse restoreSnapshotResponse = client.admin()
496-
.cluster()
497-
.prepareRestoreSnapshot(snapshotRepoName, snapshotName1)
498-
.setWaitForCompletion(true)
499-
.setIndices(indexName1)
500-
.setRenamePattern(indexName1)
501-
.setRenameReplacement(restoredIndexName1)
502-
.get();
503-
504-
assertTrue(restoreSnapshotResponse.getRestoreInfo().failedShards() > 0);
505-
506-
ensureRed(restoredIndexName1);
507-
508-
client().admin().indices().close(Requests.closeIndexRequest(restoredIndexName1)).get();
493+
client().admin().indices().close(Requests.closeIndexRequest(indexName1)).get();
509494
createRepository(remoteStoreRepoNameUpdated, "fs", remoteRepoPath);
510495
RestoreSnapshotResponse restoreSnapshotResponse2 = client.admin()
511496
.cluster()

‎server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBackpressureIT.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ private void validateBackpressure(
112112
stats = stats();
113113
indexDocAndRefresh(initialSource, initialDocsToIndex);
114114
assertEquals(rejectionCount, stats.rejectionCount);
115-
deleteRepo();
115+
cleanupRepo();
116116
}
117117

118118
private RemoteSegmentTransferTracker.Stats stats() {

‎server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java

+11-4
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.opensearch.index.IndexSettings;
2828
import org.opensearch.index.mapper.MapperService;
2929
import org.opensearch.indices.replication.common.ReplicationType;
30+
import org.opensearch.repositories.RepositoriesService;
3031
import org.opensearch.repositories.blobstore.BlobStoreRepository;
3132
import org.opensearch.repositories.fs.FsRepository;
3233
import org.opensearch.test.OpenSearchIntegTestCase;
@@ -50,7 +51,6 @@
5051
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT;
5152
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY;
5253
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY;
53-
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
5454

5555
public class RemoteStoreBaseIntegTestCase extends OpenSearchIntegTestCase {
5656
protected static final String REPOSITORY_NAME = "test-remote-store-repo";
@@ -314,8 +314,8 @@ public void teardown() {
314314
clusterSettingsSuppliedByTest = false;
315315
assertRemoteStoreRepositoryOnAllNodes(REPOSITORY_NAME);
316316
assertRemoteStoreRepositoryOnAllNodes(REPOSITORY_2_NAME);
317-
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME));
318-
assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_2_NAME));
317+
clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get();
318+
clusterAdmin().prepareCleanupRepository(REPOSITORY_2_NAME).get();
319319
}
320320

321321
public RepositoryMetadata buildRepositoryMetadata(DiscoveryNode node, String name) {
@@ -343,11 +343,18 @@ public void assertRemoteStoreRepositoryOnAllNodes(String repositoryName) {
343343
.custom(RepositoriesMetadata.TYPE);
344344
RepositoryMetadata actualRepository = repositories.repository(repositoryName);
345345

346+
final RepositoriesService repositoriesService = internalCluster().getClusterManagerNodeInstance(RepositoriesService.class);
347+
final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(repositoryName);
348+
346349
for (String nodeName : internalCluster().getNodeNames()) {
347350
ClusterService clusterService = internalCluster().getInstance(ClusterService.class, nodeName);
348351
DiscoveryNode node = clusterService.localNode();
349352
RepositoryMetadata expectedRepository = buildRepositoryMetadata(node, repositoryName);
350-
assertTrue(actualRepository.equalsIgnoreGenerations(expectedRepository));
353+
354+
// Validated that all the restricted settings are entact on all the nodes.
355+
repository.getRestrictedSystemRepositorySettings()
356+
.stream()
357+
.forEach(setting -> assertEquals(setting.get(actualRepository.settings()), setting.get(expectedRepository.settings())));
351358
}
352359
}
353360

‎server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRefreshListenerIT.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ public void testRemoteRefreshRetryOnFailure() throws Exception {
5656
logger.info("Local files = {}, Repo files = {}", sortedFilesInLocal, sortedFilesInRepo);
5757
assertTrue(filesInRepo.containsAll(filesInLocal));
5858
}, 90, TimeUnit.SECONDS);
59-
deleteRepo();
59+
cleanupRepo();
6060
}
6161

6262
public void testRemoteRefreshSegmentPressureSettingChanged() {

0 commit comments

Comments
 (0)