-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19044. AWS SDK V2 - Update S3A region logic #6479
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.
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
Changes from 7 commits
872dcac
d8c9793
cefb30b
81d345b
05225bb
2c653da
99cf2d4
12ff2f1
fcf56cf
6197723
adb10d3
54e5c66
e65a51a
2b89855
5fdb23d
b1f4df9
44760ca
12e503e
09ff933
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -267,7 +267,8 @@ protected ClientOverrideConfiguration.Builder createClientOverrideConfiguration( | |
| */ | ||
| private <BuilderT extends S3BaseClientBuilder<BuilderT, ClientT>, ClientT> void configureEndpointAndRegion( | ||
| BuilderT builder, S3ClientCreationParameters parameters, Configuration conf) { | ||
| URI endpoint = getS3Endpoint(parameters.getEndpoint(), conf); | ||
| final String endpointStr = parameters.getEndpoint(); | ||
| URI endpoint = getS3Endpoint(endpointStr, conf); | ||
|
|
||
| String configuredRegion = parameters.getRegion(); | ||
| Region region = null; | ||
|
|
@@ -294,9 +295,14 @@ private <BuilderT extends S3BaseClientBuilder<BuilderT, ClientT>, ClientT> void | |
| builder.endpointOverride(endpoint); | ||
| // No region was configured, try to determine it from the endpoint. | ||
| if (region == null) { | ||
| region = getS3RegionFromEndpoint(parameters.getEndpoint()); | ||
| boolean endpointEndsWithCentral = endpointStr.endsWith(CENTRAL_ENDPOINT); | ||
| region = getS3RegionFromEndpoint(endpointStr, endpointEndsWithCentral); | ||
| if (region != null) { | ||
| origin = "endpoint"; | ||
| if (endpointEndsWithCentral) { | ||
| builder.crossRegionAccessEnabled(true); | ||
| LOG.debug("Enabling cross region access for endpoint {}", endpointStr); | ||
|
mukund-thakur marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
| LOG.debug("Setting endpoint to {}", endpoint); | ||
|
|
@@ -354,20 +360,21 @@ private static URI getS3Endpoint(String endpoint, final Configuration conf) { | |
|
|
||
| /** | ||
| * Parses the endpoint to get the region. | ||
| * If endpoint is the central one, use US_EAST_1. | ||
| * If endpoint is the central one, use US_EAST_2. | ||
| * | ||
| * @param endpoint the configure endpoint. | ||
| * @param endpointEndsWithCentral true if the endpoint is configured as central. | ||
| * @return the S3 region, null if unable to resolve from endpoint. | ||
| */ | ||
| private static Region getS3RegionFromEndpoint(String endpoint) { | ||
| private static Region getS3RegionFromEndpoint(String endpoint, boolean endpointEndsWithCentral) { | ||
|
|
||
| if(!endpoint.endsWith(CENTRAL_ENDPOINT)) { | ||
| if (!endpointEndsWithCentral) { | ||
| LOG.debug("Endpoint {} is not the default; parsing", endpoint); | ||
| return AwsHostNameUtils.parseSigningRegion(endpoint, S3_SERVICE_NAME).orElse(null); | ||
| } | ||
|
|
||
| // endpoint is for US_EAST_1; | ||
| return Region.US_EAST_1; | ||
| // endpoint is for US_EAST_2; | ||
| return Region.US_EAST_2; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changing this causes confusion. Maybe its better to the use the variable present in Constants. |
||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.fs.s3a; | ||
|
|
||
| import org.junit.Test; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.fs.FSDataOutputStream; | ||
| import org.apache.hadoop.fs.Path; | ||
| import org.apache.hadoop.fs.contract.ContractTestUtils; | ||
|
|
||
| import static org.apache.hadoop.fs.s3a.Constants.AWS_REGION; | ||
| import static org.apache.hadoop.fs.s3a.Constants.CENTRAL_ENDPOINT; | ||
| import static org.apache.hadoop.fs.s3a.Constants.ENDPOINT; | ||
| import static org.apache.hadoop.fs.s3a.S3ATestUtils.removeBaseAndBucketOverrides; | ||
|
|
||
| /** | ||
| * Test to verify cross region bucket access. | ||
| */ | ||
| public class ITestS3ACrossRegionAccess extends AbstractS3ATestBase { | ||
|
|
||
| @Test | ||
| public void testCentralEndpointCrossRegionAccess() throws Throwable { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be better to move ITestsS3AEndpoint instead of creating a new test class?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that test, we are not trying to create/write/read file using fs, so i thought of keeping this separate. This test fails during mkdir with 400 without the source change. |
||
| describe("Create bucket on different region and access it using central endpoint"); | ||
| Configuration conf = getConfiguration(); | ||
| removeBaseAndBucketOverrides(conf, ENDPOINT, AWS_REGION); | ||
|
|
||
| Configuration newConf = new Configuration(conf); | ||
|
|
||
| newConf.set(ENDPOINT, CENTRAL_ENDPOINT); | ||
|
|
||
| try (S3AFileSystem newFs = new S3AFileSystem()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you look at the tests in For this what we want to see is that if the central point is configured, and no region is configured .. region gets set to US_EAST_2 for cross region. But if central endpoint is configured, and region is configured to US_EAST_1 , then region is US_EAST_1. that is region config takes precedence. See testCentralEndpoint in that class, does something similar.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked that test but since there was no real fs operation involved, i thought of keeping this as separate test. Does that work?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to do a mdkir to verify behaviour though? won't just doing a headBucket also work? I think that would also fail without your change?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we have our custom
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping the stacktrace here for reference:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry to nitpick here! but in ITestS3AEndpointRegion, there is also a test currently that uses the FS, see You can add another test there that does something like: This will fail without your source changes, but passes with them. |
||
| newFs.initialize(getFileSystem().getUri(), newConf); | ||
|
|
||
| final String file = getMethodName(); | ||
| Path basePath = new Path("basePath-" + getMethodName()); | ||
| final Path srcDir = new Path(basePath, "srcdir"); | ||
| newFs.mkdirs(srcDir); | ||
| Path src = new Path(srcDir, file); | ||
|
|
||
| try (FSDataOutputStream out = newFs.create(src)) { | ||
| out.write(new byte[] {1, 2, 3, 4, 5}); | ||
| } | ||
| ContractTestUtils.assertIsFile(getFileSystem(), new Path(srcDir, file)); | ||
| newFs.delete(srcDir, true); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,7 +146,17 @@ public void testCentralEndpoint() throws Throwable { | |
| describe("Create a client with the central endpoint"); | ||
| Configuration conf = getConfiguration(); | ||
|
|
||
| S3Client client = createS3Client(conf, CENTRAL_ENDPOINT, null, US_EAST_1, false); | ||
| S3Client client = createS3Client(conf, CENTRAL_ENDPOINT, null, US_EAST_2, false); | ||
|
|
||
| expectInterceptorException(client); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCentralEndpointWithRegion() throws Throwable { | ||
| describe("Create a client with the central endpoint but also specify region"); | ||
| Configuration conf = getConfiguration(); | ||
|
|
||
| S3Client client = createS3Client(conf, CENTRAL_ENDPOINT, US_WEST_2, US_WEST_2, false); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not clear to me what these different test cases are doing. looks like they call check if you set endpoint to central and also configure a region, it's always the configured region that gets set. do we really need all of them? |
||
|
|
||
| expectInterceptorException(client); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add more detail to origin, e,g 'origin with cross-region access"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done