HADOOP-18945. S3A: IAMInstanceCredentialsProvider failing. #6202
Conversation
This file contains 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
|
tested s3 london with a vpn up to make things slower
one failure in the new test from HADOOP-18939; created HADOOP-18946 to cover it |
steveloughran
commented
Oct 18, 2023
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(IAMInstanceCredentialsProvider.class); | ||
|
|
||
| private HttpCredentialsProvider iamCredentialsProvider = null; |
There was a problem hiding this comment.
add some javadocs
...doop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/TestIAMInstanceCredentialsProvider.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| // this is expected if the test is not running in a container/EC2 | ||
| LOG.info("Not running in a container/EC2"); | ||
| LOG.info("Exception raised", expected); | ||
| // and we expect to have fallen back to the EC2 provider |
There was a problem hiding this comment.
fallback to instance profile provider,
| @@ -78,23 +117,52 @@ public AwsCredentials resolveCredentials() { | |||
| * | |||
| * @return credentials | |||
| */ | |||
| private AwsCredentials getCredentials() { | |||
| private synchronized AwsCredentials getCredentials() { | |||
There was a problem hiding this comment.
shouldn't this have been synchronized from the start?
There was a problem hiding this comment.
no, because it was creating and collecting credentials on every call. so if multiple requests came in, it instantiated new ones and did new HTTP calls. inefficient and clearl a bit brittle. now we want to only have one instance which we close() after
|
Some UT's failing |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ahmarsuhail
approved these changes
Oct 20, 2023
|
|
||
| /** | ||
| * Unit tests for IAMInstanceCredentials provider. | ||
| * This is a bit tricky as don't want to |
There was a problem hiding this comment.
nit: incomplete comment? unless you just didn't want to..
There was a problem hiding this comment.
will fix and then merge. thanks
- Enable async refresh in inner credential provider. - Store provider instance in a field; use synchronized() call to handle failure and switch from container to EC2 provider. - Close inner provider in close(); - use ErrorTranslation to extract any IOE at base on stack - rename maybeExtractNetworkException to maybeExtractIOException (as requested in review of third party support) - test for error translation going through UncheckedIOException - TestIAMInstanceCredentialsProvider to test fallback process TestIAMInstanceCredentialsProvider is tricky as there are different test outcomes 1. In EC2: credentials resolved. Assert comes with a key. 2. Not in EC2: network error trying to talk to the service. Assert wrapped exception is an IOE. 3. IMDS resolution disabled by env var/sysprop. Expect the message to contain the text from the SDK This is potentially brittle; there may be followups. Change-Id: I1ad1224e31c0c3646e07a93d503a84885bf99ebe
Minor tweaks to test, javadocs, revert some changes added for testing but which aren't used -just cuts down the patch size Change-Id: Id39f1196095a2157dbf61291285885542a44a782
Change-Id: I6241091ace05c51fc26e42b99fdb00b13e161a69
Change-Id: Iac2f9821e1f5d597313145bc63d1467cd68ca6d6
Change-Id: I0135620db5228c8aa3ecac03a0e7797984f872c9
steveloughran
force-pushed
the
s3/HADOOP-18945-IAMInstanceCredentialsProvider
branch
from
a4d83fb
to
9f2ffea
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ahmarsuhail
pushed a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Nov 27, 2023
This restores asynchronous retrieval/refresh of any AWS credentials provided by the EC2 instance/container in which the process is running. Contributed by Steve Loughran
ahmarsuhail
pushed a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Dec 5, 2023
This restores asynchronous retrieval/refresh of any AWS credentials provided by the EC2 instance/container in which the process is running. Contributed by Steve Loughran
ahmarsuhail
pushed a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Dec 5, 2023
This restores asynchronous retrieval/refresh of any AWS credentials provided by the EC2 instance/container in which the process is running. Contributed by Steve Loughran
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
This restores asynchronous retrieval/refresh of any AWS credentials provided by the EC2 instance/container in which the process is running. Contributed by Steve Loughran
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.
TestIAMInstanceCredentialsProvider is tricky as there are different test outcomes
The test is potentially brittle; there may be followups.
How was this patch tested?
AWS_EC2_METADATA_DISABLED=true(my default) and with it unset.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?