From 032c335082f24aef12ee3e002ae1cfd9c5f40507 Mon Sep 17 00:00:00 2001 From: Brandon Vincent Date: Wed, 22 Oct 2025 10:51:02 -0700 Subject: [PATCH 1/2] HADOOP-19733. S3A: load credentials providers using configuration's classloader Follow-up to HADOOP-17372 and HADOOP-18993. The issue described in HADOOP-18993 still existed, because the code path to load credentials providers goes through `S3AUtils#getInstanceFromReflection` and always uses the classloader that loaded the `S3AUtils` class. With this change, credentials providers are loaded using the Configuration's classloader so they respect the user's `fs.s3a.classloader.isolation` setting --- .../org/apache/hadoop/fs/s3a/S3AUtils.java | 6 +- ...ITestS3AFileSystemIsolatedClassloader.java | 105 ++++++++++++++++-- 2 files changed, 100 insertions(+), 11 deletions(-) diff --git a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java index 63ad42dab7adb..27d7363fbc3ad 100644 --- a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java +++ b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java @@ -621,6 +621,9 @@ public static long dateToLong(final Date date) { *
  • a public default constructor.
    • * * + * Uses the Configuration's class loader, to respect the configured value of + * {@link Constants#AWS_S3_CLASSLOADER_ISOLATION}. + * * @param className name of class for which instance is to be created * @param conf configuration * @param uri URI of the FS @@ -639,7 +642,8 @@ public static InstanceT getInstanceFromReflection(String className, String methodName, String configKey) throws IOException { try { - Class instanceClass = S3AUtils.class.getClassLoader().loadClass(className); + LOG.debug("Loading class {} with Configuration classloader {}", className, conf.getClassLoader()); + Class instanceClass = conf.getClassLoader().loadClass(className); if (Modifier.isAbstract(instanceClass.getModifiers())) { throw isAbstract(uri, className, configKey); } diff --git a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java index 947bd8ab56085..8c251e9163ea0 100644 --- a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java +++ b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java @@ -21,6 +21,8 @@ import java.io.IOException; import java.util.HashMap; import java.util.Map; +import java.util.List; +import java.util.ArrayList; import java.util.function.Consumer; import org.assertj.core.api.Assertions; @@ -28,6 +30,17 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.s3a.impl.InstantiationIOException; + +import software.amazon.awssdk.auth.credentials.AwsCredentials; +import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; +import static org.apache.hadoop.fs.s3a.test.PublicDatasetTestUtils.getExternalData; +import static org.apache.hadoop.fs.s3a.auth.CredentialProviderListFactory.createAWSCredentialProviderList; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.when; /** * Checks that classloader isolation for loading extension classes is applied @@ -37,10 +50,31 @@ */ public class ITestS3AFileSystemIsolatedClassloader extends AbstractS3ATestBase { - private static class CustomClassLoader extends ClassLoader { + private static String customClassName = "custom.class.name"; + + private static class CustomCredentialsProvider implements AwsCredentialsProvider { + + public CustomCredentialsProvider() { + } + + @Override + public AwsCredentials resolveCredentials() { + return null; + } + } - private final ClassLoader customClassLoader = new CustomClassLoader(); + private static class CustomClassLoader extends ClassLoader { + + public List classesLoaded = new ArrayList<>(); + + @Override + public Class loadClass(String className) throws ClassNotFoundException { + this.classesLoaded.add(className); + return super.loadClass(className); + } + + } private S3AFileSystem createNewTestFs(Configuration conf) throws IOException { S3AFileSystem fs = new S3AFileSystem(); @@ -58,10 +92,19 @@ private S3AFileSystem createNewTestFs(Configuration conf) throws IOException { * @param asserts The assertions to be performed on the new filesystem. * @throws IOException If an I/O error occurs. */ - private void assertInNewFilesystem(Map confToSet, Consumer asserts) + private void assertInNewFilesystem(Map confToSet, Consumer asserts) throws IOException { ClassLoader previousClassloader = Thread.currentThread().getContextClassLoader(); try { + ClassLoader customClassLoader = spy(new CustomClassLoader()); + try { + doReturn(CustomCredentialsProvider.class) + .when(customClassLoader) + .loadClass(customClassName); + } catch (ClassNotFoundException ex) { + throw new IOException(ex); + } + Thread.currentThread().setContextClassLoader(customClassLoader); Configuration conf = new Configuration(); Assertions.assertThat(conf.getClassLoader()).isEqualTo(customClassLoader); @@ -100,6 +143,21 @@ public void defaultIsolatedClassloader() throws IOException { .isEqualTo(fs.getClass().getClassLoader()) .describedAs("the classloader that loaded the fs"); }); + + Throwable thrown = Assertions.catchThrowable(() -> { + assertInNewFilesystem( + mapOf(Constants.AWS_CREDENTIALS_PROVIDER, customClassName), + (fs) -> {}); + }); + + Assertions.assertThat(thrown) + .describedAs("thrown") + .isInstanceOf(InstantiationIOException.class); + + Assertions.assertThat(thrown.getCause()) + .describedAs("cause") + .isInstanceOf(ClassNotFoundException.class) + .hasMessageContaining(customClassName); } @Test @@ -115,20 +173,47 @@ public void isolatedClassloader() throws IOException { .isEqualTo(fs.getClass().getClassLoader()) .describedAs("the classloader that loaded the fs"); }); + + Throwable thrown = Assertions.catchThrowable(() -> { + assertInNewFilesystem( + Map.of(Constants.AWS_S3_CLASSLOADER_ISOLATION, "true", + Constants.AWS_CREDENTIALS_PROVIDER, customClassName), + (fs) -> {}); + }); + + Assertions.assertThat(thrown) + .describedAs("thrown") + .isInstanceOf(InstantiationIOException.class); + + Assertions.assertThat(thrown.getCause()) + .describedAs("cause") + .isInstanceOf(ClassNotFoundException.class) + .hasMessageContaining(customClassName); } @Test public void notIsolatedClassloader() throws IOException { - assertInNewFilesystem(mapOf(Constants.AWS_S3_CLASSLOADER_ISOLATION, "false"), (fs) -> { + assertInNewFilesystem(Map.of(Constants.AWS_S3_CLASSLOADER_ISOLATION, "false", + Constants.AWS_CREDENTIALS_PROVIDER, customClassName), (fs) -> { Assertions.assertThat(fs.getConf().getClassLoader()) - .describedAs("The classloader used to load s3a fs extensions") - .isEqualTo(Thread.currentThread().getContextClassLoader()) - .describedAs("the current context classloader"); + .describedAs("The classloader used to load s3a fs extensions") + .isEqualTo(Thread.currentThread().getContextClassLoader()) + .describedAs("the current context classloader"); Assertions.assertThat(fs.getConf().getClassLoader()) - .describedAs("The classloader used to load s3a fs extensions") - .isNotEqualTo(fs.getClass().getClassLoader()) - .describedAs("the classloader that loaded the fs"); + .describedAs("The classloader used to load s3a fs extensions") + .isNotEqualTo(fs.getClass().getClassLoader()) + .describedAs("the classloader that loaded the fs"); + + List providers = + fs.getS3AInternals().shareCredentials("test").getProviders(); + Assertions.assertThat(providers) + .describedAs("providers") + .hasSize(1); + Assertions.assertThat(providers.get(0)) + .describedAs("provider") + .isInstanceOf(CustomCredentialsProvider.class); }); } + } From 249aef5213fa039d252e7f7ae03c060b6c87d94f Mon Sep 17 00:00:00 2001 From: Brandon Vincent Date: Wed, 22 Oct 2025 21:41:16 -0700 Subject: [PATCH 2/2] Fix code style in tests --- ...ITestS3AFileSystemIsolatedClassloader.java | 112 ++++++++---------- 1 file changed, 47 insertions(+), 65 deletions(-) diff --git a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java index 8c251e9163ea0..83b58b18d2482 100644 --- a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java +++ b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java @@ -20,9 +20,8 @@ import java.io.IOException; import java.util.HashMap; -import java.util.Map; import java.util.List; -import java.util.ArrayList; +import java.util.Map; import java.util.function.Consumer; import org.assertj.core.api.Assertions; @@ -30,16 +29,13 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; -import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.s3a.impl.InstantiationIOException; import software.amazon.awssdk.auth.credentials.AwsCredentials; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; -import static org.apache.hadoop.fs.s3a.test.PublicDatasetTestUtils.getExternalData; -import static org.apache.hadoop.fs.s3a.auth.CredentialProviderListFactory.createAWSCredentialProviderList; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; + import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; /** @@ -65,15 +61,17 @@ public AwsCredentials resolveCredentials() { } private static class CustomClassLoader extends ClassLoader { + } - public List classesLoaded = new ArrayList<>(); - - @Override - public Class loadClass(String className) throws ClassNotFoundException { - this.classesLoaded.add(className); - return super.loadClass(className); + private final ClassLoader customClassLoader = spy(new CustomClassLoader()); + { + try { + doReturn(CustomCredentialsProvider.class) + .when(customClassLoader) + .loadClass(customClassName); + } catch (ClassNotFoundException ex) { + throw new RuntimeException(ex); } - } private S3AFileSystem createNewTestFs(Configuration conf) throws IOException { @@ -92,19 +90,10 @@ private S3AFileSystem createNewTestFs(Configuration conf) throws IOException { * @param asserts The assertions to be performed on the new filesystem. * @throws IOException If an I/O error occurs. */ - private void assertInNewFilesystem(Map confToSet, Consumer asserts) + private void assertInNewFilesystem(Map confToSet, Consumer asserts) throws IOException { ClassLoader previousClassloader = Thread.currentThread().getContextClassLoader(); try { - ClassLoader customClassLoader = spy(new CustomClassLoader()); - try { - doReturn(CustomCredentialsProvider.class) - .when(customClassLoader) - .loadClass(customClassName); - } catch (ClassNotFoundException ex) { - throw new IOException(ex); - } - Thread.currentThread().setContextClassLoader(customClassLoader); Configuration conf = new Configuration(); Assertions.assertThat(conf.getClassLoader()).isEqualTo(customClassLoader); @@ -120,19 +109,9 @@ private void assertInNewFilesystem(Map confToSet, Consumer mapOf() { - return new HashMap<>(); - } - - private Map mapOf(String key, String value) { - HashMap m = new HashMap<>(); - m.put(key, value); - return m; - } - @Test public void defaultIsolatedClassloader() throws IOException { - assertInNewFilesystem(mapOf(), (fs) -> { + assertInNewFilesystem(Map.of(), (fs) -> { Assertions.assertThat(fs.getConf().getClassLoader()) .describedAs("The classloader used to load s3a fs extensions") .isNotEqualTo(Thread.currentThread().getContextClassLoader()) @@ -146,23 +125,23 @@ public void defaultIsolatedClassloader() throws IOException { Throwable thrown = Assertions.catchThrowable(() -> { assertInNewFilesystem( - mapOf(Constants.AWS_CREDENTIALS_PROVIDER, customClassName), - (fs) -> {}); + Map.of(Constants.AWS_CREDENTIALS_PROVIDER, customClassName), + (fs) -> {}); }); Assertions.assertThat(thrown) - .describedAs("thrown") - .isInstanceOf(InstantiationIOException.class); + .describedAs("thrown") + .isInstanceOf(InstantiationIOException.class); Assertions.assertThat(thrown.getCause()) - .describedAs("cause") - .isInstanceOf(ClassNotFoundException.class) - .hasMessageContaining(customClassName); + .describedAs("cause") + .isInstanceOf(ClassNotFoundException.class) + .hasMessageContaining(customClassName); } @Test public void isolatedClassloader() throws IOException { - assertInNewFilesystem(mapOf(Constants.AWS_S3_CLASSLOADER_ISOLATION, "true"), (fs) -> { + assertInNewFilesystem(Map.of(Constants.AWS_S3_CLASSLOADER_ISOLATION, "true"), (fs) -> { Assertions.assertThat(fs.getConf().getClassLoader()) .describedAs("The classloader used to load s3a fs extensions") .isNotEqualTo(Thread.currentThread().getContextClassLoader()) @@ -176,44 +155,47 @@ public void isolatedClassloader() throws IOException { Throwable thrown = Assertions.catchThrowable(() -> { assertInNewFilesystem( - Map.of(Constants.AWS_S3_CLASSLOADER_ISOLATION, "true", - Constants.AWS_CREDENTIALS_PROVIDER, customClassName), - (fs) -> {}); + Map.of(Constants.AWS_S3_CLASSLOADER_ISOLATION, "true", + Constants.AWS_CREDENTIALS_PROVIDER, customClassName), + (fs) -> {}); }); Assertions.assertThat(thrown) - .describedAs("thrown") - .isInstanceOf(InstantiationIOException.class); + .describedAs("thrown") + .isInstanceOf(InstantiationIOException.class); Assertions.assertThat(thrown.getCause()) - .describedAs("cause") - .isInstanceOf(ClassNotFoundException.class) - .hasMessageContaining(customClassName); + .describedAs("cause") + .isInstanceOf(ClassNotFoundException.class) + .hasMessageContaining(customClassName); } @Test public void notIsolatedClassloader() throws IOException { - assertInNewFilesystem(Map.of(Constants.AWS_S3_CLASSLOADER_ISOLATION, "false", - Constants.AWS_CREDENTIALS_PROVIDER, customClassName), (fs) -> { + Map confToSet = Map.of( + Constants.AWS_S3_CLASSLOADER_ISOLATION, "false", + Constants.AWS_CREDENTIALS_PROVIDER, customClassName); + + assertInNewFilesystem(confToSet, (fs) -> { Assertions.assertThat(fs.getConf().getClassLoader()) - .describedAs("The classloader used to load s3a fs extensions") - .isEqualTo(Thread.currentThread().getContextClassLoader()) - .describedAs("the current context classloader"); + .describedAs("The classloader used to load s3a fs extensions") + .isEqualTo(Thread.currentThread().getContextClassLoader()) + .describedAs("the current context classloader"); Assertions.assertThat(fs.getConf().getClassLoader()) - .describedAs("The classloader used to load s3a fs extensions") - .isNotEqualTo(fs.getClass().getClassLoader()) - .describedAs("the classloader that loaded the fs"); + .describedAs("The classloader used to load s3a fs extensions") + .isNotEqualTo(fs.getClass().getClassLoader()) + .describedAs("the classloader that loaded the fs"); + S3AFileSystem s3a = (S3AFileSystem) fs; List providers = - fs.getS3AInternals().shareCredentials("test").getProviders(); + s3a.getS3AInternals().shareCredentials("test").getProviders(); Assertions.assertThat(providers) - .describedAs("providers") - .hasSize(1); + .describedAs("providers") + .hasSize(1); Assertions.assertThat(providers.get(0)) - .describedAs("provider") - .isInstanceOf(CustomCredentialsProvider.class); + .describedAs("provider") + .isInstanceOf(CustomCredentialsProvider.class); }); } - }