Remove default s3 region (opensearch-project#7989)

* Defaulting repository-s3 region to empty string Signed-off-by: Raghuvansh Raj <raghraaj@amazon.com> * Added region us-west-2 for S3ThirdPartyTests Signed-off-by: Raghuvansh Raj <raghraaj@amazon.com> * Always specify a region. Signed-off-by: dblock <dblock@amazon.com> * Fix: ./gradlew :plugins:repository-s3:yamlRestTest runs locally. Signed-off-by: dblock <dblock@amazon.com> --------- Signed-off-by: Raghuvansh Raj <raghraaj@amazon.com> Signed-off-by: dblock <dblock@amazon.com> Co-authored-by: Raghuvansh Raj <raghraaj@amazon.com>
sarthakaggarwal97 · Jun 16, 2023 · fb0f0e4 · fb0f0e4
1 parent a6b984b
commit fb0f0e4
Show file tree

Hide file tree

Showing 13 changed files with 56 additions and 18 deletions.
diff --git a/plugins/repository-s3/build.gradle b/plugins/repository-s3/build.gradle
@@ -148,12 +148,14 @@ String s3PermanentAccessKey = System.getenv("amazon_s3_access_key")
 String s3PermanentSecretKey = System.getenv("amazon_s3_secret_key")
 String s3PermanentBucket = System.getenv("amazon_s3_bucket")
 String s3PermanentBasePath = System.getenv("amazon_s3_base_path")
+String s3PermanentRegion = System.getenv("amazon_s3_region")
 
 String s3TemporaryAccessKey = System.getenv("amazon_s3_access_key_temporary")
 String s3TemporarySecretKey = System.getenv("amazon_s3_secret_key_temporary")
 String s3TemporarySessionToken = System.getenv("amazon_s3_session_token_temporary")
 String s3TemporaryBucket = System.getenv("amazon_s3_bucket_temporary")
 String s3TemporaryBasePath = System.getenv("amazon_s3_base_path_temporary")
+String s3TemporaryRegion = System.getenv("amazon_s3_region_temporary")
 
 String s3EC2Bucket = System.getenv("amazon_s3_bucket_ec2")
 String s3EC2BasePath = System.getenv("amazon_s3_base_path_ec2")
@@ -166,28 +168,40 @@ String s3EKSBasePath = System.getenv("amazon_s3_base_path_eks")
 
 boolean s3DisableChunkedEncoding = (new Random(Long.parseUnsignedLong(BuildParams.testSeed.tokenize(':').get(0), 16))).nextBoolean()
 
+// TODO: remove after https://github.com/opensearch-project/opensearch-build/issues/3615
+if (s3PermanentBucket && !s3PermanentRegion) {
+  s3PermanentRegion = "us-west-2"
+}
+
+if (s3TemporaryBucket && !s3TemporaryRegion) {
+  s3TemporaryRegion = "us-west-2"
+}
+// ----
+
 // If all these variables are missing then we are testing against the internal fixture instead, which has the following
 // credentials hard-coded in.
 
-if (!s3PermanentAccessKey && !s3PermanentSecretKey && !s3PermanentBucket && !s3PermanentBasePath) {
+if (!s3PermanentAccessKey && !s3PermanentSecretKey && !s3PermanentBucket && !s3PermanentBasePath && !s3PermanentRegion) {
   s3PermanentAccessKey = 'access_key'
   s3PermanentSecretKey = 'secret_key'
   s3PermanentBucket = 'bucket'
   s3PermanentBasePath = 'base_path'
+  s3PermanentRegion = 'region'
 
   apply plugin: 'opensearch.test.fixtures'
   useFixture = true
 
-} else if (!s3PermanentAccessKey || !s3PermanentSecretKey || !s3PermanentBucket || !s3PermanentBasePath) {
+} else if (!s3PermanentAccessKey || !s3PermanentSecretKey || !s3PermanentBucket || !s3PermanentBasePath || !s3PermanentRegion) {
   throw new IllegalArgumentException("not all options specified to run against external S3 service as permanent credentials are present")
 }
 
-if (!s3TemporaryAccessKey && !s3TemporarySecretKey && !s3TemporaryBucket && !s3TemporaryBasePath && !s3TemporarySessionToken) {
+if (!s3TemporaryAccessKey && !s3TemporarySecretKey && !s3TemporaryBucket && !s3TemporaryBasePath && !s3TemporarySessionToken && !s3TemporaryRegion) {
   s3TemporaryAccessKey = 'session_token_access_key'
   s3TemporarySecretKey = 'session_token_secret_key'
   s3TemporaryBucket = 'session_token_bucket'
   s3TemporaryBasePath = 'session_token_base_path'
   s3TemporarySessionToken = 'session_token'
+  s3TemporaryRegion = 'session_token_region'
 
 } else if (!s3TemporaryAccessKey || !s3TemporarySecretKey || !s3TemporaryBucket || !s3TemporaryBasePath || !s3TemporarySessionToken) {
   throw new IllegalArgumentException("not all options specified to run against external S3 service as temporary credentials are present")
@@ -208,8 +222,10 @@ processYamlRestTestResources {
   Map<String, Object> expansions = [
     'permanent_bucket': s3PermanentBucket,
     'permanent_base_path': s3PermanentBasePath + "_integration_tests_" + BuildParams.testSeed,
+    'permanent_region': s3PermanentRegion,
     'temporary_bucket': s3TemporaryBucket,
     'temporary_base_path': s3TemporaryBasePath + "_integration_tests_" + BuildParams.testSeed,
+    'temporary_region': s3TemporaryRegion,
     'ec2_bucket': s3EC2Bucket,
     'ec2_base_path': s3EC2BasePath,
     'ecs_bucket': s3ECSBucket,
@@ -384,6 +400,7 @@ TaskProvider s3ThirdPartyTest = tasks.register("s3ThirdPartyTest", Test) {
   systemProperty 'test.s3.account', s3PermanentAccessKey
   systemProperty 'test.s3.key', s3PermanentSecretKey
   systemProperty 'test.s3.bucket', s3PermanentBucket
+  systemProperty 'test.s3.region', s3PermanentRegion
   nonInputProperties.systemProperty 'test.s3.base', s3PermanentBasePath + "_third_party_tests_" + BuildParams.testSeed
   if (useFixture) {
     nonInputProperties.systemProperty 'test.s3.endpoint', "${-> fixtureAddress('minio-fixture', 'minio-fixture', '9000') }"

diff --git a/...c/internalClusterTest/java/org/opensearch/repositories/s3/S3BlobStoreRepositoryTests.java b/...c/internalClusterTest/java/org/opensearch/repositories/s3/S3BlobStoreRepositoryTests.java
@@ -72,15 +72,12 @@
 @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST)
 public class S3BlobStoreRepositoryTests extends OpenSearchMockAPIBasedRepositoryIntegTestCase {
 
-    private String region;
+    private final String region = "test-region";
     private String signerOverride;
     private String previousOpenSearchPathConf;
 
     @Override
     public void setUp() throws Exception {
-        if (randomBoolean()) {
-            region = "test-region";
-        }
         signerOverride = AwsRequestSigner.VERSION_FOUR_SIGNER.getName();
         previousOpenSearchPathConf = SocketAccess.doPrivileged(() -> System.setProperty("opensearch.path.conf", "config"));
         super.setUp();
@@ -147,9 +144,8 @@ protected Settings nodeSettings(int nodeOrdinal) {
         if (signerOverride != null) {
             builder.put(S3ClientSettings.SIGNER_OVERRIDE.getConcreteSettingForNamespace("test").getKey(), signerOverride);
         }
-        if (region != null) {
-            builder.put(S3ClientSettings.REGION.getConcreteSettingForNamespace("test").getKey(), region);
-        }
+
+        builder.put(S3ClientSettings.REGION.getConcreteSettingForNamespace("test").getKey(), region);
         return builder.build();
     }
 
@@ -218,7 +214,7 @@ private void validateAuthHeader(HttpExchange exchange) {
             if ("AWS4SignerType".equals(signerOverride)) {
                 assertThat(authorizationHeaderV4, containsString("aws4_request"));
             }
-            if (region != null && authorizationHeaderV4 != null) {
+            if (authorizationHeaderV4 != null) {
                 assertThat(authorizationHeaderV4, containsString("/" + region + "/s3/"));
             }
         }

diff --git a/.../internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java b/.../internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java
@@ -91,6 +91,7 @@ protected SecureSettings credentials() {
     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"));
         final String endpoint = System.getProperty("test.s3.endpoint");
         if (endpoint != null) {

diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java
@@ -204,7 +204,7 @@ final class S3ClientSettings {
     static final Setting.AffixSetting<String> REGION = Setting.affixKeySetting(
         PREFIX,
         "region",
-        key -> new Setting<>(key, "us-west-2", Function.identity(), Property.NodeScope)
+        key -> new Setting<>(key, "", Function.identity(), Property.NodeScope)
     );
 
     /** An override for the signer to use. */

diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java
@@ -219,7 +219,9 @@ AmazonS3WithCredentials buildClient(final S3ClientSettings clientSettings) {
         // We do this because directly constructing the client is deprecated (was already deprecated in 1.1.223 too)
         // so this change removes that usage of a deprecated API.
         builder.endpointOverride(URI.create(endpoint));
-        builder.region(Region.of(clientSettings.region));
+        if (Strings.hasText(clientSettings.region)) {
+            builder.region(Region.of(clientSettings.region));
+        }
         if (clientSettings.pathStyleAccess) {
             builder.forcePathStyle(true);
         }
@@ -352,9 +354,11 @@ static AwsCredentialsProvider buildCredentials(Logger logger, S3ClientSettings c
         if (irsaCredentials != null) {
             logger.debug("Using IRSA credentials");
 
-            final Region region = Region.of(clientSettings.region);
             StsClient stsClient = SocketAccess.doPrivileged(() -> {
-                StsClientBuilder builder = StsClient.builder().region(region);
+                StsClientBuilder builder = StsClient.builder();
+                if (Strings.hasText(clientSettings.region)) {
+                    builder.region(Region.of(clientSettings.region));
+                }
 
                 final String stsEndpoint = System.getProperty(STS_ENDPOINT_OVERRIDE_SYSTEM_PROPERTY);
                 if (stsEndpoint != null) {

diff --git a/...pository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerRetriesTests.java b/...pository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerRetriesTests.java
@@ -75,6 +75,7 @@
 import static org.hamcrest.Matchers.is;
 import static org.opensearch.repositories.s3.S3ClientSettings.DISABLE_CHUNKED_ENCODING;
 import static org.opensearch.repositories.s3.S3ClientSettings.ENDPOINT_SETTING;
+import static org.opensearch.repositories.s3.S3ClientSettings.REGION;
 import static org.opensearch.repositories.s3.S3ClientSettings.MAX_RETRIES_SETTING;
 import static org.opensearch.repositories.s3.S3ClientSettings.READ_TIMEOUT_SETTING;
 
@@ -133,6 +134,7 @@ protected BlobContainer createBlobContainer(
         final InetSocketAddress address = httpServer.getAddress();
         final String endpoint = "http://" + InetAddresses.toUriString(address.getAddress()) + ":" + address.getPort();
         clientSettings.put(ENDPOINT_SETTING.getConcreteSettingForNamespace(clientName).getKey(), endpoint);
+        clientSettings.put(REGION.getConcreteSettingForNamespace(clientName).getKey(), "region");
 
         if (maxRetries != null) {
             clientSettings.put(MAX_RETRIES_SETTING.getConcreteSettingForNamespace(clientName).getKey(), maxRetries);

diff --git a/...ins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ClientSettingsTests.java b/...ins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ClientSettingsTests.java
@@ -286,7 +286,7 @@ public void testRegionCanBeSet() {
             Settings.builder().put("s3.client.other.region", region).build(),
             configPath()
         );
-        assertThat(settings.get("default").region, is("us-west-2"));
+        assertThat(settings.get("default").region, is(""));
         assertThat(settings.get("other").region, is(region));
         try (
             S3Service s3Service = new S3Service(configPath());
@@ -303,7 +303,7 @@ public void testSignerOverrideCanBeSet() {
             Settings.builder().put("s3.client.other.signer_override", signerOverride).build(),
             configPath()
         );
-        assertThat(settings.get("default").region, is("us-west-2"));
+        assertThat(settings.get("default").region, is(""));
         assertThat(settings.get("other").signerOverride, is(signerOverride));
 
         ClientOverrideConfiguration defaultConfiguration = SocketAccess.doPrivileged(

diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java
@@ -40,7 +40,7 @@
 public class S3ServiceTests extends AbstractS3RepositoryTestCase {
     public void testCachedClientsAreReleased() {
         final S3Service s3Service = new S3Service(configPath());
-        final Settings settings = Settings.builder().put("endpoint", "http://first").build();
+        final Settings settings = Settings.builder().put("endpoint", "http://first").put("region", "region").build();
         final RepositoryMetadata metadata1 = new RepositoryMetadata("first", "s3", settings);
         final RepositoryMetadata metadata2 = new RepositoryMetadata("second", "s3", settings);
         final S3ClientSettings clientSettings = s3Service.settings(metadata2);

diff --git a/...stTest/resources/rest-api-spec/test/repository_s3/20_repository_permanent_credentials.yml b/...stTest/resources/rest-api-spec/test/repository_s3/20_repository_permanent_credentials.yml
@@ -11,6 +11,7 @@ setup:
           type: s3
           settings:
             bucket: ${permanent_bucket}
+            region: ${permanent_region}
             client: integration_test_permanent
             base_path: "${permanent_base_path}"
             canned_acl: private
@@ -42,6 +43,7 @@ setup:
           type: s3
           settings:
             bucket: ${permanent_bucket}
+            region: ${permanent_region}
             client: integration_test_permanent
             base_path: "${permanent_base_path}"
             endpoint: 127.0.0.1:5
@@ -57,6 +59,7 @@ setup:
           type: s3
           settings:
             bucket: ${permanent_bucket}
+            region: ${permanent_region}
             client: integration_test_permanent
             base_path: "${permanent_base_path}"
             endpoint: 127.0.0.1:5
@@ -275,6 +278,7 @@ setup:
           type: s3
           settings:
             bucket: zHHkfSqlbnBsbpSgvCYtxrEfFLqghXtyPvvvKPNBnRCicNHQLE
+            region: ${permanent_region}
             client: integration_test_permanent
 
 ---
@@ -288,6 +292,7 @@ setup:
           type: s3
           settings:
             bucket: repository_permanent
+            region: ${permanent_region}
             client: unknown
 
 ---
@@ -302,6 +307,7 @@ setup:
         settings:
           readonly: true
           bucket: zHHkfSqlbnBsbpSgvCYtxrEfFLqghXtyPvvvKPNBnRCicNHQLE
+          region: ${permanent_region}
           client: integration_test_permanent
 
 ---
@@ -316,6 +322,7 @@ setup:
         settings:
           readonly: true
           bucket: repository_permanent
+          region: ${permanent_region}
           client: unknown
 
 ---

diff --git a/...stTest/resources/rest-api-spec/test/repository_s3/30_repository_temporary_credentials.yml b/...stTest/resources/rest-api-spec/test/repository_s3/30_repository_temporary_credentials.yml
@@ -11,6 +11,7 @@ setup:
           type: s3
           settings:
             bucket: ${temporary_bucket}
+            region: ${temporary_region}
             client: integration_test_temporary
             base_path: "${temporary_base_path}"
             canned_acl: private
@@ -26,6 +27,7 @@ setup:
         repository: repository_temporary
 
   - match: { repository_temporary.settings.bucket : ${temporary_bucket} }
+  - match: { repository_temporary.settings.region : ${temporary_region} }
   - match: { repository_temporary.settings.client : "integration_test_temporary" }
   - match: { repository_temporary.settings.base_path : "${temporary_base_path}" }
   - match: { repository_temporary.settings.canned_acl : "private" }
@@ -185,6 +187,7 @@ setup:
           type: s3
           settings:
             bucket: zHHkfSqlbnBsbpSgvCYtxrEfFLqghXtyPvvvKPNBnRCicNHQLE
+            region: ${temporary_region}
             client: integration_test_temporary
 
 ---
@@ -198,6 +201,7 @@ setup:
           type: s3
           settings:
             bucket: repository_temporary
+            region: ${temporary_region}
             client: unknown
 
 ---
@@ -212,6 +216,7 @@ setup:
         settings:
           readonly: true
           bucket: zHHkfSqlbnBsbpSgvCYtxrEfFLqghXtyPvvvKPNBnRCicNHQLE
+          region: ${temporary_region}
           client: integration_test_temporary
 
 ---
@@ -226,6 +231,7 @@ setup:
         settings:
           readonly: true
           bucket: repository_temporary
+          region: ${temporary_region}
           client: unknown
 
 ---

diff --git a/...yamlRestTest/resources/rest-api-spec/test/repository_s3/40_repository_ec2_credentials.yml b/...yamlRestTest/resources/rest-api-spec/test/repository_s3/40_repository_ec2_credentials.yml
@@ -11,6 +11,7 @@ setup:
           type: s3
           settings:
             bucket: ${ec2_bucket}
+            region: region
             client: integration_test_ec2
             base_path: "${ec2_base_path}"
             canned_acl: private
@@ -30,6 +31,7 @@ setup:
   - match: { repository_ec2.settings.base_path : "${ec2_base_path}" }
   - match: { repository_ec2.settings.canned_acl : "private" }
   - match: { repository_ec2.settings.storage_class : "standard" }
+  - is_false: repository_ec2.settings.region
   - is_false: repository_ec2.settings.access_key
   - is_false: repository_ec2.settings.secret_key
   - is_false: repository_ec2.settings.session_token

diff --git a/...yamlRestTest/resources/rest-api-spec/test/repository_s3/50_repository_ecs_credentials.yml b/...yamlRestTest/resources/rest-api-spec/test/repository_s3/50_repository_ecs_credentials.yml
@@ -11,6 +11,7 @@ setup:
           type: s3
           settings:
             bucket: ${ecs_bucket}
+            region: region
             client: integration_test_ecs
             base_path: "${ecs_base_path}"
             canned_acl: private
@@ -30,6 +31,7 @@ setup:
   - match: { repository_ecs.settings.base_path : "${ecs_base_path}" }
   - match: { repository_ecs.settings.canned_acl : "private" }
   - match: { repository_ecs.settings.storage_class : "standard" }
+  - is_false: repository_ecs.settings.region
   - is_false: repository_ecs.settings.access_key
   - is_false: repository_ecs.settings.secret_key
   - is_false: repository_ecs.settings.session_token

diff --git a/...yamlRestTest/resources/rest-api-spec/test/repository_s3/60_repository_eks_credentials.yml b/...yamlRestTest/resources/rest-api-spec/test/repository_s3/60_repository_eks_credentials.yml
@@ -11,6 +11,7 @@ setup:
           type: s3
           settings:
             bucket: ${eks_bucket}
+            region: region
             client: integration_test_eks
             base_path: "${eks_base_path}"
             canned_acl: private