You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/10/21 18:15:05 UTC
[GitHub] [hadoop] steveloughran commented on a diff in pull request #5024: HADOOP-18233. Possible race condition with TemporaryAWSCredentialsPro…
steveloughran commented on code in PR #5024:
URL: https://github.com/apache/hadoop/pull/5024#discussion_r1002042697
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -22,14 +22,20 @@
import java.io.InterruptedIOException;
import java.net.URI;
import java.nio.file.AccessDeniedException;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.auth.AWSCredentialsProvider;
import com.amazonaws.auth.EnvironmentVariableCredentialsProvider;
import com.amazonaws.auth.InstanceProfileCredentialsProvider;
+import org.apache.hadoop.fs.s3a.auth.AbstractSessionCredentialsProvider;
Review Comment:
nit, move down to the main org.apache.hadoop block; the one here is tainted by the search/replace from guava sets
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -480,4 +488,121 @@ public void refresh() {
}
}
+ private static AWSCredentials expectedCredentials = new AWSCredentials() {
+ @Override
+ public String getAWSAccessKeyId() {
+ return "expectedAccessKey";
+ }
+
+ @Override
+ public String getAWSSecretKey() {
+ return "expectedSecret";
+ }
+ };
+
+ /**
+ * Credential provider that takes a long time.
+ */
+ private static class SlowProvider extends AbstractSessionCredentialsProvider {
+
+ public SlowProvider(@Nullable URI uri, Configuration conf) {
+ super(uri, conf);
+ }
+
+ @Override
+ protected AWSCredentials createCredentials(Configuration config) throws IOException {
+ // yield to other callers to induce race condition
+ Thread.yield();
+ return expectedCredentials;
+ }
+ }
+
+ @Test
+ public void testConcurrentAuthentication() throws Throwable {
+ Configuration conf = createProviderConfiguration(SlowProvider.class.getName());
+ Path testFile = getCSVTestPath(conf);
+
+ int threads = 10;
+
+ AWSCredentialProviderList list = createAWSCredentialProviderSet(testFile.toUri(), conf);
+
+ SlowProvider provider = (SlowProvider) list.getProviders().get(0);
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+
+ List<Future> results = new ArrayList<>(threads);
+
+ assert(!provider.isInitialized());
Review Comment:
use junit assertFalse, *with an error message*, or AssertJ, here and in the other assertions
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/AbstractSessionCredentialsProvider.java:
##########
@@ -70,21 +73,27 @@ public AbstractSessionCredentialsProvider(
/**
* Initialize the credentials by calling
* {@link #createCredentials(Configuration)} with the current config.
- * @throws IOException on any failure.
+ * Sets {@link AbstractSessionCredentialsProvider#initializationException} on failure
*/
@Retries.OnceTranslated
- protected void init() throws IOException {
+ protected void init() {
Review Comment:
we can't change the signature as subclasses may use it
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -480,4 +488,121 @@ public void refresh() {
}
}
+ private static AWSCredentials expectedCredentials = new AWSCredentials() {
+ @Override
+ public String getAWSAccessKeyId() {
+ return "expectedAccessKey";
+ }
+
+ @Override
+ public String getAWSSecretKey() {
+ return "expectedSecret";
+ }
+ };
+
+ /**
+ * Credential provider that takes a long time.
+ */
+ private static class SlowProvider extends AbstractSessionCredentialsProvider {
+
+ public SlowProvider(@Nullable URI uri, Configuration conf) {
+ super(uri, conf);
+ }
+
+ @Override
+ protected AWSCredentials createCredentials(Configuration config) throws IOException {
+ // yield to other callers to induce race condition
+ Thread.yield();
+ return expectedCredentials;
+ }
+ }
+
+ @Test
+ public void testConcurrentAuthentication() throws Throwable {
+ Configuration conf = createProviderConfiguration(SlowProvider.class.getName());
+ Path testFile = getCSVTestPath(conf);
+
+ int threads = 10;
+
+ AWSCredentialProviderList list = createAWSCredentialProviderSet(testFile.toUri(), conf);
+
+ SlowProvider provider = (SlowProvider) list.getProviders().get(0);
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+
+ List<Future> results = new ArrayList<>(threads);
+
+ assert(!provider.isInitialized());
+ assert(!provider.hasCredentials());
+ assert(provider.getInitializationException() == null);
+
+ for (int i = 0; i < threads; i++) {
+ results.add(pool.submit(() -> list.getCredentials()));
+ }
+
+ for (Future result : results) {
+ AWSCredentials credentials = (AWSCredentials) result.get();
+ assert (credentials.getAWSAccessKeyId() == "expectedAccessKey");
Review Comment:
use
```
assertEquals("expectedAccessKey", credentials.getAWSAccessKeyId())
```
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -480,4 +488,121 @@ public void refresh() {
}
}
+ private static AWSCredentials expectedCredentials = new AWSCredentials() {
+ @Override
+ public String getAWSAccessKeyId() {
+ return "expectedAccessKey";
+ }
+
+ @Override
+ public String getAWSSecretKey() {
+ return "expectedSecret";
+ }
+ };
+
+ /**
+ * Credential provider that takes a long time.
+ */
+ private static class SlowProvider extends AbstractSessionCredentialsProvider {
+
+ public SlowProvider(@Nullable URI uri, Configuration conf) {
+ super(uri, conf);
+ }
+
+ @Override
+ protected AWSCredentials createCredentials(Configuration config) throws IOException {
+ // yield to other callers to induce race condition
+ Thread.yield();
+ return expectedCredentials;
+ }
+ }
+
+ @Test
+ public void testConcurrentAuthentication() throws Throwable {
+ Configuration conf = createProviderConfiguration(SlowProvider.class.getName());
+ Path testFile = getCSVTestPath(conf);
+
+ int threads = 10;
+
+ AWSCredentialProviderList list = createAWSCredentialProviderSet(testFile.toUri(), conf);
+
+ SlowProvider provider = (SlowProvider) list.getProviders().get(0);
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+
+ List<Future> results = new ArrayList<>(threads);
+
+ assert(!provider.isInitialized());
+ assert(!provider.hasCredentials());
+ assert(provider.getInitializationException() == null);
Review Comment:
if the ex != null, throw it
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/AbstractSessionCredentialsProvider.java:
##########
@@ -70,21 +73,27 @@ public AbstractSessionCredentialsProvider(
/**
* Initialize the credentials by calling
* {@link #createCredentials(Configuration)} with the current config.
- * @throws IOException on any failure.
+ * Sets {@link AbstractSessionCredentialsProvider#initializationException} on failure
*/
@Retries.OnceTranslated
- protected void init() throws IOException {
+ protected void init() {
// stop re-entrant attempts
if (initialized.getAndSet(true)) {
return;
}
- try {
- awsCredentials = Invoker.once("create credentials", "",
- () -> createCredentials(getConf()));
- } catch (IOException e) {
- initializationException = e;
- throw e;
- }
+ Runnable initTask = () -> {
+ try {
+ AWSCredentials credentials = Invoker.once(
+ "create credentials", "", () -> createCredentials(getConf()));
+ awsCredentials.complete(credentials);
+ } catch (IOException e) {
+ awsCredentials.completeExceptionally(e);
+ }
+ };
+
+ awsCredentials.handle((result, t) -> initializationException = (IOException) t);
+
+ ForkJoinPool.commonPool().submit(initTask);
Review Comment:
this is overkill for what, in the implementations in our own source tree, are just local lookups
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -480,4 +488,121 @@ public void refresh() {
}
}
+ private static AWSCredentials expectedCredentials = new AWSCredentials() {
+ @Override
+ public String getAWSAccessKeyId() {
+ return "expectedAccessKey";
+ }
+
+ @Override
+ public String getAWSSecretKey() {
+ return "expectedSecret";
+ }
+ };
+
+ /**
+ * Credential provider that takes a long time.
+ */
+ private static class SlowProvider extends AbstractSessionCredentialsProvider {
+
+ public SlowProvider(@Nullable URI uri, Configuration conf) {
+ super(uri, conf);
+ }
+
+ @Override
+ protected AWSCredentials createCredentials(Configuration config) throws IOException {
+ // yield to other callers to induce race condition
+ Thread.yield();
+ return expectedCredentials;
+ }
+ }
+
+ @Test
+ public void testConcurrentAuthentication() throws Throwable {
+ Configuration conf = createProviderConfiguration(SlowProvider.class.getName());
+ Path testFile = getCSVTestPath(conf);
+
+ int threads = 10;
+
+ AWSCredentialProviderList list = createAWSCredentialProviderSet(testFile.toUri(), conf);
+
+ SlowProvider provider = (SlowProvider) list.getProviders().get(0);
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+
+ List<Future> results = new ArrayList<>(threads);
+
+ assert(!provider.isInitialized());
+ assert(!provider.hasCredentials());
+ assert(provider.getInitializationException() == null);
+
+ for (int i = 0; i < threads; i++) {
+ results.add(pool.submit(() -> list.getCredentials()));
+ }
+
+ for (Future result : results) {
+ AWSCredentials credentials = (AWSCredentials) result.get();
+ assert (credentials.getAWSAccessKeyId() == "expectedAccessKey");
+ assert (credentials.getAWSSecretKey() == "expectedSecret");
+ }
+
+ pool.awaitTermination(10, TimeUnit.SECONDS);
+ pool.shutdown();
+
+ assert(provider.isInitialized());
+ assert(provider.hasCredentials());
+ assert(provider.getInitializationException() == null);
+ }
+
+ /**
+ * Credential provider with error.
+ */
+ private static class ErrorProvider extends AbstractSessionCredentialsProvider {
Review Comment:
don't worry about the deprecations; that is warning about what breaks when we move to the v2 SDK
--
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