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.As gives the s3.GetObjectOutput, and its ResultMetadata carries the raw HTTP response (awsmiddleware.GetRawResponse), with the status and Content-Range.
  • azureblob: Reader.As gives a DownloadStreamResponse, which has ContentRange but not the status, so each chunk request carries policy.WithCaptureResponse from azcore in its context and the check reads the captured response. Reader.As still decides that the reader is Azure's.
  • Other drivers are not checked. The GCS client that gcsblob uses refuses an answer other than 206 to a range that starts after byte 0 (storage: partial request not satisfied), and fileblob reads 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 says GoCloud chunk response could not be read, chunk range not checked. The SDK transports in use always provide the response. TestGoCloudChunkStatusLogsUnreadableResponse covers the Azure case by reading a chunk without the context that captures its response; the S3 case is not tested, since the S3 SDK in go.mod always 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?

  • TestCacheExtractorGoCloudParallelTransferChecksChunkRange in commands/helpers: handleGoCloudURL with an s3:// URL and with an azblob:// URL whose endpoint is an httptest server. The first chunk is answered as requested and every chunk after it by the case: a 206 as requested (the parallel download succeeds, no GET without Range), 200 with the whole object (one GET without Range, archive equal to the object), a 206 for another range and a 206 without Content-Range (retryable error, no GET without Range, no archive).
  • TestCheckChunkRange in commands/helpers: the helper itself. A 206 for the requested range is accepted, with a known or an unknown total; a 200, with or without Content-Range, is marked with errRangeNotHonored; a 206 for another range, for a longer range from the same offset or without Content-Range, and a 416, fail without that mark.
  • TestGoCloudChunkStatusLogsUnreadableResponse in commands/helpers: the debug line. An Azure chunk read without the context that captures its response gives ok false and the line; a fileblob chunk gives ok false 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 ignores Range, 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 in presignedRangeFetchChunk; I will rebase the second one and move its 412 case into checkChunkRange.

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-Range

The 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/transfer

Environment: 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: TestGoCloudParallelDownloadRealStore

With 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/helpers

Environment: 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.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading