Skip to content

[AWS] S3FileIO - Add Cross-Region Bucket Access #9804

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

Closed
ebelgasmi12 wants to merge 4 commits into from
Closed

[AWS] S3FileIO - Add Cross-Region Bucket Access #9804

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4d5a935
[AWS] S3FileIO - Add Cross-Region Bucket Access
ebelgasmi12 Feb 26, 2024
d2f7d47
[AWS][Docs] S3FileIO - Add Cross-Region Bucket Access
ebelgasmi12 Feb 26, 2024
63eddfa
[AWS][doc] Fixed wrong link for Cross-Region bucket access
ebelgasmi12 Feb 26, 2024
4303eeb
[AWS][doc] S3FileIO - Add SparkSQL shell command example
ebelgasmi12 Feb 27, 2024
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
22 changes: 21 additions & 1 deletion aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,16 @@ public class S3FileIOProperties implements Serializable {

public static final boolean DUALSTACK_ENABLED_DEFAULT = false;

/**
* Determines if S3 client will allow Cross-Region bucket access, default to false.
*
* <p>For more details, see
* https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/s3-cross-region.html
*/
public static final String CROSS_REGION_ACCESS_ENABLED = "s3.cross-region-access-enabled";

public static final boolean CROSS_REGION_ACCESS_ENABLED_DEFAULT = false;

/**
* Used by {@link S3FileIO}, prefix used for bucket access point configuration. To set, we can
* pass a catalog property.
Expand Down Expand Up @@ -399,6 +409,7 @@ public class S3FileIOProperties implements Serializable {
private final Map<String, String> bucketToAccessPointMapping;
private boolean isPreloadClientEnabled;
private boolean isDualStackEnabled;
private boolean isCrossRegionAccessEnabled;
private boolean isPathStyleAccess;
private boolean isUseArnRegionEnabled;
private boolean isAccelerationEnabled;
Expand Down Expand Up @@ -431,6 +442,7 @@ public S3FileIOProperties() {
this.bucketToAccessPointMapping = Collections.emptyMap();
this.isPreloadClientEnabled = PRELOAD_CLIENT_ENABLED_DEFAULT;
this.isDualStackEnabled = DUALSTACK_ENABLED_DEFAULT;
this.isCrossRegionAccessEnabled = CROSS_REGION_ACCESS_ENABLED_DEFAULT;
this.isPathStyleAccess = PATH_STYLE_ACCESS_DEFAULT;
this.isUseArnRegionEnabled = USE_ARN_REGION_ENABLED_DEFAULT;
this.isAccelerationEnabled = ACCELERATION_ENABLED_DEFAULT;
Expand Down Expand Up @@ -472,6 +484,9 @@ public S3FileIOProperties(Map<String, String> properties) {
properties, ACCELERATION_ENABLED, ACCELERATION_ENABLED_DEFAULT);
this.isDualStackEnabled =
PropertyUtil.propertyAsBoolean(properties, DUALSTACK_ENABLED, DUALSTACK_ENABLED_DEFAULT);
this.isCrossRegionAccessEnabled =
PropertyUtil.propertyAsBoolean(
properties, CROSS_REGION_ACCESS_ENABLED, CROSS_REGION_ACCESS_ENABLED_DEFAULT);
try {
this.multiPartSize =
PropertyUtil.propertyAsInt(properties, MULTIPART_SIZE, MULTIPART_SIZE_DEFAULT);
Expand Down Expand Up @@ -625,6 +640,10 @@ public boolean isDualStackEnabled() {
return this.isDualStackEnabled;
}

public boolean isCrossRegionAccessEnabled() {
return this.isCrossRegionAccessEnabled;
}

public boolean isPathStyleAccess() {
return this.isPathStyleAccess;
}
Expand Down Expand Up @@ -749,7 +768,7 @@ public <T extends S3ClientBuilder> void applyCredentialConfigurations(

/**
* Configure services settings for an S3 client. The settings include: s3DualStack,
* s3UseArnRegion, s3PathStyleAccess, and s3Acceleration
* crossRegionAccessEnabled, s3UseArnRegion, s3PathStyleAccess, and s3Acceleration
*
* <p>Sample usage:
*
Expand All @@ -760,6 +779,7 @@ public <T extends S3ClientBuilder> void applyCredentialConfigurations(
public <T extends S3ClientBuilder> void applyServiceConfigurations(T builder) {
builder
.dualstackEnabled(isDualStackEnabled)
.crossRegionAccessEnabled(isCrossRegionAccessEnabled)
.serviceConfiguration(
S3Configuration.builder()
.pathStyleAccessEnabled(isPathStyleAccess)
Expand Down
11 changes: 11 additions & 0 deletions aws/src/test/java/org/apache/iceberg/aws/s3/TestS3FileIOProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ public void testS3FileIOPropertiesDefaultValues() {
Assertions.assertThat(S3FileIOProperties.DUALSTACK_ENABLED_DEFAULT)
.isEqualTo(s3FileIOProperties.isDualStackEnabled());

Assertions.assertThat(S3FileIOProperties.CROSS_REGION_ACCESS_ENABLED_DEFAULT)
.isEqualTo(s3FileIOProperties.isCrossRegionAccessEnabled());

Assertions.assertThat(S3FileIOProperties.PATH_STYLE_ACCESS_DEFAULT)
.isEqualTo(s3FileIOProperties.isPathStyleAccess());

Expand Down Expand Up @@ -155,6 +158,11 @@ public void testS3FileIOProperties() {
S3FileIOProperties.DUALSTACK_ENABLED,
String.valueOf(s3FileIOProperties.isDualStackEnabled()));

Assertions.assertThat(map)
.containsEntry(
S3FileIOProperties.CROSS_REGION_ACCESS_ENABLED,
String.valueOf(s3FileIOProperties.isCrossRegionAccessEnabled()));

Assertions.assertThat(map)
.containsEntry(
S3FileIOProperties.PATH_STYLE_ACCESS,
Expand Down Expand Up @@ -383,6 +391,7 @@ private Map<String, String> getTestProperties() {
map.put(S3FileIOProperties.USE_ARN_REGION_ENABLED, "true");
map.put(S3FileIOProperties.ACCELERATION_ENABLED, "true");
map.put(S3FileIOProperties.DUALSTACK_ENABLED, "true");
map.put(S3FileIOProperties.CROSS_REGION_ACCESS_ENABLED, "true");
map.put(
S3FileIOProperties.MULTIPART_SIZE,
String.valueOf(S3FileIOProperties.MULTIPART_SIZE_DEFAULT));
Expand Down Expand Up @@ -428,6 +437,7 @@ public void testApplyCredentialConfigurations() {
public void testApplyS3ServiceConfigurations() {
Map<String, String> properties = Maps.newHashMap();
properties.put(S3FileIOProperties.DUALSTACK_ENABLED, "true");
properties.put(S3FileIOProperties.CROSS_REGION_ACCESS_ENABLED, "true");
properties.put(S3FileIOProperties.PATH_STYLE_ACCESS, "true");
properties.put(S3FileIOProperties.USE_ARN_REGION_ENABLED, "true");
// acceleration enabled has to be set to false if path style is true
Expand All @@ -439,6 +449,7 @@ public void testApplyS3ServiceConfigurations() {
ArgumentCaptor.forClass(S3Configuration.class);

Mockito.doReturn(mockA).when(mockA).dualstackEnabled(Mockito.anyBoolean());
Mockito.doReturn(mockA).when(mockA).crossRegionAccessEnabled(Mockito.anyBoolean());

@nastra nastra Feb 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please also add Assertions.assertThat(s3Configuration.crossRegionAccessEnabled()).isTrue() at the bottom of this test method

@elmehdibelgasmi elmehdibelgasmi Feb 26, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@nastra
crossRegionAccessEnabled is an S3Client attribute rather than S3Configuration (same as dualstackEnabled).
Similar to dualstackEnabled attribute (not asserted in this test method), s3Configuration.crossRegionAccessEnabled() does not exist.

@amogh-jahagirdar amogh-jahagirdar Feb 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah unfortunately I think getting the value of the attributes from the client isn't possible without doing some reflection hacks.

What I think we can do is add S3 integration tests though to verify the actual cross region access behavior. That seems like a more useful test to verify that when the property is set we can perform operations across regions as expected. cc @jackye1995 @geruh @rahil-c

@jackye1995 jackye1995 Feb 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 for integ test, there is already a AWS_CROSS_REGION config there that can be used

@elmehdibelgasmi elmehdibelgasmi Feb 27, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's also a AWS_TEST_CROSS_REGION_BUCKET config (although originally added for S3 Access Point tests).
Can anybody help / provide guidance on this feature's integration tests? Thanks.

Mockito.doReturn(mockA).when(mockA).serviceConfiguration(Mockito.any(S3Configuration.class));

s3FileIOProperties.applyServiceConfigurations(mockA);
Expand Down
7 changes: 7 additions & 0 deletions docs/docs/aws.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,13 @@ spark-sql --conf spark.sql.catalog.my_catalog=org.apache.iceberg.spark.SparkCata

For more details on using S3 Access Grants, please refer to [Managing access with S3 Access Grants](https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-grants.html).

### S3 Cross-Region Access

S3 Cross-Region bucket access can be turned on by setting catalog property `s3.cross-region-access-enabled` to `true`.
This is turned off by default to avoid first S3 API call increased latency.
For more details, please refer to [Cross-Region access for Amazon S3

@geruh geruh Feb 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we follow the convention of the other sections and add an example here?

Something like:

spark-sql --conf spark.sql.catalog.my_catalog=org.apache.iceberg.spark.SparkCatalog \
    --conf spark.sql.catalog.my_catalog.warehouse=s3://my-bucket2/my/key/prefix \
    --conf spark.sql.catalog.my_catalog.type=glue \
    --conf spark.sql.catalog.my_catalog.io-impl=org.apache.iceberg.aws.s3.S3FileIO \
    --conf spark.sql.catalog.my_catalog.s3.cross-region-access-enabled=true

@elmehdibelgasmi elmehdibelgasmi Feb 26, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I did the same as in S3 Access Control List and and S3 Write Checksum Verification sections, since it's a single parameter configuration. But I can definitely change it.

@elmehdibelgasmi elmehdibelgasmi Feb 27, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Doc updated.

](https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/s3-cross-region.html).

### S3 Acceleration

[S3 Acceleration](https://aws.amazon.com/s3/transfer-acceleration/) can be used to speed up transfers to and from Amazon S3 by as much as 50-500% for long-distance transfer of larger objects.
Expand Down