[GitHub] [hadoop] lmccay commented on a change in pull request #3393: HADOOP-17894. CredentialProviderFactory.getProviders() recursion loading JCEKS file from S3A

[GitBox <gi...@apache.org>]

lmccay commented on a change in pull request #3393:
URL: https://github.com/apache/hadoop/pull/3393#discussion_r703036389



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/CredentialProviderFactory.java
##########
@@ -59,23 +61,40 @@ public abstract CredentialProvider createProvider(URI providerName,
     }
   }
 
+  /**
+   * Fail fast on any recursive load of credential providers, which can
+   * happen if the FS itself triggers the load.
+   * A simple boolean could be used here, as the synchronized block ensures
+   * that only one thread can be active at a time. An atomic is used
+   * for rigorousness.
+   */
+  private static final AtomicBoolean serviceLoaderLocked = new AtomicBoolean(false);
+
   public static List<CredentialProvider> getProviders(Configuration conf
                                                ) throws IOException {
-    List<CredentialProvider> result = new ArrayList<CredentialProvider>();
+    List<CredentialProvider> result = new ArrayList<>();
     for(String path: conf.getStringCollection(CREDENTIAL_PROVIDER_PATH)) {
       try {
         URI uri = new URI(path);
         boolean found = false;
         // Iterate serviceLoader in a synchronized block since
         // serviceLoader iterator is not thread-safe.
         synchronized (serviceLoader) {
-          for (CredentialProviderFactory factory : serviceLoader) {
-            CredentialProvider kp = factory.createProvider(uri, conf);
-            if (kp != null) {
-              result.add(kp);
-              found = true;
-              break;
+          try {
+            if (serviceLoaderLocked.getAndSet(true)) {
+              throw new PathIOException(path,
+                  "Recursive load of credential provider!");

Review comment:
       This could be a bit less implementation specific and provide more guidance - especially if this ends up being exposed to the client/CLI. I'll also nit-pick the use of bang there.




-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org