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

Allow pinning to WebIdentityTokenCredentialsProvider in native S3 client #22163

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

nineinchnick
Copy link
Member

@nineinchnick nineinchnick commented May 27, 2024

Description

Allow users to only use the StsWebIdentityTokenFileCredentialsProvider instead of the default credentials provider chain.

This complements the same flag in the legacy library, see #22162. This at least makes the workaround for #15267 easier, I'm not sure if it will allow closing that issue. I haven't updated the docs yet, I'll wait for some initial feedback first.

I used StsWebIdentityTokenFileCredentialsProvider instead of WebIdentityTokenFileCredentialsProvider based on the Javadoc recommendations around periodic updates, and because we can configure a custom STS client.

Additional context and related issues

Release notes

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

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 27, 2024
@nineinchnick nineinchnick requested a review from electrum May 27, 2024 15:58
@nineinchnick nineinchnick force-pushed the native-s3-web-identity-token-auth branch 2 times, most recently from b36701f to b165ef1 Compare May 28, 2024 09:22
@mehulbatra-d11
Copy link

we are eagerly waiting on this, hope this will be merged and released soon!

@findinpath findinpath requested a review from anusudarsan May 31, 2024 13:21
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.

I don't love this config name, but I can't think of anything better. Thanks for fixing this.

@nineinchnick
Copy link
Member Author

@electrum me too. Thanks for reviewing it, can you merge this?

@rohanag12 rohanag12 mentioned this pull request Jun 3, 2024
@nineinchnick
Copy link
Member Author

nineinchnick commented Jun 4, 2024

I tested this by deploying Trino on EKS, using https://github.com/binayakd/trino-on-eks
and these chart values:

image:
  repository: nineinchnick/test
  tag: 449-SNAPSHOT-amd64
server:
  workers: 1
additionalCatalogs:
  s3_hive: |-
    connector.name=hive
    hive.metastore.uri=thrift://metastore:9083
    fs.native-s3.enabled=true
    s3.region=eu-central-1
    s3.use-web-identity-token-credentials-provider=true
coordinator:
  additionalJVMConfig:
  - -XX:+UnlockDiagnosticVMOptions
  - -XX:G1NumCollectionsKeepPinned=10000000
worker:
  additionalJVMConfig:
  - -XX:+UnlockDiagnosticVMOptions
  - -XX:G1NumCollectionsKeepPinned=10000000
additionalLogProperties:
- software.amazon.awssdk=DEBUG

I checked the logs and I see it making the request:

2024-06-04T09:53:59.066Z        DEBUG   SplitRunner-20240604_095351_00000_crysi.0.0.0-1-97      software.amazon.awssdk.utils.cache.CachedSupplier       (StsAssumeRoleWithWebIdentityCredentialsProvider()) Cached value is stale and will be refreshe
d.
2024-06-04T09:53:59.066Z        DEBUG   SplitRunner-20240604_095351_00000_crysi.0.0.0-1-97      software.amazon.awssdk.utils.cache.CachedSupplier       (StsAssumeRoleWithWebIdentityCredentialsProvider()) Refreshing cached value.

It is using software.amazon.awssdk.auth.credentials.AwsCredentialsProviderChain but for the STS client.
Further in logs:

2024-06-04T09:53:59.705Z        DEBUG   SplitRunner-20240604_095351_00000_crysi.0.0.0-1-97      software.amazon.awssdk.services.sts.auth.StsCredentialsProvider Using STS credentials with expiration time of 2024-06-04T10:53:59Z
2024-06-04T09:53:59.706Z        DEBUG   SplitRunner-20240604_095351_00000_crysi.0.0.0-1-97      software.amazon.awssdk.auth.credentials.AwsCredentialsProviderChain     Loading credentials from WebIdentityTokenCredentialsProvider()

Allow users to only use the WebIdentityTokenCredentialsProvider instead
of the default credentials provider chain.
@nineinchnick nineinchnick force-pushed the native-s3-web-identity-token-auth branch from b165ef1 to 4c8d321 Compare June 4, 2024 10:04
@github-actions github-actions bot added the docs label Jun 4, 2024
@nineinchnick
Copy link
Member Author

@ebyhr could you merge this?

@ebyhr ebyhr merged commit ad9aa23 into trinodb:master Jun 11, 2024
61 of 62 checks passed
@github-actions github-actions bot added this to the 450 milestone Jun 11, 2024
@nineinchnick nineinchnick deleted the native-s3-web-identity-token-auth branch September 26, 2024 16:55
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.

6 participants