Add reloadable check, Update metadata validation

opensearch-project · Oct 4, 2023 · 6055c09 · 6055c09
1 parent 6eb1b37
commit 6055c09
Show file tree

Hide file tree

Showing 7 changed files with 20 additions and 18 deletions.
diff --git a/.../internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java b/.../internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java
@@ -93,8 +93,7 @@ protected void createRepository(String repoName) {
         Settings.Builder settings = Settings.builder()
             .put("bucket", System.getProperty("test.s3.bucket"))
             .put("region", System.getProperty("test.s3.region", "us-west-2"))
-            .put("base_path", System.getProperty("test.s3.base", "testpath"))
-            .put("storage_class", "standard");
+            .put("base_path", System.getProperty("test.s3.base", "testpath"));
         final String endpoint = System.getProperty("test.s3.endpoint");
         if (endpoint != null) {
             settings.put("endpoint", endpoint);

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
@@ -194,13 +194,13 @@ class S3Repository extends MeteredBlobStoreRepository {
      * Sets the S3 storage class type for the backup files. Values may be standard, reduced_redundancy,
      * standard_ia, onezone_ia and intelligent_tiering. Defaults to standard.
      */
-    static final Setting<String> STORAGE_CLASS_SETTING = Setting.simpleString("storage_class", StorageClass.STANDARD.toString());
+    static final Setting<String> STORAGE_CLASS_SETTING = Setting.simpleString("storage_class");
 
     /**
      * The S3 repository supports all S3 canned ACLs : private, public-read, public-read-write,
      * authenticated-read, log-delivery-write, bucket-owner-read, bucket-owner-full-control. Defaults to private.
      */
-    static final Setting<String> CANNED_ACL_SETTING = Setting.simpleString("canned_acl", ObjectCannedACL.PRIVATE.toString());
+    static final Setting<String> CANNED_ACL_SETTING = Setting.simpleString("canned_acl");
 
     static final Setting<String> CLIENT_NAME = new Setting<>("client", "default", Function.identity());
 
@@ -365,6 +365,10 @@ public boolean isReloadable() {
 
     @Override
     public void reload(RepositoryMetadata newRepositoryMetadata) {
+        if (isReloadable() == false) {
+            return;
+        }
+
         // Reload configs for S3Repository
         super.reload(newRepositoryMetadata);
         readRepositoryMetadata();
@@ -448,6 +452,10 @@ private void validateRepositoryMetadata(RepositoryMetadata newRepositoryMetadata
     }
 
     private static void validateStorageClass(String storageClassStringValue) {
+        if ((storageClassStringValue == null) || storageClassStringValue.equals("")) {
+            return;
+        }
+
         final StorageClass storageClass = StorageClass.fromValue(storageClassStringValue.toUpperCase(Locale.ENGLISH));
         if (storageClass.equals(StorageClass.GLACIER)) {
             throw new BlobStoreException("Glacier storage class is not supported");

diff --git a/...epository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java b/...epository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java
@@ -113,8 +113,7 @@ public void testRepositoryCredentialsOverrideSecureCredentials() {
         final Settings.Builder repositorySettings = Settings.builder()
             // repository settings for credentials override node secure settings
             .put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key")
-            .put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret")
-            .put("storage_class", "standard");
+            .put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret");
 
         final String clientName = randomFrom("default", "other", null);
         if (clientName != null) {
@@ -151,7 +150,6 @@ public void testReinitSecureCredentials() {
         final String clientName = randomFrom("default", "other");
 
         final Settings.Builder repositorySettings = Settings.builder();
-        repositorySettings.put("storage_class", "standard");
         final boolean hasInsecureSettings = randomBoolean();
         if (hasInsecureSettings) {
             // repository settings for credentials override node secure settings
@@ -243,7 +241,6 @@ public void testInsecureRepositoryCredentials() throws Exception {
             Settings.builder()
                 .put(S3Repository.ACCESS_KEY_SETTING.getKey(), "insecure_aws_key")
                 .put(S3Repository.SECRET_KEY_SETTING.getKey(), "insecure_aws_secret")
-                .put("storage_class", "standard")
                 .build()
         );
 

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
@@ -104,7 +104,6 @@ public void testInvalidChunkBufferSizeSettings() {
 
     private Settings bufferAndChunkSettings(long buffer, long chunk) {
         return Settings.builder()
-            .put("storage_class", "standard")
             .put(S3Repository.BUFFER_SIZE_SETTING.getKey(), new ByteSizeValue(buffer, ByteSizeUnit.MB).getStringRep())
             .put(S3Repository.CHUNK_SIZE_SETTING.getKey(), new ByteSizeValue(chunk, ByteSizeUnit.MB).getStringRep())
             .build();
@@ -118,15 +117,15 @@ public void testBasePathSetting() {
         final RepositoryMetadata metadata = new RepositoryMetadata(
             "dummy-repo",
             "mock",
-            Settings.builder().put(S3Repository.BASE_PATH_SETTING.getKey(), "foo/bar").put("storage_class", "standard").build()
+            Settings.builder().put(S3Repository.BASE_PATH_SETTING.getKey(), "foo/bar").build()
         );
         try (S3Repository s3repo = createS3Repo(metadata)) {
             assertEquals("foo/bar/", s3repo.basePath().buildAsString());
         }
     }
 
     public void testDefaultBufferSize() {
-        Settings settings = Settings.builder().put("storage_class", "standard").build();
+        Settings settings = Settings.builder().build();
         final RepositoryMetadata metadata = new RepositoryMetadata("dummy-repo", "mock", settings);
         try (S3Repository s3repo = createS3Repo(metadata)) {
             assertThat(s3repo.getBlobStore(), is(nullValue()));
@@ -139,16 +138,14 @@ public void testDefaultBufferSize() {
     }
 
     public void testIsReloadable() {
-        Settings settings = Settings.builder().put("storage_class", "standard").build();
-        final RepositoryMetadata metadata = new RepositoryMetadata("dummy-repo", "mock", settings);
+        final RepositoryMetadata metadata = new RepositoryMetadata("dummy-repo", "mock", Settings.EMPTY);
         try (S3Repository s3repo = createS3Repo(metadata)) {
             assertTrue(s3repo.isReloadable());
         }
     }
 
     public void testRestrictedSettingsDefault() {
-        Settings settings = Settings.builder().put("storage_class", "standard").build();
-        final RepositoryMetadata metadata = new RepositoryMetadata("dummy-repo", "mock", settings);
+        final RepositoryMetadata metadata = new RepositoryMetadata("dummy-repo", "mock", Settings.EMPTY);
         try (S3Repository s3repo = createS3Repo(metadata)) {
             List<Setting<?>> restrictedSettings = s3repo.getRestrictedSystemRepositorySettings();
             assertThat(restrictedSettings.size(), is(5));

diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRestoreIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteStoreRestoreIT.java
@@ -19,7 +19,6 @@
 import org.opensearch.core.common.unit.ByteSizeUnit;
 import org.opensearch.repositories.RepositoriesService;
 import org.opensearch.repositories.Repository;
-import org.opensearch.repositories.blobstore.BlobStoreRepository;
 import org.opensearch.test.InternalTestCluster;
 import org.opensearch.test.OpenSearchIntegTestCase;
 
@@ -424,7 +423,6 @@ public void testRateLimitedRemoteDownloads() throws Exception {
             .collect(Collectors.toMap(key -> key.replace(settingsAttributeKeyPrefix, ""), key -> node.getAttributes().get(key)));
         Settings.Builder settings = Settings.builder();
         settingsMap.entrySet().forEach(entry -> settings.put(entry.getKey(), entry.getValue()));
-        settings.put(BlobStoreRepository.SYSTEM_REPOSITORY_SETTING.getKey(), true);
         settings.put("location", segmentRepoPath).put("max_remote_download_bytes_per_sec", 4, ByteSizeUnit.KB);
 
         assertAcked(client().admin().cluster().preparePutRepository(REPOSITORY_NAME).setType("fs").setSettings(settings).get());

diff --git a/server/src/main/java/org/opensearch/repositories/fs/ReloadableFsRepository.java b/server/src/main/java/org/opensearch/repositories/fs/ReloadableFsRepository.java
@@ -40,6 +40,10 @@ public boolean isReloadable() {
 
     @Override
     public void reload(RepositoryMetadata repositoryMetadata) {
+        if (isReloadable() == false) {
+            return;
+        }
+
         super.reload(repositoryMetadata);
         validateLocation();
         readMetadata();

diff --git a/...ava/org/opensearch/repositories/blobstore/OpenSearchBlobStoreRepositoryIntegTestCase.java b/...ava/org/opensearch/repositories/blobstore/OpenSearchBlobStoreRepositoryIntegTestCase.java
@@ -96,7 +96,6 @@ protected Settings repositorySettings() {
         final boolean compress = randomBoolean();
         final Settings.Builder builder = Settings.builder();
         builder.put("compress", compress);
-        builder.put("storage_class", "standard");
         if (compress) {
             builder.put("compression_type", randomFrom(CompressorRegistry.registeredCompressors().keySet()));
         }