Check each GoCloud chunk response in the parallel cache download
What does this MR do?
Makes the parallel GoCloud cache download check what each chunk response holds before writing it, as !7635 (merged) did for the presigned cache path and the artifact path. For the s3blob and azureblob drivers the chunk fetcher now reads the HTTP response of every chunk and accepts only a 206 whose Content-Range is the requested range. A chunk answered with 200 and the whole object makes handleGoCloudURL download the object with a single NewReader instead. A 206 for another range, or one without Content-Range, fails the download with a retryable error. The presigned and the GoCloud chunk fetchers now decide each answer with the same helper, checkChunkRange.
Why was this MR needed?
With FF_USE_PARALLEL_CACHE_TRANSFER enabled, handleGoCloudURL skips the range probe for the s3, gs and azblob schemes (cache_extractor.go#L212-L236) and hands NewRangeReader to downloadParallel as the chunk fetcher without looking at the answer (#L514-L517). That path is used for S3 when RoleARN is set, and for Azure. s3blob in gocloud.dev v0.46.0 returns the GetObject body without looking at the status or ContentRange, and the AWS SDK accepts any 2xx. The azblob v1.8.1 download accepts both 200 and 206. So when a server in front of the store ignores Range and answers 200 with the whole object, transfer.ParallelRangeDownload writes the opening bytes of the object at the chunk's offset. The download reports success with a corrupt archive that keeps the object's modification time, so a later run takes it as up to date.
The drivers hide the response, so I read it where each one exposes it:
s3blob:Reader.Asgives thes3.GetObjectOutput, and itsResultMetadatacarries the raw HTTP response (awsmiddleware.GetRawResponse), with the status andContent-Range.azureblob:Reader.Asgives aDownloadStreamResponse, which hasContentRangebut not the status, so each chunk request carriespolicy.WithCaptureResponsefrom azcore in its context and the check reads the captured response.Reader.Asstill decides that the reader is Azure's.- Other drivers are not checked. The GCS client that
gcsblobuses refuses an answer other than206to a range that starts after byte 0 (storage: partial request not satisfied), andfileblobreads a local file. - If the driver is S3 or Azure but the response cannot be read (a raw response that is not a
*smithyhttp.Response, or no response captured), the chunk is written unchecked, as for the other drivers, and a debug line saysGoCloud chunk response could not be read, chunk range not checked. The SDK transports in use always provide the response.TestGoCloudChunkStatusLogsUnreadableResponsecovers the Azure case by reading a chunk without the context that captures its response; the S3 case is not tested, since the S3 SDK ingo.modalways returns a*smithyhttp.Response.
The check is the one the presigned path has made since !7635 (merged), and so is the split between the answers: both fetchers call checkChunkRange, which holds the rule the presigned fetcher used to apply on its own. A 200 with the whole object means the server ignored Range, and one NewReader downloads the object correctly. A 206 for another range is not treated that way: a caching proxy whose cache key leaves out Range answers every request with the partial 206 it stored first, and a single read would get the same partial answer.
The new imports are packages of modules go.mod already requires directly (aws-sdk-go-v2, service/s3, smithy-go, azcore, azblob), and the helper binary already links them through the two drivers, so go.mod and go.sum do not change.
What's the best way to test this MR?
TestCacheExtractorGoCloudParallelTransferChecksChunkRangeincommands/helpers:handleGoCloudURLwith ans3://URL and with anazblob://URL whose endpoint is anhttptestserver. The first chunk is answered as requested and every chunk after it by the case: a206as requested (the parallel download succeeds, no GET withoutRange),200with the whole object (one GET withoutRange, archive equal to the object), a206for another range and a206withoutContent-Range(retryable error, no GET withoutRange, no archive).TestCheckChunkRangeincommands/helpers: the helper itself. A206for the requested range is accepted, with a known or an unknown total; a200, with or withoutContent-Range, is marked witherrRangeNotHonored; a206for another range, for a longer range from the same offset or withoutContent-Range, and a416, fail without that mark.TestGoCloudChunkStatusLogsUnreadableResponseincommands/helpers: the debug line. An Azure chunk read without the context that captures its response givesokfalse and the line; afileblobchunk givesokfalse without it.
go test -race -count=1 -run 'TestCacheExtractorGoCloud|TestCheckChunkRange|TestGoCloudChunkStatus|TestCacheExtractorPresignedParallel' ./commands/helpers/With main, the three cases per driver that do not answer as requested fail, six in all: the download succeeds and the archive holds the object's opening bytes at every chunk offset. The two 206 as requested cases pass by design. I also checked that each part of the change is needed. Not reading the response for S3, or for Azure, fails the three cases of that driver. Falling back to NewReader on any mismatch fails both 206 cases of both drivers, and so does comparing only the status and not Content-Range. Not falling back on a 200 fails the 200 case of both drivers. The presigned download tests from !7635 (merged) cover the other caller of checkChunkRange. Removing the debug line fails the Azure case of TestGoCloudChunkStatusLogsUnreadableResponse, and logging it for the drivers that are not checked fails its fileblob case.
The second commit adds integration tests that Lachlan Grant wrote for the review of !7635 (merged) (note), where two of them failed on the gaps that this MR and the MR for #40109 close. As in that review, they use the integration build tag, read their endpoints from RANGE_IT_* variables and skip when those are unset, so CI skips them:
commands/helpers/cache_extractor_gocloud_range_integration_test.go: the GoCloud download through a load balancer whose second node is a static server that ignoresRange, now with the archive required on disk.commands/helpers/cache_extractor_range_integration_test.go: the helpers, and the tests that every fix of the parallel cache download keeps passing. The MR for #40109 adds the same file, byte for byte. That file merges cleanly whichever goes second, but both MRs now change the chunk check inpresignedRangeFetchChunk; I will rebase the second one and move its412case intocheckChunkRange.
I ran them against SeaweedFS 3.85 and LocalStack 4.4.0 behind nginx 1.27.5 with the configuration of that note, three times on each store. For LocalStack I added one server to that configuration, the same static load balancer in front of it. With main, TestGoCloudParallelDownloadRangeIgnoringLoadBalancer fails every time on both stores, with an archive that matches no object. With this MR the download logs that a chunk range read was not honored and falls back to one reader, and every test passes. The caching proxy of that configuration is in front of SeaweedFS only, so its test skips in the LocalStack run. After the review I ran them again on the new head, three times on each store, with the same result. newRangeITStore now matches the S3 SDK's typed BucketAlreadyOwnedByYou and BucketAlreadyExists errors rather than their text: SeaweedFS answers a bucket that exists with 409 BucketAlreadyExists, which matches, and LocalStack, in us-east-1, with success.
I ran go test -race -count=1 on ./commands/helpers/ and ./helpers/transfer/, GOOS=linux, GOOS=windows and GOOS=darwin go vet on those packages, go mod tidy (no change), make lint in the runner-linters:2.12.2-go1.26.5 image the go:lint job uses (0 issues., test directives included), and the commands of the compile tests jobs for Linux and Windows. make lint does not build the integration files, so I also ran golangci-lint with the integration tag on ./commands/helpers/ and ./network/: no issues in the new files. Since main moved to Go 1.26.10 and golangci-lint 2.14.0, I also ran make lint in runner-linters:2.14.0-go1.26.10 and go test -race with Go 1.26.10 on this branch merged into main: 0 issues., and both packages pass.
Not run: real AWS S3 or Azure. The httptest stand-in answers the requests the real SDKs send, and the integration tests run the S3 case against SeaweedFS and LocalStack. Nothing here runs Azure against a real store.
Test output on main
With the code from main at 5d62e0cc:
--- FAIL: TestCacheExtractorGoCloudParallelTransferChecksChunkRange
--- PASS: .../s3/206_as_requested
--- FAIL: .../s3/200_with_the_whole_object
Error: Not equal: expected: 1, actual: 0
Messages: GETs without Range
Error: Not equal: expected: []byte{0x50, 0x4b, 0x3, 0x4, 0x14, 0x0, 0x8, 0x0, ...}
actual : []byte{0x50, 0x4b, 0x3, 0x4, 0x14, 0x0, 0x8, 0x50, 0x4b, ...}
--- FAIL: .../s3/206_for_another_range
--- FAIL: .../s3/206_without_Content-Range
Messages: a chunk outside its range should fail with a retryable error
--- PASS: .../azblob/206_as_requested
--- FAIL: .../azblob/200_with_the_whole_object
--- FAIL: .../azblob/206_for_another_range
--- FAIL: .../azblob/206_without_Content-RangeThe archive repeats the zip header (PK\x03\x04) at every 7-byte chunk offset.
With this MR:
ok gitlab.com/gitlab-org/gitlab-runner/commands/helpers
ok gitlab.com/gitlab-org/gitlab-runner/helpers/transferEnvironment: Linux amd64, Go 1.26.5 (from go.mod).
Integration test output
go test -count=1 -v -tags integration on ./commands/helpers/, with main and the two new files, on SeaweedFS (LocalStack fails the same test with the same message, and skips the caching proxy):
cache_extractor_gocloud_range_integration_test.go:21: handleGoCloudURL: err=<nil>
Error: corrupt archive on disk
Messages: archive has 8400953 bytes and matches no expected object
--- FAIL: TestGoCloudParallelDownloadRangeIgnoringLoadBalancer
--- PASS: TestPresignedParallelDownloadRealStore
--- PASS: TestPresignedParallelDownloadRangeDroppingLoadBalancer
--- PASS: TestPresignedParallelDownloadRangeBlindCachingProxy
--- PASS: TestPresignedDownloadServerWithoutRangeSupport
--- PASS: TestGoCloudParallelDownloadRealStoreWith this MR, SeaweedFS (LocalStack the same, with the caching proxy skipped):
level=info msg="GoCloud backend did not honor a chunk range read, using sequential download" error="range request failed: 200 OK: range not honored"
cache_extractor_gocloud_range_integration_test.go:21: handleGoCloudURL: err=<nil>
--- PASS: TestGoCloudParallelDownloadRangeIgnoringLoadBalancer
--- PASS: TestPresignedParallelDownloadRealStore
--- PASS: TestPresignedParallelDownloadRangeDroppingLoadBalancer
--- PASS: TestPresignedParallelDownloadRangeBlindCachingProxy
--- PASS: TestPresignedDownloadServerWithoutRangeSupport
--- PASS: TestGoCloudParallelDownloadRealStore
ok gitlab.com/gitlab-org/gitlab-runner/commands/helpersEnvironment: Linux amd64, Go 1.26.5, SeaweedFS 3.85 with one identity, LocalStack 4.4.0 with SERVICES=s3, nginx 1.27.5, each in Docker on one network.
What are the relevant issue numbers?
Closes #40108 (closed)
Related to #40102 (closed), whose presigned cache and artifact paths !7635 (merged) fixed. #40108 (closed) is the GoCloud part of it that was left.
Related to #40109, whose MR adds the same cache_extractor_range_integration_test.go.