HADOOP-18938. AWS SDK v2: Fix endpoint region parsing for vpc endpoints. #6466
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
what I'd really like, as it'd make so many support calls go away, is to go from an endpoint like
<property>
<name>fs.s3a.bucket.stevel2-usw2.endpoint</name>
<value>https://bucket.vpce-0117f6033eaf7aee5.s3.us-west-2.vpce.amazonaws.com/</value>
</property>to knowing that the region is us-west-2. It's there, and we've modified the v1 sdk override json to make it go away for some people.
can we have that same logic here?
| /** | ||
| * The vpc endpoint :{@value}. | ||
| */ | ||
| public static final String VPC_ENDPOINT = "vpce.amazonaws.com"; |
There was a problem hiding this comment.
- move to InternalConstants
- is there a .cn equivalent?
|
💔 -1 overall
This message was automatically generated. |
shintaroonuma
force-pushed
the
HADOOP-18938-s3a-vpce-region-handling
branch
from
1c39495 to
b32bb52
Compare
|
Thanks for the review! Added parsing of S3 VPC endpoints to help reduce the support calls. |
|
🎊 +1 overall
This message was automatically generated. |
shintaroonuma
changed the title
steveloughran
requested changes
Jan 25, 2024
|
|
||
| // S3 VPC endpoint parsing | ||
| Matcher matcher = VPC_ENDPOINT_PATTERN.matcher(endpoint); | ||
| if(matcher.find()) { |
There was a problem hiding this comment.
nit, add a space after if
| // S3 VPC endpoint parsing | ||
| Matcher matcher = VPC_ENDPOINT_PATTERN.matcher(endpoint); | ||
| if(matcher.find()) { | ||
| LOG.debug("Endpoint {} is vpc endpoint; parsing", endpoint); |
There was a problem hiding this comment.
so this is going to match on anything with .vpce. isn't it? I think it should include amazonaws.{com,com.cn} at the end so if someone ever sets up an internal host called vpce there's no confusion.
There was a problem hiding this comment.
Updated PR to match entire endpoint to avoid the confusion.
| Matcher matcher = VPC_ENDPOINT_PATTERN.matcher(endpoint); | ||
| if(matcher.find()) { | ||
| LOG.debug("Endpoint {} is vpc endpoint; parsing", endpoint); | ||
| return Region.of(matcher.group(1)); |
There was a problem hiding this comment.
add a debug log saying "mapping to vpce"
There was a problem hiding this comment.
could you log the group(1) value in the debug log so we can see exactly which one it is. region and endpoint bindings are the cause of many of our support calls.
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Jan 30, 2024
| * @param endpoint the configure endpoint. | ||
| * @return the S3 region, null if unable to resolve from endpoint. | ||
| */ | ||
| private static Region getS3RegionFromEndpoint(String endpoint) { |
There was a problem hiding this comment.
this could be made package private/visible for testing and then have some unit tests which will run every time yetus does its work. this should include a test which passes an endpoint which shouldn't match the regexp -and verifies that it is rejected
There was a problem hiding this comment.
Added some unit tests on endpoint parsing.
| Matcher matcher = VPC_ENDPOINT_PATTERN.matcher(endpoint); | ||
| if(matcher.find()) { | ||
| LOG.debug("Endpoint {} is vpc endpoint; parsing", endpoint); | ||
| return Region.of(matcher.group(1)); |
There was a problem hiding this comment.
could you log the group(1) value in the debug log so we can see exactly which one it is. region and endpoint bindings are the cause of many of our support calls.
| private static final String S3_SERVICE_NAME = "s3"; | ||
|
|
||
| private static final Pattern VPC_ENDPOINT_PATTERN = | ||
| Pattern.compile("^(?:.+\\.)?([a-z0-9-]+)\\.vpce\\.amazonaws\\.(?:com|com\\.cn)$"); |
There was a problem hiding this comment.
does aws govcloud have a different pattern? or is it just .cn
There was a problem hiding this comment.
Govcloud is amazonaws.com aswell.
There was a problem hiding this comment.
@shintaroonuma could you do a quick rebase and revise of this...the faster it goes in the better it is for all.
shintaroonuma
force-pushed
the
HADOOP-18938-s3a-vpce-region-handling
branch
from
487921f to
5580fa2
Compare
|
Thanks for the comments, rebased and updated the pr. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
sorry, forgotten about this...don't be afraid to remind me! |
steveloughran
approved these changes
Sep 5, 2024
|
merged. @shintaroonuma what is your jira account, so we can assign the issue to you there too. |
Thanks for merging the commit in. @steveloughran , my jira account username is shintaro |
|
thanks. credited you there. also backported with testing onto branches 3.4 and 3.4.1; the next RC of that release is coming soon! |
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
…ache#6466) Contributed by Shintaro Onuma
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
…ache#6466) Contributed by Shintaro Onuma
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-18938
Perform endpoint parsing for regions when vpc endpoint is configured. Previous endpoint parsing behaviour would parse vpc endpoints incorrectly, setting the region as "vpce".
How was this patch tested?
Tested with a bucket in eu-west-1 with
mvn -Dparallel-tests -DtestsThreadCount=16 clean verifyAll tests passing other than 2 failures:
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?