HADOOP-16806: AWS AssumedRoleCredentialProvider needs ExternalId add #4753
+75
−5
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
|
which s3 region did you run the hadoop-aws integration tests against? |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Aug 18, 2022
| @@ -182,6 +196,12 @@ protected Configuration createValidRoleConf() throws JsonProcessingException { | |||
| return conf; | |||
| } | |||
|
|
|||
| protected Configuration createValidRoleConfWithExternalId() throws JsonProcessingException { | |||
There was a problem hiding this comment.
why this method? on L160 its setting is completely overwritten
| @@ -152,6 +152,20 @@ public void testCreateCredentialProvider() throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testCreateCredentialProviderWithExternalId() throws IOException { | |||
There was a problem hiding this comment.
how about a test where a known invalid id set, with the expectation of a failure. that way wire up can be verified and a new stack trace for troubleshooting.md/assumed_roles.md is created.
use intercept() to catch the exception by type, but don't include any string match on the message text...that has turned out to be very brittle with SDK updates
|
I've updated the test, and I attest that I ran all of the hadoop-aws tests, as well as integration tests against us-east-1. However, I was unable to successfully run This seems unrelated to the feature at hand, but I can revisit if required. |
|
🎊 +1 overall
This message was automatically generated. |
got a stack trace? |
|
Tried with and without an |
|
maybe its a permissions thing. show the full stack trace for the failed tests, please |
The other test stack trace is basically the same |
|
that test adds "s3a://osm-pds/planet/planet-latest.orc" as a requester-pays source of data. make sure your role has the permissions to access it |
|
It looks like the issue is in retrieving I've enabled the low-level request tracing. Here's what it looks like for the first parameterized test: I've also tried adding this to my auth-keys, which likewise does not work: I've seen mention in HADOOP-13551 and HADOOP-18340 that folks have run into similar issues with this test, although I haven't been able to determine what, if anything, was done to correct it. |
|
Unfortunately, the errors can be a bit vague for S3 HeadObject (used by getFileStatus). There was a similar issue when the credentials had expired in HADOOP-18353. The signature is signed for us-east-1, but the If this is the root cause, we may need to consider resetting this configuration for all public data set tests if the bucket was not reconfigured. |
|
Thanks @dannycjones, that did it! All integration tests have now passed in this PR. I was indeed using |
|
need to fix hadoop-tools/hadoop-aws/src/test/resources/core-site.xml ; it is set there |
|
Specifying There is an issue with Commenting out my region settings allows these tests to pass, however. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Aug 25, 2022
| @@ -38,6 +38,11 @@ | |||
| <description>The endpoint for s3a://landsat-pds URLs</description> | |||
| </property> | |||
|
|
|||
| <property> | |||
There was a problem hiding this comment.
can you change the endpoint property above to match?
|
🎊 +1 overall
This message was automatically generated. |
|
Is there anything left to do on this PR @steveloughran ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
This applies an
externalIdvalue to theSTSAssumeRoleSessionCredentialsProvider.Builder, if provided in the Hadoop config fieldfs.s3a.assumed.role.externalid.This allows for AWS resources to have a trust policy for
sts:AssumeRolethat can match on the externalId which is now provided as part of the assume role request, in order to solve the confused deputy problemI'm happy to take guidance on an improved unit test or any other changes. I'm relatively unfamiliar with the Hadoop unit testing framework.
How was this patch tested?
Manual testing, and now running in a production SaaS offering.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?