Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it easier to use AWS classes as a custom credentials provider #22007

Conversation

nineinchnick
Copy link
Member

Description

The trino.s3.credentials-provider configuration property allows specifying a class used as the credentials provider, but it requires the class to have a constructor that accepts a URI and a Configuration object. The AWS SDK classes implementing AWSCredentialsProvider have constructors without any arguments, so to use them, a wrapper class had to be created, and an extra JAR had to be added to the deployment.

Trying to instantiate the custom credentials provider class without any arguments makes it easier to pin to exactly one provider without using the default providers chain.

This actually makes the docs correct, because it already mentioned using providers from the AWS SDK, which was not possible without a wrapper class.

This at least makes the workaround for #15267 easier, I'm not sure if it will allow closing that issue.

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

The `trino.s3.credentials-provider` configuration property allows
specifying a class used as the credentials provider, but it requires the
class to have a constructor that accepts a URI and a Configuration
object. The AWS SDK classes implementing `AWSCredentialsProvider` have
constructors without any arguments, so to use them, a wrapper class had
to be created, and an extra JAR had to be added to the deployment.

Trying to instantiate the custom credentials provider class without any
arguments makes it easier to pin to exactly one provider without using
the default providers chain.
@nineinchnick
Copy link
Member Author

nineinchnick commented May 17, 2024

@electrum should we allow pinning to a specific provider in the new FS and Glue implementations? If yes, should we add a similar config option for a class name? Maybe also in here: https://github.com/trinodb/trino/blob/master/plugin/trino-exchange-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/s3/S3FileSystemExchangeStorage.java

interface and provide a two-argument constructor that takes a
`java.net.URI` and a Hadoop `org.apache.hadoop.conf.Configuration`
as arguments. A custom credentials provider can be used to provide
temporary credentials from STS (using `STSSessionCredentialsProvider`),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trino-hdfs is legacy & to be removed rather sooner than later. if this was broken and didn't work, maybe no need to fix it? Just remove incorrect stuff from the docs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked, just was hard to use. This feature is missing in new native fs library, so I'd also like to discuss if we want to add it there. Being able to choose a single credentials provider instead of relying on the (default) chain solves some issues.

How soon are we going to remove this, weeks or months?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd hope it's sooner than later (so weeks rather than months).
I am not against having this feature (done correctly) in the new native fs library, if it solves problems. Would it be possible to have an issue describing those problems?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the issue linked in the PR description

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The linked issue is sufficient to me to justify adding the configuration to the new s3 filesystem. @nineinchnick do you plan on working on this?

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are intentionally not supporting custom credential providers in the new file systems. If people need support for this, let's talk about the specific use cases.

For the IRSA issue, we should support pinning to WebIdentityTokenFileCredentialsProvider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants