-
Notifications
You must be signed in to change notification settings - Fork 2
fix: S3 wrap PutObject into ReadSeeker - OB-51476 #412
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
base: main
Are you sure you want to change the base?
fix: S3 wrap PutObject into ReadSeeker - OB-51476 #412
Conversation
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.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
92237d4 to
be03e1d
Compare
be03e1d to
b9c3737
Compare
| // This allows AWS SDK to retry S3 PutObject operations by rewinding the stream | ||
| var body io.ReadSeeker | ||
| if r != nil { | ||
| data, err := io.ReadAll(r) |
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.
This is still my original concern, we should avoid reading things into memory entirely unless fully necessary. It leads to OOMs if the data is too large. Readers should be preferred unless absolutely necessary vs reading into memory being the default

Fix will resolve "failed to rewind transport stream for retry" errors during S3
PutObjectoperations.The AWS SDK retries failed
PutObjectrequests, but the original request body (io.Reader) was not seekable, causing rewind failures on retry.Fix:
Wrap the request body in a seekable io.ReadSeeker by reading it into memory first. This ensures retries can reset the stream position.