[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #7066: S3 Credentials provider support in DefaultAwsClientFactory #7063

["JonasJ-ap (via GitHub)" <gi...@apache.org>]

JonasJ-ap commented on code in PR #7066:
URL: https://github.com/apache/iceberg/pull/7066#discussion_r1133317973


##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -1173,14 +1205,12 @@ public <T extends AwsSyncClientBuilder> void applyHttpClientConfigurations(T bui
     switch (httpClientType) {
       case HTTP_CLIENT_TYPE_URLCONNECTION:
         UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
-            (UrlConnectionHttpClientConfigurations)
-                loadHttpClientConfigurations(UrlConnectionHttpClientConfigurations.class.getName());
+            loadHttpClientConfigurations(UrlConnectionHttpClientConfigurations.class.getName());

Review Comment:
   Thank you for refactoring this.



##########
aws/src/test/java/org/apache/iceberg/aws/TestAwsClientFactories.java:
##########
@@ -132,6 +136,41 @@ public void testLakeFormationAwsClientFactorySerializable() throws IOException {
         .isInstanceOf(LakeFormationAwsClientFactory.class);
   }
 
+  @Test
+  public void testDefaultAwsClientFactoryWithCredentialsProvider() {
+    Map<String, String> properties = Maps.newHashMap();
+    properties.put(AwsProperties.AWS_REGION, Region.AWS_GLOBAL.toString());
+    properties.put(
+        AwsProperties.S3FILEIO_CREDENTIALS_PROVIDER,
+        SystemPropertyCredentialsProvider.class.getName());
+
+    AwsClientFactory defaultAwsClientFactory = AwsClientFactories.from(properties);
+    Assertions.assertThat(defaultAwsClientFactory)
+        .isInstanceOf(AwsClientFactories.DefaultAwsClientFactory.class);
+
+    try (S3Client s3Client = defaultAwsClientFactory.s3()) {
+      Assertions.assertThat(s3Client != null);
+      URL url =
+          s3Client
+              .utilities()
+              .getUrl(GetUrlRequest.builder().bucket("test-bucket").key("test-key").build());
+      Assert.assertTrue(
+          url != null && url.toString().equals("https://test-bucket.s3.amazonaws.com/test-key"));
+    }
+  }
+
+  @Test
+  public void testDefaultAwsClientFactoryWithInvalidCredentialsProvider() {
+    Map<String, String> properties = Maps.newHashMap();
+    properties.put(AwsProperties.AWS_REGION, Region.AWS_GLOBAL.toString());
+    properties.put(AwsProperties.S3FILEIO_CREDENTIALS_PROVIDER, "invalidClassName");
+    AssertHelpers.assertThrows(
+        "Should fail if invalid credentials provider set",
+        IllegalArgumentException.class,
+        "The creation of an instance of invalidClassName failed",
+        () -> AwsClientFactories.from(properties).s3().close());

Review Comment:
   [suggestion] Since there is another PR (#6977) planning on deprecating `AssertHelpers`, shall we use `Assertions.assertThatThrownBy` here instead?
   
   It may be something like:
   ```java
   Assertions.assertThatThrownBy(() -> AwsClientFactories.from(properties).s3().close())
   .isInstanceOf(IllegalArgumentException.class)
   .hasMessageContaining("The creation of an instance of invalidClassName failed");
   ```
   



##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -91,13 +92,16 @@ private static AwsClientFactory loadClientFactory(String impl, Map<String, Strin
   static class DefaultAwsClientFactory implements AwsClientFactory {
     private AwsProperties awsProperties;
 
+    private Region region;
+
     DefaultAwsClientFactory() {
       awsProperties = new AwsProperties();
     }
 
     @Override
     public S3Client s3() {
       return S3Client.builder()
+          .region(this.region)

Review Comment:
   [Suggestion] Shall we follow the `applyMutation` pattern here
   ```java
   .applyMutation(awsProperties:applyAwsRegionConfiguration)
   ```
   and have the following method in `AwsProperties.java`
   ```java
   public <T extends AwsSyncClientBuilder> void applyAwsRegionConfiguration(T builder) {
      if (awsRegion != null) {
          builder.region(Region.of(awsRegion))
     }
   }
   ```
   
   In this way, we do not need to do any further initialization in the DefaultAwsClientFactories
   
   What do you think about this change?



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

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


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