fresh-borzoni commented on code in PR #3540:
URL: https://github.com/apache/fluss/pull/3540#discussion_r3520836625
##########
fluss-filesystems/fluss-fs-s3/src/main/java/org/apache/fluss/fs/s3/token/S3DelegationTokenProvider.java:
##########
@@ -63,21 +73,31 @@ public class S3DelegationTokenProvider {
@Nullable private final String secretKey;
@Nullable private final String roleArn;
@Nullable private final String stsEndpoint;
+ @Nullable private final AWSCredentialProviderList credentialProviderList;
private final Map<String, String> additionInfos;
- public S3DelegationTokenProvider(String scheme, Configuration conf) {
+ public S3DelegationTokenProvider(String scheme, Configuration conf) throws
IOException {
this.scheme = scheme;
this.region = conf.get(REGION_KEY);
checkArgument(region != null, "Region is not set.");
this.accessKey = conf.get(ACCESS_KEY_ID);
this.secretKey = conf.get(ACCESS_KEY_SECRET);
this.roleArn = conf.get(ROLE_ARN_KEY);
this.stsEndpoint = conf.get(STS_ENDPOINT_KEY);
+ boolean hasCredentialProvider =
+ conf.getBoolean(CREDENTIAL_PROVIDER_EXPLICITLY_CONFIGURED,
false)
+ &&
StringUtils.isNotBlank(conf.getTrimmed(AWS_CREDENTIALS_PROVIDER));
checkArgument(
(accessKey == null) == (secretKey == null),
"S3 access key and secret key must both be set or both be
unset.");
- if (accessKey == null) {
+ if (hasCredentialProvider && roleArn != null) {
+ throw new IllegalArgumentException(
+ "AssumeRole and a custom AWS credentials provider cannot
be configured together.");
+ }
+ this.credentialProviderList =
Review Comment:
The description says configuring DynamicTemporaryAWSCredentialsProvider for
server mode is "rejected", but there's no guard, so it just gets instantiated
here and throws NoAwsCredentialsException at the first token request. Can you
clarify?
##########
fluss-filesystems/fluss-fs-s3/src/main/java/org/apache/fluss/fs/s3/S3FileSystemPlugin.java:
##########
@@ -23,14 +23,17 @@
import org.apache.fluss.fs.FileSystem;
import org.apache.fluss.fs.FileSystemPlugin;
import org.apache.fluss.fs.s3.token.S3ADelegationTokenReceiver;
+import org.apache.fluss.fs.s3.token.S3DelegationTokenProvider;
import org.apache.fluss.fs.s3.token.S3DelegationTokenReceiver;
+import org.apache.commons.lang3.StringUtils;
Review Comment:
ditto
##########
fluss-filesystems/fluss-fs-s3/src/main/java/org/apache/fluss/fs/s3/token/S3DelegationTokenProvider.java:
##########
@@ -147,6 +168,33 @@ private AWSSecurityTokenService buildStsClient() {
return builder.build();
}
+ @Nullable
+ AWSCredentialsProvider createStsCredentialsProvider() {
+ if (credentialProviderList != null) {
+ AWSCredentials credentials =
credentialProviderList.getCredentials();
+ checkArgument(
Review Comment:
This is effectively the "long-term creds only" gate, and it's the thing
people will trip on instance profiles/IRSA roles all return session creds and
land here at token time (lazily).
Given s3.md already has an IRSA section, can we add a short note there for
this new mode: long-term creds only, not compatible with AssumeRole? Otherwise
it's a bit hidden for operational usage
##########
fluss-filesystems/fluss-fs-s3/src/main/java/org/apache/fluss/fs/s3/token/S3DelegationTokenProvider.java:
##########
@@ -29,12 +32,16 @@
import com.amazonaws.services.securitytoken.model.AssumeRoleResult;
import com.amazonaws.services.securitytoken.model.Credentials;
import com.amazonaws.services.securitytoken.model.GetSessionTokenResult;
+import org.apache.commons.lang3.StringUtils;
Review Comment:
commons-lang3 is only transitive here, shall we use
org.apache.commons.lang3.StringUtils?
##########
fluss-filesystems/fluss-fs-s3/src/main/java/org/apache/fluss/fs/s3/token/S3DelegationTokenProvider.java:
##########
@@ -63,21 +73,31 @@ public class S3DelegationTokenProvider {
@Nullable private final String secretKey;
@Nullable private final String roleArn;
@Nullable private final String stsEndpoint;
+ @Nullable private final AWSCredentialProviderList credentialProviderList;
private final Map<String, String> additionInfos;
- public S3DelegationTokenProvider(String scheme, Configuration conf) {
+ public S3DelegationTokenProvider(String scheme, Configuration conf) throws
IOException {
this.scheme = scheme;
this.region = conf.get(REGION_KEY);
checkArgument(region != null, "Region is not set.");
this.accessKey = conf.get(ACCESS_KEY_ID);
this.secretKey = conf.get(ACCESS_KEY_SECRET);
this.roleArn = conf.get(ROLE_ARN_KEY);
this.stsEndpoint = conf.get(STS_ENDPOINT_KEY);
+ boolean hasCredentialProvider =
+ conf.getBoolean(CREDENTIAL_PROVIDER_EXPLICITLY_CONFIGURED,
false)
+ &&
StringUtils.isNotBlank(conf.getTrimmed(AWS_CREDENTIALS_PROVIDER));
checkArgument(
(accessKey == null) == (secretKey == null),
"S3 access key and secret key must both be set or both be
unset.");
- if (accessKey == null) {
+ if (hasCredentialProvider && roleArn != null) {
Review Comment:
Since we're adding all these checks in this PR - this one fails fast, but
the session/empty-cred cases only fail on the first token request.
Could we check those at construction too, so they're consistent? Not
blocking though
--
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]
