Conversation
javuto
added
refactor
There was a problem hiding this comment.
Pull request overview
This PR replaces usage of the AWS SDK Go v2 feature/s3/manager helpers with direct S3 client calls, aligning the codebase with newer/non-deprecated S3 usage patterns.
Changes:
- Removed
manager.Uploaderusage and switched uploads tos3.Client.PutObjectfor S3 logging and carve block uploads. - Replaced S3 downloads from
manager.Downloaderwiths3.Client.GetObject+ in-memory buffering. - Added a small in-memory
io.WriterAtimplementation (bytesWriterAt) to support the updated download path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/logging/s3.go | Removes S3 manager uploader and uses PutObject directly for log uploads. |
| pkg/carves/s3.go | Removes S3 manager uploader/downloader, uses PutObject for carve blocks, and GetObject for downloads with a new in-memory buffer type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uploadOutput, err := carveS3.Client.PutObject(ctx, &s3.PutObjectInput{ | ||
| Bucket: aws.String(carveS3.S3Config.Bucket), | ||
| Key: aws.String(GenerateS3Key(block.Environment, uuid, block.SessionID, block.BlockID)), | ||
| Body: bytes.NewBuffer(toUpload), | ||
| Body: bytes.NewReader(toUpload), | ||
| ContentLength: &ptrContentLength, |
There was a problem hiding this comment.
Switching from manager.Uploader.Upload to s3.Client.PutObject changes behavior: PutObject is a single-request upload (no multipart) and has the 5GB object size limit, while the uploader would automatically use multipart uploads and provide additional resiliency for larger payloads. If carve blocks can be large or you relied on multipart semantics, consider using the multipart upload APIs (or an equivalent uploader helper) instead of PutObject.
| data, err := io.ReadAll(output.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Download - reading object body - %w", err) | ||
| } | ||
| fileReader := newBytesWriterAt(len(data)) |
There was a problem hiding this comment.
Download reads the entire S3 object into memory via io.ReadAll and then copies it into a second buffer (bytesWriterAt), which can double peak memory usage and risk OOM for large carves. Consider streaming the response to a file/io.Writer, or reading directly into a single preallocated buffer, instead of ReadAll + copy.
| fileReader := newBytesWriterAt(len(data)) | ||
| if _, err := fileReader.WriteAt(data, 0); err != nil { | ||
| return nil, fmt.Errorf("Download - buffering object body - %w", err) | ||
| } | ||
| if carveS3.Debug { |
There was a problem hiding this comment.
After buffering the object in memory, Download still returns io.WriterAt, which doesn’t let callers read the downloaded content back. Consider changing the return type to something readable (e.g., io.ReadCloser, []byte, or io.ReaderAt) so callers can actually consume the downloaded data.
| type bytesWriterAt struct { | ||
| data []byte | ||
| } | ||
|
|
||
| func newBytesWriterAt(size int) *bytesWriterAt { |
There was a problem hiding this comment.
bytesWriterAt is an in-memory buffer type, but it only supports WriteAt and doesn’t expose any method to read/extract the buffered bytes (and the type is unexported). If this is meant to be returned from Download, consider implementing a readable interface (e.g., io.ReaderAt) and/or adding an accessor like Bytes().
| result, err := logS3.Client.PutObject(ctx, &s3.PutObjectInput{ | ||
| Bucket: aws.String(logS3.S3Config.Bucket), | ||
| Key: aws.String(environment + "/" + logType + "/" + uuid + ":" + strconv.FormatInt(time.Now().UnixMilli(), 10) + ".json"), | ||
| Body: bytes.NewBuffer(data), | ||
| Body: bytes.NewReader(data), | ||
| ContentLength: &ptrContentLength, |
There was a problem hiding this comment.
Replacing manager.Uploader.Upload with s3.Client.PutObject removes automatic multipart upload behavior and associated handling for large payloads; PutObject is limited to 5GB per object and performs a single request. If log payloads could be large or you previously depended on multipart semantics, consider using multipart upload APIs (or an equivalent uploader helper) here.
Replacing deprecated code in s3 aws SDK: aws/aws-sdk-go-v2#3306