fix(upload): opt out of flexible checksum headers for SDA inbox compatibility#689
Open
jhagberg wants to merge 2 commits into
Open
fix(upload): opt out of flexible checksum headers for SDA inbox compatibility#689jhagberg wants to merge 2 commits into
jhagberg wants to merge 2 commits into
Conversation
nanjiangshu
reviewed
May 27, 2026
| ContentEncoding: aws.String(config.Encoding), | ||
| }, func(o *transfermanager.Options) { | ||
| // Preserve RequestChecksumCalculationWhenRequired for S3-compatible inboxes. | ||
| o.ChecksumAlgorithm = "" |
Contributor
There was a problem hiding this comment.
Suggested change
| o.ChecksumAlgorithm = "" | |
| o.ChecksumAlgorithm = types.ChecksumAlgorithm("") |
To ensure the field is typed correctly by using a explicit conversion or the empty string variant of the type:
Contributor
Author
There was a problem hiding this comment.
Done in eca7598, pushed. Agreed the explicit conversion reads better — the bare "" only compiles because Go assigns untyped string constants to named string types automatically, and that's not a rule anyone wants to think about mid-review. Added the types import too.
…tibility After #682 moved uploads to feature/s3/transfermanager, single-part PUTs started carrying x-amz-sdk-checksum-algorithm / x-amz-checksum-* / x-amz-trailer headers even though the s3 client is configured with RequestChecksumCalculationWhenRequired. transfermanager.New resolves an unset ChecksumAlgorithm to CRC32, after which its own value wins over the s3 client setting on PutObjectInput. Reset ChecksumAlgorithm to "" via a per-call optFn on UploadObject; the empty value also propagates to CreateMultipartUpload and UploadPart, so the opt-out covers both single- and multipart paths. New test wraps the gofakes3 mock to capture real PUT headers and asserts none of the flexible-checksum headers are sent.
Address review feedback from @nanjiangshu — assign the empty value through the named type rather than relying on untyped-string-to-named- type assignability. Functionally identical, just clearer at the call site.
jhagberg
force-pushed
the
fix/658-transfermanager-checksum-optout
branch
from
eca7598 to
f8dae2c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue(s) and PR(s)
Follow-up to #682 (the transfermanager migration for #658). Fixes a regression spotted in Codex review of that PR.
Description
After #682 moved uploads to
feature/s3/transfermanager, small (single-part) uploads suddenly carried the AWS flexible-checksum headers (x-amz-sdk-checksum-algorithm: CRC32,x-amz-checksum-crc32,x-amz-trailer) — even though the s3 client is built withRequestChecksumCalculationWhenRequired, which is supposed to suppress them.What's going on:
transfermanager.NewrunsresolveChecksumAlgorithmafter applying user options, so an unsetChecksumAlgorithmgets rewritten toCRC32at construction time. From then on the transfer manager's own value wins over the s3 client setting and ends up on thePutObjectInput. The old (now-deprecated) manager path didn't do this, so SDA-style inbox endpoints that don't accept flexible checksums started rejecting normal small uploads after the migration.The fix is a per-call optFn on
UploadObjectthat resetsChecksumAlgorithmback to"". Per-call optFns run on a copy of the client options and execute before the upload reads the field, so the empty value sticks and nox-amz-sdk-checksum-algorithm/x-amz-checksum-*/x-amz-trailerheaders go out. The multipart path reads the same field, so this also clears the headers fromCreateMultipartUploadandUploadPartrequests.How to test
go test ./upload/... -count=1. The new testTestUploadSinglePartDoesNotSendFlexibleChecksumHeaderswraps the gofakes3 mock with a handler that captures the real PUT request headers, then asserts the three flexible-checksum headers are absent.sda-cli upload <small-file>against an SDA inbox that was rejecting uploads after [upload,list] Migrate to feature/s3/transfermanager #682 landed, and confirm it now succeeds.