JonasJ-ap commented on code in PR #5684:
URL: https://github.com/apache/iceberg/pull/5684#discussion_r971226025
##########
aws/src/main/java/org/apache/iceberg/aws/AssumeRoleAwsClientFactory.java:
##########
@@ -34,49 +32,54 @@
import software.amazon.awssdk.services.sts.StsClient;
import
software.amazon.awssdk.services.sts.auth.StsAssumeRoleCredentialsProvider;
import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
-import software.amazon.awssdk.services.sts.model.Tag;
public class AssumeRoleAwsClientFactory implements AwsClientFactory {
private String roleArn;
private String externalId;
- private Set<Tag> tags;
private int timeout;
private String region;
- private String s3Endpoint;
- private boolean s3UseArnRegionEnabled;
- private String dynamoDbEndpoint;
- private String httpClientType;
+ private AwsProperties awsProperties;
+ private AssumeRoleRequest assumeRoleRequest;
@Override
public S3Client s3() {
return S3Client.builder()
- .applyMutation(this::configure)
- .applyMutation(builder ->
AwsClientFactories.configureEndpoint(builder, s3Endpoint))
- .serviceConfiguration(s ->
s.useArnRegionEnabled(s3UseArnRegionEnabled).build())
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .applyMutation(awsProperties::applyS3EndpointConfigurations)
+ .applyMutation(awsProperties::applyS3ServiceConfigurations)
.build();
}
@Override
public GlueClient glue() {
- return GlueClient.builder().applyMutation(this::configure).build();
+ return GlueClient.builder()
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .build();
}
@Override
public KmsClient kms() {
- return KmsClient.builder().applyMutation(this::configure).build();
+ return KmsClient.builder()
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .build();
}
@Override
public DynamoDbClient dynamo() {
return DynamoDbClient.builder()
- .applyMutation(this::configure)
- .applyMutation(builder ->
AwsClientFactories.configureEndpoint(builder, dynamoDbEndpoint))
+ .applyMutation(this::applyAssumeRoleConfigurations)
+ .applyMutation(awsProperties::applyHttpClientConfigurations)
+ .applyMutation(awsProperties::applyDynamoDbEndpointConfigurations)
.build();
}
@Override
public void initialize(Map<String, String> properties) {
+ this.awsProperties = new AwsProperties(properties);
Review Comment:
Thank you for your suggestion. I updated the code so that all instance
variables are initialized in `AwsPropertires`. Moreover, with this change, it
seems that we can remove local variables in initialize(). Instead, we can make
the initialization process like this:
```
...
this.assumeRoleRequest =
AssumeRoleRequest.builder()
.roleArn(awsProperties.clientAssumeRoleArn())
.roleSessionName(genSessionName())
.durationSeconds(awsProperties.clientAssumeRoleTimeoutSec())
.externalId(awsProperties.clientAssumeRoleExternalId())
.tags(awsProperties.stsClientAssumeRoleTags())
.build();
```
Do you think it is ok to write in this way?
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -726,13 +829,129 @@ public void setS3DeleteEnabled(boolean s3DeleteEnabled) {
this.isS3DeleteEnabled = s3DeleteEnabled;
}
- private Set<Tag> toTags(Map<String, String> properties, String prefix) {
+ private Set<Tag> toS3Tags(Map<String, String> properties, String prefix) {
return PropertyUtil.propertiesWithPrefix(properties,
prefix).entrySet().stream()
.map(e -> Tag.builder().key(e.getKey()).value(e.getValue()).build())
.collect(Collectors.toSet());
}
+ private Set<software.amazon.awssdk.services.sts.model.Tag> toStsTags(
+ Map<String, String> properties, String prefix) {
+ return PropertyUtil.propertiesWithPrefix(properties,
prefix).entrySet().stream()
+ .map(
+ e ->
+ software.amazon.awssdk.services.sts.model.Tag.builder()
+ .key(e.getKey())
+ .value(e.getValue())
+ .build())
+ .collect(Collectors.toSet());
+ }
+
public Map<String, String> s3BucketToAccessPointMapping() {
return s3BucketToAccessPointMapping;
}
+
+ private boolean s3KeyIdAccessKeyBothConfigured() {
+ return (s3AccessKeyId == null) == (s3SecretAccessKey == null);
+ }
+
+ private AwsCredentialsProvider credentialsProvider(
+ String accessKeyId, String secretAccessKey, String sessionToken) {
+ if (accessKeyId != null) {
+ if (sessionToken == null) {
+ return StaticCredentialsProvider.create(
+ AwsBasicCredentials.create(accessKeyId, secretAccessKey));
+ } else {
+ return StaticCredentialsProvider.create(
+ AwsSessionCredentials.create(accessKeyId, secretAccessKey,
sessionToken));
+ }
+ } else {
+ return DefaultCredentialsProvider.create();
+ }
+ }
+
+ /**
+ * Configure the credentials for an S3 client.
+ *
+ * <p>Sample usage:
Review Comment:
Thank you for your suggestions!
##########
aws/src/main/java/org/apache/iceberg/aws/AwsClientFactories.java:
##########
@@ -86,99 +85,58 @@ private static AwsClientFactory loadClientFactory(String
impl, Map<String, Strin
}
static class DefaultAwsClientFactory implements AwsClientFactory {
+ private AwsProperties awsProperties;
- private String glueEndpoint;
- private String s3Endpoint;
- private String s3AccessKeyId;
- private String s3SecretAccessKey;
- private String s3SessionToken;
- private Boolean s3PathStyleAccess;
- private Boolean s3UseArnRegionEnabled;
- private Boolean s3AccelerationEnabled;
- private String dynamoDbEndpoint;
- private String httpClientType;
- private Boolean s3DualStackEnabled;
-
- DefaultAwsClientFactory() {}
+ DefaultAwsClientFactory() {
+ awsProperties = new AwsProperties();
Review Comment:
Added this default initialization to avoid `NullPointerException` issue when
using `DefaultAwsClientFactory()` to create clients.
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -726,13 +829,150 @@ public void setS3DeleteEnabled(boolean s3DeleteEnabled) {
this.isS3DeleteEnabled = s3DeleteEnabled;
}
- private Set<Tag> toTags(Map<String, String> properties, String prefix) {
+ private Set<Tag> toS3Tags(Map<String, String> properties, String prefix) {
Review Comment:
Thank you for your suggestion. I moved all the private methods to the bottom.
##########
aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java:
##########
@@ -467,39 +513,61 @@ public AwsProperties() {
this.isS3DeleteEnabled = S3_DELETE_ENABLED_DEFAULT;
this.s3BucketToAccessPointMapping = ImmutableMap.of();
this.s3PreloadClientEnabled = S3_PRELOAD_CLIENT_ENABLED_DEFAULT;
+ this.s3DualStackEnabled = S3_DUALSTACK_ENABLED_DEFAULT;
+ this.s3PathStyleAccess = S3FILEIO_PATH_STYLE_ACCESS_DEFAULT;
+ this.s3UseArnRegionEnabled = S3_USE_ARN_REGION_ENABLED_DEFAULT;
+ this.s3AccelerationEnabled = S3_ACCELERATION_ENABLED_DEFAULT;
this.glueCatalogId = null;
+ this.glueEndpoint = null;
this.glueCatalogSkipArchive = GLUE_CATALOG_SKIP_ARCHIVE_DEFAULT;
this.glueCatalogSkipNameValidation =
GLUE_CATALOG_SKIP_NAME_VALIDATION_DEFAULT;
this.glueLakeFormationEnabled = GLUE_LAKEFORMATION_ENABLED_DEFAULT;
+ this.dynamoDbEndpoint = null;
this.dynamoDbTableName = DYNAMODB_TABLE_NAME_DEFAULT;
+
+ ValidationException.check(
+ s3KeyIdAccessKeyBothConfigured(),
+ "S3 client access key ID and secret access key must be set at the same
time");
}
public AwsProperties(Map<String, String> properties) {
- this.s3FileIoSseType =
- properties.getOrDefault(
- AwsProperties.S3FILEIO_SSE_TYPE,
AwsProperties.S3FILEIO_SSE_TYPE_NONE);
- this.s3FileIoSseKey = properties.get(AwsProperties.S3FILEIO_SSE_KEY);
- this.s3FileIoSseMd5 = properties.get(AwsProperties.S3FILEIO_SSE_MD5);
- if (AwsProperties.S3FILEIO_SSE_TYPE_CUSTOM.equals(s3FileIoSseType)) {
+ this.httpClientType =
+ PropertyUtil.propertyAsString(properties, HTTP_CLIENT_TYPE,
HTTP_CLIENT_TYPE_DEFAULT);
+ this.stsClientAssumeRoleTags = toStsTags(properties,
CLIENT_ASSUME_ROLE_TAGS_PREFIX);
+
+ this.clientAssumeRoleArn = properties.get(CLIENT_ASSUME_ROLE_ARN);
+ this.clientAssumeRoleTimeoutSec =
+ PropertyUtil.propertyAsInt(
+ properties, CLIENT_ASSUME_ROLE_TIMEOUT_SEC,
CLIENT_ASSUME_ROLE_TIMEOUT_SEC_DEFAULT);
+ this.clientAssumeRoleExternalId =
properties.get(CLIENT_ASSUME_ROLE_EXTERNAL_ID);
+ this.clientAssumeRoleRegion = properties.get(CLIENT_ASSUME_ROLE_REGION);
+
+ this.s3FileIoSseType = properties.getOrDefault(S3FILEIO_SSE_TYPE,
S3FILEIO_SSE_TYPE_NONE);
+ this.s3FileIoSseKey = properties.get(S3FILEIO_SSE_KEY);
+ this.s3FileIoSseMd5 = properties.get(S3FILEIO_SSE_MD5);
+ this.s3AccessKeyId = properties.get(S3FILEIO_ACCESS_KEY_ID);
+ this.s3SecretAccessKey = properties.get(S3FILEIO_SECRET_ACCESS_KEY);
+ this.s3SessionToken = properties.get(S3FILEIO_SESSION_TOKEN);
+ if (S3FILEIO_SSE_TYPE_CUSTOM.equals(s3FileIoSseType)) {
Preconditions.checkNotNull(
s3FileIoSseKey, "Cannot initialize SSE-C S3FileIO with null
encryption key");
Preconditions.checkNotNull(
s3FileIoSseMd5, "Cannot initialize SSE-C S3FileIO with null
encryption key MD5");
}
+ this.s3Endpoint = properties.get(S3FILEIO_ENDPOINT);
+ this.glueEndpoint = properties.get(GLUE_CATALOG_ENDPOINT);
this.glueCatalogId = properties.get(GLUE_CATALOG_ID);
this.glueCatalogSkipArchive =
PropertyUtil.propertyAsBoolean(
- properties,
- AwsProperties.GLUE_CATALOG_SKIP_ARCHIVE,
- AwsProperties.GLUE_CATALOG_SKIP_ARCHIVE_DEFAULT);
+ properties, GLUE_CATALOG_SKIP_ARCHIVE,
GLUE_CATALOG_SKIP_ARCHIVE_DEFAULT);
this.glueCatalogSkipNameValidation =
PropertyUtil.propertyAsBoolean(
properties,
- AwsProperties.GLUE_CATALOG_SKIP_NAME_VALIDATION,
- AwsProperties.GLUE_CATALOG_SKIP_NAME_VALIDATION_DEFAULT);
+ GLUE_CATALOG_SKIP_NAME_VALIDATION,
+ GLUE_CATALOG_SKIP_NAME_VALIDATION_DEFAULT);
Review Comment:
It seems the current format is favored by `spotlessCheck`. When I moved them
into one line and click `spotlessApply`, they just rollback to two separate
lines. I think these two flags are two long to be in one line.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
