Add IAM Role support for exchange spooling on S3 #12536
Conversation
|
Please note |
| @@ -455,6 +459,22 @@ private static AwsCredentialsProvider createAwsCredentialsProvider(ExchangeS3Con | |||
| return StaticCredentialsProvider.create(AwsBasicCredentials.create(config.getS3AwsAccessKey(), config.getS3AwsSecretKey())); | |||
| } | |||
|
|
|||
| if (config.getS3IamRole().isPresent()) { | |||
There was a problem hiding this comment.
Can you validate config here that different configuration options are mutually exclusive?
- both
config.getS3AwsAccessKey()andconfig.getS3AwsSecretKey()must be either set or unset - if accessKey and secretKey are set then other auth related config options are not set.
- externalId can only be set if iaRole is set
There was a problem hiding this comment.
I'm not sure if we need to do that because TrinoS3FileSystem didn't perform such checks. Also they are not really mutually exclusive, it's just that priority is different (static credentials > iamRole > default credentials)
There was a problem hiding this comment.
I think the fact that TS3FS does not do that is an omission.
Also unless I am missing sth they are mutually exclusive. If you set S3 Access and Secret key it does not matter what you set in IamRole as you would not get to line in code when you use that.
You would return in return StaticCredentialsProvider.create(AwsBasicCredentials.create(config.getS3AwsAccessKey(), config.getS3AwsSecretKey())); right?
Hence if user provides both access/secretKey and IAM role it is fishy, I would throw in such case.
There was a problem hiding this comment.
OK, let me add the validations
There was a problem hiding this comment.
Thanks. I made the assertions a bit more explicit and used IllegalArgumentException instead of verify (verify should rather be used for consistency assertions - for things that would never happen if system is working correctly).
PTAL if I did not mess anything up.
There was a problem hiding this comment.
I'm fine with this, but I think my version is more concise. We can merge it still
There was a problem hiding this comment.
Yeah it is :) But I think actionable error messages are more important than concise code.
I am also dumb - and reading concise code is hard for me :P
linzebing
force-pushed
the
exchange-spooling-iam-role
branch
from
64a1610 to
4138771
Compare
See https://github.com/aws/aws-sdk-java-v2/blob/ecf680e826bc2ab496ca29e8dea7e58c61df2184/core/auth/src/main/java/software/amazon/awssdk/auth/credentials/DefaultCredentialsProvider.java#L73-L88, DefaultCredentialsProvider already have WebIdentityTokenFileCredentialsProvider supported
linzebing
force-pushed
the
exchange-spooling-iam-role
branch
2 times, most recently
from
e753799 to
33b1336
Compare
|
CI: #12300 |
losipiuk
force-pushed
the
exchange-spooling-iam-role
branch
from
33b1336 to
41f4084
Compare
Description
As title. Also drop a useless config.
New feature
trino-exchange-filesystem
Related issues, pull requests, and links
Documentation
( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
(x) Release notes entries required with the following suggested text: