steveloughran commented on code in PR #7284: URL: https://github.com/apache/hadoop/pull/7284#discussion_r1951317234
########## hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/ProfileAWSCredentialsProvider.java: ########## @@ -0,0 +1,56 @@ +package org.apache.hadoop.fs.s3a.auth; Review Comment: need the license...just copy from another class ########## hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java: ########## @@ -18,8 +18,7 @@ package org.apache.hadoop.fs.s3a; -import java.io.IOException; -import java.io.InterruptedIOException; +import java.io.*; Review Comment: revert; .* is not allowed in the hadoop code as it is too brittle ########## hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/ProfileAWSCredentialsProvider.java: ########## @@ -0,0 +1,56 @@ +package org.apache.hadoop.fs.s3a.auth; + +import software.amazon.awssdk.auth.credentials.AwsCredentials; +import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; +import software.amazon.awssdk.profiles.ProfileFile; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; + +import org.apache.commons.lang3.SystemUtils; +import org.apache.hadoop.conf.Configuration; + +import java.net.URI; +import java.nio.file.FileSystems; +import java.nio.file.Path; + [email protected] [email protected] +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"; Review Comment: add javadocs for these and the field at L25 ########## hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/ProfileAWSCredentialsProvider.java: ########## @@ -0,0 +1,56 @@ +package org.apache.hadoop.fs.s3a.auth; + +import software.amazon.awssdk.auth.credentials.AwsCredentials; +import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; +import software.amazon.awssdk.profiles.ProfileFile; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; + +import org.apache.commons.lang3.SystemUtils; +import org.apache.hadoop.conf.Configuration; + +import java.net.URI; +import java.nio.file.FileSystems; +import java.nio.file.Path; + [email protected] [email protected] +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"; + public static final String PROFILE_NAME = "fs.s3a.auth.profile.name"; + + private final ProfileCredentialsProvider pcp; + + private static Path getCredentialsPath(Configuration conf) { + String credentialsFile = conf.get(PROFILE_FILE, null); + if (credentialsFile == null) { + credentialsFile = SystemUtils.getEnvironmentVariable("AWS_SHARED_CREDENTIALS_FILE", null); + } + Path path = (credentialsFile == null) ? + FileSystems.getDefault().getPath(SystemUtils.getUserHome().getPath(),".aws","credentials") Review Comment: nit: can you add a space after the commas in the arguments ########## hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java: ########## @@ -139,6 +134,32 @@ public void testInstantiationChain() throws Throwable { assertCredentialProviders(expectedClasses, list); } + @Test + 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)) { Review 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 ########## hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/ProfileAWSCredentialsProvider.java: ########## @@ -0,0 +1,56 @@ +package org.apache.hadoop.fs.s3a.auth; + +import software.amazon.awssdk.auth.credentials.AwsCredentials; Review Comment: import ordering (should be) ``` java. javax. non-apache-stuff. org.apache. static everything --- it's close but a bit mixed up...please fix ########## hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java: ########## @@ -35,6 +34,7 @@ import java.util.stream.Collectors; import javax.annotation.Nullable; +import org.apache.hadoop.fs.s3a.auth.*; Review Comment: revert ########## hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/ProfileAWSCredentialsProvider.java: ########## @@ -0,0 +1,56 @@ +package org.apache.hadoop.fs.s3a.auth; + +import software.amazon.awssdk.auth.credentials.AwsCredentials; +import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; +import software.amazon.awssdk.profiles.ProfileFile; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; + +import org.apache.commons.lang3.SystemUtils; +import org.apache.hadoop.conf.Configuration; + +import java.net.URI; +import java.nio.file.FileSystems; +import java.nio.file.Path; + [email protected] [email protected] +public class ProfileAWSCredentialsProvider extends AbstractAWSCredentialProvider { Review 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. ########## hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java: ########## @@ -139,6 +134,32 @@ public void testInstantiationChain() throws Throwable { assertCredentialProviders(expectedClasses, list); } + @Test + 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)) { + bufferedWriter.write("[default]\n" + + "aws_access_key_id = defaultaccesskeyid\n" + + "aws_secret_access_key = defaultsecretkeyid\n"); + bufferedWriter.write("[nondefault]\n" + + "aws_access_key_id = nondefaultaccesskeyid\n" + + "aws_secret_access_key = nondefaultsecretkeyid\n"); + } + conf.set(ProfileAWSCredentialsProvider.PROFILE_FILE, "testcred"); + URI testUri = new URI("s3a://bucket1"); + AWSCredentialProviderList list = createAWSCredentialProviderList(testUri, conf); + assertCredentialProviders(Collections.singletonList(ProfileAWSCredentialsProvider.class), list); + AwsCredentials credentials = list.resolveCredentials(); + assertEquals("defaultaccesskeyid", credentials.accessKeyId()); Review Comment: Use AssertJ assertions. We need this to ease our migration to junit5 ########## hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/ProfileAWSCredentialsProvider.java: ########## @@ -0,0 +1,56 @@ +package org.apache.hadoop.fs.s3a.auth; + +import software.amazon.awssdk.auth.credentials.AwsCredentials; +import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; +import software.amazon.awssdk.profiles.ProfileFile; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; + +import org.apache.commons.lang3.SystemUtils; +import org.apache.hadoop.conf.Configuration; + +import java.net.URI; +import java.nio.file.FileSystems; +import java.nio.file.Path; + [email protected] [email protected] +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"; + public static final String PROFILE_NAME = "fs.s3a.auth.profile.name"; + + private final ProfileCredentialsProvider pcp; + + private static Path getCredentialsPath(Configuration conf) { + String credentialsFile = conf.get(PROFILE_FILE, null); + if (credentialsFile == null) { + credentialsFile = SystemUtils.getEnvironmentVariable("AWS_SHARED_CREDENTIALS_FILE", null); Review Comment: move all env vars to string constants; add javadocs for each of these ########## hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/ProfileAWSCredentialsProvider.java: ########## @@ -0,0 +1,56 @@ +package org.apache.hadoop.fs.s3a.auth; + +import software.amazon.awssdk.auth.credentials.AwsCredentials; +import software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider; +import software.amazon.awssdk.profiles.ProfileFile; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; + +import org.apache.commons.lang3.SystemUtils; +import org.apache.hadoop.conf.Configuration; + +import java.net.URI; +import java.nio.file.FileSystems; +import java.nio.file.Path; + [email protected] [email protected] +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"; + public static final String PROFILE_NAME = "fs.s3a.auth.profile.name"; + + private final ProfileCredentialsProvider pcp; + + private static Path getCredentialsPath(Configuration conf) { + String credentialsFile = conf.get(PROFILE_FILE, null); + if (credentialsFile == null) { + credentialsFile = SystemUtils.getEnvironmentVariable("AWS_SHARED_CREDENTIALS_FILE", null); + } + Path path = (credentialsFile == null) ? + FileSystems.getDefault().getPath(SystemUtils.getUserHome().getPath(),".aws","credentials") + : FileSystems.getDefault().getPath(credentialsFile); + return path; + } + + private static String getCredentialsName(Configuration conf) { + String profileName = conf.get(PROFILE_NAME, null); + if (profileName == null) { + profileName = SystemUtils.getEnvironmentVariable("AWS_PROFILE", "default"); + } + return profileName; + } + + 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()); Review Comment: should be split across lines, one per build attribute -- 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]
