HADOOP-19384. S3A: Add support for ProfileCredentialsProvider #7284
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
This commit adds a wrapper for the AWS ProfileCredentialsProvider.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
changed the title
steveloughran
requested changes
Jan 10, 2025
There was a problem hiding this comment.
This needs to be optional.
Adding something new to the chain has already caused problems and failures in the unit tests.
Because a lot of those developers have the AWS SDK installed, our test runs can accidentally pick this up when we do not intend to -hiding regression which then only surface in production. We have a hit exactly this problem in the past -and it is exactly the reason that the Yetus test runs have so many failing unit tests while you do not.
Instead then, please add it as a new class, apply the suggestions, and then document how to use it in the S3A documentation file hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/authentication.md
And as requested provide ways through the configuration to actually provide the path to the the profile file and which profile to use.
You can just ask for the software.amazonaws provider today; the S3A connector can authenticate with any in the SDK itself, unless it needs a special configuration. Picking up paths and profiles from the Configuration object is exactly the kind of configuration which justifies the effort.
On the topic of configuration
- nI would suggest the following two names
fs.s3a.auth.profile.file
fs.s3a.auth.profile.name
- If these are set then they MUST override the env vars of AWS_SHARED_CREDENTIALS_FILE, and AWS_PROFILE respectively.
Test wise: you should be able to write a unit test to attempt to load a dummy file the test setup writes to a temp dir, with the default profile and another one, returning the credentials you expect in both cases.
Finally, regarding the Itest failures, one of them looks like an intermittent test timing one. Proxy one is new. Does your test setup actually include a proxy? It would be good if you could debug this.
| @@ -1430,7 +1430,8 @@ | |||
| org.apache.hadoop.fs.s3a.TemporaryAWSCredentialsProvider, | |||
| org.apache.hadoop.fs.s3a.SimpleAWSCredentialsProvider, | |||
| software.amazon.awssdk.auth.credentials.EnvironmentVariableCredentialsProvider, | |||
| org.apache.hadoop.fs.s3a.auth.IAMInstanceCredentialsProvider | |||
| org.apache.hadoop.fs.s3a.auth.IAMInstanceCredentialsProvider, | |||
| org.apache.hadoop.fs.s3a.ProfileAWSCredentialsProvider | |||
There was a problem hiding this comment.
make this optional but document it
| import java.nio.file.Path; | ||
|
|
||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Stable |
| @@ -0,0 +1,46 @@ | |||
| package org.apache.hadoop.fs.s3a; | |||
|
|
|||
| import org.apache.commons.lang3.SystemUtils; | |||
There was a problem hiding this comment.
nit: use same import ordering as other classes
|
|
||
| public ProfileAWSCredentialsProvider(Configuration conf) { | ||
| ProfileCredentialsProvider.Builder builder = ProfileCredentialsProvider.builder(); | ||
| builder.profileName("default").profileFile(ProfileFile.builder().content(getCredentialsPath()).build()); |
There was a problem hiding this comment.
profile name should be configurable
| public static final String NAME | ||
| = "org.apache.hadoop.fs.s3a.ProfileAWSCredentialsProvider"; | ||
|
|
||
| private ProfileCredentialsProvider pcp; |
| private static Path getCredentialsPath() { | ||
| String credentialsFile = SystemUtils.getEnvironmentVariable("AWS_SHARED_CREDENTIALS_FILE", null); | ||
| Path path = (credentialsFile == null) ? | ||
| FileSystems.getDefault().getPath(SystemUtils.getUserHome().getPath(),".aws","credentials") |
There was a problem hiding this comment.
this path should be configurable
| @@ -84,7 +85,8 @@ public final class CredentialProviderListFactory { | |||
| EnvironmentVariableCredentialsProvider.class, | |||
| IAMInstanceCredentialsProvider.class, | |||
| SimpleAWSCredentialsProvider.class, | |||
| TemporaryAWSCredentialsProvider.class)); | |||
| TemporaryAWSCredentialsProvider.class, | |||
| ProfileAWSCredentialsProvider.class)); | |||
There was a problem hiding this comment.
not a standard one...will need to be explicitly configured
|
|
||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Stable | ||
| public class ProfileAWSCredentialsProvider implements AwsCredentialsProvider { |
There was a problem hiding this comment.
extends AbstractAWSCredentialProvider
and move into package auth
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Feb 11, 2025
| @@ -18,8 +18,7 @@ | |||
|
|
|||
| package org.apache.hadoop.fs.s3a; | |||
|
|
|||
| import java.io.IOException; | |||
| import java.io.InterruptedIOException; | |||
| import java.io.*; | |||
There was a problem hiding this comment.
revert; .* is not allowed in the hadoop code as it is too brittle
| @@ -35,6 +34,7 @@ | |||
| import java.util.stream.Collectors; | |||
| import javax.annotation.Nullable; | |||
|
|
|||
| import org.apache.hadoop.fs.s3a.auth.*; | |||
| @@ -0,0 +1,56 @@ | |||
| package org.apache.hadoop.fs.s3a.auth; | |||
There was a problem hiding this comment.
need the license...just copy from another class
| @@ -0,0 +1,56 @@ | |||
| package org.apache.hadoop.fs.s3a.auth; | |||
|
|
|||
| import software.amazon.awssdk.auth.credentials.AwsCredentials; | |||
There was a problem hiding this comment.
import ordering (should be)
java.
javax.
non-apache-stuff.
org.apache.
static everything
---
it's close but a bit mixed up...please fix
| private static Path getCredentialsPath(Configuration conf) { | ||
| String credentialsFile = conf.get(PROFILE_FILE, null); | ||
| if (credentialsFile == null) { | ||
| credentialsFile = SystemUtils.getEnvironmentVariable("AWS_SHARED_CREDENTIALS_FILE", null); |
There was a problem hiding this comment.
move all env vars to string constants; add javadocs for each of these
| public ProfileAWSCredentialsProvider(URI uri, Configuration conf) { | ||
| super(uri, conf); | ||
| ProfileCredentialsProvider.Builder builder = ProfileCredentialsProvider.builder(); | ||
| builder.profileName(getCredentialsName(conf)).profileFile(ProfileFile.builder().content(getCredentialsPath(conf)).type(ProfileFile.Type.CREDENTIALS).build()); |
There was a problem hiding this comment.
should be split across lines, one per build attribute
|
|
||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Evolving | ||
| public class ProfileAWSCredentialsProvider extends AbstractAWSCredentialProvider { |
There was a problem hiding this comment.
I wonder if we should be logging at debug when the env vars are being used? Nothing secret will be logged and it could be useful.
| public class ProfileAWSCredentialsProvider extends AbstractAWSCredentialProvider { | ||
| public static final String NAME | ||
| = "org.apache.hadoop.fs.s3a.auth.ProfileAWSCredentialsProvider"; | ||
| public static final String PROFILE_FILE = "fs.s3a.auth.profile.file"; |
There was a problem hiding this comment.
add javadocs for these and the field at L25
| public void testProfileAWSCredentialsProvider() throws Throwable { | ||
| Configuration conf = new Configuration(false); | ||
| conf.set(AWS_CREDENTIALS_PROVIDER, ProfileAWSCredentialsProvider.NAME); | ||
| try (FileWriter fileWriter = new FileWriter("testcred"); BufferedWriter bufferedWriter = new BufferedWriter(fileWriter)) { |
There was a problem hiding this comment.
the test files have to go into target/, whether run on the command line or IDE.
use File.createTempFile() to get one in the temp dir
| AWSCredentialProviderList list = createAWSCredentialProviderList(testUri, conf); | ||
| assertCredentialProviders(Collections.singletonList(ProfileAWSCredentialsProvider.class), list); | ||
| AwsCredentials credentials = list.resolveCredentials(); | ||
| assertEquals("defaultaccesskeyid", credentials.accessKeyId()); |
There was a problem hiding this comment.
Use AssertJ assertions. We need this to ease our migration to junit5
|
🎊 +1 overall
This message was automatically generated. |
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.
This commit adds a wrapper for the AWS
ProfileCredentialsProvider.
How was this patch tested?
The patch was tested by running the hadoop-aws integration tests with fs.s3a.aws.credentials.provider and fs.s3a.assumed.role.credentials.provider configured to only include org.apache.hadoop.fs.s3a.ProfileAWSCredentialsProvider. Buckets/endpoints used were in the us-east-1 region. 2 test failures that seem unrelated to this change have details in the JIRA.