diff --git a/CHANGELOG.md b/CHANGELOG.md index 64c6382d45a26..e504faed58173 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased 2.x] ### Added - [Remote Store] Introducing concept of RestrictedSystemRepositorySettings for system repository ([#9839](https://github.com/opensearch-project/OpenSearch/pull/9839)) +- [Remote Store] Adding 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)) ### Dependencies - Bump `peter-evans/create-or-update-comment` from 2 to 3 ([#9575](https://github.com/opensearch-project/OpenSearch/pull/9575)) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteIndexRecoveryIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteIndexRecoveryIT.java index a6c140e4ed8f0..7de013fe8b6fc 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteIndexRecoveryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteIndexRecoveryIT.java @@ -23,7 +23,6 @@ import java.nio.file.Path; import static org.opensearch.remotestore.RemoteStoreBaseIntegTestCase.remoteStoreClusterSettings; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) public class RemoteIndexRecoveryIT extends IndexRecoveryIT { @@ -58,7 +57,7 @@ public Settings indexSettings() { @After public void teardown() { internalCluster().wipeIndices("_all"); - assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME)); + clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get(); } @Override diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index 611cba75df106..8e4f298debd43 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -58,7 +58,7 @@ public void setup() { @After public void teardown() { internalCluster().wipeIndices("_all"); - assertAcked(clusterAdmin().prepareDeleteRepository(BASE_REMOTE_REPO)); + clusterAdmin().prepareCleanupRepository(BASE_REMOTE_REPO).get(); } @Override diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java index a46c0c24425e8..fe9d047bdd778 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java @@ -52,7 +52,6 @@ import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT; import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY; import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; public class RemoteStoreBaseIntegTestCase extends OpenSearchIntegTestCase { protected static final String REPOSITORY_NAME = "test-remote-store-repo"; @@ -274,8 +273,8 @@ public void teardown() { assertRemoteStoreRepositoryOnAllNodes(REPOSITORY_NAME); assertRemoteStoreRepositoryOnAllNodes(REPOSITORY_2_NAME); internalCluster().wipeIndices("_all"); - assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME)); - assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_2_NAME)); + clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get(); + clusterAdmin().prepareCleanupRepository(REPOSITORY_2_NAME).get(); } public RepositoryMetadata buildRepositoryMetadata(DiscoveryNode node, String name) { diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java index 9f2d989eb8ced..648cba383822e 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationUsingRemoteStoreIT.java @@ -17,7 +17,6 @@ import java.nio.file.Path; import static org.opensearch.remotestore.RemoteStoreBaseIntegTestCase.remoteStoreClusterSettings; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; /** * This class runs Segment Replication Integ test suite with remote store enabled. @@ -51,6 +50,6 @@ public void setup() { @After public void teardown() { internalCluster().wipeIndices("_all"); - assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME)); + clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get(); } } diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationWithRemoteStorePressureIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationWithRemoteStorePressureIT.java index 171880f29c35e..5a1612a452a49 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationWithRemoteStorePressureIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationWithRemoteStorePressureIT.java @@ -17,7 +17,6 @@ import java.nio.file.Path; import static org.opensearch.remotestore.RemoteStoreBaseIntegTestCase.remoteStoreClusterSettings; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; /** * This class executes the SegmentReplicationPressureIT suite with remote store integration enabled. @@ -50,6 +49,6 @@ public void setup() { @After public void teardown() { internalCluster().wipeIndices("_all"); - assertAcked(clusterAdmin().prepareDeleteRepository(REPOSITORY_NAME)); + clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get(); } } diff --git a/server/src/main/java/org/opensearch/node/remotestore/RemoteStoreNodeService.java b/server/src/main/java/org/opensearch/node/remotestore/RemoteStoreNodeService.java index d9607700adde8..39fbb748f457f 100644 --- a/server/src/main/java/org/opensearch/node/remotestore/RemoteStoreNodeService.java +++ b/server/src/main/java/org/opensearch/node/remotestore/RemoteStoreNodeService.java @@ -135,8 +135,8 @@ public RepositoriesMetadata updateRepositoriesMetadata(DiscoveryNode joiningNode for (RepositoryMetadata existingRepositoryMetadata : existingRepositories.repositories()) { if (newRepositoryMetadata.name().equals(existingRepositoryMetadata.name())) { try { - newRepositoryMetadata = repositoriesService.get() - .ensureValidSystemRepositoryUpdate(newRepositoryMetadata, existingRepositoryMetadata); + repositoriesService.get().ensureValidSystemRepositoryUpdate(newRepositoryMetadata, existingRepositoryMetadata); + newRepositoryMetadata = existingRepositoryMetadata; repositoryAlreadyPresent = true; break; } catch (RepositoryException e) { diff --git a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java index 8aea1aa73ba9d..b549170a50011 100644 --- a/server/src/main/java/org/opensearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/opensearch/repositories/RepositoriesService.java @@ -50,7 +50,6 @@ import org.opensearch.cluster.SnapshotsInProgress; import org.opensearch.cluster.ack.ClusterStateUpdateResponse; import org.opensearch.cluster.metadata.CryptoMetadata; -import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.Metadata; import org.opensearch.cluster.metadata.RepositoriesMetadata; import org.opensearch.cluster.metadata.RepositoryMetadata; @@ -238,16 +237,34 @@ public ClusterState execute(ClusterState currentState) { List repositoriesMetadata = new ArrayList<>(repositories.repositories().size() + 1); for (RepositoryMetadata repositoryMetadata : repositories.repositories()) { - if (repositoryMetadata.name().equals(newRepositoryMetadata.name())) { - if (newRepositoryMetadata.equalsIgnoreGenerations(repositoryMetadata)) { + RepositoryMetadata updatedRepositoryMetadata = newRepositoryMetadata; + if (isSystemRepositorySettingPresent(repositoryMetadata.settings())) { + Settings updatedSettings = Settings.builder() + .put(newRepositoryMetadata.settings()) + .put(SYSTEM_REPOSITORY_SETTING.getKey(), true) + .build(); + updatedRepositoryMetadata = new RepositoryMetadata( + newRepositoryMetadata.name(), + newRepositoryMetadata.type(), + updatedSettings, + newRepositoryMetadata.cryptoMetadata() + ); + } + if (repositoryMetadata.name().equals(updatedRepositoryMetadata.name())) { + if (updatedRepositoryMetadata.equalsIgnoreGenerations(repositoryMetadata)) { // Previous version is the same as this one no update is needed. return currentState; } ensureCryptoSettingsAreSame(repositoryMetadata, request); found = true; - repositoriesMetadata.add(ensureValidSystemRepositoryUpdate(newRepositoryMetadata, repositoryMetadata)); + if (isSystemRepositorySettingPresent(repositoryMetadata.settings())) { + ensureValidSystemRepositoryUpdate(updatedRepositoryMetadata, repositoryMetadata); + repositoriesMetadata.add(updatedRepositoryMetadata); + } else { + repositoriesMetadata.add(newRepositoryMetadata); + } } else { - repositoriesMetadata.add(repositoryMetadata); + repositoriesMetadata.add(updatedRepositoryMetadata); } } if (!found) { @@ -317,9 +334,7 @@ public ClusterState execute(ClusterState currentState) { for (RepositoryMetadata repositoryMetadata : repositories.repositories()) { if (Regex.simpleMatch(request.name(), repositoryMetadata.name())) { ensureRepositoryNotInUse(currentState, repositoryMetadata.name()); - if (isSystemRepositorySettingPresent(repositoryMetadata.settings())) { - ensureSystemRepositoryNotInUse(currentState, repositoryMetadata); - } + ensureNotSystemRepository(repositoryMetadata); logger.info("delete repository [{}]", repositoryMetadata.name()); changed = true; } else { @@ -770,40 +785,28 @@ public void updateRepositoriesMap(Map repos) { } } - private static void ensureSystemRepositoryNotInUse(ClusterState clusterState, RepositoryMetadata repositoryMetadata) { - if (isRepositoryInUse(clusterState, repositoryMetadata)) { - throw new IllegalStateException("trying to modify or unregister repository that is currently used"); - } - } - - private static boolean isRepositoryInUse(ClusterState clusterState, RepositoryMetadata repositoryMetadata) { - // We can add more validations where we want to ensure that the system repository is not deleted if in use. - final Metadata idxMetadata = clusterState.getMetadata(); - final String repository = repositoryMetadata.name(); - for (IndexMetadata metadata : idxMetadata) { - if (IndexMetadata.INDEX_REMOTE_STORE_ENABLED_SETTING.get(metadata.getSettings())) { - String segmentRepositoryInIndex = IndexMetadata.INDEX_REMOTE_SEGMENT_STORE_REPOSITORY_SETTING.get(metadata.getSettings()); - String translogRepositoryInIndex = IndexMetadata.INDEX_REMOTE_TRANSLOG_REPOSITORY_SETTING.get(metadata.getSettings()); - - if (segmentRepositoryInIndex.equals(repository) || translogRepositoryInIndex.equals(repository)) { - return true; - } - } + private static void ensureNotSystemRepository(RepositoryMetadata repositoryMetadata) { + if (isSystemRepositorySettingPresent(repositoryMetadata.settings())) { + throw new RepositoryException(repositoryMetadata.name(), "cannot delete a system repository"); } - return false; } private static boolean isSystemRepositorySettingPresent(Settings repositoryMetadataSettings) { return SYSTEM_REPOSITORY_SETTING.get(repositoryMetadataSettings); } - private static boolean isValueEqual(String newValue, String currentValue) { + private static boolean isValueEqual(String key, String newValue, String currentValue) { + if (newValue == null && currentValue == null) { + return true; + } if (newValue == null) { - throw new IllegalArgumentException("new value cannot be empty, " + "current value [" + currentValue + "]"); + throw new IllegalArgumentException("[" + key + "] cannot be empty, " + "current value [" + currentValue + "]"); } if (newValue.equals(currentValue) == false) { throw new IllegalArgumentException( - "trying to modify an unmodifiable attribute of system repository from " + "trying to modify an unmodifiable attribute " + + key + + " of system repository from " + "current value [" + currentValue + "] to new value [" @@ -814,14 +817,10 @@ private static boolean isValueEqual(String newValue, String currentValue) { return true; } - public RepositoryMetadata ensureValidSystemRepositoryUpdate( - RepositoryMetadata newRepositoryMetadata, - RepositoryMetadata currentRepositoryMetadata - ) { + public void ensureValidSystemRepositoryUpdate(RepositoryMetadata newRepositoryMetadata, RepositoryMetadata currentRepositoryMetadata) { if (isSystemRepositorySettingPresent(currentRepositoryMetadata.settings())) { - Settings.Builder updatedSettings = Settings.builder().put(newRepositoryMetadata.settings()); try { - isValueEqual(newRepositoryMetadata.type(), currentRepositoryMetadata.type()); + isValueEqual("type", newRepositoryMetadata.type(), currentRepositoryMetadata.type()); Repository repository = repositories.get(currentRepositoryMetadata.name()); Settings newRepositoryMetadataSettings = newRepositoryMetadata.settings(); @@ -833,21 +832,16 @@ public RepositoryMetadata ensureValidSystemRepositoryUpdate( .collect(Collectors.toList()); for (String restrictedSettingKey : restrictedSettings) { - String newSettingValue = newRepositoryMetadataSettings.get(restrictedSettingKey); - String currentSettingValue = currentRepositoryMetadataSettings.get(restrictedSettingKey); - - isValueEqual(newSettingValue, currentSettingValue); - - if (currentSettingValue != null) { - updatedSettings.put(restrictedSettingKey, currentSettingValue); - } + isValueEqual( + restrictedSettingKey, + newRepositoryMetadataSettings.get(restrictedSettingKey), + currentRepositoryMetadataSettings.get(restrictedSettingKey) + ); } } catch (IllegalArgumentException e) { throw new RepositoryException(currentRepositoryMetadata.name(), e.getMessage()); } - return new RepositoryMetadata(currentRepositoryMetadata.name(), currentRepositoryMetadata.type(), updatedSettings.build()); } - return newRepositoryMetadata; } @Override diff --git a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java index 7253a77b96c1a..cc4f4f7b6413d 100644 --- a/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java @@ -94,6 +94,7 @@ import java.util.function.Consumer; import java.util.function.Function; +import static org.opensearch.repositories.blobstore.BlobStoreRepository.SYSTEM_REPOSITORY_SETTING; import static org.hamcrest.Matchers.equalTo; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -201,6 +202,13 @@ public void testRegisterRejectsInvalidRepositoryNames() { } } + public void testUpdateOrRegisterRejectsForSystemRepository() { + String repoName = "name"; + PutRepositoryRequest request = new PutRepositoryRequest(repoName); + request.settings(Settings.builder().put(SYSTEM_REPOSITORY_SETTING.getKey(), true).build()); + expectThrows(RepositoryException.class, () -> repositoriesService.registerOrUpdateRepository(request, null)); + } + public void testRepositoriesStatsCanHaveTheSameNameAndDifferentTypeOverTime() { String repoName = "name"; expectThrows(RepositoryMissingException.class, () -> repositoriesService.repository(repoName)); diff --git a/test/framework/src/main/java/org/opensearch/test/TestCluster.java b/test/framework/src/main/java/org/opensearch/test/TestCluster.java index 61742cd4fb827..b70e5c51f73d6 100644 --- a/test/framework/src/main/java/org/opensearch/test/TestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/TestCluster.java @@ -46,6 +46,7 @@ import org.opensearch.index.IndexNotFoundException; import org.opensearch.indices.IndexTemplateMissingException; import org.opensearch.repositories.RepositoryMissingException; +import org.opensearch.repositories.blobstore.BlobStoreRepository; import org.opensearch.test.hamcrest.OpenSearchAssertions; import java.io.Closeable; @@ -253,7 +254,13 @@ public void wipeRepositories(String... repositories) { } for (String repository : repositories) { try { - client().admin().cluster().prepareDeleteRepository(repository).execute().actionGet(); + if (BlobStoreRepository.SYSTEM_REPOSITORY_SETTING.get( + client().admin().cluster().prepareGetRepositories(repository).execute().actionGet().repositories().get(0).settings() + ) == false) { + client().admin().cluster().prepareDeleteRepository(repository).execute().actionGet(); + } else { + client().admin().cluster().prepareCleanupRepository(repository).execute().actionGet(); + } } catch (RepositoryMissingException ex) { // ignore }