feat: Add functionality to use temporary session token for authentication to s3 #123
Conversation
WalkthroughThe changes modify the S3 filesystem implementation in the Changes
The changes are relatively straightforward, focusing on configuration flexibility and improved error visibility in the S3 filesystem implementation. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@hwbrzzl please review this mr and merge if possible. |
hwbrzzl
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #123 +/- ##
=========================================
- Coverage 5.53% 5.32% -0.21%
=========================================
Files 3 4 +1
Lines 307 319 +12
=========================================
Hits 17 17
- Misses 289 301 +12
Partials 1 1 ☔ View full report in Codecov by Sentry. |
| @@ -42,13 +43,14 @@ func NewS3(ctx context.Context, config config.Config, disk string) (*S3, error) | |||
| region := config.GetString(fmt.Sprintf("filesystems.disks.%s.region", disk)) | |||
| bucket := config.GetString(fmt.Sprintf("filesystems.disks.%s.bucket", disk)) | |||
| url := config.GetString(fmt.Sprintf("filesystems.disks.%s.url", disk)) | |||
| token := config.GetString(fmt.Sprintf("filesystems.disks.%s.token", disk), "") | |||
There was a problem hiding this comment.
The token is not fixed data, right? In current logic, the token will be fixed when initiating S3, I wonder if it's expected.
There was a problem hiding this comment.
yes It will be inject via config like other variables. I'll send an MR for goravel config as well later.
There was a problem hiding this comment.
most of the cases accesskeyId and accessKeySecret will be used. But on lambda those two does not work and lambda provide a temporary access token for the time span of lambda function. So it will be injected via environmet > goravel config > s3 driver.
As lambda instances are short term so it will be destroyed and recreated with new token after few minutes so no worry here.
Let me know if you mean something else.
There was a problem hiding this comment.
So the token is fixed in the short term; it's safe to inject it via environment directly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
s3.go (2)
46-46: Consider validating or documenting the optional token usage.You’re correctly retrieving the
tokenfrom the config with a default"". However, it might be helpful to add a code comment or validation indicating that this is optional. This ensures developers using your code understand when and why the token is needed.
210-214: Fix the spelling of “existance” and consider providing more details in the log message.There’s a minor spelling mistake in the log message: “existance” should be “existence.” If the error could relate to permissions or missing files, adding context can help with debugging.
Proposed fix:
- log.Println("error while checking file existance:", err) + log.Println("error while checking file existence:", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
s3.go(3 hunks)
🔇 Additional comments (1)
s3.go (1)
53-53: S3 client now supports session tokens.
Passing the token to credentials.NewStaticCredentialsProvider increases security for environments using temporary credentials. This change looks good!
📑 Description
This merge request added functionality to support session tokens for authentication on S3, enabling it to work with temporary credentials, such as those used in an AWS Lambda environment.
Summary by CodeRabbit
New Features
Bug Fixes
✅ Checks