From 61144d4720048e5893037fdadafe95b342a7e56b Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Mon, 20 Jul 2026 23:48:32 +0400 Subject: [PATCH 1/4] fix(agent): default S3 RequestChecksumCalculation to WhenRequired aws-sdk-go-v2 defaults RequestChecksumCalculation to WhenSupported (since early 2025), attaching a flexible checksum to every PutObject. Non-AWS S3-compatible backends (Ceph RGW, some MinIO/R2) reject the accompanying header with "InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD, STREAMING-AWS4-HMAC-SHA256-PAYLOAD or a valid sha256 value", so the snapshot-agent reads the etcd snapshot fine but fails at S3 upload. Set the S3 client to WhenRequired so a checksum is only added when the operation requires one; a plain PutObject then carries none, which both AWS S3 and the affected backends accept. No API/CRD change and no new knob. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Andrey Kolkov --- internal/agent/agent.go | 12 ++++++++++++ internal/agent/agent_test.go | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index c15d678b..2ca6cf77 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -166,6 +166,18 @@ func (d destination) s3Client(ctx context.Context) (*s3.Client, error) { o.BaseEndpoint = aws.String(d.s3Endpoint) } o.UsePathStyle = d.s3PathStyle + // Only add a request checksum when the operation actually requires one. + // + // Since early 2025, aws-sdk-go-v2 defaults RequestChecksumCalculation to + // WhenSupported, which attaches a CRC32 to every PutObject. Many + // S3-compatible backends that are not AWS S3 (Ceph RGW, some MinIO and + // Cloudflare R2 versions, etc.) reject the accompanying header with + // "InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD, + // STREAMING-AWS4-HMAC-SHA256-PAYLOAD or a valid sha256 value", so the + // snapshot upload fails against them. 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. + o.RequestChecksumCalculation = aws.RequestChecksumCalculationWhenRequired }), nil } diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 3b391146..1e99abfd 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -10,7 +10,12 @@ You may obtain a copy of the License at package agent -import "testing" +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" +) func TestObjectKey(t *testing.T) { cases := []struct { @@ -31,6 +36,32 @@ func TestObjectKey(t *testing.T) { } } +func TestS3ClientChecksumWhenRequired(t *testing.T) { + d := destination{ + kind: "s3", + s3Endpoint: "https://s3.example.internal", + s3PathStyle: true, + } + c, err := d.s3Client(context.Background()) + if err != nil { + t.Fatalf("s3Client: %v", err) + } + o := c.Options() + // Non-AWS S3-compatible backends (Ceph RGW, some MinIO/R2) reject the + // aws-sdk-go-v2 default (WhenSupported) flexible checksum on PutObject, so + // the agent must only add a checksum when the operation requires one. + if o.RequestChecksumCalculation != aws.RequestChecksumCalculationWhenRequired { + t.Errorf("RequestChecksumCalculation = %v, want WhenRequired (%v)", + o.RequestChecksumCalculation, aws.RequestChecksumCalculationWhenRequired) + } + if !o.UsePathStyle { + t.Error("UsePathStyle = false, want true") + } + if o.BaseEndpoint == nil || *o.BaseEndpoint != d.s3Endpoint { + t.Errorf("BaseEndpoint = %v, want %q", o.BaseEndpoint, d.s3Endpoint) + } +} + func TestURI(t *testing.T) { s3 := destination{kind: "s3", s3Bucket: "bucket", s3Key: "pre"} if got, want := s3.uri("snap"), "s3://bucket/pre/snap.db"; got != want { From a8bc4ed37e992d427cacc79b0f756e68f8af2c50 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 21 Jul 2026 11:09:35 +0400 Subject: [PATCH 2/4] fix(agent): set WhenRequired on the transfer manager too, not just the client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #342 found the client-only option never reached the code path that uploads real snapshots. manager.Uploader carries its OWN RequestChecksumCalculation (NewUploader hardcodes WhenSupported) and never reads the s3.Client option, so any snapshot over the manager's 5 MiB part size — every real etcd snapshot — took the multipart branch and re-stamped the CRC32 STREAMING-UNSIGNED-PAYLOAD-TRAILER that Ceph RGW rejects. The previous fix only covered the degenerate sub-5-MiB single-part case. - Derive both knobs from one package-level constant (snapshotChecksumCalculation) and set it on the uploader via newSnapshotUploader as well as the client, so the two independent settings cannot silently drift. - Add TestUploadS3StreamMultipartNoChecksumTrailer: drives the real uploadS3Stream against a TLS httptest server (trusted via AWS_CA_BUNDLE) with a >5 MiB body and asserts no UploadPart carries a checksum trailer/algorithm. Fails without the uploader option; a check on s3.Options alone stays green while the bug is live. - Make TestS3ClientChecksumWhenRequired hermetic (AWS_CONFIG_FILE/… = /dev/null, empty AWS_PROFILE) and assert the response-side setting; keep it as a client-option unit check only. - Set ResponseChecksumValidation=WhenRequired symmetrically for the restore download path (same mismatch class; defensive, the downloader sets no ChecksumMode today). - Rewrite the agent.go comment that wrongly claimed a plain PutObject, and fix the stale "temp file it then multipart-uploads" sentence in docs/concepts.md. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Andrey Kolkov --- docs/concepts.md | 2 +- internal/agent/agent.go | 55 +++++++++++++---- internal/agent/agent_test.go | 32 +++++++++- internal/agent/restore.go | 5 ++ internal/agent/snapshot.go | 16 ++++- internal/agent/snapshot_test.go | 105 ++++++++++++++++++++++++++++++++ 6 files changed, 198 insertions(+), 17 deletions(-) diff --git a/docs/concepts.md b/docs/concepts.md index a39c983e..6cd535ba 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -449,7 +449,7 @@ Two surfaces, one agent. The operator image doubles as a snapshot agent: `main.g An `EtcdSnapshot` is a one-shot record. The controller resolves `spec.clusterRef`, builds a Job (owned by the `EtcdSnapshot`, with `ttlSecondsAfterFinished` so it self-GCs, `automountServiceAccountToken: false`, and a restricted security context), and tracks `status.phase` through `Pending → Started → Complete | Failed`. The Job's Pod runs the snapshot agent, which: 1. dials the cluster's client Service — TLS material (server CA + operator-client cert) is mounted from the same Secrets the operator dials with, and when `status.authEnabled` is latched the agent authenticates as `root` using the password from `spec.auth.rootCredentialsSecretRef`; -2. streams `clientv3` `Maintenance.Snapshot` to a local file (PVC destination) or a temp file it then multipart-uploads to S3, hashing (sha256) and counting bytes as it goes; +2. streams `clientv3` `Maintenance.Snapshot` to a local file (PVC destination) or directly to S3 via the transfer manager — no local staging, multipart above the manager's 5 MiB part size — hashing (sha256) and counting bytes as it goes; 3. prints a marker line — `snapshot uploaded: uri="..." size=N sha256=` — that the controller scans out of the Pod log (via an uncached `APIReader` to find the Pod and the typed Clientset to read its log) to populate `status.artifact{uri,sizeBytes,checksum}`. Why parse a log line rather than have the agent write status? The agent has no Kubernetes API access by design (`automountServiceAccountToken: false`), so the controller — which does — is the one that records the result. The marker is the agent→controller channel. Terminal phases are sticky: a `Complete`/`Failed` snapshot is a historical record and never re-runs. diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 2ca6cf77..96f673f3 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -148,10 +148,37 @@ func (d destination) localPath(name string) string { return base + "/" + name + ".db" } +// snapshotChecksumCalculation is the S3 request-checksum policy the agent applies +// on the upload path. It has to be set in TWO independent places, because the +// snapshot upload touches two SDK layers that each carry their own copy of this +// knob and do NOT share it: +// +// - the s3.Client option (s3Client, below) — covers the single-part PutObject +// the transfer manager issues for a sub-5-MiB body, plus HeadObject; +// - the transfer manager's own Uploader.RequestChecksumCalculation +// (newSnapshotUploader in snapshot.go) — covers the multipart parts the +// manager issues for a body over its 5-MiB part size, which is every real +// etcd snapshot. manager.NewUploader hardcodes this field to WhenSupported +// and never reads the client option, so setting only the client option +// leaves the multipart path — the one that matters — broken. +// +// The two are separate knobs that MUST agree; deriving both from this single +// constant is what keeps them from silently drifting (delete either site and the +// failure returns — for the multipart case, invisibly). +// +// Why WhenRequired: since early 2025 aws-sdk-go-v2 defaults to WhenSupported, +// which stamps a CRC32 on every PutObject/UploadPart — over HTTPS that rides as a +// STREAMING-UNSIGNED-PAYLOAD-TRAILER. Ceph RGW (the confirmed case) rejects the +// accompanying header with "InvalidArgument: x-amz-content-sha256 must be +// UNSIGNED-PAYLOAD, STREAMING-AWS4-HMAC-SHA256-PAYLOAD or a valid sha256 value". +// WhenRequired attaches a checksum only when the operation mandates one (none for +// a plain PutObject or UploadPart) and is equally accepted by real AWS S3. +const snapshotChecksumCalculation = aws.RequestChecksumCalculationWhenRequired + // s3Client builds an S3 client honoring a custom endpoint, region and // path-style addressing. Credentials come from AWS_ACCESS_KEY_ID / // AWS_SECRET_ACCESS_KEY in the environment (loaded by the default config -// chain). +// chain). Used by BOTH the upload (snapshot) and download (restore) paths. func (d destination) s3Client(ctx context.Context) (*s3.Client, error) { region := d.s3Region if region == "" { @@ -166,18 +193,20 @@ func (d destination) s3Client(ctx context.Context) (*s3.Client, error) { o.BaseEndpoint = aws.String(d.s3Endpoint) } o.UsePathStyle = d.s3PathStyle - // Only add a request checksum when the operation actually requires one. - // - // Since early 2025, aws-sdk-go-v2 defaults RequestChecksumCalculation to - // WhenSupported, which attaches a CRC32 to every PutObject. Many - // S3-compatible backends that are not AWS S3 (Ceph RGW, some MinIO and - // Cloudflare R2 versions, etc.) reject the accompanying header with - // "InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD, - // STREAMING-AWS4-HMAC-SHA256-PAYLOAD or a valid sha256 value", so the - // snapshot upload fails against them. 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. - o.RequestChecksumCalculation = aws.RequestChecksumCalculationWhenRequired + // Request-checksum policy for this client. This is only ONE of the two + // sites that need it — the transfer manager carries its own copy for the + // multipart path (see snapshotChecksumCalculation and newSnapshotUploader). + // LoadDefaultConfig would otherwise populate this from + // AWS_REQUEST_CHECKSUM_CALCULATION, but the agent's environment is a closed + // list built by the controller (no operator can set that var on the Pod), + // so we pin it here unconditionally rather than leave an escape hatch that + // nothing can reach. + o.RequestChecksumCalculation = snapshotChecksumCalculation + // Symmetric response-side setting for the restore/download path (this + // client is shared with downloadS3): don't demand a response checksum the + // backend may not emit — the same AWS-default-vs-S3-compatible mismatch + // class. Harmless on the upload path, which issues no checksum-bearing GET. + o.ResponseChecksumValidation = aws.ResponseChecksumValidationWhenRequired }), nil } diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 1e99abfd..8ee71ec3 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -36,7 +36,30 @@ func TestObjectKey(t *testing.T) { } } +// hermeticAWSEnv isolates the aws-sdk-go-v2 default config chain from the host: +// LoadDefaultConfig otherwise reads ~/.aws/config and AWS_PROFILE, so a runner +// whose AWS_PROFILE points at a missing profile would fail these tests for +// reasons unrelated to the assertion. It also pins static credentials and a +// region so signing never reaches the EC2/ECS metadata endpoint. +func hermeticAWSEnv(t *testing.T) { + t.Helper() + t.Setenv("AWS_CONFIG_FILE", "/dev/null") + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", "/dev/null") + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_ACCESS_KEY_ID", "test") + t.Setenv("AWS_SECRET_ACCESS_KEY", "test") + t.Setenv("AWS_REGION", "us-east-1") +} + +// TestS3ClientChecksumWhenRequired is a unit check on the s3.Client OPTION only. +// It does NOT exercise the path that actually broke: the transfer manager carries +// its own RequestChecksumCalculation and overrides this on the multipart upload. +// The end-to-end contract — that a real (>5 MiB) multipart snapshot upload emits +// no checksum trailer — lives in TestUploadS3StreamMultipartNoChecksumTrailer +// (snapshot_test.go). Keep both: this pins the client/HeadObject/single-part +// knob, that one pins multipart. func TestS3ClientChecksumWhenRequired(t *testing.T) { + hermeticAWSEnv(t) d := destination{ kind: "s3", s3Endpoint: "https://s3.example.internal", @@ -48,12 +71,17 @@ func TestS3ClientChecksumWhenRequired(t *testing.T) { } o := c.Options() // Non-AWS S3-compatible backends (Ceph RGW, some MinIO/R2) reject the - // aws-sdk-go-v2 default (WhenSupported) flexible checksum on PutObject, so - // the agent must only add a checksum when the operation requires one. + // aws-sdk-go-v2 default (WhenSupported) flexible checksum, so the agent must + // only add a checksum when the operation requires one. if o.RequestChecksumCalculation != aws.RequestChecksumCalculationWhenRequired { t.Errorf("RequestChecksumCalculation = %v, want WhenRequired (%v)", o.RequestChecksumCalculation, aws.RequestChecksumCalculationWhenRequired) } + // Symmetric response-side setting covering the restore/download path. + if o.ResponseChecksumValidation != aws.ResponseChecksumValidationWhenRequired { + t.Errorf("ResponseChecksumValidation = %v, want WhenRequired (%v)", + o.ResponseChecksumValidation, aws.ResponseChecksumValidationWhenRequired) + } if !o.UsePathStyle { t.Error("UsePathStyle = false, want true") } diff --git a/internal/agent/restore.go b/internal/agent/restore.go index 93da58ab..45e46089 100644 --- a/internal/agent/restore.go +++ b/internal/agent/restore.go @@ -270,6 +270,11 @@ func downloadS3(ctx context.Context, src destination, stageDir string) (string, return "", err } defer f.Close() + // client is built by s3Client, which pins ResponseChecksumValidation to + // WhenRequired — so the downloader doesn't demand a response checksum an + // S3-compatible backend may not return (symmetric to the upload-side request + // checksum). manager.Downloader has no response-validation knob of its own, so + // the client option is the only site for this. downloader := manager.NewDownloader(client) if _, err := downloader.Download(ctx, f, &s3.GetObjectInput{ Bucket: aws.String(src.s3Bucket), diff --git a/internal/agent/snapshot.go b/internal/agent/snapshot.go index 5a8b82ca..6fde0e79 100644 --- a/internal/agent/snapshot.go +++ b/internal/agent/snapshot.go @@ -237,13 +237,27 @@ func uploadS3Stream(ctx context.Context, dest destination, key string, body io.R if uid != "" { metadata = map[string]string{snapshotUIDMetaKey: uid} // stamp ownership so a retry recognizes its own object } - return uploadStreamHashed(ctx, manager.NewUploader(client), &s3.PutObjectInput{ + return uploadStreamHashed(ctx, newSnapshotUploader(client), &s3.PutObjectInput{ Bucket: aws.String(dest.s3Bucket), Key: aws.String(key), Metadata: metadata, }, body) } +// newSnapshotUploader builds the S3 transfer manager for snapshot uploads. +// +// manager.Uploader carries its OWN RequestChecksumCalculation (NewUploader +// hardcodes it to WhenSupported) and never consults the s3.Client option, so the +// multipart path — taken for every body over the 5-MiB part size, i.e. every real +// etcd snapshot — would re-stamp the CRC32 trailer that the client option was set +// to avoid. Pin it here too, from the same constant s3Client uses, so the two +// knobs cannot drift. See snapshotChecksumCalculation. +func newSnapshotUploader(client *s3.Client) *manager.Uploader { + return manager.NewUploader(client, func(u *manager.Uploader) { + u.RequestChecksumCalculation = snapshotChecksumCalculation + }) +} + // s3Uploader abstracts manager.Uploader.Upload so uploadStreamHashed is testable // without a live S3 endpoint. type s3Uploader interface { diff --git a/internal/agent/snapshot_test.go b/internal/agent/snapshot_test.go index 390e8749..125980db 100644 --- a/internal/agent/snapshot_test.go +++ b/internal/agent/snapshot_test.go @@ -11,15 +11,20 @@ You may obtain a copy of the License at package agent import ( + "bytes" "context" "crypto/sha256" "encoding/hex" + "encoding/pem" "errors" "fmt" "io" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" + "sync" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -81,6 +86,106 @@ func TestUploadStreamHashed_UploadError(t *testing.T) { } } +// TestUploadS3StreamMultipartNoChecksumTrailer is the guard for the actual +// failure mode. A real etcd snapshot exceeds the transfer manager's 5 MiB part +// size, so the upload takes the MULTIPART branch, where manager.Uploader's own +// RequestChecksumCalculation (NOT the s3.Client option) decides whether a CRC32 +// trailer is stamped on each UploadPart. Without newSnapshotUploader pinning it +// to WhenRequired, every part rides `x-amz-content-sha256: +// STREAMING-UNSIGNED-PAYLOAD-TRAILER` + `x-amz-trailer: x-amz-checksum-crc32` — +// exactly the header Ceph RGW rejects. +// +// This drives the production uploadS3Stream against a TLS test server (trusted via +// AWS_CA_BUNDLE, which LoadDefaultConfig wires into the real client, so s3Client +// and newSnapshotUploader run exactly as in production) with a >5 MiB body, and +// asserts no request carries a checksum trailer or algorithm. It fails if the +// uploader option is dropped; a check on s3.Options alone (see agent_test.go) +// would stay green while this real path stayed broken. +func TestUploadS3StreamMultipartNoChecksumTrailer(t *testing.T) { + hermeticAWSEnv(t) + + type capturedReq struct{ method, sha, trailer, algo string } + var mu sync.Mutex + var reqs []capturedReq + + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.Copy(io.Discard, r.Body) + mu.Lock() + reqs = append(reqs, capturedReq{ + method: r.Method, + sha: r.Header.Get("X-Amz-Content-Sha256"), + trailer: r.Header.Get("X-Amz-Trailer"), + algo: r.Header.Get("X-Amz-Sdk-Checksum-Algorithm"), + }) + mu.Unlock() + + q := r.URL.Query() + switch { + case r.Method == http.MethodHead: + // ensureObjectAbsent's HeadObject: report absent so the upload proceeds. + w.WriteHeader(http.StatusNotFound) + case r.Method == http.MethodPost && q.Has("uploads"): + io.WriteString(w, ``+ + `bk`+ + `test-upload-id`) + case r.Method == http.MethodPut && q.Has("partNumber"): + w.Header().Set("ETag", `"etag-`+q.Get("partNumber")+`"`) + w.WriteHeader(http.StatusOK) + case r.Method == http.MethodPost && q.Get("uploadId") != "": + io.WriteString(w, ``+ + `bk`+ + `"final-etag"`) + default: + w.WriteHeader(http.StatusOK) + } + })) + defer srv.Close() + + // Trust the test server's self-signed cert through the SDK's default config + // chain, so uploadS3Stream's own s3Client reaches it — no hand-built client. + caFile := filepath.Join(t.TempDir(), "ca.pem") + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw}) + if err := os.WriteFile(caFile, certPEM, 0o600); err != nil { + t.Fatalf("write CA bundle: %v", err) + } + t.Setenv("AWS_CA_BUNDLE", caFile) + + const bodySize = 12 << 20 // 12 MiB > 5 MiB part size ⇒ three parts ⇒ multipart branch + dest := destination{kind: "s3", s3Endpoint: srv.URL, s3Bucket: "b", s3PathStyle: true} + + size, sum, err := uploadS3Stream(context.Background(), dest, "k", bytes.NewReader(make([]byte, bodySize)), "test-uid") + if err != nil { + t.Fatalf("uploadS3Stream: %v", err) + } + if size != bodySize { + t.Errorf("streamed size = %d, want %d", size, bodySize) + } + if sum == "" { + t.Error("empty sha256 digest") + } + + mu.Lock() + defer mu.Unlock() + var sawUploadPart bool + for _, rq := range reqs { + if rq.method == http.MethodPut { + sawUploadPart = true + } + if rq.sha == "STREAMING-UNSIGNED-PAYLOAD-TRAILER" { + t.Errorf("%s carried x-amz-content-sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER — the checksum trailer Ceph RGW rejects", rq.method) + } + if rq.trailer != "" { + t.Errorf("%s carried x-amz-trailer=%q, want none", rq.method, rq.trailer) + } + if rq.algo != "" { + t.Errorf("%s carried x-amz-sdk-checksum-algorithm=%q, want none", rq.method, rq.algo) + } + } + if !sawUploadPart { + t.Fatal("no UploadPart (PUT) request seen — the upload did not take the multipart branch, so this test is not exercising the path it guards") + } +} + type fakeHead struct { out *s3.HeadObjectOutput err error From 562193dd74e3f86f15a3213bcc949b3db79ee6c3 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 21 Jul 2026 16:00:59 +0400 Subject: [PATCH 3/4] fix(agent): drop unjustified response-checksum pin, guard single-part path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three blockers from the second review, all on the current HEAD: - ResponseChecksumValidation = WhenRequired was unjustified and untested. The SDK default (WhenSupported) demands nothing: it validates a checksum only when the backend returns one (middleware_validate_output.go returns cleanly when absent) and GetObject registers IgnoreMultipartValidation, so the composite checksum every multipart-uploaded snapshot carries is skipped anyway. Pinning it to WhenRequired fixed no real failure and discarded the only server-side integrity signal the restore path has (downloadS3 runs with SkipHashCheck). Dropped the line and both comments; the client-option test now asserts the response side stays at the WhenSupported default, so re-pinning it fails. - The single-part upload path had no end-to-end guard. A small or freshly bootstrapped cluster's snapshot is under the 5 MiB part size, so the transfer manager issues a single-part PutObject where the s3.Client option is the only thing keeping the trailer off — previously pinned by a struct-field check alone, the same class of assertion that stayed green in 61144d4 while every real snapshot was broken. Added TestUploadS3StreamSinglePartNoChecksumTrailer (1 MiB body) asserting no checksum trailer and exactly one single-part PUT; removing the client option makes it fail. - Comments overstated multipart as "every real etcd snapshot" / "the one that matters", nudging a future maintainer to delete the load-bearing client option. Reworded to "any body over the 5 MiB part size — every snapshot of a non-trivial cluster" and dropped "the one that matters". Also renamed snapshotChecksumCalculation -> s3RequestChecksumCalculation (it governs the restore/download client too), added an operations.md troubleshooting note for the InvalidArgument x-amz-content-sha256 error, and factored the fake S3 endpoint into a shared test helper. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Andrey Kolkov --- docs/operations.md | 2 + internal/agent/agent.go | 46 +++++----- internal/agent/agent_test.go | 13 ++- internal/agent/restore.go | 5 -- internal/agent/snapshot.go | 10 +-- internal/agent/snapshot_test.go | 153 +++++++++++++++++++++++--------- 6 files changed, 153 insertions(+), 76 deletions(-) diff --git a/docs/operations.md b/docs/operations.md index d616696f..48649db8 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -267,6 +267,8 @@ For a PVC destination use `destination.pvc.{claimName,subPath}` instead of `s3`. > **Object names must be unique.** The stored object is keyed by the `EtcdSnapshot` *name* (`/.db` for S3, `.db` on a PVC). The agent **refuses to overwrite an existing snapshot it did not write** — for **both** S3 and PVC destinations — so a snapshot whose key/path already exists fails rather than silently clobbering an earlier snapshot. Give each snapshot a distinct name (the date-stamped CronJob pattern above does this) or a distinct destination `key`/`subPath`. (A *retry* of the same `EtcdSnapshot` is exempt: each snapshot is stamped with the snapshot's UID — S3 object metadata, or a sibling `.db.uid` file on a PVC — so the agent recognizes its own prior write and the retry is idempotent.) The CRD's "immutability" is about the object: it does not version snapshots, so reusing a name after deleting the object replaces it. > > **S3 credentials need `s3:ListBucket` (or equivalent), not just `s3:GetObject`/`PutObject`.** The overwrite guard does a `HeadObject` on the target key; with `GetObject` but no `ListBucket`, S3 (and some MinIO/Ceph policies) returns **403 AccessDenied** for a *missing* key instead of 404. The agent cannot distinguish that from a real permission problem, so it **fails closed** (refuses the snapshot) rather than risk an overwrite. Grant the snapshot credentials `ListBucket` on the bucket so a HEAD on a missing key returns 404. +> +> **`InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD, STREAMING-AWS4-HMAC-SHA256-PAYLOAD or a valid sha256 value`.** Some non-AWS, S3-compatible backends (Ceph RGW confirmed; some MinIO / Cloudflare R2 versions) reject the flexible-checksum trailer that recent `aws-sdk-go-v2` attaches to uploads by default. The agent pins its S3 request-checksum policy to *when-required* on both the client and the multipart transfer manager, so it does not send that trailer and uploads succeed on those backends. If you still hit this error, your agent image predates that fix — upgrade the operator image. If a snapshot ends up `Failed`, inspect the agent Job's Pod logs (the Job is `-snapshot` and is GC'd a few minutes after finishing via `ttlSecondsAfterFinished`): diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 96f673f3..f16be6a1 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -148,23 +148,26 @@ func (d destination) localPath(name string) string { return base + "/" + name + ".db" } -// snapshotChecksumCalculation is the S3 request-checksum policy the agent applies -// on the upload path. It has to be set in TWO independent places, because the -// snapshot upload touches two SDK layers that each carry their own copy of this -// knob and do NOT share it: +// s3RequestChecksumCalculation is the S3 request-checksum policy applied to every +// S3 client the agent builds (s3Client). It has to be set in TWO independent +// places, because the snapshot upload touches two SDK layers that each carry their +// own copy of this knob and do NOT share it: // // - the s3.Client option (s3Client, below) — covers the single-part PutObject -// the transfer manager issues for a sub-5-MiB body, plus HeadObject; +// the transfer manager issues for a sub-5-MiB body (a small or freshly +// bootstrapped cluster), plus HeadObject; // - the transfer manager's own Uploader.RequestChecksumCalculation // (newSnapshotUploader in snapshot.go) — covers the multipart parts the -// manager issues for a body over its 5-MiB part size, which is every real -// etcd snapshot. manager.NewUploader hardcodes this field to WhenSupported -// and never reads the client option, so setting only the client option -// leaves the multipart path — the one that matters — broken. +// manager issues for any body over its 5-MiB part size, i.e. every snapshot +// of a non-trivial cluster. manager.NewUploader hardcodes this field to +// WhenSupported and never reads the client option, so setting only the client +// option leaves the multipart path broken. // -// The two are separate knobs that MUST agree; deriving both from this single -// constant is what keeps them from silently drifting (delete either site and the -// failure returns — for the multipart case, invisibly). +// Both sites are load-bearing — the client option for the single-part path and +// HeadObject, the uploader option for multipart parts — and the two MUST agree. +// Deriving both from this single constant is what keeps them from silently +// drifting (delete either site and the RGW failure returns — for the multipart +// case, invisibly). // // Why WhenRequired: since early 2025 aws-sdk-go-v2 defaults to WhenSupported, // which stamps a CRC32 on every PutObject/UploadPart — over HTTPS that rides as a @@ -173,7 +176,7 @@ func (d destination) localPath(name string) string { // UNSIGNED-PAYLOAD, STREAMING-AWS4-HMAC-SHA256-PAYLOAD or a valid sha256 value". // WhenRequired attaches a checksum only when the operation mandates one (none for // a plain PutObject or UploadPart) and is equally accepted by real AWS S3. -const snapshotChecksumCalculation = aws.RequestChecksumCalculationWhenRequired +const s3RequestChecksumCalculation = aws.RequestChecksumCalculationWhenRequired // s3Client builds an S3 client honoring a custom endpoint, region and // path-style addressing. Credentials come from AWS_ACCESS_KEY_ID / @@ -195,18 +198,21 @@ func (d destination) s3Client(ctx context.Context) (*s3.Client, error) { o.UsePathStyle = d.s3PathStyle // Request-checksum policy for this client. This is only ONE of the two // sites that need it — the transfer manager carries its own copy for the - // multipart path (see snapshotChecksumCalculation and newSnapshotUploader). + // multipart path (see s3RequestChecksumCalculation and newSnapshotUploader). // LoadDefaultConfig would otherwise populate this from // AWS_REQUEST_CHECKSUM_CALCULATION, but the agent's environment is a closed // list built by the controller (no operator can set that var on the Pod), // so we pin it here unconditionally rather than leave an escape hatch that // nothing can reach. - o.RequestChecksumCalculation = snapshotChecksumCalculation - // Symmetric response-side setting for the restore/download path (this - // client is shared with downloadS3): don't demand a response checksum the - // backend may not emit — the same AWS-default-vs-S3-compatible mismatch - // class. Harmless on the upload path, which issues no checksum-bearing GET. - o.ResponseChecksumValidation = aws.ResponseChecksumValidationWhenRequired + // + // The response side (ResponseChecksumValidation) is deliberately left at + // the SDK default (WhenSupported): it validates a checksum only when the + // backend returns one and skips composite (multipart) checksums, so it does + // not break S3-compatible backends — and it is the only server-side + // integrity signal the restore path has, since downloadS3 runs with + // SkipHashCheck. Pinning it to WhenRequired would silently discard that + // signal, so we don't. + o.RequestChecksumCalculation = s3RequestChecksumCalculation }), nil } diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 8ee71ec3..17791ff8 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -77,10 +77,15 @@ func TestS3ClientChecksumWhenRequired(t *testing.T) { t.Errorf("RequestChecksumCalculation = %v, want WhenRequired (%v)", o.RequestChecksumCalculation, aws.RequestChecksumCalculationWhenRequired) } - // Symmetric response-side setting covering the restore/download path. - if o.ResponseChecksumValidation != aws.ResponseChecksumValidationWhenRequired { - t.Errorf("ResponseChecksumValidation = %v, want WhenRequired (%v)", - o.ResponseChecksumValidation, aws.ResponseChecksumValidationWhenRequired) + // The response side is deliberately left at the SDK default (WhenSupported): + // it validates only a checksum the backend actually returns and skips composite + // (multipart) checksums, so it neither breaks S3-compatible backends nor needs + // pinning — and it is the only server-side integrity signal the restore path + // has (downloadS3 runs with SkipHashCheck). This asserts we do NOT pin it to + // WhenRequired; doing so would silently discard that signal. + if o.ResponseChecksumValidation != aws.ResponseChecksumValidationWhenSupported { + t.Errorf("ResponseChecksumValidation = %v, want the SDK default WhenSupported (%v)", + o.ResponseChecksumValidation, aws.ResponseChecksumValidationWhenSupported) } if !o.UsePathStyle { t.Error("UsePathStyle = false, want true") diff --git a/internal/agent/restore.go b/internal/agent/restore.go index 45e46089..93da58ab 100644 --- a/internal/agent/restore.go +++ b/internal/agent/restore.go @@ -270,11 +270,6 @@ func downloadS3(ctx context.Context, src destination, stageDir string) (string, return "", err } defer f.Close() - // client is built by s3Client, which pins ResponseChecksumValidation to - // WhenRequired — so the downloader doesn't demand a response checksum an - // S3-compatible backend may not return (symmetric to the upload-side request - // checksum). manager.Downloader has no response-validation knob of its own, so - // the client option is the only site for this. downloader := manager.NewDownloader(client) if _, err := downloader.Download(ctx, f, &s3.GetObjectInput{ Bucket: aws.String(src.s3Bucket), diff --git a/internal/agent/snapshot.go b/internal/agent/snapshot.go index 6fde0e79..37fbb261 100644 --- a/internal/agent/snapshot.go +++ b/internal/agent/snapshot.go @@ -248,13 +248,13 @@ func uploadS3Stream(ctx context.Context, dest destination, key string, body io.R // // manager.Uploader carries its OWN RequestChecksumCalculation (NewUploader // hardcodes it to WhenSupported) and never consults the s3.Client option, so the -// multipart path — taken for every body over the 5-MiB part size, i.e. every real -// etcd snapshot — would re-stamp the CRC32 trailer that the client option was set -// to avoid. Pin it here too, from the same constant s3Client uses, so the two -// knobs cannot drift. See snapshotChecksumCalculation. +// multipart path — taken for any body over the 5-MiB part size, i.e. every +// snapshot of a non-trivial cluster — would re-stamp the CRC32 trailer that the +// client option was set to avoid. Pin it here too, from the same constant +// s3Client uses, so the two knobs cannot drift. See s3RequestChecksumCalculation. func newSnapshotUploader(client *s3.Client) *manager.Uploader { return manager.NewUploader(client, func(u *manager.Uploader) { - u.RequestChecksumCalculation = snapshotChecksumCalculation + u.RequestChecksumCalculation = s3RequestChecksumCalculation }) } diff --git a/internal/agent/snapshot_test.go b/internal/agent/snapshot_test.go index 125980db..339864c3 100644 --- a/internal/agent/snapshot_test.go +++ b/internal/agent/snapshot_test.go @@ -86,63 +86,61 @@ func TestUploadStreamHashed_UploadError(t *testing.T) { } } -// TestUploadS3StreamMultipartNoChecksumTrailer is the guard for the actual -// failure mode. A real etcd snapshot exceeds the transfer manager's 5 MiB part -// size, so the upload takes the MULTIPART branch, where manager.Uploader's own -// RequestChecksumCalculation (NOT the s3.Client option) decides whether a CRC32 -// trailer is stamped on each UploadPart. Without newSnapshotUploader pinning it -// to WhenRequired, every part rides `x-amz-content-sha256: -// STREAMING-UNSIGNED-PAYLOAD-TRAILER` + `x-amz-trailer: x-amz-checksum-crc32` — -// exactly the header Ceph RGW rejects. -// -// This drives the production uploadS3Stream against a TLS test server (trusted via -// AWS_CA_BUNDLE, which LoadDefaultConfig wires into the real client, so s3Client -// and newSnapshotUploader run exactly as in production) with a >5 MiB body, and -// asserts no request carries a checksum trailer or algorithm. It fails if the -// uploader option is dropped; a check on s3.Options alone (see agent_test.go) -// would stay green while this real path stayed broken. -func TestUploadS3StreamMultipartNoChecksumTrailer(t *testing.T) { - hermeticAWSEnv(t) +// capturedReq records the checksum-related surface of one request the fake S3 +// endpoint received, so a test can assert no flexible-checksum trailer was sent. +type capturedReq struct{ method, sha, trailer, algo, partNumber string } + +// startS3CaptureServer stands up a TLS httptest server that speaks just enough of +// the S3 protocol for uploadS3Stream to run against — HeadObject, single-part +// PutObject, and the multipart initiate/part/complete trio — trusts its cert +// through the SDK default config chain (AWS_CA_BUNDLE, which LoadDefaultConfig +// wires into the real client, so s3Client and newSnapshotUploader run exactly as +// in production), and records every request's checksum headers. The returned func +// snapshots the captured requests. +func startS3CaptureServer(t *testing.T) (endpoint string, captured func() []capturedReq) { + t.Helper() - type capturedReq struct{ method, sha, trailer, algo string } var mu sync.Mutex var reqs []capturedReq srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, _ = io.Copy(io.Discard, r.Body) + q := r.URL.Query() mu.Lock() reqs = append(reqs, capturedReq{ - method: r.Method, - sha: r.Header.Get("X-Amz-Content-Sha256"), - trailer: r.Header.Get("X-Amz-Trailer"), - algo: r.Header.Get("X-Amz-Sdk-Checksum-Algorithm"), + method: r.Method, + sha: r.Header.Get("X-Amz-Content-Sha256"), + trailer: r.Header.Get("X-Amz-Trailer"), + algo: r.Header.Get("X-Amz-Sdk-Checksum-Algorithm"), + partNumber: q.Get("partNumber"), }) mu.Unlock() - q := r.URL.Query() switch { case r.Method == http.MethodHead: // ensureObjectAbsent's HeadObject: report absent so the upload proceeds. w.WriteHeader(http.StatusNotFound) case r.Method == http.MethodPost && q.Has("uploads"): - io.WriteString(w, ``+ + _, _ = io.WriteString(w, ``+ `bk`+ `test-upload-id`) case r.Method == http.MethodPut && q.Has("partNumber"): w.Header().Set("ETag", `"etag-`+q.Get("partNumber")+`"`) w.WriteHeader(http.StatusOK) case r.Method == http.MethodPost && q.Get("uploadId") != "": - io.WriteString(w, ``+ + _, _ = io.WriteString(w, ``+ `bk`+ `"final-etag"`) + case r.Method == http.MethodPut: + // single-part PutObject (body under the manager's 5 MiB part size) + w.Header().Set("ETag", `"single-etag"`) + w.WriteHeader(http.StatusOK) default: w.WriteHeader(http.StatusOK) } })) - defer srv.Close() + t.Cleanup(srv.Close) - // Trust the test server's self-signed cert through the SDK's default config - // chain, so uploadS3Stream's own s3Client reaches it — no hand-built client. caFile := filepath.Join(t.TempDir(), "ca.pem") certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw}) if err := os.WriteFile(caFile, certPEM, 0o600); err != nil { @@ -150,8 +148,48 @@ func TestUploadS3StreamMultipartNoChecksumTrailer(t *testing.T) { } t.Setenv("AWS_CA_BUNDLE", caFile) + return srv.URL, func() []capturedReq { + mu.Lock() + defer mu.Unlock() + out := make([]capturedReq, len(reqs)) + copy(out, reqs) + return out + } +} + +// assertNoChecksumTrailer fails if a request carries any of the flexible-checksum +// surface that Ceph RGW rejects: the streaming-trailer content-sha256, an +// x-amz-trailer, or an x-amz-sdk-checksum-algorithm. +func assertNoChecksumTrailer(t *testing.T, rq capturedReq) { + t.Helper() + if rq.sha == "STREAMING-UNSIGNED-PAYLOAD-TRAILER" { + t.Errorf("%s carried x-amz-content-sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER — the checksum trailer Ceph RGW rejects", rq.method) + } + if rq.trailer != "" { + t.Errorf("%s carried x-amz-trailer=%q, want none", rq.method, rq.trailer) + } + if rq.algo != "" { + t.Errorf("%s carried x-amz-sdk-checksum-algorithm=%q, want none", rq.method, rq.algo) + } +} + +// TestUploadS3StreamMultipartNoChecksumTrailer is the guard for the actual +// failure mode. A snapshot of a non-trivial cluster exceeds the transfer manager's +// 5 MiB part size, so the upload takes the MULTIPART branch, where manager.Uploader's +// own RequestChecksumCalculation (NOT the s3.Client option) decides whether a CRC32 +// trailer is stamped on each UploadPart. Without newSnapshotUploader pinning it +// to WhenRequired, every part rides `x-amz-content-sha256: +// STREAMING-UNSIGNED-PAYLOAD-TRAILER` + `x-amz-trailer: x-amz-checksum-crc32` — +// exactly the header Ceph RGW rejects. +// +// It fails if the uploader option is dropped; a check on s3.Options alone (see +// agent_test.go) would stay green while this real path stayed broken. +func TestUploadS3StreamMultipartNoChecksumTrailer(t *testing.T) { + hermeticAWSEnv(t) + endpoint, captured := startS3CaptureServer(t) + const bodySize = 12 << 20 // 12 MiB > 5 MiB part size ⇒ three parts ⇒ multipart branch - dest := destination{kind: "s3", s3Endpoint: srv.URL, s3Bucket: "b", s3PathStyle: true} + dest := destination{kind: "s3", s3Endpoint: endpoint, s3Bucket: "b", s3PathStyle: true} size, sum, err := uploadS3Stream(context.Background(), dest, "k", bytes.NewReader(make([]byte, bodySize)), "test-uid") if err != nil { @@ -164,25 +202,56 @@ func TestUploadS3StreamMultipartNoChecksumTrailer(t *testing.T) { t.Error("empty sha256 digest") } - mu.Lock() - defer mu.Unlock() var sawUploadPart bool - for _, rq := range reqs { - if rq.method == http.MethodPut { + for _, rq := range captured() { + if rq.method == http.MethodPut && rq.partNumber != "" { sawUploadPart = true } - if rq.sha == "STREAMING-UNSIGNED-PAYLOAD-TRAILER" { - t.Errorf("%s carried x-amz-content-sha256=STREAMING-UNSIGNED-PAYLOAD-TRAILER — the checksum trailer Ceph RGW rejects", rq.method) - } - if rq.trailer != "" { - t.Errorf("%s carried x-amz-trailer=%q, want none", rq.method, rq.trailer) + assertNoChecksumTrailer(t, rq) + } + if !sawUploadPart { + t.Fatal("no UploadPart (PUT with partNumber) seen — the upload did not take the multipart branch, so this test is not exercising the path it guards") + } +} + +// TestUploadS3StreamSinglePartNoChecksumTrailer guards the OTHER upload branch. +// A snapshot of a small or freshly bootstrapped cluster is well under the transfer +// manager's 5 MiB part size, so the manager issues a single-part PutObject rather +// than multipart. There the s3.Client option (not the uploader's) is the only +// thing keeping the CRC32 trailer off — so this pins the client-side setting +// end-to-end. A struct-field check on s3.Options (agent_test.go) stays green even +// when the real request carries the trailer, which is exactly how commit 61144d4 +// looked correct while every real snapshot was broken. +func TestUploadS3StreamSinglePartNoChecksumTrailer(t *testing.T) { + hermeticAWSEnv(t) + endpoint, captured := startS3CaptureServer(t) + + const bodySize = 1 << 20 // 1 MiB < 5 MiB part size ⇒ single-part PutObject branch + dest := destination{kind: "s3", s3Endpoint: endpoint, s3Bucket: "b", s3PathStyle: true} + + size, sum, err := uploadS3Stream(context.Background(), dest, "k", bytes.NewReader(make([]byte, bodySize)), "test-uid") + if err != nil { + t.Fatalf("uploadS3Stream: %v", err) + } + if size != bodySize { + t.Errorf("streamed size = %d, want %d", size, bodySize) + } + if sum == "" { + t.Error("empty sha256 digest") + } + + var putObjects int + for _, rq := range captured() { + if rq.method == http.MethodPut && rq.partNumber == "" { + putObjects++ } - if rq.algo != "" { - t.Errorf("%s carried x-amz-sdk-checksum-algorithm=%q, want none", rq.method, rq.algo) + if rq.partNumber != "" { + t.Errorf("saw a multipart UploadPart (partNumber=%q); a %d-byte body must take the single-part branch this test guards", rq.partNumber, bodySize) } + assertNoChecksumTrailer(t, rq) } - if !sawUploadPart { - t.Fatal("no UploadPart (PUT) request seen — the upload did not take the multipart branch, so this test is not exercising the path it guards") + if putObjects != 1 { + t.Fatalf("saw %d single-part PutObject PUTs, want exactly 1 — the body no longer takes the single-part branch, so this test is not exercising the path it guards", putObjects) } } From f3338e3af15db8e641c2d5f8c1c380b583560a77 Mon Sep 17 00:00:00 2001 From: Timofei Larkin Date: Tue, 21 Jul 2026 17:34:42 +0300 Subject: [PATCH 4/4] docs(agent): correct the response-checksum rationale The comment on the s3.Client options claimed ResponseChecksumValidation is "the only server-side integrity signal the restore path has, since downloadS3 runs with SkipHashCheck". Both halves were wrong: - SkipHashCheck is a field of etcdutl's snapshot.RestoreConfig (restore.go), covering etcd's own appended snapshot hash. downloadS3 passes no such option and has no equivalent. - The signal it credits is inert for objects this agent writes: response validation fires only when the backend returns an x-amz-checksum-* header, and pinning requests to WhenRequired means no checksum is ever stored. Leaving ResponseChecksumValidation at the SDK default is still right, so the decision stands; only the reason changes. The same wrong text was duplicated in the test comment and is corrected there too. Also trim the rationale to one home on s3RequestChecksumCalculation with pointers from the use sites, drop a commit hash that will not survive a squash-merge, and use os.DevNull instead of a hardcoded /dev/null. No behavior change: both mutation checks still fail closed (drop the client option and the single-part test fails; drop the uploader option and the multipart test fails). Assisted-By: Claude Opus 4.8 (1M context) Signed-off-by: Timofei Larkin --- internal/agent/agent.go | 38 +++++++++++++++------------------ internal/agent/agent_test.go | 15 ++++++------- internal/agent/snapshot.go | 9 +++----- internal/agent/snapshot_test.go | 4 ++-- 4 files changed, 29 insertions(+), 37 deletions(-) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index f16be6a1..c7d54ac7 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -163,11 +163,9 @@ func (d destination) localPath(name string) string { // WhenSupported and never reads the client option, so setting only the client // option leaves the multipart path broken. // -// Both sites are load-bearing — the client option for the single-part path and -// HeadObject, the uploader option for multipart parts — and the two MUST agree. -// Deriving both from this single constant is what keeps them from silently -// drifting (delete either site and the RGW failure returns — for the multipart -// case, invisibly). +// Both sites are load-bearing and MUST agree; deriving them from this one +// constant is what keeps them from drifting. Delete either and the RGW failure +// returns — for the multipart case, invisibly. // // Why WhenRequired: since early 2025 aws-sdk-go-v2 defaults to WhenSupported, // which stamps a CRC32 on every PutObject/UploadPart — over HTTPS that rides as a @@ -196,23 +194,21 @@ func (d destination) s3Client(ctx context.Context) (*s3.Client, error) { o.BaseEndpoint = aws.String(d.s3Endpoint) } o.UsePathStyle = d.s3PathStyle - // Request-checksum policy for this client. This is only ONE of the two - // sites that need it — the transfer manager carries its own copy for the - // multipart path (see s3RequestChecksumCalculation and newSnapshotUploader). - // LoadDefaultConfig would otherwise populate this from - // AWS_REQUEST_CHECKSUM_CALCULATION, but the agent's environment is a closed - // list built by the controller (no operator can set that var on the Pod), - // so we pin it here unconditionally rather than leave an escape hatch that - // nothing can reach. - // - // The response side (ResponseChecksumValidation) is deliberately left at - // the SDK default (WhenSupported): it validates a checksum only when the - // backend returns one and skips composite (multipart) checksums, so it does - // not break S3-compatible backends — and it is the only server-side - // integrity signal the restore path has, since downloadS3 runs with - // SkipHashCheck. Pinning it to WhenRequired would silently discard that - // signal, so we don't. + // One of the two sites that must pin this; see + // s3RequestChecksumCalculation for the full rationale and + // newSnapshotUploader for the other. Pinned unconditionally because + // LoadDefaultConfig would otherwise take it from + // AWS_REQUEST_CHECKSUM_CALCULATION, and the agent's environment is a closed + // list built by the controller — nothing can set that var on the Pod. o.RequestChecksumCalculation = s3RequestChecksumCalculation + // ResponseChecksumValidation is deliberately left at the SDK default + // (WhenSupported): it validates only a checksum the backend actually + // returns and skips composite (multipart) ones, so it cannot break an + // S3-compatible backend. It is not an integrity guarantee for restore + // either — WhenRequired above means the agent stores no checksum, so for + // objects it wrote there is nothing to validate and the setting is inert. + // Left alone because there is no reason to change it, not because it + // protects anything. }), nil } diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 17791ff8..ef403ab9 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -12,6 +12,7 @@ package agent import ( "context" + "os" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -43,8 +44,8 @@ func TestObjectKey(t *testing.T) { // region so signing never reaches the EC2/ECS metadata endpoint. func hermeticAWSEnv(t *testing.T) { t.Helper() - t.Setenv("AWS_CONFIG_FILE", "/dev/null") - t.Setenv("AWS_SHARED_CREDENTIALS_FILE", "/dev/null") + t.Setenv("AWS_CONFIG_FILE", os.DevNull) + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", os.DevNull) t.Setenv("AWS_PROFILE", "") t.Setenv("AWS_ACCESS_KEY_ID", "test") t.Setenv("AWS_SECRET_ACCESS_KEY", "test") @@ -77,12 +78,10 @@ func TestS3ClientChecksumWhenRequired(t *testing.T) { t.Errorf("RequestChecksumCalculation = %v, want WhenRequired (%v)", o.RequestChecksumCalculation, aws.RequestChecksumCalculationWhenRequired) } - // The response side is deliberately left at the SDK default (WhenSupported): - // it validates only a checksum the backend actually returns and skips composite - // (multipart) checksums, so it neither breaks S3-compatible backends nor needs - // pinning — and it is the only server-side integrity signal the restore path - // has (downloadS3 runs with SkipHashCheck). This asserts we do NOT pin it to - // WhenRequired; doing so would silently discard that signal. + // The response side stays at the SDK default (WhenSupported): it validates only + // a checksum the backend actually returns and skips composite (multipart) ones, + // so it cannot break an S3-compatible backend and needs no pinning. This asserts + // we do NOT pin it, so a future "symmetry" change has to justify itself. if o.ResponseChecksumValidation != aws.ResponseChecksumValidationWhenSupported { t.Errorf("ResponseChecksumValidation = %v, want the SDK default WhenSupported (%v)", o.ResponseChecksumValidation, aws.ResponseChecksumValidationWhenSupported) diff --git a/internal/agent/snapshot.go b/internal/agent/snapshot.go index 37fbb261..10aa0a86 100644 --- a/internal/agent/snapshot.go +++ b/internal/agent/snapshot.go @@ -246,12 +246,9 @@ func uploadS3Stream(ctx context.Context, dest destination, key string, body io.R // newSnapshotUploader builds the S3 transfer manager for snapshot uploads. // -// manager.Uploader carries its OWN RequestChecksumCalculation (NewUploader -// hardcodes it to WhenSupported) and never consults the s3.Client option, so the -// multipart path — taken for any body over the 5-MiB part size, i.e. every -// snapshot of a non-trivial cluster — would re-stamp the CRC32 trailer that the -// client option was set to avoid. Pin it here too, from the same constant -// s3Client uses, so the two knobs cannot drift. See s3RequestChecksumCalculation. +// manager.Uploader carries its OWN RequestChecksumCalculation and never consults +// the s3.Client option, so the multipart path needs the same pin the client got. +// See s3RequestChecksumCalculation for why, and why both sites are load-bearing. func newSnapshotUploader(client *s3.Client) *manager.Uploader { return manager.NewUploader(client, func(u *manager.Uploader) { u.RequestChecksumCalculation = s3RequestChecksumCalculation diff --git a/internal/agent/snapshot_test.go b/internal/agent/snapshot_test.go index 339864c3..54264880 100644 --- a/internal/agent/snapshot_test.go +++ b/internal/agent/snapshot_test.go @@ -220,8 +220,8 @@ func TestUploadS3StreamMultipartNoChecksumTrailer(t *testing.T) { // than multipart. There the s3.Client option (not the uploader's) is the only // thing keeping the CRC32 trailer off — so this pins the client-side setting // end-to-end. A struct-field check on s3.Options (agent_test.go) stays green even -// when the real request carries the trailer, which is exactly how commit 61144d4 -// looked correct while every real snapshot was broken. +// when the real request carries the trailer — that gap is how a client-only pin +// can look correct while every upload is still broken on the wire. func TestUploadS3StreamSinglePartNoChecksumTrailer(t *testing.T) { hermeticAWSEnv(t) endpoint, captured := startS3CaptureServer(t)