AWS: update test cases to verify credentials for the prefixed S3 client by plusplusjiajia · Pull Request #13118 · apache/iceberg

plusplusjiajia · 2025-05-21T10:08:45Z

Pass the storage credential when initializing S3FileIOAwsClientFactories.

lliangyu-lin

LGTM!

nastra · 2025-05-22T07:15:17Z

+    assertThat(actualConfiguration).isNotNull();
+    try {
+      actualConfiguration.credentialsProvider().resolveIdentity();
+    } catch (SdkClientException e) {


please use assertThatThrownBy(..).isInstanceOf(..).hasMessage(..) instead of a try-catch block

please use assertThatThrownBy(..).isInstanceOf(..).hasMessage(..) instead of a try-catch block

thanks for your advice, updated.

nastra · 2025-05-22T07:20:13Z

    // Do not override s3 client if it was provided
    if (s3 == null) {
-      Object clientFactory = S3FileIOAwsClientFactories.initialize(props);
+      Object clientFactory = S3FileIOAwsClientFactories.initialize(propertiesWithCredentials);


FYI this is going to be fixed by #12799 but it's still good to get the tests in

FYI this is going to be fixed by #12799 but it's still good to get the tests in
Thanks for the reminder, this feature looks great!

nastra · 2025-05-22T17:36:09Z

#12799 has been merged before I had the chance to merge your PR. Can you rebase this please so that we can get the tests in

plusplusjiajia · 2025-05-23T00:39:16Z

#12799 has been merged before I had the chance to merge your PR. Can you rebase this please so that we can get the tests in

sure, I've reverted the change.

nastra

we pretty much have already existing tests where the storage credentials (and the credentials in the properties) are configured. So rather than introducing new test methods I would suggest to just add the important assertion to the existing tests. This would then look like the following diff and should cover all cases that are needed.

Also please make sure to update the PR title to reflect the scope/intention (since it's not passing storage credentials anymore)

diff --git a/aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3FileIO.java b/aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3FileIO.java
index 85453cba97..965e1c6068 100644
--- a/aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3FileIO.java
+++ b/aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3FileIO.java
@@ -41,6 +41,7 @@ import java.util.Random;
 import java.util.Spliterator;
 import java.util.Spliterators;
 import java.util.UUID;
+import java.util.concurrent.ExecutionException;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
@@ -89,8 +90,12 @@ import org.mockito.Mockito;
 import org.testcontainers.containers.MinIOContainer;
 import org.testcontainers.junit.jupiter.Container;
 import org.testcontainers.junit.jupiter.Testcontainers;
+import software.amazon.awssdk.awscore.AwsServiceClientConfiguration;
+import software.amazon.awssdk.core.exception.SdkClientException;
 import software.amazon.awssdk.core.sync.RequestBody;
 import software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient;
+import software.amazon.awssdk.identity.spi.AwsSessionCredentialsIdentity;
+import software.amazon.awssdk.identity.spi.IdentityProvider;
 import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.services.s3.S3AsyncClient;
 import software.amazon.awssdk.services.s3.S3Client;
@@ -730,10 +735,23 @@ public class TestS3FileIO {
     s3FileIOProperties
         .extracting(S3FileIOProperties::sessionToken)
         .isEqualTo("sessionTokenFromProperties");
+
+    // verify that the credentials identity gets the correct credentials for the prefixed S3 client
+    assertThat(fileIO.client("s3://my-bucket/table1").serviceClientConfiguration())
+        .extracting(AwsServiceClientConfiguration::credentialsProvider)
+        .extracting(IdentityProvider::resolveIdentity)
+        .satisfies(
+            x -> {
+              AwsSessionCredentialsIdentity identity = (AwsSessionCredentialsIdentity) x.get();
+              assertThat(identity).isNotNull();
+              assertThat(identity.accessKeyId()).isEqualTo("keyIdFromProperties");
+              assertThat(identity.secretAccessKey()).isEqualTo("accessKeyFromProperties");
+              assertThat(identity.sessionToken()).isEqualTo("sessionTokenFromProperties");
+            });
   }
 
   @Test
-  public void singleStorageCredentialConfigured() {
+  public void singleStorageCredentialConfigured() throws ExecutionException, InterruptedException {
     StorageCredential s3Credential =
         StorageCredential.create(
             "s3://custom-uri",
@@ -782,6 +800,32 @@ public class TestS3FileIO {
     s3FileIOProperties
         .extracting(S3FileIOProperties::sessionToken)
         .isEqualTo("sessionTokenFromCredential");
+
+    // verify that the credentials identity gets the correct credentials for the generic S3 client
+    assertThat(fileIO.client().serviceClientConfiguration())
+        .extracting(AwsServiceClientConfiguration::credentialsProvider)
+        .extracting(IdentityProvider::resolveIdentity)
+        .satisfies(
+            x -> {
+              AwsSessionCredentialsIdentity identity = (AwsSessionCredentialsIdentity) x.get();
+              assertThat(identity).isNotNull();
+              assertThat(identity.accessKeyId()).isEqualTo("keyIdFromProperties");
+              assertThat(identity.secretAccessKey()).isEqualTo("accessKeyFromProperties");
+              assertThat(identity.sessionToken()).isEqualTo("sessionTokenFromProperties");
+            });
+
+    // verify that the credentials identity gets the correct credentials for the prefixed S3 client
+    assertThat(fileIO.client("s3://custom-uri/table2").serviceClientConfiguration())
+        .extracting(AwsServiceClientConfiguration::credentialsProvider)
+        .extracting(IdentityProvider::resolveIdentity)
+        .satisfies(
+            x -> {
+              AwsSessionCredentialsIdentity identity = (AwsSessionCredentialsIdentity) x.get();
+              assertThat(identity).isNotNull();
+              assertThat(identity.accessKeyId()).isEqualTo("keyIdFromCredential");
+              assertThat(identity.secretAccessKey()).isEqualTo("accessKeyFromCredential");
+              assertThat(identity.sessionToken()).isEqualTo("sessionTokenFromCredential");
+            });
   }
 
   @Test
@@ -851,6 +895,19 @@ public class TestS3FileIO {
         .extracting(S3FileIOProperties::sessionToken)
         .isEqualTo("sessionTokenFromCredential1");
 
+    // verify that the credentials identity gets the correct credentials for the prefixed S3 client
+    assertThat(fileIO.client("s3://custom-uri/1").serviceClientConfiguration())
+        .extracting(AwsServiceClientConfiguration::credentialsProvider)
+        .extracting(IdentityProvider::resolveIdentity)
+        .satisfies(
+            x -> {
+              AwsSessionCredentialsIdentity identity = (AwsSessionCredentialsIdentity) x.get();
+              assertThat(identity).isNotNull();
+              assertThat(identity.accessKeyId()).isEqualTo("keyIdFromCredential1");
+              assertThat(identity.secretAccessKey()).isEqualTo("accessKeyFromCredential1");
+              assertThat(identity.sessionToken()).isEqualTo("sessionTokenFromCredential1");
+            });
+
     ObjectAssert<S3FileIOProperties> s3FileIOProperties2 =
         assertThat(fileIO.clientForStoragePath("s3://custom-uri/2/table1").s3FileIOProperties());
     s3FileIOProperties2
@@ -862,6 +919,62 @@ public class TestS3FileIO {
     s3FileIOProperties2
         .extracting(S3FileIOProperties::sessionToken)
         .isEqualTo("sessionTokenFromCredential2");
+
+    // verify that the credentials identity gets the correct credentials for the prefixed S3 client
+    assertThat(fileIO.client("s3://custom-uri/2").serviceClientConfiguration())
+        .extracting(AwsServiceClientConfiguration::credentialsProvider)
+        .extracting(IdentityProvider::resolveIdentity)
+        .satisfies(
+            x -> {
+              AwsSessionCredentialsIdentity identity = (AwsSessionCredentialsIdentity) x.get();
+              assertThat(identity).isNotNull();
+              assertThat(identity.accessKeyId()).isEqualTo("keyIdFromCredential2");
+              assertThat(identity.secretAccessKey()).isEqualTo("accessKeyFromCredential2");
+              assertThat(identity.sessionToken()).isEqualTo("sessionTokenFromCredential2");
+            });
+  }
+
+  @Test
+  public void storageCredentialsConfiguredWithoutCredentialsInProperties() {
+    StorageCredential s3Credential =
+        StorageCredential.create(
+            "s3://custom-uri/1",
+            ImmutableMap.of(
+                "s3.access-key-id",
+                "keyIdFromCredential",
+                "s3.secret-access-key",
+                "accessKeyFromCredential",
+                "s3.session-token",
+                "sessionTokenFromCredential"));
+
+    S3FileIO fileIO = new S3FileIO();
+    fileIO.setCredentials(ImmutableList.of(s3Credential));
+    fileIO.initialize(ImmutableMap.of("client.region", "us-east-1"));
+
+    // verify that the credentials identity gets the correct credentials for the prefixed S3 client
+    assertThat(fileIO.client("s3://custom-uri/1").serviceClientConfiguration())
+        .extracting(AwsServiceClientConfiguration::credentialsProvider)
+        .extracting(IdentityProvider::resolveIdentity)
+        .satisfies(
+            x -> {
+              AwsSessionCredentialsIdentity identity = (AwsSessionCredentialsIdentity) x.get();
+              assertThat(identity).isNotNull();
+              assertThat(identity.accessKeyId()).isEqualTo("keyIdFromCredential");
+              assertThat(identity.secretAccessKey()).isEqualTo("accessKeyFromCredential");
+              assertThat(identity.sessionToken()).isEqualTo("sessionTokenFromCredential");
+            });
+
+    // there are no credentials for the generic S3 client configured, since there are no credentials
+    // passed in the properties
+    assertThatThrownBy(
+            () ->
+                fileIO
+                    .client()
+                    .serviceClientConfiguration()
+                    .credentialsProvider()
+                    .resolveIdentity())
+        .isInstanceOf(SdkClientException.class)
+        .hasMessageContaining("Unable to load credentials from any of the providers");
   }
 
   private void createRandomObjects(String prefix, int count) {

nastra · 2025-05-26T06:35:16Z

+  }
+
+  @Test
+  public void storageCredentialsConfiguredOverwriteCredentialsInProperties()


this is already covered in existing tests

nastra · 2025-05-26T06:35:34Z

+  }
+
+  @Test
+  public void storageCredentialsConfiguredWithoutCredentialsInProperties() {


this is already covered in existing tests

nastra

LGTM but I would remove the two test methods that I commented on, since those are already covered in existing tests

…nt (apache#13118)

AWS: pass storage credentials to s3 client.

7bfb20b

github-actions Bot added the AWS label May 21, 2025

lliangyu-lin approved these changes May 21, 2025

View reviewed changes

nastra reviewed May 22, 2025

View reviewed changes

refine test case.

25380da

nastra approved these changes May 22, 2025

View reviewed changes

nastra closed this May 22, 2025

nastra reopened this May 22, 2025

plusplusjiajia force-pushed the fix-s3fileio branch from 667dcc5 to 25380da Compare May 23, 2025 00:31

revert S3FileIO change.

7eb6fa0

plusplusjiajia force-pushed the fix-s3fileio branch from d177782 to 7eb6fa0 Compare May 23, 2025 01:47

plusplusjiajia added 3 commits May 23, 2025 09:50

Merge remote-tracking branch 'plusplusjiajia/main' into fix-s3fileio

c4ef9b5

refine test case.

9853d66

add more test case for S3FileIO.

6101aae

plusplusjiajia force-pushed the fix-s3fileio branch from 53823a2 to 6101aae Compare May 23, 2025 02:36

nastra requested changes May 23, 2025

View reviewed changes

update s3 fileio test cases.

594e60d

plusplusjiajia changed the title ~~AWS: pass storage credentials to s3 client~~ AWS: update test cases to verify credentials for the prefixed S3 client May 24, 2025

nastra reviewed May 26, 2025

View reviewed changes

nastra approved these changes May 26, 2025

View reviewed changes

Update TestS3FileIO.java

1fc9f14

nastra approved these changes Jun 2, 2025

View reviewed changes

Update TestS3FileIO.java

ecf84a4

nastra merged commit 4218554 into apache:main Jun 2, 2025
42 checks passed

talatuyarer pushed a commit to talatuyarer/iceberg that referenced this pull request Jun 2, 2025

AWS: update test cases to verify credentials for the prefixed S3 clie…

3dac458

…nt (apache#13118)

devendra-nr pushed a commit to devendra-nr/iceberg that referenced this pull request Dec 8, 2025

AWS: update test cases to verify credentials for the prefixed S3 clie…

518ec1e

…nt (apache#13118)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

AWS: update test cases to verify credentials for the prefixed S3 client#13118

AWS: update test cases to verify credentials for the prefixed S3 client#13118
nastra merged 9 commits into
apache:mainfrom
plusplusjiajia:fix-s3fileio

plusplusjiajia commented May 21, 2025

Uh oh!

lliangyu-lin left a comment

Uh oh!

nastra May 22, 2025

Uh oh!

plusplusjiajia May 22, 2025

Uh oh!

nastra May 22, 2025

Uh oh!

plusplusjiajia May 22, 2025

Uh oh!

nastra commented May 22, 2025

Uh oh!

plusplusjiajia commented May 23, 2025

Uh oh!

nastra left a comment

Uh oh!

nastra May 26, 2025

Uh oh!

nastra May 26, 2025

Uh oh!

nastra left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

plusplusjiajia commented May 21, 2025

Uh oh!

lliangyu-lin left a comment

Choose a reason for hiding this comment

Uh oh!

nastra May 22, 2025

Choose a reason for hiding this comment

Uh oh!

plusplusjiajia May 22, 2025

Choose a reason for hiding this comment

Uh oh!

nastra May 22, 2025

Choose a reason for hiding this comment

Uh oh!

plusplusjiajia May 22, 2025

Choose a reason for hiding this comment

Uh oh!

nastra commented May 22, 2025

Uh oh!

plusplusjiajia commented May 23, 2025

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

nastra May 26, 2025

Choose a reason for hiding this comment

Uh oh!

nastra May 26, 2025

Choose a reason for hiding this comment

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants