steveloughran commented on code in PR #6479:
URL: https://github.com/apache/hadoop/pull/6479#discussion_r1471515567
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEndpointRegion.java:
##########
@@ -257,6 +275,65 @@ public void testWithVPCE() throws Throwable {
expectInterceptorException(client);
}
+ @Test
+ public void testCentralEndpointCrossRegionAccess() throws Throwable {
+ describe("Create bucket on different region and access it using central
endpoint");
+ final Configuration conf = getConfiguration();
+ removeBaseAndBucketOverrides(conf, ENDPOINT);
Review Comment:
what should region be set to here? either unset it or explicitly set it.
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java:
##########
@@ -289,17 +290,36 @@ private <BuilderT extends S3BaseClientBuilder<BuilderT,
ClientT>, ClientT> void
builder.fipsEnabled(fipsEnabled);
if (endpoint != null) {
+ boolean overrideEndpoint = true;
checkArgument(!fipsEnabled,
"%s : %s", ERROR_ENDPOINT_WITH_FIPS, endpoint);
- 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);
+ // No region was configured or the endpoint is central,
+ // determine the region from the endpoint.
+ if (region == null || endpointEndsWithCentral) {
Review Comment:
I don't think anyone should set region=us-west-2 and endpoint = us-west-1
unless they like debugging things.
all we want is to handle situations where things are not set.
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEndpointRegion.java:
##########
@@ -146,7 +150,21 @@ 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_EAST_2, false);
Review Comment:
as in #6466 I'm going to propose we make the static methods accessible and
unit tests to validate them, because
* this stuff is so important and complicated we need it running on every pr
* everyone's ITest setup is different, so may miss things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
