[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5684: AWS: Refactor util methods for applying AWS clients configurations

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

jackye1995 commented on code in PR #5684:
URL: https://github.com/apache/iceberg/pull/5684#discussion_r963119759


##########
aws/src/main/java/org/apache/iceberg/aws/AssumeRoleAwsClientFactory.java:
##########
@@ -91,37 +93,26 @@ public void initialize(Map<String, String> properties) {
     Preconditions.checkNotNull(
         region, "Cannot initialize AssumeRoleClientConfigFactory with null region");
 
-    this.s3Endpoint = properties.get(AwsProperties.S3FILEIO_ENDPOINT);
     this.tags = toTags(properties);
-    this.s3UseArnRegionEnabled =
-        PropertyUtil.propertyAsBoolean(
-            properties,
-            AwsProperties.S3_USE_ARN_REGION_ENABLED,
-            AwsProperties.S3_USE_ARN_REGION_ENABLED_DEFAULT);
-    this.dynamoDbEndpoint = properties.get(AwsProperties.DYNAMODB_ENDPOINT);
-    this.httpClientType =
-        PropertyUtil.propertyAsString(
-            properties, AwsProperties.HTTP_CLIENT_TYPE, AwsProperties.HTTP_CLIENT_TYPE_DEFAULT);
-  }
-
-  protected <T extends AwsClientBuilder & AwsSyncClientBuilder> T configure(T clientBuilder) {
-    AssumeRoleRequest request =
+    this.assumeRoleRequest =
         AssumeRoleRequest.builder()
             .roleArn(roleArn)
             .roleSessionName(genSessionName())

Review Comment:
   yes I agree that session name should be configurable in 2 ways:
   1. the session name itself
   2. a boolean flag indicating if session name should be random, which is done by appending a random UUID suffix
   
   However, I think the session names in the request used by the `StsAssumeRoleCredentialsProvider ` should be consistent and not changing for each request. Considering from the audit log perspective, we should expect the same session name to show even if there are multiple assume role requests sent to refresh the credential, because they all come from the same program from start to end. So I think `roleSessionName` should be an instance variable with a fixed name.



-- 
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