user contribution: s3 remote support - #11571
Merged
Merged
Conversation
….Open blobstorePersister.Open falls back to newBSArchiveChunkSource when the bare table name is not found, but noConjoinBlobstorePersister.Open did not. A push that copies archive-format table files (<name>.darc) into a no-conjoin blobstore store then fails at openChunkSources with 'Blob not found: <name>'. Mirror the same fallback.
Implements the file-backed manifest CAS design discussed in #509: an endpoint-configurable S3 Blobstore that treats object ETags as opaque version tokens. CheckAndPut uses conditional PutObject (If-None-Match:* for create, If-Match:<etag> for replace); HTTP 412, AWS conditional 409, and If-Match-on-missing-key all map to blobstore.CheckAndPutError so the existing NBS manifest reread/retry path handles races unchanged. Table files go through the no-conjoin store, so no server-side compose operation is required. Works against any provider implementing S3 conditional writes: AWS S3, Cloudflare R2, MinIO. New remote params: s3-endpoint, s3-region, s3-path-style (the AWS_ENDPOINT_URL_S3 env var and standard AWS credential chain are honored when params are omitted). Integration tests are env-gated on TEST_S3_BUCKET / TEST_S3_ENDPOINT / TEST_S3_PATH_STYLE, following the GCS/OCI convention, and cover put/get/exists/ranged reads, conditional create/update/stale/missing-key semantics, an 8-writer concurrent CheckAndPut race, multipart Put, and unsupported Concatenate.
Contributor
|
@macneale4 DOLT
|
Contributor
|
@macneale4 DOLT
|
Contributor
|
@macneale4 DOLT
|
store/blobstore/s3.go imports github.com/aws/smithy-go directly to detect AWS's conditional-write conflict, but go.mod still listed it as indirect. ci-check-repo runs 'go mod tidy' and fails on any resulting tree change. go.sum and Godeps/LICENSES are unaffected: the module graph does not change, only the require block it is recorded in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dolt module requires google.golang.org/grpc v1.82.1, but this module, which replaces dolt with ../../go, still recorded v1.79.3. Module resolution selects v1.82.1, so 'go test .' under the default -mod=readonly fails with 'updates to go.mod needed'. This is pre-existing on main and is not caused by the s3 work. The sql-server integration test job runs against the pull request merge commit, so it fails on every open PR touching go/, including 11572, 11573 and 11574. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@macneale4 DOLT
|
blobstoreManifest hands CheckAndPut a *bytes.Buffer, which is not an
io.Seeker. The aws sdk needs a seekable body for two things, and failed
both:
- Signing. Without a rewindable stream it cannot compute the payload
hash, and it refuses to substitute the aws-chunked trailing-checksum
encoding without TLS. Against a plain-http endpoint, the common MinIO
deployment, PutObject failed before issuing a request. Table files
upload fine through the manager, which buffers into seekable parts,
so a push uploaded everything and then failed writing the manifest.
- Retries. Over TLS the sdk streams aws-chunked instead, but a transient
5xx then fails with 'failed to rewind transport stream for retry'.
updateBSWithChecker returns any non-CheckAndPutError straight to the
caller, so one blip fails the push.
Read the body up front: it is a manifest, it is small, and the length is
already known. ContentLength now comes from the buffered bytes rather than
the caller's totalSize.
The tests run a real s3.Client against an in-process fake over both http
and TLS, so they need no credentials, no provider, and no docker, and they
run in CI by default. They cover conditional create and replace, the
412/404 mapping to CheckAndPutError, transient-5xx retry, and ranged and
suffix reads. Four of the five fail without this fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@macneale4 DOLT
|
Contributor
|
@macneale4 DOLT
|
These three params are routing, not authentication: they set BaseEndpoint,
the signing region, and UsePathStyle. Credentials already come entirely
from the AWS SDK chain.
Two problems with carrying them as creation params:
- Every cloud backend added after aws:// takes no params at all. gs, oci
and az each define zero and rely on the ambient credential and config
chain. Reintroducing a param map for s3:// revives the one pattern the
codebase moved away from.
- The SDK already resolves endpoint and region ambiently, via
AWS_ENDPOINT_URL, the per-service AWS_ENDPOINT_URL_S3, the shared
config endpoint_url key including its per-service form, and AWS_REGION.
s3-endpoint and s3-region duplicated that surface while covering less
of it.
Routing will move into the url as query parameters, which earl.Parse
already handles and which is persisted per remote, so a repo can address
two different providers at once without a params map or new flags. Until
that lands, endpoint and region come from the SDK chain and there is no way
to request path-style addressing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reaching a non-AWS S3 provider needs three values that are addressing, not authentication: the endpoint host, the signing region, and whether to use path-style addressing. Credentials stay on the AWS SDK chain and are now explicitly refused in the url. dolt remote add r2 's3://bucket/db?endpoint=https://acct.r2.cloudflarestorage.com®ion=auto&path-style=true' The url is persisted with the remote, so routing survives clone, push and DOLT_BACKUP without a params map, and one repository can address several providers at once. Endpoint and region fall back to the SDK chain when absent. Path-style has no ambient equivalent at all: the SDK exposes it only as a client option, with no environment variable or shared config key, so the url is the only way to ask for it. It stays off unless requested, matching both the SDK default and gocloud.dev's s3blob, which is the closest precedent for this url shape. There is no s3:// specification to follow. The scheme is an AWS CLI convention meaning bucket plus key, and its authority slot is already the bucket, so routing cannot go there without invented syntax. Malformed urls are rejected when the remote is added rather than at first push, via ValidateS3Url from parseRemoteArgs, which covers dolt remote add and dolt clone. Unknown parameters, non-boolean path-style, empty values, repeated parameters and embedded credentials all error. Verified end to end against a local stand-in: a routed remote sends its requests to the configured endpoint, and path-style=true demonstrably changes the request from virtual-hosted to path addressing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@macneale4 DOLT
|
The 409 ConditionalRequestConflict branch had no test at all, and it is the one mapping a fake cannot produce by behaving correctly: only AWS returns it, when concurrent conditional writes collide. Add sticky error injection to the fake and table-drive every response CheckAndPut classifies. The negative cases carry the weight. An unrelated 409 such as OperationAborted must not be reported as a lost race, or the manifest layer retries a real failure forever. A 404 with no expected version means a missing bucket, not a stale view. 403 must surface as itself. Injection is sticky rather than one-shot so an SDK retry cannot quietly turn an injected failure into a success. Also cover Exists, including that a 403 surfaces rather than reading as an empty store, Put overwrite semantics, and the Concatenate error. Verified by mutation: removing the isS3ConditionalConflict case fails exactly the two conditional-conflict cases and nothing else. Multipart Put is still only covered by the env-gated tests. Teaching the fake CreateMultipartUpload and friends is worth doing only if Put stays on the blobstore, which the conjoin work may change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@macneale4 DOLT
|
CheckAndPut looked like a general check-and-set primitive but had exactly
one production caller in the tree, bs_manifest.go, always with the manifest
key. Table files go through Put; nothing else does a check-and-set. The
abstraction leaked visibly: GitBlobstore branched on the key and did
something entirely different for the manifest, flushing all deferred writes
in one commit and push, than for any other key, and the other branch was
reached only by tests.
Three changes together:
- Rename to CheckAndPutManifest across the interface and all eight
implementations.
- Drop the key parameter. Implementations use blobstore.ManifestKey, so
the git branch is deleted outright rather than left as a runtime check.
nbs manifestFile now aliases the same constant so the two cannot drift.
- Take []byte instead of io.Reader plus totalSize. The caller already
holds a filled buffer.
The last of those removes three separate buffering copies that existed only
because implementations needed a materialized or rewindable view: s3 to
sign the payload, azure because UploadBuffer takes []byte, and git to parse
table names for pruning. The contract is now in the signature instead of
being rediscovered per backend.
Two git tests asserted that a version mismatch does not consume the reader.
There is no reader to consume, so they keep the mismatch assertion and drop
the reader machinery. The shared blobstore suite now exercises the manifest
key, which is the only thing this method can address.
Verified beyond the unit tests with a real file:// round trip: init, push
creating the manifest, clone, second push replacing it, pull. Git blobstore
tests confirmed to run rather than skip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Exists, Get, Put, CheckAndPutManifest, Path and Concatenate are documented on the interface. Repeating that on each implementation adds nothing and drifts: the azure CheckAndPut comment still described a key parameter that no longer exists, and the gcs and oci Exists comments both talked about InMemoryBlobstore, copy-pasted from inmem.go. Kept only the comments that say something the interface cannot: azure's streaming block upload and server-side StageBlockFromURL, git's teardown of owned refs, and why oci and s3 have no Concatenate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@macneale4 DOLT
|
Five tests need no network and always run: adding a plain and a routed remote, and the three url validations, unknown parameter, non-boolean path-style, and credentials embedded in the url. The rest exercise real providers. The minio tests start and stop their own server, following how the git ssh tests manage sshd: helper/s3-common.bash provides setup_minio and teardown_minio, one server per test on a port from definePORT, the pid captured and killed in teardown, and a skip when the binary is absent. Nothing is left running for the life of the job. CI only installs the binary into .ci_bin, which is already on PATH. The aws tests run against a real bucket and are gated on DOLT_BATS_AWS_BUCKET; unlike aws://, they need no DynamoDB table, so that bucket is all they require. Each provider gets the same round trip: push, clone, verify the row count, push again, pull. The second push earns its place because creating a manifest and replacing one take different conditional-write paths, If-None-Match against If-Match. The two url shapes differ on purpose. MinIO needs endpoint, region and path-style in the query string, the last because it has no wildcard DNS for virtual-hosted buckets; against AWS the SDK resolves the endpoint from the region, so the url is bucket and database alone. setup_minio also points the credential chain at MinIO, because the job exports real AWS credentials for the aws:// tests and MinIO rejects both those keys and the session token a role-assumed identity carries. Each bats test runs in its own subshell, so this does not leak to the aws tests. stdio is redirected to /dev/null for the same reason setup_git_sshd does it: otherwise the server holds bats' pipes open and bats never reaches EOF. Verified locally: 8 run and 2 skip with a role-shaped AWS key and session token in the environment, no minio process or listening socket survives the run, and every network test skips cleanly when nothing is configured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@macneale4 DOLT
|
…dentials Under SQL_ENGINE=remote-engine the push runs inside the sql-server that setup_common started, not in the dolt client. That server was started before the test body ran, so it inherited the ambient AWS credentials the job exports for the aws:// tests, and reached MinIO with an AWS access key: api error InvalidAccessKeyId: The Access Key Id you provided does not exist in our records Exporting MinIO's credentials in the test only fixes the client, which is why this passed in local-engine and failed in remote-engine. Restart the server after setting them, the same remedy sql-backup.bats uses for the same reason with DOLT_BACKUP_PRUNE_MIN_GRACE. The aws:// half was unaffected and passed in both modes, since the server already had the credentials it needed. Verified by reproducing the failure: with a fake AWS key in the environment and SQL_ENGINE=remote-engine, removing the restart reproduces the identical InvalidAccessKeyId error, and restoring it passes. Full file green in both engine modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@macneale4 DOLT
|
macneale4
marked this pull request as ready for review
August 20, 2026 23:24
coffeegoddd
approved these changes
Aug 21, 2026
coffeegoddd
left a comment
coffeegoddd
left a comment
Contributor
There was a problem hiding this comment.
LGTM, mostly nits about removing what seem like fine legacy comments (while the newly added comments are quite large sometimes (those can be pared down)). In a perfect world we'd use a conjoin blobstore, but seems like we have many others that use the are non-conjoining blobstores.
Restores the doc comments 4486673 removed from the exported methods of the pre-existing blobstores: Path, Exists, Get, Put and Concatenate on az, gcs, inmem, local and oci. Those predate this branch and removing them was not warranted. CheckAndPutManifest is the exception. Every implementation does exactly what the interface documents and nothing more, so a comment on each one repeats the interface and earns nothing. None of the eight carries one now, s3 included, and the contract lives in one place. Three of the restored comments named CheckAndPut, which no longer exists. Also pares back the two largest comments this branch added, the CheckAndPutManifest contract on the interface and the per-field commentary on s3Routing, which said at length what one sentence covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@macneale4 DOLT
|
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
This was referenced Aug 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Original PR: #11433
Fixes: #509
Documentation Change: dolthub/docs-2#174