fix(agent): default S3 RequestChecksumCalculation to WhenRequired

[Andrey Kolkov (androndo)]

Summary

The snapshot-agent fails to upload etcd snapshots to non-AWS, S3-compatible
object stores (Ceph RADOS Gateway confirmed; likely some MinIO / Cloudflare R2
versions too). This makes the agent compute an S3 request checksum only when
the operation requires one, on both SDK layers the upload touches — the
s3.Client and the transfer manager — which restores uploads on those backends
while remaining fully compatible with real AWS S3.

Root cause

Since early 2025, aws-sdk-go-v2 changed the default RequestChecksumCalculation
from WhenRequired to WhenSupported. With WhenSupported the SDK attaches a
flexible checksum (CRC32 by default) to every upload. Over HTTPS a multipart part
rides this as x-amz-content-sha256: STREAMING-UNSIGNED-PAYLOAD-TRAILER +
x-amz-trailer: x-amz-checksum-crc32. AWS S3 accepts it; Ceph RGW rejects it:

InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD,
STREAMING-AWS4-HMAC-SHA256-PAYLOAD or a valid sha256 value.

The etcd snapshot itself is read fine (opened snapshot stream → completed snapshot read); only the S3 upload fails, leaving the EtcdSnapshot without
a stored object.

Crucially, the agent does not issue a plain PutObject for a real snapshot.
internal/agent/snapshot.go streams through manager.Uploader, which multiparts
any body over its 5 MiB part size — i.e. every real etcd snapshot. And
manager.Uploader carries its own RequestChecksumCalculation (hardcoded to
WhenSupported in NewUploader) that it never derives from the s3.Client
option. So setting the option on the client alone fixes only the degenerate
sub-5-MiB single-part case and leaves real backups broken. (Thanks to Timofei Larkin (@lllamnyp)
for catching this in review — the first revision of this PR had exactly that gap.)

Fix

Derive one package-level constant snapshotChecksumCalculation = WhenRequired
and apply it in both places that carry the knob:

• destination.s3Client (agent.go) — the client option, covering the
  single-part PutObject and HeadObject;
• newSnapshotUploader (snapshot.go) — the transfer manager option, covering
  the multipart parts.

WhenRequired computes a checksum only for operations that mandate one, so neither
a PutObject nor a multipart UploadPart carries a flexible checksum — accepted
by both AWS S3 and the affected backends. The response side (ResponseChecksumValidation)
is set symmetrically on the shared client for the restore/download path.

Impact / compatibility

• Non-AWS S3-compatible backends (Ceph RGW confirmed): snapshot uploads,
  including multipart, now succeed — the bug being fixed.
• AWS S3: no behavioral change; a PutObject/UploadPart does not require a
  checksum, so nothing is added.
• No API/CRD change, no new config value, no new dependency. The knob is a
  fixed internal default; the agent's env is a closed controller-built list, so no
  operator could set AWS_REQUEST_CHECKSUM_CALCULATION on the Pod anyway.

Integrity trade-off (stated plainly)

With WhenSupported the backend validated each part's CRC32 and would reject a
corrupted part. After this change there is no server-side verification of the
uploaded bytes. uploadStreamHashed does tee the body through sha256, but that
digest is over what the agent sent — never compared server-side, and not
re-checked on restore (restore.go sets SkipHashCheck, since a streamed
snapshot carries no etcd hash footer). Concretely, over a plain http:// path a
byte-flipping proxy would today produce BadDigest; after this it stores corrupt
and only surfaces at restore. The alternative is backups that don't work at all on
RGW, so this doesn't block the change — but a good follow-up is to verify the
recorded sha256 against the downloaded object in downloadS3, restoring an
end-to-end guard independent of any SDK checksum setting. (Follow-up, not folded
into this PR.)

Verification

• Multipart guard test TestUploadS3StreamMultipartNoChecksumTrailer
  (snapshot_test.go): drives the real uploadS3Stream against a TLS
  httptest server (trusted via AWS_CA_BUNDLE, so s3Client +
  newSnapshotUploader run exactly as in production) with a 12 MiB / three-part
  body, and asserts no request carries STREAMING-UNSIGNED-PAYLOAD-TRAILER,
  x-amz-trailer, or x-amz-sdk-checksum-algorithm. It fails on the pre-fix
  uploader (reproducing the RGW trailer trace) and passes with the fix.
• TestS3ClientChecksumWhenRequired kept as a client-option unit check only
  (now hermetic; also asserts the response-side setting).
• go build ./..., go vet ./..., go test ./... all green.

Note on the earlier "verified on RGW" claim: the manual canary that succeeded
uploaded an 86 KB object — under the 5 MiB part size, so it took the single-part
path where the client option does apply. That's consistent with the analysis
above, and is why it didn't expose the multipart gap. The multipart test is now
the authoritative guard.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved S3 snapshot upload compatibility by explicitly aligning S3 request checksum behavior across upload paths and avoiding unsupported checksum trailer headers.
  • Preserved server-provided response integrity validation for S3-compatible backends.

• Documentation

  • Refreshed snapshot concepts and troubleshooting notes, including guidance for the InvalidArgument: x-amz-content-sha256 error seen with some S3 backends and older agent images.

• Tests

  • Added deterministic checksum-request coverage for both single-part and multipart S3 snapshot uploads, asserting the absence of checksum-trailer-related headers.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e2a5c6c8-1d56-49ad-8b23-5fd31f84b2b4

📥 Commits

Reviewing files that changed from the base of the PR and between 562193d and f3338e3.

📒 Files selected for processing (4)

• internal/agent/agent.go
• internal/agent/agent_test.go
• internal/agent/snapshot.go
• internal/agent/snapshot_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/agent/snapshot.go
• internal/agent/snapshot_test.go
• internal/agent/agent_test.go

---

📝 Walkthrough

Walkthrough

The S3 client and snapshot transfer manager now share a required-only request checksum policy. Tests verify checksum headers for single-part and multipart uploads, while documentation describes direct streaming and troubleshooting for incompatible S3 backends.

Changes

S3 checksum handling

Layer / File(s)	Summary
Configure and test S3 client options internal/agent/agent.go, internal/agent/agent_test.go	The S3 client pins request checksum calculation to WhenRequired, preserves default response validation, and tests endpoint and path-style settings.
Apply checksum policy to snapshot uploads internal/agent/snapshot.go, internal/agent/snapshot_test.go	Snapshot uploads configure the transfer manager with the shared policy, and single-part/multipart tests verify checksum trailers are absent.
Document streaming and compatibility behavior docs/concepts.md, docs/operations.md	Documentation describes direct snapshot streaming and the related S3 compatibility error.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: lllamnyp

Sequence Diagram(s)

sequenceDiagram
  participant SnapshotAgent
  participant S3TransferManager
  participant S3Backend
  SnapshotAgent->>S3TransferManager: upload snapshot stream
  S3TransferManager->>S3Backend: send single-part or multipart requests
  S3Backend-->>S3TransferManager: return upload responses
  S3TransferManager-->>SnapshotAgent: return upload result

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the main change: defaulting S3 request checksum calculation to WhenRequired.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/agent-s3-checksum-when-required

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Code Review

This pull request configures the S3 client to only calculate request checksums when required (RequestChecksumCalculationWhenRequired). This prevents upload failures on S3-compatible backends (such as Ceph RGW, MinIO, and Cloudflare R2) that reject the default flexible checksums on PutObject operations. Additionally, a unit test has been added to verify this configuration. There are no review comments, and I have no further feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Timofei Larkin (lllamnyp)]

Thanks for chasing this down — the diagnosis of why S3-compatible backends reject the upload is right, and the root-cause writeup is genuinely useful. But the fix as written does not reach the code path that uploads real snapshots, so the reported failure survives this PR for any snapshot above 5 MiB. Details below.

Blocker 1 — the fix does not reach the code path that actually uploads snapshots

internal/agent/agent.go:180 sets RequestChecksumCalculation = WhenRequired on the S3 client options. But the snapshot upload never issues a bare client.PutObject for a realistically-sized snapshot — internal/agent/snapshot.go:240 goes through the transfer manager:

return uploadStreamHashed(ctx, manager.NewUploader(client), &s3.PutObjectInput{...}, body)

manager.Uploader carries its own RequestChecksumCalculation field, and NewUploader hardcodes it to WhenSupported — it never reads the client's options (feature/s3/manager@v1.22.22/upload.go:318):

u := &Uploader{
    S3:                         client,
    PartSize:                   DefaultUploadPartSize,
    ...
    RequestChecksumCalculation: aws.RequestChecksumCalculationWhenSupported,
}

For a body larger than DefaultUploadPartSize (5 MiB) the uploader takes the multipart branch, and initChecksumAlgorithm (upload.go:834-855) then stamps ChecksumAlgorithm = CRC32 onto CreateMultipartUpload and every UploadPart:

default:
    if u.cfg.RequestChecksumCalculation != aws.RequestChecksumCalculationWhenRequired {
        u.in.ChecksumAlgorithm = types.ChecksumAlgorithmCrc32
    }

An explicitly-set ChecksumAlgorithm input member then bypasses the client-level setting entirely — SetupInputContext.HandleInitialize (service/internal/checksum@v1.9.18/middleware_setup_context.go:51-55) returns early with the algorithm taken from the input and never consults RequestChecksumCalculation:

if m.GetAlgorithm != nil {
    if algorithm, ok := m.GetAlgorithm(in.Parameters); ok {
        ctx = internalcontext.SetChecksumInputAlgorithm(ctx, algorithm)
        return next.HandleInitialize(ctx, in)
    }
}

UploadPart is registered with EnableTrailingChecksum: true (service/s3@v1.102.2/api_op_UploadPart.go:688-695), so over HTTPS the compute-input-checksum middleware takes the trailer path and sets x-amz-content-sha256: STREAMING-UNSIGNED-PAYLOAD-TRAILER — exactly the header this PR quotes Ceph RGW rejecting.

Verified empirically on this branch. Building the client via destination.s3Client exactly as production does (i.e. with this PR's fix applied) and the uploader via manager.NewUploader(client) exactly as uploadS3Stream does, then uploading a 12 MiB body at a local TLS test server:

POST /b/k?uploads=                                   sha256=e3b0c442...                        trailer=                      algo=
PUT  /b/k?partNumber=1&uploadId=uid&x-id=UploadPart  sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER trailer=x-amz-checksum-crc32  algo=CRC32
PUT  /b/k?partNumber=2&uploadId=uid&x-id=UploadPart  sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER trailer=x-amz-checksum-crc32  algo=CRC32
PUT  /b/k?partNumber=3&uploadId=uid&x-id=UploadPart  sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER trailer=x-amz-checksum-crc32  algo=CRC32

With the uploader option added (below), the same run produces:

PUT  /b/k?partNumber=1&uploadId=uid&x-id=UploadPart  sha256=UNSIGNED-PAYLOAD  trailer=  algo=
PUT  /b/k?partNumber=2&uploadId=uid&x-id=UploadPart  sha256=UNSIGNED-PAYLOAD  trailer=  algo=
PUT  /b/k?partNumber=3&uploadId=uid&x-id=UploadPart  sha256=UNSIGNED-PAYLOAD  trailer=  algo=

Practical impact: a real etcd snapshot is essentially never under 5 MiB, so as merged this PR fixes only the degenerate tiny-snapshot case and leaves the reported failure fully intact for real clusters.

A note on the verification in the description: setting AWS_REQUEST_CHECKSUM_CALCULATION=when_required lands in aws.Config → s3.Options, and manager.NewUploader ignores that too. If the Ceph RGW run really did succeed end-to-end, something other than the checksum trailer explains it and the diagnosis is worth revisiting.

Fix — set the option on the uploader as well, in internal/agent/snapshot.go:240:

uploader := manager.NewUploader(client, func(u *manager.Uploader) {
    u.RequestChecksumCalculation = aws.RequestChecksumCalculationWhenRequired
})
return uploadStreamHashed(ctx, uploader, &s3.PutObjectInput{...}, body)

Keep the client-level setting too — it covers PutObject on the single-part path and HeadObject.

On the two settings. Needing to set the same intent in two independent places is a footgun: either one can be deleted later and the bug comes back silently, with the multipart case being the one that isn't obvious. Worth making that hard to get wrong — derive both from a single package-level constant at minimum, and have the comment state explicitly that the client option and the uploader option are separate knobs that must agree. (Unrelated but adjacent: manager is now deprecated in favour of feature/s3/transfermanager, so this duplication may be worth revisiting wholesale rather than patching twice.)

Test that fails without the fix. In internal/agent/snapshot_test.go, stand up an httptest.NewTLSServer answering ?uploads with an InitiateMultipartUploadResult, PUT ...uploadId= with 200 + ETag, and POST ...uploadId= with a CompleteMultipartUploadResult, recording each request's x-amz-content-sha256, x-amz-trailer and x-amz-sdk-checksum-algorithm. Point destination{kind:"s3", s3Endpoint: srv.URL, s3PathStyle:true} at it (overriding HTTPClient with srv.Client()), run uploadS3Stream with a >5 MiB reader, and assert no request carries x-amz-content-sha256: STREAMING-UNSIGNED-PAYLOAD-TRAILER, no x-amz-trailer, and no x-amz-sdk-checksum-algorithm. That test fails on the current branch and passes with the uploader option. Anything cheaper — asserting only on s3.Options — does not exercise the path that broke.

Blocker 2 — the code comment asserts something the code does not do, and that assertion is what hides Blocker 1

internal/agent/agent.go:178-179:

"WhenRequired keeps uploads working on those backends and is equally accepted by real AWS S3, which does not require a checksum on a plain PutObject."

The agent does not issue a plain PutObject for anything of realistic size — snapshot.go:228-245 hands the stream to manager.Uploader, which multiparts above 5 MiB. The comment, and the matching claim in the description that a plain PutObject is "all the agent ever issues", is wrong about the very operation the change is meant to fix. Please rewrite it to say that the agent uploads via the transfer manager, that the manager multiparts above 5 MiB, and that the manager carries its own RequestChecksumCalculation that must be set alongside the client option.

The Blocker 1 test doubles as the guard here — it pins the multipart behaviour the comment describes.

Blocker 3 — stale documentation in the exact area being changed

docs/concepts.md:452 describes the snapshot agent as streaming the snapshot "to a local file (PVC destination) or a temp file it then multipart-uploads to S3". The S3 path stages nothing: snapshot.go:159-167 streams straight to S3, and its comment says so explicitly — "Stream straight to S3 — no local staging. The snapshot Pod has no sized volume…". Staging-to-disk exists only on the restore side. Since this PR changes the S3 upload configuration, please correct the sentence describing that upload to something like "streams directly to S3 via the transfer manager (multipart above 5 MiB), with no local staging".

This is prose, so the guard is the Blocker 1 multipart test plus the existing TestUploadStreamHashedStreamsThrough in internal/agent/snapshot_test.go, which already proves no staging occurs.

Recommended to fix

• internal/agent/agent_test.go:39-63 — the new test asserts a setting the production upload path overrides. As written, TestS3ClientChecksumWhenRequired passes green while the reported bug is fully live. Keep it as a unit check on s3Client if you like, but it must not be the only checksum test in the PR; the multipart test from Blocker 1 is the one carrying the contract.
• internal/agent/agent_test.go:43 — non-hermetic unit test. d.s3Client(ctx) calls awsconfig.LoadDefaultConfig, which reads AWS_* env vars and ~/.aws/config; a runner with AWS_PROFILE pointing at a nonexistent profile fails this test for reasons unrelated to the assertion. Add t.Setenv("AWS_CONFIG_FILE", "/dev/null"), t.Setenv("AWS_SHARED_CREDENTIALS_FILE", "/dev/null") and t.Setenv("AWS_PROFILE", "").
• Hardcoding removes the escape hatch. s3.NewFromConfig copies aws.Config.RequestChecksumCalculation (populated by LoadDefaultConfig from AWS_REQUEST_CHECKSUM_CALCULATION / request_checksum_calculation) into s3.Options, and the new line unconditionally overwrites it. That is very likely the intent — the agent's environment is a closed list built by the controller, so no operator could set that var on the Pod anyway — but one sentence in the comment would save the next person the archaeology.
• internal/agent/restore.go:273 — consider the symmetric setting on the download side. manager.NewDownloader(client) issues ranged GetObjects with ChecksumMode: ENABLED (client default ResponseChecksumValidation = WhenSupported). Not the reported failure and usually harmless, but the same class of AWS-default-vs-S3-compatible mismatch, and untested. Either set o.ResponseChecksumValidation = aws.ResponseChecksumValidationWhenRequired next to the request-side setting with an httptest-based restore test asserting no x-amz-checksum-mode header, or say in the comment why it is deliberately left alone.

Note on the integrity trade-off

Worth stating plainly in the description, since the current text slightly oversells it. After this change there is no server-side verification of the uploaded bytes at all. uploadStreamHashed (snapshot.go:256) does tee the body through sha256, but that digest is computed over what the agent sent — it is never compared against anything server-side, and never re-verified on the way back, since restore.go:148 sets SkipHashCheck (deliberately, as a streamed snapshot carries no etcd hash footer). With WhenSupported the backend validated each part's CRC32 and would have rejected a corrupted part.

Concretely: with an endpoint configured as plain http:// behind a proxy that flips a byte, today you get BadDigest and a failed backup; after this change the object stores corrupt, the recorded sha256 matches the pre-corruption stream so status looks healthy, and it surfaces only at restore where the hash check is skipped. I don't think that blocks the change — the alternative is backups that don't work at all — but a good follow-up would be verifying the recorded sha256 against the downloaded object in downloadS3, which restores an end-to-end guard independent of any SDK checksum setting.

Title

The change is global; nothing distinguishes AWS from non-AWS endpoints. Suggest dropping "for non-AWS backends" from the title, or rewording to "…to unbreak non-AWS backends", so nobody goes looking for the conditional. Similarly, the comment names "some MinIO and Cloudflare R2 versions" while only Ceph RGW was confirmed — worth naming RGW as the verified case and softening the rest.

[Andrey Kolkov (androndo)]

Thanks — the diagnosis is exactly right, and I could reproduce the trailer trace. Pushed e1c9abd addressing all three blockers plus the recommended items.

Blocker 1 — fix now reaches the multipart path

manager.NewUploader hardcoding RequestChecksumCalculation: WhenSupported (upload.go:318) and initChecksumAlgorithm re-stamping CRC32 (upload.go:851) is confirmed. The client-only option indeed left every real (>5 MiB) snapshot broken.

• Both settings now derive from one package-level constant snapshotChecksumCalculation, set on both the client (s3Client) and the transfer manager via a new newSnapshotUploader helper — so the two knobs can't silently drift, and the multipart-is-the-non-obvious-one footgun is documented at the constant.
• Added TestUploadS3StreamMultipartNoChecksumTrailer (snapshot_test.go): it drives the real uploadS3Stream (not a hand-built client — the test trusts an httptest.NewTLSServer cert via AWS_CA_BUNDLE, so s3Client/newSnapshotUploader run exactly as in production) with a 12 MiB body across three parts, and asserts no request carries x-amz-content-sha256: STREAMING-UNSIGNED-PAYLOAD-TRAILER, x-amz-trailer, or x-amz-sdk-checksum-algorithm.

Confirmed it fails on the pre-fix uploader and passes with it — the failing run reproduces your trace verbatim:

snapshot_test.go:175: PUT carried x-amz-content-sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER — the checksum trailer Ceph RGW rejects
snapshot_test.go:178: PUT carried x-amz-trailer="x-amz-checksum-crc32", want none
snapshot_test.go:181: PUT carried x-amz-sdk-checksum-algorithm="CRC32", want none

Blocker 2 — comment corrected

The agent.go comment no longer claims a plain PutObject. The rationale now lives at snapshotChecksumCalculation and states plainly that the upload goes through the transfer manager, that it multiparts above 5 MiB, and that the manager carries its own RequestChecksumCalculation that must be set alongside the client option.

Blocker 3 — stale docs fixed

docs/concepts.md now reads "directly to S3 via the transfer manager — no local staging, multipart above the manager's 5 MiB part size" (dropped the incorrect "temp file it then multipart-uploads").

Recommended

• TestS3ClientChecksumWhenRequired kept as a client-option unit check only, with a doc-comment pointing at the multipart test as the real contract; made hermetic (AWS_CONFIG_FILE/AWS_SHARED_CREDENTIALS_FILE = /dev/null, empty AWS_PROFILE) and it now also asserts the response-side setting.
• Escape-hatch note added: the comment explains s3.NewFromConfig would copy AWS_REQUEST_CHECKSUM_CALCULATION but the agent's env is a closed controller-built list, so pinning is intentional.
• Download side (restore.go) — set ResponseChecksumValidation = WhenRequired on the shared s3Client. I went with "set it + explain" rather than a header test, because manager.Downloader sets no GetObjectInput.ChecksumMode in production (only its integ tests do), so there's no request-header difference to assert — a header test would pass regardless and guard nothing. The setting is defensive against the same mismatch class and is documented as such at both sites.

Integrity trade-off

Agreed and worth stating: after this there's no server-side verification of the uploaded bytes, and the recorded sha256 is over what the agent sent, never re-checked (restore uses SkipHashCheck). I'll file a follow-up to verify the recorded sha256 against the object in downloadS3, restoring an end-to-end guard independent of any SDK checksum setting. Not folding it into this PR to keep it to the checksum fix.

Title / verification note

Retitled to drop "for non-AWS backends" (the change is global). On the description's RGW claim: the manual canary that succeeded uploaded an 86 KB object — under the 5 MiB part size, so it took the single-part PutObject where the client option does apply; that's consistent with your analysis, not counter-evidence, and it's exactly why it didn't surface the multipart gap. The multipart unit test is now the authoritative guard. (I also re-ran the built image against SeaweedFS, but SeaweedFS accepts the trailer either way, so it doesn't distinguish fix from no-fix — RGW is the case that does.)

[Timofei Larkin (lllamnyp)]

Thanks for the rework — the request-side fix is now right, and the two-knob structure is exactly what was needed. Confirmed by mutation test rather than reading: deleting the manager.Uploader option makes TestUploadS3StreamMultipartNoChecksumTrailer fail with precisely the production symptom (x-amz-content-sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER, x-amz-trailer=x-amz-checksum-crc32, x-amz-sdk-checksum-algorithm=CRC32 on every UploadPart). go test ./internal/agent/, go vet, gofmt -l all clean. The single constant feeding both sites is the right shape, docs/concepts.md:452 is now accurate against snapshot.go:159-168, and the "closed env list" claim at agent.go:199-203 holds — controllers/snapshot_job.go:145-159, controllers/etcdmember_controller.go:866-880 and internal/migrate/snapshotjob.go:118-129 all build Env from fixed lists with no EnvFrom.

Three things still block, all in the new response-side rider and its coverage.

Blocking

1. ResponseChecksumValidation = WhenRequired is unjustified, mis-explained and untested — internal/agent/agent.go:205-209, internal/agent/restore.go:273-277.

The comments justify it as avoiding "demanding a response checksum the backend may not emit". The SDK default does not do that. Three source facts, each checked against the pinned versions:

• service/internal/checksum@v1.9.18/middleware_setup_context.go:95 — the default WhenSupported only sets ChecksumMode=ENABLED on the request. It demands nothing.
• middleware_validate_output.go:101-111 — if the response carries no checksum header, validation is skipped and the middleware returns cleanly: if len(expectedChecksum) == 0 || len(algorithmToUse) == 0 { … return out, metadata, nil }.
• service/s3@v1.102.2/api_op_GetObject.go:861 — GetObject is registered with IgnoreMultipartValidation: true, so composite checksums (the …-N form every multipart-uploaded object carries, i.e. every snapshot this agent writes) are skipped rather than rejected.

So the failure mode the comment describes cannot occur, and the line isn't needed to fix the reported bug. What it actually does is stop validating checksums that are returned — and the restore path has no other integrity check to fall back on: downloadS3 (restore.go:258-283) never hashes what it receives, nothing compares status.artifact.checksum, and the restore itself runs with SkipHashCheck (restore.go:148, and documented at docs/concepts.md:457). With the request-side checksum also off, a corrupted object now has no detection point anywhere in the round trip.

Either fix works:

• Drop the line and both comments, keeping the PR scoped to the observed request-side bug. Test: assert o.ResponseChecksumValidation == aws.ResponseChecksumValidationWhenSupported in agent_test.go:57-70 — that assertion fails today, which is what makes it a guard.
• Or keep it and add real verification in its place: tee downloadS3 through sha256 and compare against the recorded digest (plumbed alongside the other SNAPSHOT_* env vars at etcdmember_controller.go:866-880). Test: TestDownloadS3ChecksumMismatch against an httptest TLS server serving a body whose digest differs from the expected one, asserting both an error and removal of the staged file.

The first is the smaller change and I'd suggest it; the second is more valuable if you want it.

2. The single-part upload path has no end-to-end guard — agent.go:204, snapshot_test.go:89-186.

TestUploadS3StreamMultipartNoChecksumTrailer uses a 12 MiB body and so drives only the multipart branch. Below the 5 MiB part size the transfer manager issues a single-part PutObject, where the client option is the only thing keeping the trailer off — and that option is currently pinned by a struct-field comparison alone (agent_test.go:66). That is the same class of check that stayed green through 61144d4 while the real path was broken.

This is not hypothetical: a snapshot of a small or freshly-bootstrapped cluster is tens of KiB and takes the single-part path in production, against the same RGW endpoint. I probed the current branch with a throwaway 1 MiB upload and the behaviour is correct today — PUT /b/k?x-id=PutObject sha=UNSIGNED-PAYLOAD trailer= algo= — but nothing pins it.

Fix: add TestUploadS3StreamSinglePartNoChecksumTrailer reusing the same handler and CA setup with a ~1 MiB body, asserting no X-Amz-Trailer, no X-Amz-Sdk-Checksum-Algorithm, and X-Amz-Content-Sha256 != STREAMING-UNSIGNED-PAYLOAD-TRAILER, plus a guard that exactly one PUT was seen carrying no partNumber (so the test fails loudly if the body ever stops taking the single-part branch). Removing the client option must make it fail.

3. Comments overstate which path is affected — agent.go:161-164, snapshot.go:250-253.

Both say multipart is taken "for every real etcd snapshot" and call it "the one that matters". Small clusters take the single-part path, where the client option is load-bearing. A reader who trusts this wording is one cleanup away from deleting the client option and reintroducing the RGW failure for exactly the clusters most likely to be someone's first try. Suggest "any body over the 5-MiB part size — every snapshot of a non-trivial cluster", and dropping "the one that matters". Issue 2's test is what makes the corrected statement enforceable.

Recommended to fix

• agent.go:151,176 — snapshotChecksumCalculation also governs the restore/download client (restore.go:241,261), so the snapshot prefix is misleading. Suggest s3RequestChecksumCalculation.
• docs/operations.md:269 — worth a sibling troubleshooting line next to the existing ListBucket/403 note, so an operator hitting InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD… finds the answer in the docs rather than only in a Go comment.
• snapshot_test.go:113,118 — io.WriteString(w, …) returns are dropped while _, _ = io.Copy(…) two lines above is explicit; pick one convention.

Clean

No leaked secrets — test credentials are the literal test against loopback. hermeticAWSEnv properly isolates ~/.aws, AWS_PROFILE and the metadata endpoint. All four S3 construction sites in the repo (snapshot.go:229, restore.go:241,261,278) route through destination.s3Client, so no path is left unpatched.

[Andrey Kolkov (androndo)]

Thanks — all three confirmed against the pinned SDK sources and addressed in 562193d.

Blocker 1 — response-side pin dropped

You're right on every point. middleware_validate_output.go:102-109 returns cleanly when no checksum header is present, and api_op_GetObject.go:861 registers IgnoreMultipartValidation: true, so the composite (-N) checksum every multipart-uploaded snapshot carries is skipped regardless. The default WhenSupported demands nothing, so the pin fixed no real failure and only discarded the one server-side integrity signal the restore path has (downloadS3 runs with SkipHashCheck).

Went with the smaller change: dropped o.ResponseChecksumValidation = WhenRequired and both comments, scoping the PR to the observed request-side bug. restore.go is back to no diff. TestS3ClientChecksumWhenRequired now asserts ResponseChecksumValidation == WhenSupported (the SDK default) — mutation-checked: re-pinning it to WhenRequired fails the test with ResponseChecksumValidation = 2, want the SDK default WhenSupported (1).

Blocker 2 — single-part path now guarded end-to-end

Added TestUploadS3StreamSinglePartNoChecksumTrailer: 1 MiB body (single-part PutObject branch), same shared TLS capture server, asserting no X-Amz-Trailer / X-Amz-Sdk-Checksum-Algorithm, X-Amz-Content-Sha256 != STREAMING-UNSIGNED-PAYLOAD-TRAILER, and exactly one PUT with no partNumber (fails loudly if the body ever stops taking the single-part branch). Mutation-checked: deleting the client-side option makes it fail with PUT carried x-amz-content-sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER / x-amz-trailer="x-amz-checksum-crc32". The multipart test's part-detection also switched to partNumber != "" so the two branches can't be confused.

Blocker 3 — comments corrected

Both sites now read "any body over the 5-MiB part size — every snapshot of a non-trivial cluster", and "the one that matters" is gone. The constant comment now states plainly that both sites are load-bearing — the client option for the single-part path + HeadObject, the uploader option for multipart parts.

Recommended (all done)

• Renamed snapshotChecksumCalculation → s3RequestChecksumCalculation (it governs the restore/download client via the shared s3Client too).
• Added an operations.md troubleshooting note next to the ListBucket/403 one for the InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD… error.
• Factored the fake S3 endpoint into a shared startS3CaptureServer helper (used by both trailer tests) and made the two io.WriteString calls explicit-return like the adjacent io.Copy.

gofmt -l, go vet ./..., go build ./..., go test ./... all clean.

[coderabbitai]

🧹 Nitpick comments (1)

internal/agent/snapshot.go (1)

255-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Consider migrating this helper to feature/s3/transfermanager. The newer package isn’t in go.mod yet, so this would require adding a dependency as well as updating the uploader helper.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/agent/snapshot.go` around lines 255 - 259, Update
newSnapshotUploader to use the newer feature/s3/transfermanager package, add its
dependency to go.mod, and preserve the existing request checksum configuration
when constructing the uploader.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/agent/snapshot.go`:
- Around line 255-259: Update newSnapshotUploader to use the newer
feature/s3/transfermanager package, add its dependency to go.mod, and preserve
the existing request checksum configuration when constructing the uploader.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1bb849ce-832b-4a34-a8fb-2297f635c667

📥 Commits

Reviewing files that changed from the base of the PR and between e1c9abd and 562193d.

📒 Files selected for processing (6)

• docs/concepts.md
• docs/operations.md
• internal/agent/agent.go
• internal/agent/agent_test.go
• internal/agent/snapshot.go
• internal/agent/snapshot_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• docs/concepts.md
• internal/agent/agent_test.go

[Timofei Larkin (lllamnyp)]

All three rounds of findings are addressed and verified.

The request-checksum pin is correct at both load-bearing sites — the s3.Client option for the single-part PutObject/HeadObject path, and manager.Uploader.RequestChecksumCalculation for the multipart parts, which NewUploader hardcodes to WhenSupported and never reads from the client. Both derive from one constant, so they cannot drift.

Both guards are mutation-tested rather than assumed: dropping the client option fails TestUploadS3StreamSinglePartNoChecksumTrailer, dropping the uploader option fails TestUploadS3StreamMultipartNoChecksumTrailer, each with the exact production symptom (x-amz-content-sha256: STREAMING-UNSIGNED-PAYLOAD-TRAILER, x-amz-trailer: x-amz-checksum-crc32). ResponseChecksumValidation is left at the SDK default, with a test asserting it stays there.

Thanks for the thorough iteration on this one.