-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat: Support anonymous S3 access for public buckets #27758
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -159,6 +159,7 @@ public static RetryStrategy getRetryStrategy(RetryMode retryMode) | |||||||||||||||||||||
| private String sseKmsKeyId; | ||||||||||||||||||||||
| private String sseCustomerKey; | ||||||||||||||||||||||
| private boolean useWebIdentityTokenCredentialsProvider; | ||||||||||||||||||||||
| private boolean anonymousAccess; | ||||||||||||||||||||||
| private SignerType signerType; | ||||||||||||||||||||||
| private DataSize streamingPartSize = DataSize.of(32, MEGABYTE); | ||||||||||||||||||||||
| private boolean requesterPays; | ||||||||||||||||||||||
|
|
@@ -397,6 +398,19 @@ public S3FileSystemConfig setUseWebIdentityTokenCredentialsProvider(boolean useW | |||||||||||||||||||||
| return this; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public boolean isAnonymousAccess() | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| return anonymousAccess; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @Config("s3.anonymous-access") | ||||||||||||||||||||||
| @ConfigDescription("Use anonymous credentials for accessing public S3 buckets") | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add config validation. when anonymous access is set, other authentication option should be left unset, as they are ignored (eg access/secret key) see here for example validation: trino/core/trino-main/src/test/java/io/trino/sql/TestSqlEnvironmentConfig.java Lines 64 to 73 in b892a70
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a good point! I've added validation to ensure that certain options are mutually exclusive. |
||||||||||||||||||||||
| public S3FileSystemConfig setAnonymousAccess(boolean anonymousAccess) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| this.anonymousAccess = anonymousAccess; | ||||||||||||||||||||||
| return this; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public String getSseCustomerKey() | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| return sseCustomerKey; | ||||||||||||||||||||||
|
|
@@ -420,6 +434,21 @@ public boolean isSseWithCustomerKeyConfigValid() | |||||||||||||||||||||
| return true; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @AssertTrue(message = "s3.anonymous-access cannot be used with other authentication methods (s3.aws-access-key, s3.aws-secret-key, s3.iam-role, s3.external-id, s3.sts.endpoint, s3.sts.region, s3.use-web-identity-token-credentials-provider)") | ||||||||||||||||||||||
| public boolean isAnonymousAccessConfigValid() | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| if (anonymousAccess) { | ||||||||||||||||||||||
| return awsAccessKey == null && | ||||||||||||||||||||||
| awsSecretKey == null && | ||||||||||||||||||||||
| iamRole == null && | ||||||||||||||||||||||
| externalId == null && | ||||||||||||||||||||||
| stsEndpoint == null && | ||||||||||||||||||||||
| stsRegion == null && | ||||||||||||||||||||||
| !useWebIdentityTokenCredentialsProvider; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return true; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public Optional<SignerType> getSignerType() | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| return Optional.ofNullable(signerType); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at:
I see the use of an "authentication type" field to distinguish between the many potential ways to configure the authentication for s3 access.
This can be done though EVENTUALLY in a follow-up PR.
Or if we decide to add it - then we'd be working with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! I really like this direction to be honest.
s3.auth-typewould be more consistent with Azure/GCS and eventually, if there would be more types, they could be easily supported. I went with a boolean flag for simplicity, but see the value in the proposed idea.However, before we implement this (now or in the follow-up PR), we need to clarify how this will interact with existing config parameters. For example, if the user doesn't specify an auth type, should we fail with an error or use the old logic (default credentials chain)?
I would be happy moving
s3.auth-typeto a separate PR. But let's create an issue or track it somewhere so that S3 isn't an exception when it comes to authentication configuration. In the meantime, I will see, how this can be implemented -- I'm relatively new to the codebase, but willing to learn more and contribute :DThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When introducing the new configuration, we should deprecate the existing one. This preserves backward compatibility while providing users with a warning, giving them a transition period to adjust if necessary. For reference, see #26681 for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing this PR, it's helpful 👍