-
Notifications
You must be signed in to change notification settings - Fork 361
fix(s3stream): Set default aws provider as fallback provider #2384
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
Conversation
TwoOnefour
commented
Mar 26, 2025
- ([Enhancement] S3 Client AWS Credentials Provider #2026)
Hi @TwoOnefour, Thanks for your contribution! Just a quick note: to link the PR to the issue automatically, GitHub requires keywords like "fixes" or "resolves" before the issue link (e.g., "resolves #123"). You can check the docs here: Linking a pull request to an issue. I've manually linked this PR to the issue, but future PRs would benefit from using the standard linking format. Appreciate your effort! |
@SCNieh Mind reviewing this PR? |
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.
Pull Request Overview
This PR updates the AWS credentials provider logic in S3 stream by setting the default AWS provider as the fallback mechanism.
- Added a new constant DEFAULT_AUTH_TYPE and its corresponding case in the authentication switch-case.
- Introduced a helper method to retrieve static credentials and adjusted the credentials chain accordingly.
- Removed the explicit addition of InstanceProfileCredentialsProvider in the default chain to align with the new fallback logic.
Comments suppressed due to low confidence (2)
s3stream/src/main/java/com/automq/stream/s3/operator/AwsObjectStorage.java:385
- [nitpick] Consider enclosing the if statement with braces to ensure clarity and prevent potential issues if additional logic is added in the future.
if (acp == null)
s3stream/src/main/java/com/automq/stream/s3/operator/AwsObjectStorage.java:387
- [nitpick] Consider caching the result of DefaultCredentialsProvider.create() in a local variable to avoid duplicate instantiation and enhance clarity.
return List.of(acp, DefaultCredentialsProvider.create());
hello @Chillax-0v0 @SCNieh , Any progress on this pr? Please let me know if you need unit test or anything else. |
Hi there @TwoOnefour, Regarding the PR status, some GitHub Actions workflows ("Checkstyle" and "Unit Test") are currently failing. Could you please try running the relevant commands locally to troubleshoot? For example: You can find the exact command sequence from the GitHub Action execution logs. Once identified, please commit the necessary fixes to resolve these issues. Let us know if you encounter any specific obstacles during this process. Thanks! |
44b2a28
to
fb943dc
Compare
} | ||
case INSTANCE_AUTH_TYPE: { | ||
return List.of(instanceProfileCredentialsProvider()); | ||
} | ||
case DEFAULT_AUTH_TYPE: { | ||
AwsCredentialsProvider acp = staticProfileCredentialsProvider(); |
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.
it's unnecessary to add static credential provider to the chain when using default provider
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.
it's unnecessary to add static credential provider to the chain when using default provider
ok, I have removed staticProfileCredentialsProvider
from default. See #95b2f9a