HADOOP-14135. Remove URI parameter in AWSCredentialProvider constructors. Contributed by Mingliang Liu

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

@@ -186,7 +186,7 @@ public void initialize(URI name, Configuration originalConf)
           S3_CLIENT_FACTORY_IMPL, DEFAULT_S3_CLIENT_FACTORY_IMPL,
           S3ClientFactory.class);
       s3 = ReflectionUtils.newInstance(s3ClientFactoryClass, conf)
-          .createS3Client(name, uri);
+          .createS3Client(name);
 
       maxKeys = intOption(conf, MAX_PAGING_KEYS, DEFAULT_MAX_PAGING_KEYS, 1);
       listing = new Listing(this);

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java

@@ -318,15 +318,12 @@ public static long dateToLong(final Date date) {
    * Create the AWS credentials from the providers and the URI.
    * @param binding Binding URI, may contain user:pass login details
    * @param conf filesystem configuration
-   * @param fsURI fS URI —after any login details have been stripped.
    * @return a credentials provider list
    * @throws IOException Problems loading the providers (including reading
    * secrets from credential files).
    */
   public static AWSCredentialProviderList createAWSCredentialProviderSet(
-      URI binding,
+      URI binding, Configuration conf) throws IOException {
-      Configuration conf,
-      URI fsURI) throws IOException {
     AWSCredentialProviderList credentials = new AWSCredentialProviderList();
 
     Class<?>[] awsClasses;
@@ -351,11 +348,12 @@ public static AWSCredentialProviderList createAWSCredentialProviderSet(
               SharedInstanceProfileCredentialsProvider.class.getName());
           aClass = SharedInstanceProfileCredentialsProvider.class;
         }
-        credentials.add(createAWSCredentialProvider(conf,
+        credentials.add(createAWSCredentialProvider(conf, aClass));
-            aClass,
-            fsURI));
       }
     }
+    // make sure the logging message strips out any auth details
+    LOG.debug("For URI {}, using credentials {}",
+        S3xLoginHelper.toString(binding), credentials);
     return credentials;
   }
 
@@ -365,8 +363,8 @@ public static AWSCredentialProviderList createAWSCredentialProviderSet(
    * attempted in order:
    *
    * <ol>
-   * <li>a public constructor accepting java.net.URI and
+   * <li>a public constructor accepting
-   *     org.apache.hadoop.conf.Configuration</li>
+   *    org.apache.hadoop.conf.Configuration</li>
    * <li>a public static method named getInstance that accepts no
    *    arguments and returns an instance of
    *    com.amazonaws.auth.AWSCredentialsProvider, or</li>
@@ -375,14 +373,11 @@ public static AWSCredentialProviderList createAWSCredentialProviderSet(
    *
    * @param conf configuration
    * @param credClass credential class
-   * @param uri URI of the FS
    * @return the instantiated class
    * @throws IOException on any instantiation failure.
    */
   static AWSCredentialsProvider createAWSCredentialProvider(
-      Configuration conf,
+      Configuration conf, Class<?> credClass) throws IOException {
-      Class<?> credClass,
-      URI uri) throws IOException {
     AWSCredentialsProvider credentials = null;
     String className = credClass.getName();
     if (!AWSCredentialsProvider.class.isAssignableFrom(credClass)) {
@@ -394,11 +389,10 @@ static AWSCredentialsProvider createAWSCredentialProvider(
     LOG.debug("Credential provider class is {}", className);
 
     try {
-      // new X(uri, conf)
+      // new X(conf)
-      Constructor cons = getConstructor(credClass, URI.class,
+      Constructor cons = getConstructor(credClass, Configuration.class);
-          Configuration.class);
       if (cons != null) {
-        credentials = (AWSCredentialsProvider)cons.newInstance(uri, conf);
+        credentials = (AWSCredentialsProvider)cons.newInstance(conf);
         return credentials;
       }
 
@@ -420,16 +414,12 @@ static AWSCredentialsProvider createAWSCredentialProvider(
       // no supported constructor or factory method found
       throw new IOException(String.format("%s " + CONSTRUCTOR_EXCEPTION
           + ".  A class specified in %s must provide a public constructor "
-          + "accepting URI and Configuration, or a public factory method named "
+          + "accepting Configuration, or a public factory method named "
           + "getInstance that accepts no arguments, or a public default "
           + "constructor.", className, AWS_CREDENTIALS_PROVIDER));
     } catch (ReflectiveOperationException | IllegalArgumentException e) {
       // supported constructor or factory method found, but the call failed
       throw new IOException(className + " " + INSTANTIATION_EXCEPTION +".", e);
-    } finally {
-      if (credentials != null) {
-        LOG.debug("Using {} for {}.", credentials, uri);
-      }
     }
   }
 

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ClientFactory.java

@@ -52,11 +52,10 @@ interface S3ClientFactory {
    * because both values may be useful in logging.
    *
    * @param name raw input S3A file system URI
-   * @param uri validated form of S3A file system URI
    * @return S3 client
    * @throws IOException IO problem
    */
-  AmazonS3 createS3Client(URI name, URI uri) throws IOException;
+  AmazonS3 createS3Client(URI name) throws IOException;
 
   /**
    * The default factory implementation, which calls the AWS SDK to configure
@@ -68,10 +67,10 @@ static class DefaultS3ClientFactory extends Configured
     private static final Logger LOG = S3AFileSystem.LOG;
 
     @Override
-    public AmazonS3 createS3Client(URI name, URI uri) throws IOException {
+    public AmazonS3 createS3Client(URI name) throws IOException {
       Configuration conf = getConf();
       AWSCredentialsProvider credentials =
-          createAWSCredentialProviderSet(name, conf, uri);
+          createAWSCredentialProviderSet(name, conf);
       ClientConfiguration awsConf = new ClientConfiguration();
       initConnectionSettings(conf, awsConf);
       initProxySupport(conf, awsConf);

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/SimpleAWSCredentialsProvider.java

@@ -28,7 +28,6 @@
 import org.apache.hadoop.security.ProviderUtils;
 
 import java.io.IOException;
-import java.net.URI;
 
 import static org.apache.hadoop.fs.s3a.Constants.ACCESS_KEY;
 import static org.apache.hadoop.fs.s3a.Constants.SECRET_KEY;
@@ -51,7 +50,7 @@ public class SimpleAWSCredentialsProvider implements AWSCredentialsProvider {
   private String secretKey;
   private IOException lookupIOE;
 
-  public SimpleAWSCredentialsProvider(URI uri, Configuration conf) {
+  public SimpleAWSCredentialsProvider(Configuration conf) {
     try {
       Configuration c = ProviderUtils.excludeIncompatibleCredentialProviders(
           conf, S3AFileSystem.class);

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/TemporaryAWSCredentialsProvider.java

@@ -24,7 +24,6 @@
 import org.apache.commons.lang.StringUtils;
 
 import java.io.IOException;
-import java.net.URI;
 
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
@@ -51,7 +50,7 @@ public class TemporaryAWSCredentialsProvider implements AWSCredentialsProvider {
   private String sessionToken;
   private IOException lookupIOE;
 
-  public TemporaryAWSCredentialsProvider(URI uri, Configuration conf) {
+  public TemporaryAWSCredentialsProvider(Configuration conf) {
     try {
       Configuration c = ProviderUtils.excludeIncompatibleCredentialProviders(
           conf, S3AFileSystem.class);

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AAWSCredentialsProvider.java

@@ -19,13 +19,14 @@
 package org.apache.hadoop.fs.s3a;
 
 import java.io.IOException;
-import java.net.URI;
 import java.nio.file.AccessDeniedException;
 
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.test.GenericTestUtils;
+
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.Timeout;
@@ -40,6 +41,7 @@
 
 import static org.apache.hadoop.fs.s3a.Constants.*;
 import static org.apache.hadoop.fs.s3a.S3ATestConstants.*;
+import static org.apache.hadoop.fs.s3a.S3AUtils.*;
 import static org.junit.Assert.*;
 
 /**
@@ -66,6 +68,42 @@ public void testBadConfiguration() throws IOException {
     }
   }
 
+  /**
+   * A bad CredentialsProvider which has no suitable constructor.
+   *
+   * This class does not provide a public constructor accepting Configuration,
+   * or a public factory method named getInstance that accepts no arguments,
+   * or a public default constructor.
+   */
+  static class BadCredentialsProviderConstructor
+      implements AWSCredentialsProvider {
+
+    @SuppressWarnings("unused")
+    public BadCredentialsProviderConstructor(String fsUri, Configuration conf) {
+    }
+
+    @Override
+    public AWSCredentials getCredentials() {
+      return new BasicAWSCredentials("dummy_key", "dummy_secret");
+    }
+
+    @Override
+    public void refresh() {
+    }
+  }
+
+  @Test
+  public void testBadCredentialsConstructor() throws Exception {
+    Configuration conf = new Configuration();
+    conf.set(AWS_CREDENTIALS_PROVIDER,
+        BadCredentialsProviderConstructor.class.getName());
+    try {
+      createFailingFS(conf);
+    } catch (IOException e) {
+      GenericTestUtils.assertExceptionContains(CONSTRUCTOR_EXCEPTION, e);
+    }
+  }
+
   /**
    * Create a filesystem, expect it to fail by raising an IOException.
    * Raises an assertion exception if in fact the FS does get instantiated.
@@ -81,7 +119,7 @@ private void createFailingFS(Configuration conf) throws IOException {
   static class BadCredentialsProvider implements AWSCredentialsProvider {
 
     @SuppressWarnings("unused")
-    public BadCredentialsProvider(URI name, Configuration conf) {
+    public BadCredentialsProvider(Configuration conf) {
     }
 
     @Override
@@ -108,7 +146,7 @@ public void testBadCredentials() throws Exception {
   static class GoodCredentialsProvider extends AWSCredentialsProviderChain {
 
     @SuppressWarnings("unused")
-    public GoodCredentialsProvider(URI name, Configuration conf) {
+    public GoodCredentialsProvider(Configuration conf) {
       super(new BasicAWSCredentialsProvider(conf.get(ACCESS_KEY),
           conf.get(SECRET_KEY)),
           InstanceProfileCredentialsProvider.getInstance());

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3ATemporaryCredentials.java

@@ -126,7 +126,7 @@ public void testTemporaryCredentialValidation() throws Throwable {
     conf.set(SECRET_KEY, "secretkey");
     conf.set(SESSION_TOKEN, "");
     TemporaryAWSCredentialsProvider provider
-        = new TemporaryAWSCredentialsProvider(getFileSystem().getUri(), conf);
+        = new TemporaryAWSCredentialsProvider(conf);
     try {
       AWSCredentials credentials = provider.getCredentials();
       fail("Expected a CredentialInitializationException,"

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/MockS3ClientFactory.java

@@ -31,7 +31,7 @@
 public class MockS3ClientFactory implements S3ClientFactory {
 
   @Override
-  public AmazonS3 createS3Client(URI name, URI uri) {
+  public AmazonS3 createS3Client(URI name) {
     String bucket = name.getHost();
     AmazonS3 s3 = mock(AmazonS3.class);
     when(s3.doesBucketExist(bucket)).thenReturn(true);

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java

@@ -93,7 +93,7 @@ public void testInstantiationChain() throws Throwable {
 
     URI uri = testFile.toUri();
     AWSCredentialProviderList list = S3AUtils.createAWSCredentialProviderSet(
-        uri, conf, uri);
+        uri, conf);
     List<Class<? extends AWSCredentialsProvider>> expectedClasses =
         Arrays.asList(
             TemporaryAWSCredentialsProvider.class,
@@ -107,9 +107,9 @@ public void testDefaultChain() throws Exception {
     URI uri1 = new URI("s3a://bucket1"), uri2 = new URI("s3a://bucket2");
     Configuration conf = new Configuration();
     AWSCredentialProviderList list1 = S3AUtils.createAWSCredentialProviderSet(
-        uri1, conf, uri1);
+        uri1, conf);
     AWSCredentialProviderList list2 = S3AUtils.createAWSCredentialProviderSet(
-        uri2, conf, uri2);
+        uri2, conf);
     List<Class<? extends AWSCredentialsProvider>> expectedClasses =
         Arrays.asList(
             BasicAWSCredentialsProvider.class,
@@ -132,9 +132,9 @@ public void testConfiguredChain() throws Exception {
             AnonymousAWSCredentialsProvider.class);
     conf.set(AWS_CREDENTIALS_PROVIDER, buildClassListString(expectedClasses));
     AWSCredentialProviderList list1 = S3AUtils.createAWSCredentialProviderSet(
-        uri1, conf, uri1);
+        uri1, conf);
     AWSCredentialProviderList list2 = S3AUtils.createAWSCredentialProviderSet(
-        uri2, conf, uri2);
+        uri2, conf);
     assertCredentialProviders(expectedClasses, list1);
     assertCredentialProviders(expectedClasses, list2);
     assertSameInstanceProfileCredentialsProvider(list1.getProviders().get(1),
@@ -150,9 +150,9 @@ public void testConfiguredChainUsesSharedInstanceProfile() throws Exception {
             InstanceProfileCredentialsProvider.class);
     conf.set(AWS_CREDENTIALS_PROVIDER, buildClassListString(expectedClasses));
     AWSCredentialProviderList list1 = S3AUtils.createAWSCredentialProviderSet(
-        uri1, conf, uri1);
+        uri1, conf);
     AWSCredentialProviderList list2 = S3AUtils.createAWSCredentialProviderSet(
-        uri2, conf, uri2);
+        uri2, conf);
     assertCredentialProviders(expectedClasses, list1);
     assertCredentialProviders(expectedClasses, list2);
     assertSameInstanceProfileCredentialsProvider(list1.getProviders().get(0),
@@ -226,7 +226,7 @@ private void expectProviderInstantiationFailure(String option,
         conf.getTrimmed(KEY_CSVTEST_FILE, DEFAULT_CSVTEST_FILE));
     expectException(IOException.class, expectedErrorText);
     URI uri = testFile.toUri();
-    S3AUtils.createAWSCredentialProviderSet(uri, conf, uri);
+    S3AUtils.createAWSCredentialProviderSet(uri, conf);
   }
 
   /**