Skip to content

Add option to force S3 region to connect to #14398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Praveen2112 merged 1 commit into from
Oct 5, 2022
Merged

Add option to force S3 region to connect to #14398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/HiveS3Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public class HiveS3Config
private String s3AwsAccessKey;
private String s3AwsSecretKey;
private String s3Endpoint;
private String s3Region;
private TrinoS3StorageClass s3StorageClass = TrinoS3StorageClass.STANDARD;
private TrinoS3SignerType s3SignerType;
private String s3SignerClass;
Expand Down Expand Up @@ -118,6 +119,18 @@ public HiveS3Config setS3Endpoint(String s3Endpoint)
return this;
}

public String getS3Region()
{
return s3Region;
}

@Config("hive.s3.region")
public HiveS3Config setS3Region(String s3Region)
{
this.s3Region = s3Region;
return this;
}

@NotNull
public TrinoS3StorageClass getS3StorageClass()
{
Expand Down
6 changes: 6 additions & 0 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/TrinoS3ConfigurationInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_PROXY_PORT;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_PROXY_PROTOCOL;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_PROXY_USERNAME;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_REGION;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_REQUESTER_PAYS_ENABLED;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_SECRET_KEY;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_SIGNER_CLASS;
Expand All @@ -72,6 +73,7 @@ public class TrinoS3ConfigurationInitializer
private final String awsAccessKey;
private final String awsSecretKey;
private final String endpoint;
private final String region;
private final TrinoS3StorageClass s3StorageClass;
private final TrinoS3SignerType signerType;
private final boolean pathStyleAccess;
Expand Down Expand Up @@ -117,6 +119,7 @@ public TrinoS3ConfigurationInitializer(HiveS3Config config)
this.awsAccessKey = config.getS3AwsAccessKey();
this.awsSecretKey = config.getS3AwsSecretKey();
this.endpoint = config.getS3Endpoint();
this.region = config.getS3Region();
this.s3StorageClass = config.getS3StorageClass();
this.signerType = config.getS3SignerType();
this.signerClass = config.getS3SignerClass();
Expand Down Expand Up @@ -174,6 +177,9 @@ public void initializeConfiguration(Configuration config)
if (endpoint != null) {
config.set(S3_ENDPOINT, endpoint);
}
if (region != null) {
config.set(S3_REGION, region);
}
config.set(S3_STORAGE_CLASS, s3StorageClass.name());
if (signerType != null) {
config.set(S3_SIGNER_TYPE, signerType.name());
Expand Down
8 changes: 7 additions & 1 deletion plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/TrinoS3FileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ public class TrinoS3FileSystem
public static final String S3_SIGNER_TYPE = "trino.s3.signer-type";
public static final String S3_SIGNER_CLASS = "trino.s3.signer-class";
public static final String S3_ENDPOINT = "trino.s3.endpoint";
public static final String S3_REGION = "trino.s3.region";
public static final String S3_SECRET_KEY = "trino.s3.secret-key";
public static final String S3_ACCESS_KEY = "trino.s3.access-key";
public static final String S3_SESSION_TOKEN = "trino.s3.session-token";
Expand Down Expand Up @@ -925,8 +926,13 @@ private AmazonS3 createAmazonS3Client(Configuration hadoopConfig, ClientConfigur
}

Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the region even if endpoint isn't set

        String endpoint = hadoopConfig.get(S3_ENDPOINT);
        String region = hadoopConfig.get(S3_REGION);
        if (endpoint != null) {
            clientBuilder.setEndpointConfiguration(new EndpointConfiguration(endpoint, region));
            regionOrEndpointSet = true;
        }
        else if (region != null) {
            clientBuilder.setRegion(region);
            regionOrEndpointSet = true;
        }

Copy link
Copy Markdown
Member Author

@skrzypo987 skrzypo987 Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are actually right. I totally ignored the fact that setting regionOrEndpointSet to true will prevent US_EAST_1 to be set as a default few lines later.
It will not effectively change anything as US_EAST_1 is always chosen as a default one, but we should set it nonetheless.

Made the change and altered tests.

String endpoint = hadoopConfig.get(S3_ENDPOINT);
String region = hadoopConfig.get(S3_REGION);
if (endpoint != null) {
clientBuilder.setEndpointConfiguration(new EndpointConfiguration(endpoint, null));
clientBuilder.setEndpointConfiguration(new EndpointConfiguration(endpoint, region));
regionOrEndpointSet = true;
}
else if (region != null) {
clientBuilder.setRegion(region);
regionOrEndpointSet = true;
}

Expand Down
3 changes: 3 additions & 0 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestHiveS3Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public void testDefaults()
.setS3AwsAccessKey(null)
.setS3AwsSecretKey(null)
.setS3Endpoint(null)
.setS3Region(null)
.setS3SignerType(null)
.setS3SignerClass(null)
.setS3PathStyleAccess(false)
Expand Down Expand Up @@ -91,6 +92,7 @@ public void testExplicitPropertyMappings()
.put("hive.s3.aws-access-key", "abc123")
.put("hive.s3.aws-secret-key", "secret")
.put("hive.s3.endpoint", "endpoint.example.com")
.put("hive.s3.region", "eu-central-1")
.put("hive.s3.signer-type", "S3SignerType")
.put("hive.s3.signer-class", "com.amazonaws.services.s3.internal.AWSS3V4Signer")
.put("hive.s3.path-style-access", "true")
Expand Down Expand Up @@ -135,6 +137,7 @@ public void testExplicitPropertyMappings()
.setS3AwsAccessKey("abc123")
.setS3AwsSecretKey("secret")
.setS3Endpoint("endpoint.example.com")
.setS3Region("eu-central-1")
.setS3SignerType(TrinoS3SignerType.S3SignerType)
.setS3SignerClass("com.amazonaws.services.s3.internal.AWSS3V4Signer")
.setS3PathStyleAccess(true)
Expand Down
30 changes: 30 additions & 0 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestTrinoS3FileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_MAX_RETRY_TIME;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_PATH_STYLE_ACCESS;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_PIN_CLIENT_TO_CURRENT_REGION;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_REGION;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_SECRET_KEY;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_SESSION_TOKEN;
import static io.trino.plugin.hive.s3.TrinoS3FileSystem.S3_SKIP_GLACIER_OBJECTS;
Expand Down Expand Up @@ -171,6 +172,35 @@ public void testEndpointWithPinToCurrentRegionConfiguration()
}
}

@Test
public void testEndpointWithExplicitRegionConfiguration()
throws Exception
{
Configuration config = newEmptyConfiguration();

// Only endpoint set
config.set(S3_ENDPOINT, "test.example.endpoint.com");
try (TrinoS3FileSystem fs = new TrinoS3FileSystem()) {
fs.initialize(new URI("s3a://test-bucket/"), config);
assertThat(((AmazonS3Client) fs.getS3Client()).getSignerRegionOverride()).isNull();
}

// Endpoint and region set
config.set(S3_ENDPOINT, "test.example.endpoint.com");
config.set(S3_REGION, "region1");
try (TrinoS3FileSystem fs = new TrinoS3FileSystem()) {
fs.initialize(new URI("s3a://test-bucket/"), config);
assertThat(((AmazonS3Client) fs.getS3Client()).getSignerRegionOverride()).isEqualTo("region1");
}

// Only region set
config.set(S3_REGION, "region1");
try (TrinoS3FileSystem fs = new TrinoS3FileSystem()) {
fs.initialize(new URI("s3a://test-bucket/"), config);
assertThat(((AmazonS3Client) fs.getS3Client()).getSignerRegionOverride()).isEqualTo("region1");
}
}

@Test
public void testAssumeRoleDefaultCredentials()
throws Exception
Expand Down