From 6720bbd5b8a287987ba3cdb0510bf8640b46f720 Mon Sep 17 00:00:00 2001 From: "Daniel (dB.) Doubrovkine" Date: Fri, 16 Jun 2023 14:50:31 -0400 Subject: [PATCH] Remove default s3 region (#7989) * Defaulting repository-s3 region to empty string Signed-off-by: Raghuvansh Raj * Added region us-west-2 for S3ThirdPartyTests Signed-off-by: Raghuvansh Raj * Always specify a region. Signed-off-by: dblock * Fix: ./gradlew :plugins:repository-s3:yamlRestTest runs locally. Signed-off-by: dblock --------- Signed-off-by: Raghuvansh Raj Signed-off-by: dblock Co-authored-by: Raghuvansh Raj Signed-off-by: Rishab Nahata --- plugins/repository-s3/build.gradle | 23 ++++++++++++++++--- .../s3/S3BlobStoreRepositoryTests.java | 12 ++++------ .../s3/S3RepositoryThirdPartyTests.java | 1 + .../repositories/s3/S3ClientSettings.java | 2 +- .../opensearch/repositories/s3/S3Service.java | 10 +++++--- .../s3/S3BlobContainerRetriesTests.java | 2 ++ .../s3/S3ClientSettingsTests.java | 4 ++-- .../repositories/s3/S3ServiceTests.java | 2 +- .../20_repository_permanent_credentials.yml | 7 ++++++ .../30_repository_temporary_credentials.yml | 6 +++++ .../40_repository_ec2_credentials.yml | 2 ++ .../50_repository_ecs_credentials.yml | 2 ++ .../60_repository_eks_credentials.yml | 1 + 13 files changed, 56 insertions(+), 18 deletions(-) diff --git a/plugins/repository-s3/build.gradle b/plugins/repository-s3/build.gradle index 5bd9747ade9da..2250f2fc88f05 100644 --- 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 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/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3BlobStoreRepositoryTests.java b/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3BlobStoreRepositoryTests.java index 1bbc973dd712f..61268cf00a77a 100644 --- a/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3BlobStoreRepositoryTests.java +++ b/plugins/repository-s3/src/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/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java b/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java index 301fd288bc672..c8b1670bfdd83 100644 --- a/plugins/repository-s3/src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java +++ b/plugins/repository-s3/src/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 index 66439aa820c12..5f6be6ac01e76 100644 --- 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 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 index f63c691a920ad..c13e5b76b9269 100644 --- 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/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerRetriesTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerRetriesTests.java index 9ef83d09f56f6..9f5ebc5afe017 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerRetriesTests.java +++ b/plugins/repository-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/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ClientSettingsTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ClientSettingsTests.java index 5124fe60e30e7..130b8efca0512 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ClientSettingsTests.java +++ b/plugins/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 index 6facea7d534ca..400905eec8b1c 100644 --- 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/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/20_repository_permanent_credentials.yml b/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/20_repository_permanent_credentials.yml index a0c2d2e593a47..5590122e26b86 100644 --- a/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/20_repository_permanent_credentials.yml +++ b/plugins/repository-s3/src/yamlRestTest/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/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/30_repository_temporary_credentials.yml b/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/30_repository_temporary_credentials.yml index d26a07eec85dc..741e96110ea96 100644 --- a/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/30_repository_temporary_credentials.yml +++ b/plugins/repository-s3/src/yamlRestTest/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/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/40_repository_ec2_credentials.yml b/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/40_repository_ec2_credentials.yml index 6d3b174b99863..8d4349845a1f6 100644 --- a/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/40_repository_ec2_credentials.yml +++ b/plugins/repository-s3/src/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/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/50_repository_ecs_credentials.yml b/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/50_repository_ecs_credentials.yml index 79d1e3d9c3bbe..8650af2d29852 100644 --- a/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/50_repository_ecs_credentials.yml +++ b/plugins/repository-s3/src/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/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/60_repository_eks_credentials.yml b/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/60_repository_eks_credentials.yml index 15f2c9612a2ba..e01d3e87eb35f 100644 --- a/plugins/repository-s3/src/yamlRestTest/resources/rest-api-spec/test/repository_s3/60_repository_eks_credentials.yml +++ b/plugins/repository-s3/src/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