[GitHub] [iceberg] danielcweeks commented on a change in pull request #1844: AWS: support custom credentials, region and assume role functionality

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

danielcweeks commented on a change in pull request #1844:
URL: https://github.com/apache/iceberg/pull/1844#discussion_r533579377



##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsClientCredentialsFactory.java
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws;
+
+import java.io.Serializable;
+import org.apache.iceberg.catalog.CatalogConfigurable;
+import software.amazon.awssdk.awscore.client.builder.AwsClientBuilder;
+
+/**
+ * Interface for loading a custom factory to configure AWS credentials for different services.
+ * See {@link AssumeRoleCredentialsFactory} as an example.

Review comment:
       So, I'm just catching up on all of the discussion and may not be following everything just yet, but I feel like we should try to avoid most S3Client configuration in Iceberg.  Part of the reason the S3FileIO takes a supplier for an S3Client is the we can externalize all of the configuration.
   
   The places where we have exposed configuration (part size, acl, etc.) is because they are request level options.  Since credentials are at the client level, we should be able to have just a S3ClientFactory that supplies all of the custom client configuration as opposed to exposing it directly. 
   
   I feel like that would really reduce the complexity in Iceberg and at the same time expose all of the possible configuration for the S3Client that anyone may want to leverage.




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

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