From 07e91421897152ba9d6213ba1d9ac25e8e8bfdf8 Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Sat, 22 Jul 2023 13:52:15 +0530 Subject: [PATCH 01/12] Adding @sohami to OpenSearch maintainers Signed-off-by: Bukhtawar Khan --- .github/CODEOWNERS | 2 +- MAINTAINERS.md | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index e5d34bfc1e6dd..1a108c35429ae 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1 +1 @@ -* @reta @anasalkouz @andrross @reta @Bukhtawar @CEHENKLE @dblock @gbbafna @setiah @kartg @kotwanikunal @mch2 @nknize @owaiskazi19 @Rishikesh1159 @ryanbogan @saratvemulapalli @shwetathareja @dreamer-89 @tlfeng @VachaShah @dbwiddis @sachinpkale +* @reta @anasalkouz @andrross @reta @Bukhtawar @CEHENKLE @dblock @gbbafna @setiah @kartg @kotwanikunal @mch2 @nknize @owaiskazi19 @Rishikesh1159 @ryanbogan @saratvemulapalli @shwetathareja @dreamer-89 @tlfeng @VachaShah @dbwiddis @sachinpkale @sohami diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 8027c5daffc69..95e87f4be43bf 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -25,6 +25,7 @@ This document contains a list of maintainers in this repo. See [opensearch-proje | Sachin Kale | [sachinpkale](https://github.com/sachinpkale) | Amazon | | Sarat Vemulapalli | [saratvemulapalli](https://github.com/saratvemulapalli) | Amazon | | Shweta Thareja | [shwetathareja](https://github.com/shwetathareja) | Amazon | +| Sorabh Hamirwasia | [sohami](https://github.com/sohami) | Amazon | | Suraj Singh | [dreamer-89](https://github.com/dreamer-89) | Amazon | | Tianli Feng | [tlfeng](https://github.com/tlfeng) | Amazon | | Vacha Shah | [VachaShah](https://github.com/VachaShah) | Amazon | From a5d0efc20cca6452926096962b8d5b19914bfec1 Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Thu, 3 Aug 2023 18:31:36 +0530 Subject: [PATCH 02/12] Introduce system repository and wire up translog and segment store Signed-off-by: Bukhtawar Khan --- .../repositories/s3/S3Repository.java | 10 +++++ .../put/TransportPutRepositoryAction.java | 2 +- .../RemoteSegmentStoreDirectoryFactory.java | 2 +- ...emoteBlobStoreInternalTranslogFactory.java | 2 +- .../repositories/FilterRepository.java | 5 +++ .../repositories/RepositoriesService.java | 45 ++++++++++++++++++- .../opensearch/repositories/Repository.java | 18 ++++++++ .../blobstore/BlobStoreRepository.java | 22 +++++++++ .../RepositoriesServiceTests.java | 2 +- 9 files changed, 102 insertions(+), 6 deletions(-) diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java index f98b775d9ce4b..7461a530573b3 100644 --- a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java @@ -44,6 +44,7 @@ import org.opensearch.common.blobstore.BlobStore; import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.common.settings.SecureSetting; +import org.opensearch.common.settings.Settings; import org.opensearch.core.common.settings.SecureString; import org.opensearch.common.settings.Setting; import org.opensearch.core.common.unit.ByteSizeUnit; @@ -63,6 +64,8 @@ import org.opensearch.threadpool.Scheduler; import java.util.Collection; +import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; @@ -388,6 +391,13 @@ protected ByteSizeValue chunkSize() { return chunkSize; } + @Override + public List restrictedSystemRepositorySettings() { + List restrictedSystemRepositorySettings = super.restrictedSystemRepositorySettings(); + restrictedSystemRepositorySettings.addAll(List.of(BUCKET_SETTING, BASE_PATH_SETTING)); + return Collections.unmodifiableList(restrictedSystemRepositorySettings); + } + @Override protected void doClose() { final Scheduler.Cancellable cancellable = finalizationFuture.getAndSet(null); diff --git a/server/src/main/java/org/opensearch/action/admin/cluster/repositories/put/TransportPutRepositoryAction.java b/server/src/main/java/org/opensearch/action/admin/cluster/repositories/put/TransportPutRepositoryAction.java index 16f7d6d5700bf..992d34b3859b7 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/repositories/put/TransportPutRepositoryAction.java +++ b/server/src/main/java/org/opensearch/action/admin/cluster/repositories/put/TransportPutRepositoryAction.java @@ -100,7 +100,7 @@ protected void clusterManagerOperation( ClusterState state, final ActionListener listener ) { - repositoriesService.registerRepository( + repositoriesService.registerOrUpdateRepository( request, ActionListener.delegateFailure( listener, diff --git a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java index 3bec84f287ce4..6a3a93f1ed0e6 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java @@ -52,7 +52,7 @@ public Directory newDirectory(IndexSettings indexSettings, ShardPath path) throw } public Directory newDirectory(String repositoryName, String indexUUID, String shardId) throws IOException { - try (Repository repository = repositoriesService.get().repository(repositoryName)) { + try (Repository repository = repositoriesService.get().getSystemRepository(repositoryName)) { assert repository instanceof BlobStoreRepository : "repository should be instance of BlobStoreRepository"; BlobPath commonBlobPath = ((BlobStoreRepository) repository).basePath(); commonBlobPath = commonBlobPath.add(indexUUID).add(shardId).add(SEGMENTS); diff --git a/server/src/main/java/org/opensearch/index/translog/RemoteBlobStoreInternalTranslogFactory.java b/server/src/main/java/org/opensearch/index/translog/RemoteBlobStoreInternalTranslogFactory.java index 339e16db6f360..1c0a8c6f45168 100644 --- a/server/src/main/java/org/opensearch/index/translog/RemoteBlobStoreInternalTranslogFactory.java +++ b/server/src/main/java/org/opensearch/index/translog/RemoteBlobStoreInternalTranslogFactory.java @@ -38,7 +38,7 @@ public RemoteBlobStoreInternalTranslogFactory( ) { Repository repository; try { - repository = repositoriesServiceSupplier.get().repository(repositoryName); + repository = repositoriesServiceSupplier.get().getSystemRepository(repositoryName); } catch (RepositoryMissingException ex) { throw new IllegalArgumentException("Repository should be created before creating index with remote_store enabled setting", ex); } diff --git a/server/src/main/java/org/opensearch/repositories/FilterRepository.java b/server/src/main/java/org/opensearch/repositories/FilterRepository.java index 52e7f374507d4..27d473e48feda 100644 --- a/server/src/main/java/org/opensearch/repositories/FilterRepository.java +++ b/server/src/main/java/org/opensearch/repositories/FilterRepository.java @@ -157,6 +157,11 @@ public boolean isReadOnly() { return in.isReadOnly(); } + @Override + public boolean isSystemRepository() { + return in.isSystemRepository(); + } + @Override public void snapshotShard( Store store, diff --git a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java index e66f8ddee5678..8f56ff53ff9f3 100644 --- a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java @@ -82,6 +82,7 @@ import java.util.stream.Stream; import static org.opensearch.repositories.blobstore.BlobStoreRepository.REMOTE_STORE_INDEX_SHALLOW_COPY; +import static org.opensearch.repositories.blobstore.BlobStoreRepository.SYSTEM_REPOSITORY_SETTING; /** * Service responsible for maintaining and providing access to snapshot repositories on nodes. @@ -159,7 +160,7 @@ public RepositoriesService( * @param request register repository request * @param listener register repository listener */ - public void registerRepository(final PutRepositoryRequest request, final ActionListener listener) { + public void registerOrUpdateRepository(final PutRepositoryRequest request, final ActionListener listener) { assert lifecycle.started() : "Trying to register new repository but service is in state [" + lifecycle.state() + "]"; final RepositoryMetadata newRepositoryMetadata = new RepositoryMetadata(request.name(), request.type(), request.settings()); @@ -216,14 +217,18 @@ public ClusterState execute(ClusterState currentState) { } else { boolean found = false; List repositoriesMetadata = new ArrayList<>(repositories.repositories().size() + 1); - + RepositoryMetadata currentRepositoryMetadata = null; for (RepositoryMetadata repositoryMetadata : repositories.repositories()) { if (repositoryMetadata.name().equals(newRepositoryMetadata.name())) { if (newRepositoryMetadata.equalsIgnoreGenerations(repositoryMetadata)) { // Previous version is the same as this one no update is needed. return currentState; + } else if (SYSTEM_REPOSITORY_SETTING.get(repositoryMetadata.settings()) + && SYSTEM_REPOSITORY_SETTING.get(newRepositoryMetadata.settings()) == false) { + throw new IllegalStateException("trying to modify system repository attribute for a repository"); } found = true; + currentRepositoryMetadata = repositoryMetadata; repositoriesMetadata.add(newRepositoryMetadata); } else { repositoriesMetadata.add(repositoryMetadata); @@ -231,9 +236,11 @@ public ClusterState execute(ClusterState currentState) { } if (!found) { logger.info("put repository [{}]", request.name()); + ensureNonSystemRepository(newRepositoryMetadata.settings(), newRepositoryMetadata.name()); repositoriesMetadata.add(new RepositoryMetadata(request.name(), request.type(), request.settings())); } else { logger.info("update repository [{}]", request.name()); + validateRestrictedSystemRespositorySettings(currentRepositoryMetadata, newRepositoryMetadata); } repositories = new RepositoriesMetadata(repositoriesMetadata); } @@ -261,6 +268,17 @@ public boolean mustAck(DiscoveryNode discoveryNode) { ); } + private void validateRestrictedSystemRespositorySettings(RepositoryMetadata currentRepositoryMetadata, RepositoryMetadata newRepositoryMetadata) { + if (SYSTEM_REPOSITORY_SETTING.get(newRepositoryMetadata.settings())) { + Repository repository = repositories.get(currentRepositoryMetadata.name()); + for (Setting setting : repository.restrictedSystemRepositorySettings()) { + if (currentRepositoryMetadata.settings().get(setting.getKey()).equals(newRepositoryMetadata.settings().get(setting.getKey())) == false) { + throw new RepositoryException(repository.getMetadata().name(), "trying to modify immutable property " + setting.getKey()); + } + } + } + } + /** * Unregisters repository in the cluster *

@@ -288,6 +306,7 @@ public ClusterState execute(ClusterState currentState) { boolean changed = false; for (RepositoryMetadata repositoryMetadata : repositories.repositories()) { if (Regex.simpleMatch(request.name(), repositoryMetadata.name())) { + ensureNonSystemRepository(repositoryMetadata.settings(), request.name()); ensureRepositoryNotInUse(currentState, repositoryMetadata.name()); logger.info("delete repository [{}]", repositoryMetadata.name()); changed = true; @@ -499,6 +518,22 @@ public Repository repository(String repositoryName) { throw new RepositoryMissingException(repositoryName); } + /** + * Returns registered system repository + *

* + * @param repositoryName repository name + * @return registered system repository + * @throws RepositoryMissingException if repository with such name isn't registered + */ + public Repository getSystemRepository(String repositoryName) { + Repository repository = repositories.get(repositoryName); + if (repository != null && SYSTEM_REPOSITORY_SETTING.get(repository.getMetadata().settings())) { + return repository; + } + throw new RepositoryMissingException(repositoryName); + } + + public List repositoriesStats() { List archivedRepoStats = repositoriesStatsArchive.getArchivedStats(); List activeRepoStats = getRepositoryStatsForActiveRepositories(); @@ -610,6 +645,12 @@ private static void validate(final String repositoryName) { } } + public static void ensureNonSystemRepository(Settings repoSettings, String repository) { + if (SYSTEM_REPOSITORY_SETTING.get(repoSettings)) { + throw new RepositoryException(repository, "cannot register a system repository externally"); + } + } + public static void validateRepositoryMetadataSettings( ClusterService clusterService, final String repositoryName, diff --git a/server/src/main/java/org/opensearch/repositories/Repository.java b/server/src/main/java/org/opensearch/repositories/Repository.java index 683177b3c3639..856a6c883fdc3 100644 --- a/server/src/main/java/org/opensearch/repositories/Repository.java +++ b/server/src/main/java/org/opensearch/repositories/Repository.java @@ -43,6 +43,7 @@ import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.common.Nullable; import org.opensearch.common.lifecycle.LifecycleComponent; +import org.opensearch.common.settings.Setting; import org.opensearch.index.mapper.MapperService; import org.opensearch.core.index.shard.ShardId; import org.opensearch.index.snapshots.IndexShardSnapshotStatus; @@ -55,6 +56,7 @@ import java.io.IOException; import java.util.Collection; +import java.util.List; import java.util.Map; import java.util.function.Consumer; import java.util.function.Function; @@ -237,6 +239,14 @@ default RepositoryStats stats() { */ boolean isReadOnly(); + /** + * Returns true if the repository is managed by the system directly and doesn't allow managing the lifetime of the + * repository through external APIs + * @return true if the repository is system managed + */ + boolean isSystemRepository(); + + /** * Creates a snapshot of the shard based on the index commit point. *

@@ -341,6 +351,14 @@ default RemoteStoreShardShallowCopySnapshot getRemoteStoreShallowCopyShardMetada throw new UnsupportedOperationException(); } + /** + * Returns the list of restricted system repository settings that cannot be mutated once repository is created* + * @return the list of settings + */ + default List restrictedSystemRepositorySettings() { + throw new UnsupportedOperationException(); + } + /** * Retrieve shard snapshot status for the stored snapshot * diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java index 70db2e0c0a9bd..ec338c953d3ff 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -75,6 +75,7 @@ import org.opensearch.common.blobstore.BlobStore; import org.opensearch.common.blobstore.DeleteResult; import org.opensearch.common.blobstore.fs.FsBlobContainer; +import org.opensearch.common.util.CollectionUtils; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.common.collect.Tuple; @@ -282,6 +283,8 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp */ public static final Setting READONLY_SETTING = Setting.boolSetting("readonly", false, Setting.Property.NodeScope); + public static final Setting SYSTEM_REPOSITORY_SETTING = Setting.boolSetting("system_repository", false, Setting.Property.NodeScope); + protected final boolean supportURLRepo; private final int maxShardBlobDeleteBatch; @@ -335,6 +338,8 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp private final boolean readOnly; + private final boolean isSystemRepository; + private final Object lock = new Object(); private final SetOnce blobContainer = new SetOnce<>(); @@ -398,6 +403,7 @@ protected BlobStoreRepository( snapshotRateLimiter = getRateLimiter(metadata.settings(), "max_snapshot_bytes_per_sec", new ByteSizeValue(40, ByteSizeUnit.MB)); restoreRateLimiter = getRateLimiter(metadata.settings(), "max_restore_bytes_per_sec", ByteSizeValue.ZERO); readOnly = READONLY_SETTING.get(metadata.settings()); + isSystemRepository = SYSTEM_REPOSITORY_SETTING.get(metadata.settings()); cacheRepositoryData = CACHE_REPOSITORY_DATA.get(metadata.settings()); bufferSize = Math.toIntExact(BUFFER_SIZE_SETTING.get(metadata.settings()).getBytes()); maxShardBlobDeleteBatch = MAX_SNAPSHOT_SHARD_BLOB_DELETE_BATCH_SIZE.get(metadata.settings()); @@ -783,6 +789,17 @@ protected ByteSizeValue chunkSize() { return null; } + /** + * Returns the list of restricted system repository settings that cannot be mutated once repository is created* + * @return the list of settings + */ + @Override + public List restrictedSystemRepositorySettings() { + List settings = List.of(SYSTEM_REPOSITORY_SETTING, REMOTE_STORE_INDEX_SHALLOW_COPY); + return settings; + } + + @Override public RepositoryMetadata getMetadata() { return metadata; @@ -2074,6 +2091,11 @@ public boolean isReadOnly() { return readOnly; } + @Override + public boolean isSystemRepository() { + return isSystemRepository; + } + /** * Writing a new index generation is a three step process. * First, the {@link RepositoryMetadata} entry for this repository is set into a pending state by incrementing its diff --git a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java index 085a64b439bbe..34aa0d41b13f5 100644 --- a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java @@ -208,7 +208,7 @@ private ClusterState emptyState() { private void assertThrowsOnRegister(String repoName) { PutRepositoryRequest request = new PutRepositoryRequest(repoName); - expectThrows(RepositoryException.class, () -> repositoriesService.registerRepository(request, null)); + expectThrows(RepositoryException.class, () -> repositoriesService.registerOrUpdateRepository(request, null)); } private static class TestRepository implements Repository { From 5cf828b95af0f3b41841f67ea09f680c0bffa7b8 Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Thu, 3 Aug 2023 18:31:36 +0530 Subject: [PATCH 03/12] Introduce system repository and wire up translog and segment store Signed-off-by: Bukhtawar Khan --- .../repositories/RepositoriesServiceIT.java | 45 +++++++++++++++++++ .../RemoteSegmentStoreDirectoryFactory.java | 3 +- ...emoteBlobStoreInternalTranslogFactory.java | 3 +- .../repositories/RepositoriesService.java | 28 ++++++++---- .../opensearch/repositories/Repository.java | 4 +- .../blobstore/BlobStoreRepository.java | 7 ++- .../RepositoriesServiceTests.java | 5 +++ .../index/shard/RestoreOnlyRepository.java | 5 +++ 8 files changed, 85 insertions(+), 15 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java b/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java index 84178f0255d81..4e725964bec02 100644 --- a/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java @@ -108,4 +108,49 @@ public void testUpdateRepository() { final Repository updatedRepository = repositoriesService.repository(repositoryName); assertThat(updatedRepository, updated ? not(sameInstance(originalRepository)) : sameInstance(originalRepository)); } + + public void testExternalSystemRepositoryCreationFails() { + final String repositoryName = "test-system-repo"; + final Client client = client(); + + final Settings.Builder repoSettings = Settings.builder().put("location", randomRepoPath()).put("system_repository", true); + + assertThrows( + "[" + repositoryName + "] cannot register a system repository externally", + RepositoryException.class, + () -> client.admin().cluster().preparePutRepository(repositoryName).setType(FsRepository.TYPE).setSettings(repoSettings).get() + ); + + assertThrows(RepositoryException.class, () -> client.admin().cluster().prepareGetRepositories(repositoryName).get()); + } + + public void testSystemRepositorySettingsUpdationFails() { + final InternalTestCluster cluster = internalCluster(); + final String repositoryName = "test-repo"; + final Client client = client(); + + final RepositoriesService repositoriesService = cluster.getDataOrClusterManagerNodeInstances(RepositoriesService.class) + .iterator() + .next(); + final Settings.Builder repoSettings = Settings.builder().put("location", randomRepoPath()); + + assertAcked( + client.admin().cluster().preparePutRepository(repositoryName).setType(FsRepository.TYPE).setSettings(repoSettings).get() + ); + + final Settings.Builder updatedRepoSettings = Settings.builder().put("location", randomRepoPath()).put("system_repository", true); + + assertThrows( + "[" + repositoryName + "] trying to modify system repository attribute for a repository", + RepositoryException.class, + () -> client.admin() + .cluster() + .preparePutRepository(repositoryName) + .setType(FsRepository.TYPE) + .setSettings(updatedRepoSettings) + .get() + ); + assertThrows(RepositoryException.class, () -> repositoriesService.getSystemRepository(repositoryName)); + assertThat(repositoriesService.repository(repositoryName), instanceOf(FsRepository.class)); + } } diff --git a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java index 6a3a93f1ed0e6..dfeb51c831693 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java @@ -52,7 +52,8 @@ public Directory newDirectory(IndexSettings indexSettings, ShardPath path) throw } public Directory newDirectory(String repositoryName, String indexUUID, String shardId) throws IOException { - try (Repository repository = repositoriesService.get().getSystemRepository(repositoryName)) { + // TODO switch to using {@RepositoriesService#getSystemRespository} once repository auto-bootstrap changes are in + try (Repository repository = repositoriesService.get().repository(repositoryName)) { assert repository instanceof BlobStoreRepository : "repository should be instance of BlobStoreRepository"; BlobPath commonBlobPath = ((BlobStoreRepository) repository).basePath(); commonBlobPath = commonBlobPath.add(indexUUID).add(shardId).add(SEGMENTS); diff --git a/server/src/main/java/org/opensearch/index/translog/RemoteBlobStoreInternalTranslogFactory.java b/server/src/main/java/org/opensearch/index/translog/RemoteBlobStoreInternalTranslogFactory.java index 1c0a8c6f45168..cb2a208eb1dc8 100644 --- a/server/src/main/java/org/opensearch/index/translog/RemoteBlobStoreInternalTranslogFactory.java +++ b/server/src/main/java/org/opensearch/index/translog/RemoteBlobStoreInternalTranslogFactory.java @@ -38,7 +38,8 @@ public RemoteBlobStoreInternalTranslogFactory( ) { Repository repository; try { - repository = repositoriesServiceSupplier.get().getSystemRepository(repositoryName); + // TODO switch to using {@RepositoriesService#getSystemRespository} once repository auto-bootstrap changes are in + repository = repositoriesServiceSupplier.get().repository(repositoryName); } catch (RepositoryMissingException ex) { throw new IllegalArgumentException("Repository should be created before creating index with remote_store enabled setting", ex); } diff --git a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java index 8f56ff53ff9f3..59f2d77a0f954 100644 --- a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java @@ -211,6 +211,7 @@ public ClusterState execute(ClusterState currentState) { RepositoriesMetadata repositories = metadata.custom(RepositoriesMetadata.TYPE); if (repositories == null) { logger.info("put repository [{}]", request.name()); + ensureNonSystemRepository(request.settings(), request.name()); repositories = new RepositoriesMetadata( Collections.singletonList(new RepositoryMetadata(request.name(), request.type(), request.settings())) ); @@ -223,9 +224,13 @@ public ClusterState execute(ClusterState currentState) { if (newRepositoryMetadata.equalsIgnoreGenerations(repositoryMetadata)) { // Previous version is the same as this one no update is needed. return currentState; - } else if (SYSTEM_REPOSITORY_SETTING.get(repositoryMetadata.settings()) - && SYSTEM_REPOSITORY_SETTING.get(newRepositoryMetadata.settings()) == false) { - throw new IllegalStateException("trying to modify system repository attribute for a repository"); + } else if (SYSTEM_REPOSITORY_SETTING.get(repositoryMetadata.settings()) != SYSTEM_REPOSITORY_SETTING.get( + newRepositoryMetadata.settings() + )) { + throw new RepositoryException( + repositoryMetadata.name(), + "trying to modify system repository attribute for a repository" + ); } found = true; currentRepositoryMetadata = repositoryMetadata; @@ -240,7 +245,7 @@ public ClusterState execute(ClusterState currentState) { repositoriesMetadata.add(new RepositoryMetadata(request.name(), request.type(), request.settings())); } else { logger.info("update repository [{}]", request.name()); - validateRestrictedSystemRespositorySettings(currentRepositoryMetadata, newRepositoryMetadata); + validateRestrictedSystemRepositorySettings(currentRepositoryMetadata, newRepositoryMetadata); } repositories = new RepositoriesMetadata(repositoriesMetadata); } @@ -268,12 +273,18 @@ public boolean mustAck(DiscoveryNode discoveryNode) { ); } - private void validateRestrictedSystemRespositorySettings(RepositoryMetadata currentRepositoryMetadata, RepositoryMetadata newRepositoryMetadata) { - if (SYSTEM_REPOSITORY_SETTING.get(newRepositoryMetadata.settings())) { + private void validateRestrictedSystemRepositorySettings( + RepositoryMetadata currentRepositoryMetadata, + RepositoryMetadata newRepositoryMetadata + ) { + if (SYSTEM_REPOSITORY_SETTING.get(currentRepositoryMetadata.settings())) { Repository repository = repositories.get(currentRepositoryMetadata.name()); for (Setting setting : repository.restrictedSystemRepositorySettings()) { - if (currentRepositoryMetadata.settings().get(setting.getKey()).equals(newRepositoryMetadata.settings().get(setting.getKey())) == false) { - throw new RepositoryException(repository.getMetadata().name(), "trying to modify immutable property " + setting.getKey()); + if (newRepositoryMetadata.settings().get(setting.getKey()) != null) { + throw new RepositoryException( + repository.getMetadata().name(), + "trying to modify restricted system repository settings " + setting.getKey() + ); } } } @@ -444,7 +455,6 @@ public void applyClusterState(ClusterChangedEvent event) { || previousMetadata.settings().equals(repositoryMetadata.settings()) == false) { // Previous version is different from the version in settings logger.debug("updating repository [{}]", repositoryMetadata.name()); - closeRepository(repository); archiveRepositoryStats(repository, state.version()); repository = null; try { diff --git a/server/src/main/java/org/opensearch/repositories/Repository.java b/server/src/main/java/org/opensearch/repositories/Repository.java index 856a6c883fdc3..052d38512f6e3 100644 --- a/server/src/main/java/org/opensearch/repositories/Repository.java +++ b/server/src/main/java/org/opensearch/repositories/Repository.java @@ -56,6 +56,7 @@ import java.io.IOException; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.function.Consumer; @@ -246,7 +247,6 @@ default RepositoryStats stats() { */ boolean isSystemRepository(); - /** * Creates a snapshot of the shard based on the index commit point. *

@@ -356,7 +356,7 @@ default RemoteStoreShardShallowCopySnapshot getRemoteStoreShallowCopyShardMetada * @return the list of settings */ default List restrictedSystemRepositorySettings() { - throw new UnsupportedOperationException(); + return Collections.emptyList(); } /** diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java index ec338c953d3ff..23f9c22a5b61e 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -283,7 +283,11 @@ public abstract class BlobStoreRepository extends AbstractLifecycleComponent imp */ public static final Setting READONLY_SETTING = Setting.boolSetting("readonly", false, Setting.Property.NodeScope); - public static final Setting SYSTEM_REPOSITORY_SETTING = Setting.boolSetting("system_repository", false, Setting.Property.NodeScope); + public static final Setting SYSTEM_REPOSITORY_SETTING = Setting.boolSetting( + "system_repository", + false, + Setting.Property.NodeScope + ); protected final boolean supportURLRepo; @@ -799,7 +803,6 @@ public List restrictedSystemRepositorySettings() { return settings; } - @Override public RepositoryMetadata getMetadata() { return metadata; diff --git a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java index 34aa0d41b13f5..1c1177f3a9d3d 100644 --- a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java @@ -301,6 +301,11 @@ public boolean isReadOnly() { return false; } + @Override + public boolean isSystemRepository() { + return false; + } + @Override public void snapshotShard( Store store, diff --git a/test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java b/test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java index 3744b06c35a72..1ac4e4f94453b 100644 --- a/test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java +++ b/test/framework/src/main/java/org/opensearch/index/shard/RestoreOnlyRepository.java @@ -163,6 +163,11 @@ public boolean isReadOnly() { return false; } + @Override + public boolean isSystemRepository() { + return false; + } + @Override public void snapshotShard( Store store, From 12d070f1e13394f748beb2d4e8ce47ae654ce83c Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Sun, 6 Aug 2023 22:09:49 +0530 Subject: [PATCH 04/12] Minor fix up Signed-off-by: Bukhtawar Khan --- .../java/org/opensearch/repositories/RepositoriesService.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java index 59f2d77a0f954..df043669188d1 100644 --- a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java @@ -455,6 +455,7 @@ public void applyClusterState(ClusterChangedEvent event) { || previousMetadata.settings().equals(repositoryMetadata.settings()) == false) { // Previous version is different from the version in settings logger.debug("updating repository [{}]", repositoryMetadata.name()); + closeRepository(repository); archiveRepositoryStats(repository, state.version()); repository = null; try { From c92eddd39781878d8191b69677d80b7cf56e5cce Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Sun, 6 Aug 2023 22:26:41 +0530 Subject: [PATCH 05/12] Merge conflicts Signed-off-by: Bukhtawar Khan --- CHANGELOG.md | 1 + .../java/org/opensearch/repositories/RepositoriesService.java | 1 - .../opensearch/repositories/blobstore/BlobStoreRepository.java | 1 - 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bb21fb425ccf..9a1d498cac9ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Remote Store] Add support to restore only unassigned shards of an index ([#8792](https://github.com/opensearch-project/OpenSearch/pull/8792)) - Replace the deprecated IndexReader APIs with new storedFields() & termVectors() ([#7792](https://github.com/opensearch-project/OpenSearch/pull/7792)) - [Remote Store] Restrict user override for remote store index level settings ([#8812](https://github.com/opensearch-project/OpenSearch/pull/8812)) +- [Remote Store] Introduce System Repository for Remote Store ([#9088](https://github.com/opensearch-project/OpenSearch/pull/9088/)) ### Deprecated diff --git a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java index f56db2458a05b..9a7cf633493e7 100644 --- a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java @@ -544,7 +544,6 @@ public Repository getSystemRepository(String repositoryName) { throw new RepositoryMissingException(repositoryName); } - public List repositoriesStats() { List archivedRepoStats = repositoriesStatsArchive.getArchivedStats(); List activeRepoStats = getRepositoryStatsForActiveRepositories(); diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java index 4abbe02afc7ef..8bbb8391e5d72 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -74,7 +74,6 @@ import org.opensearch.common.blobstore.BlobStore; import org.opensearch.common.blobstore.DeleteResult; import org.opensearch.common.blobstore.fs.FsBlobContainer; -import org.opensearch.common.util.CollectionUtils; import org.opensearch.core.common.Strings; import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; From 84d7030d8f0f5438ee7d75b52efacfca1443101f Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Sun, 6 Aug 2023 22:36:20 +0530 Subject: [PATCH 06/12] Fixup spotless Signed-off-by: Bukhtawar Khan --- .../main/java/org/opensearch/repositories/s3/S3Repository.java | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java index 7461a530573b3..0bb7e4142588c 100644 --- a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java @@ -44,7 +44,6 @@ import org.opensearch.common.blobstore.BlobStore; import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.common.settings.SecureSetting; -import org.opensearch.common.settings.Settings; import org.opensearch.core.common.settings.SecureString; import org.opensearch.common.settings.Setting; import org.opensearch.core.common.unit.ByteSizeUnit; From 338cb55e7143c981e72e2640c521c9a4f2dc5a20 Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Sun, 6 Aug 2023 23:07:45 +0530 Subject: [PATCH 07/12] Fixup spotless Signed-off-by: Bukhtawar Khan --- .../java/org/opensearch/repositories/s3/S3Repository.java | 7 ++++--- .../org/opensearch/repositories/RepositoriesService.java | 2 +- .../main/java/org/opensearch/repositories/Repository.java | 2 +- .../repositories/blobstore/BlobStoreRepository.java | 5 +++-- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java index 0bb7e4142588c..b45e3f69249f0 100644 --- a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java @@ -62,6 +62,7 @@ import org.opensearch.snapshots.SnapshotInfo; import org.opensearch.threadpool.Scheduler; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -391,9 +392,9 @@ protected ByteSizeValue chunkSize() { } @Override - public List restrictedSystemRepositorySettings() { - List restrictedSystemRepositorySettings = super.restrictedSystemRepositorySettings(); - restrictedSystemRepositorySettings.addAll(List.of(BUCKET_SETTING, BASE_PATH_SETTING)); + public List> restrictedSystemRepositorySettings() { + List> restrictedSystemRepositorySettings = super.restrictedSystemRepositorySettings(); + restrictedSystemRepositorySettings.addAll(Arrays.asList(BUCKET_SETTING, BASE_PATH_SETTING)); return Collections.unmodifiableList(restrictedSystemRepositorySettings); } diff --git a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java index 9a7cf633493e7..76b447948b115 100644 --- a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java @@ -279,7 +279,7 @@ private void validateRestrictedSystemRepositorySettings( ) { if (SYSTEM_REPOSITORY_SETTING.get(currentRepositoryMetadata.settings())) { Repository repository = repositories.get(currentRepositoryMetadata.name()); - for (Setting setting : repository.restrictedSystemRepositorySettings()) { + for (Setting setting : repository.restrictedSystemRepositorySettings()) { if (newRepositoryMetadata.settings().get(setting.getKey()) != null) { throw new RepositoryException( repository.getMetadata().name(), diff --git a/server/src/main/java/org/opensearch/repositories/Repository.java b/server/src/main/java/org/opensearch/repositories/Repository.java index 052d38512f6e3..edcc5a74f58e8 100644 --- a/server/src/main/java/org/opensearch/repositories/Repository.java +++ b/server/src/main/java/org/opensearch/repositories/Repository.java @@ -355,7 +355,7 @@ default RemoteStoreShardShallowCopySnapshot getRemoteStoreShallowCopyShardMetada * Returns the list of restricted system repository settings that cannot be mutated once repository is created* * @return the list of settings */ - default List restrictedSystemRepositorySettings() { + default List> restrictedSystemRepositorySettings() { return Collections.emptyList(); } diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java index 8bbb8391e5d72..8f844decb4ae2 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -145,6 +145,7 @@ import java.io.InputStream; import java.nio.file.NoSuchFileException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -798,8 +799,8 @@ protected ByteSizeValue chunkSize() { * @return the list of settings */ @Override - public List restrictedSystemRepositorySettings() { - List settings = List.of(SYSTEM_REPOSITORY_SETTING, REMOTE_STORE_INDEX_SHALLOW_COPY); + public List> restrictedSystemRepositorySettings() { + List> settings = Arrays.asList(SYSTEM_REPOSITORY_SETTING, REMOTE_STORE_INDEX_SHALLOW_COPY); return settings; } From 7cdc767692fd60370ca9132e27a62b57d9d4889d Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Mon, 7 Aug 2023 14:41:01 +0530 Subject: [PATCH 08/12] Restricted system repository tests Signed-off-by: Bukhtawar Khan --- .../repositories/RepositoriesServiceIT.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java b/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java index 4e725964bec02..2f3f24b8d7e5e 100644 --- a/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/repositories/RepositoriesServiceIT.java @@ -35,6 +35,7 @@ import org.opensearch.action.admin.cluster.repositories.get.GetRepositoriesResponse; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.RepositoryMetadata; +import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.plugins.Plugin; import org.opensearch.repositories.fs.FsRepository; @@ -44,6 +45,7 @@ import java.util.Collection; import java.util.Collections; +import java.util.List; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; @@ -153,4 +155,22 @@ public void testSystemRepositorySettingsUpdationFails() { assertThrows(RepositoryException.class, () -> repositoriesService.getSystemRepository(repositoryName)); assertThat(repositoriesService.repository(repositoryName), instanceOf(FsRepository.class)); } + + public void testRestrictedSystemSettings() { + final InternalTestCluster cluster = internalCluster(); + final String repositoryName = "test-repo"; + final Client client = client(); + + final RepositoriesService repositoriesService = cluster.getDataOrClusterManagerNodeInstances(RepositoriesService.class) + .iterator() + .next(); + final Settings.Builder repoSettings = Settings.builder().put("location", randomRepoPath()); + + assertAcked( + client.admin().cluster().preparePutRepository(repositoryName).setType(FsRepository.TYPE).setSettings(repoSettings).get() + ); + List> restrictedSettings = repositoriesService.repository(repositoryName).restrictedSystemRepositorySettings(); + assertThat(restrictedSettings, hasSize(2)); + + } } From 23a82f728855d49046d9d46e8dd907190e1adc9c Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Mon, 7 Aug 2023 19:59:57 +0530 Subject: [PATCH 09/12] respository-s3 tests Signed-off-by: Bukhtawar Khan --- .../opensearch/repositories/s3/S3Repository.java | 5 ++++- .../repositories/s3/S3RepositoryTests.java | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java index b45e3f69249f0..f60f38d47275b 100644 --- a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java @@ -62,6 +62,7 @@ import org.opensearch.snapshots.SnapshotInfo; import org.opensearch.threadpool.Scheduler; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -393,8 +394,10 @@ protected ByteSizeValue chunkSize() { @Override public List> restrictedSystemRepositorySettings() { + List> settings = new ArrayList<>(); List> restrictedSystemRepositorySettings = super.restrictedSystemRepositorySettings(); - restrictedSystemRepositorySettings.addAll(Arrays.asList(BUCKET_SETTING, BASE_PATH_SETTING)); + settings.addAll(restrictedSystemRepositorySettings); + settings.addAll(new ArrayList<>(List.of(BUCKET_SETTING, BASE_PATH_SETTING))); return Collections.unmodifiableList(restrictedSystemRepositorySettings); } diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java index e5fd9e5caab9c..8369eae5053b3 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java @@ -49,6 +49,7 @@ import java.util.Map; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; @@ -132,6 +133,21 @@ public void testDefaultBufferSize() { } } + public void testCreateS3SystemRepository() { + final RepositoryMetadata metadata = new RepositoryMetadata( + "dummy-repo", + "mock", + Settings.builder() + .put(S3Repository.BASE_PATH_SETTING.getKey(), "foo/bar") + .put(S3Repository.BUCKET_SETTING.getKey(), "bucket") + .put(S3Repository.SYSTEM_REPOSITORY_SETTING.getKey(), true) + .build()); + try (S3Repository s3repo = createS3Repo(metadata)) { + assertTrue(s3repo.isSystemRepository()); + assertThat(s3repo.restrictedSystemRepositorySettings(), hasSize(2)); + } + } + private S3Repository createS3Repo(RepositoryMetadata metadata) { return new S3Repository( metadata, From 137d05d4418409eab1e06964c4611f703dc1dd99 Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Mon, 7 Aug 2023 20:17:39 +0530 Subject: [PATCH 10/12] Spotless check Signed-off-by: Bukhtawar Khan --- .../main/java/org/opensearch/repositories/s3/S3Repository.java | 1 - .../java/org/opensearch/repositories/s3/S3RepositoryTests.java | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java index f60f38d47275b..e095cd15efd5c 100644 --- a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java @@ -63,7 +63,6 @@ import org.opensearch.threadpool.Scheduler; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.List; diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java index 8369eae5053b3..0c5c980ec5c18 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java @@ -141,7 +141,8 @@ public void testCreateS3SystemRepository() { .put(S3Repository.BASE_PATH_SETTING.getKey(), "foo/bar") .put(S3Repository.BUCKET_SETTING.getKey(), "bucket") .put(S3Repository.SYSTEM_REPOSITORY_SETTING.getKey(), true) - .build()); + .build() + ); try (S3Repository s3repo = createS3Repo(metadata)) { assertTrue(s3repo.isSystemRepository()); assertThat(s3repo.restrictedSystemRepositorySettings(), hasSize(2)); From 8ec8b5cf91290cc1f6ac49c3422de06a85aaccd0 Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Mon, 7 Aug 2023 20:40:54 +0530 Subject: [PATCH 11/12] Test case fix up Signed-off-by: Bukhtawar Khan --- .../main/java/org/opensearch/repositories/s3/S3Repository.java | 2 +- .../java/org/opensearch/repositories/s3/S3RepositoryTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java index e095cd15efd5c..162fa83f16028 100644 --- a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java +++ b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java @@ -397,7 +397,7 @@ public List> restrictedSystemRepositorySettings() { List> restrictedSystemRepositorySettings = super.restrictedSystemRepositorySettings(); settings.addAll(restrictedSystemRepositorySettings); settings.addAll(new ArrayList<>(List.of(BUCKET_SETTING, BASE_PATH_SETTING))); - return Collections.unmodifiableList(restrictedSystemRepositorySettings); + return Collections.unmodifiableList(settings); } @Override diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java index 0c5c980ec5c18..411dcb23826b8 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3RepositoryTests.java @@ -145,7 +145,7 @@ public void testCreateS3SystemRepository() { ); try (S3Repository s3repo = createS3Repo(metadata)) { assertTrue(s3repo.isSystemRepository()); - assertThat(s3repo.restrictedSystemRepositorySettings(), hasSize(2)); + assertThat(s3repo.restrictedSystemRepositorySettings(), hasSize(4)); } } From 840055c4388a36971b5ff43155bbdd9f126eb705 Mon Sep 17 00:00:00 2001 From: Bukhtawar Khan Date: Tue, 8 Aug 2023 13:23:00 +0530 Subject: [PATCH 12/12] Include type validation Signed-off-by: Bukhtawar Khan --- .../opensearch/repositories/RepositoriesService.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java index 76b447948b115..c1039d5518799 100644 --- a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java @@ -245,7 +245,7 @@ public ClusterState execute(ClusterState currentState) { repositoriesMetadata.add(new RepositoryMetadata(request.name(), request.type(), request.settings())); } else { logger.info("update repository [{}]", request.name()); - validateRestrictedSystemRepositorySettings(currentRepositoryMetadata, newRepositoryMetadata); + validateRestrictedSystemRepositorySettingsAndType(currentRepositoryMetadata, newRepositoryMetadata); } repositories = new RepositoriesMetadata(repositoriesMetadata); } @@ -273,11 +273,17 @@ public boolean mustAck(DiscoveryNode discoveryNode) { ); } - private void validateRestrictedSystemRepositorySettings( + private void validateRestrictedSystemRepositorySettingsAndType( RepositoryMetadata currentRepositoryMetadata, RepositoryMetadata newRepositoryMetadata ) { if (SYSTEM_REPOSITORY_SETTING.get(currentRepositoryMetadata.settings())) { + if (newRepositoryMetadata.type() != currentRepositoryMetadata.type()) { + throw new RepositoryException( + currentRepositoryMetadata.name(), + "trying to modify system repository type " + currentRepositoryMetadata.type() + ); + } Repository repository = repositories.get(currentRepositoryMetadata.name()); for (Setting setting : repository.restrictedSystemRepositorySettings()) { if (newRepositoryMetadata.settings().get(setting.getKey()) != null) {