Repository navigation
Storage: one interface for every store, context everywhere, S3 semantics - #52
Open
theomonnom wants to merge 14 commits into
Open
theomonnom wants to merge 14 commits into
theomonnom wants to merge 14 commits into
Conversation
NewObjectStorage returns an ObjectStorage for S3 (and S3-compatible endpoints) and local storage. Every call takes a context; PutObject and UploadObject take PutOptions (ContentType, IfAbsent); GetObjectRange, DownloadObject and ObjectExists read; DeleteObjects reports per-key failures inside an accepted batch. Missing objects and refused writes come back as ErrObjectNotFound and ErrObjectExists, still wrapping the status code. Storage is unchanged.
… does for configs
NewObjectStorage now covers every Config. DeleteObjects succeeds for paths that hold nothing, and a range starting at or past the end returns no bytes, on every backend. The S3 uploader's checksum workaround for S3-compatible stores is shared with UploadObject.
…temp files unlisted UploadObject sends If-None-Match on CompleteMultipartUpload, which the uploader drops; MinIO and OCI's S3 API both refuse with it. The per-backend constructors are unexported behind NewObjectStorage, GetObject is shared over DownloadObject, and local ListObjects leaves out UploadObject's temporary files.
…nedUrl take a context ObjectStorage folds into Storage: every backend type implements the new calls, and ListObjects, DeleteObjects and GeneratePresignedUrl take a context with the new semantics (DeleteObjects succeeds for missing paths and fails on any key it could not delete). The interface lives in storage/objects; storage.Storage aliases it. Each provider test runs both suites; TestS3 takes an optional S3_ENDPOINT. Breaking: callers of the three calls pass a context, and implementations of Storage add the new methods.
theomonnom
changed the title
objects.Bucket holds the context calls, so a fake or a consumer that only manages objects takes the small interface; Storage is Bucket plus the file calls. DownloadData and DownloadFile report ErrObjectNotFound too, ListObjects is in byte order on every backend (local sorts), and S3 GetObjectRange refuses an answer without Content-Range. The doc comments state IfAbsent's limits, UploadObject's memory per backend, the range arguments, and when DeleteObjects is safe to retry.
…moved UploadData, UploadFile and DownloadFile take a context. DownloadData and DeleteObject duplicated GetObject and DeleteObjects and are gone. The file calls go through the object calls, so S3's config metadata, tagging and content disposition apply to every write. OCI has one client: the SDK's 60s whole-request timeout gives way to dial, TLS and response header timeouts, which S3 and GCS clients get too.
floci_test.go starts floci's S3, Azure Blob, GCS and OCI emulators with dockertest and runs the Storage and Bucket suites on each; without Docker they skip, except under CI. AzureConfig and GCPConfig gain an Endpoint for emulators. Found by the emulators: - GCS presign signs with CredentialsJSON's service account: passed only as a token source, the SDK had no signer off GCE. URLs are V4 (7 days at most). - OCI presign and locations keep an http:// endpoint's scheme. - OCI answers a range starting at the end with 400 on floci; the object's size decides. UploadObject with IfAbsent checks first, as S3 and Azure do, since floci-oci ignores if-none-match on multipart uploads.
milos-lk
reviewed
Oct 17, 2026
milos-lk
reviewed
Oct 17, 2026
milos-lk
reviewed
Oct 17, 2026
milos-lk
reviewed
Oct 17, 2026
milos-lk
reviewed
Oct 17, 2026
milos-lk
reviewed
Oct 17, 2026
milos-lk
reviewed
Oct 17, 2026
…dule API: - UploadData removed: PutObject with a content type does the same - GeneratePresignedUrl is PresignedURL; ErrObjectNotFound/ErrObjectExists are ErrNotFound/ErrExists, as in objects - ProxyConfig.URL, S3Config.AssumeRoleARN/AssumeRoleExternalID (YAML unchanged) - S3Logger unexported Behavior: - DeleteObjects follows S3: a missing key succeeds, any key still present fails the call (S3 per-key errors were ignored on main) - Connections are cut after 5 minutes without bytes either way, replacing the response header timeout; S3 pinned to legacy defaults mode so the SDK keeps the dialer - OCI: UploadData/UploadFile one PutObject as on main, Content-Disposition inline, SDK TLS wrapper kept, OCI_CUSTOM_CLIENT_TIMEOUT honored - S3 GetObject uses the ranged downloader again - GCS: V2 signing as on main, V4 only for an http:// emulator - Local: keys outside StorageDir refused, byte-prefix listing, a directory is no object, fsync before publish, os.Create's file mode - A wrapped http.DefaultTransport no longer panics Tests: - Suites move to the e2e module, so users don't inherit dockertest's deps - New: refused deletes, conditional commit on the wire, idle timeout, proxy and GCS endpoint credentials, local keys, CI matrix completeness - floci images pinned by digest
…t per Storage; legacy defaults mode explained
…es, enterprise) API: - UploadFile(ctx, storagePath, filepath, PutOptions) and DownloadFile(ctx, storagePath, filepath): key first, IfAbsent on files - ErrorWithStatusCode lives in objects, aliased in root - A precondition failure is ErrExists only on IfAbsent writes: S3's and Azure's readers pin the ETag, so a 412 on a read is no conflict - Listings sorted on every backend, as documented Robustness: - S3: no client-side retry quota (one client serves every call), backoff between MinRetryDelay and MaxRetryDelay, MaxRetries counts retries, multipart aborted on a context of its own, idle timeout kept under any AWS_DEFAULTS_MODE, checksums dropped only for non-AWS hosts (Object Lock behind VPC/FIPS endpoints), 16 MiB parts for files, bare 416, no session token in failure logs, bounded region lookup - GCS: own transport through the SDK hook (no swap of http.DefaultTransport, which raced with the process and leaked the proxy), tokens fetched through it, 10-minute chunk retry deadline, retried deletes, bounded presign retries, public host on presign behind a private endpoint - Azure: this package's transport (idle timeout, 32 idle connections), token-only credentials sign requests, SAS clock-skew allowance - AliOSS: retries on 5xx, 429 and network errors, which its SDK never does - OCI: abort in the background, bounded namespace lookup - Proxy: credentials for http targets too, NO_PROXY, metadata services never proxied - idleConn never extends a deadline set from above - DownloadFile writes through a temporary file, so a failed download leaves the destination as it was - Local: plain writes in place as on main (mounted volumes, file watchers), IfAbsent by link; listing survives concurrent deletes and prunes directories; directory keys read as not found; parent fsync Tests: - failed bodies store nothing, race winner read back, content type read back, DownloadObject not found, DownloadFile keeps the destination, multipart abort after a deadline, AliOSS retries, proxy credentials and NO_PROXY, idle deadlines from above, listing during deletes, no write to http.DefaultTransport under -race - CI caches through setup-go keyed on both modules
…finds it with either
theomonnom
changed the title
milos-lk
reviewed
Oct 17, 2026
| // downloadFile does for the other stores. | ||
| func (s *aliOSSStorage) DownloadFile(ctx context.Context, storagePath, filepath string) (int64, error) { | ||
| if err := aliOSSRetry(ctx, func() error { | ||
| return s.bucket.GetObjectToFile(storagePath, filepath, oss.WithContext(ctx)) |
There was a problem hiding this comment.
Does AliOSS keep the DownloadFile contract? The interface says it makes the destination's directory, but GetObjectToFile opens <filepath>.temp without creating it, so a download into a directory that doesn't exist yet fails with "no such file or directory". It also doesn't remove the .temp file when the copy fails partway, so a failed download leaves it behind. I reproduced both with a fake server, like TestAliOSSRetries. Would going through downloadFile with GetObject, as the other backends do, cover both?
milos-lk
reviewed
Oct 17, 2026
| return wrapGCPError(err) | ||
| } | ||
|
|
||
| // gcsTransportMu serializes newGCSClient's swap of http.DefaultTransport. |
There was a problem hiding this comment.
nit: is the gcsTransportMu line left over? There's no mutex or http.DefaultTransport swap anymore, so it now describes code that doesn't exist.
milos-lk
reviewed
Oct 17, 2026
| func (s *ociStorage) UploadData(data []byte, storagePath, contentType string) (string, int64, error) { | ||
| size := int64(len(data)) | ||
| body := common.NewOCIReadSeekCloser(io.NopCloser(bytes.NewReader(data))) | ||
| // UploadFile sends the file in one PutObject, up to OCI's 50 GiB, which the SDK |
There was a problem hiding this comment.
nit: this doc comment appears twice in a row, in two slightly different versions. Should the second one (with "as on main" and the 100 MiB note) be the only one kept?
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
2 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.
One interface over S3 (and S3-compatible stores), GCS, Azure Blob, OCI, AliOSS and a local directory, for services that keep and manage their own objects. Breaking: every call takes a context, and the API is cleaned up (callers updated, below).
API
objects.BucketPutObject,UploadObject(stream),GetObject,GetObjectRange,DownloadObject(to anio.Writer),ObjectExists,ListObjects,DeleteObjects,PresignedURLobjects.StorageBucket+UploadFile(ctx, storagePath, filepath, PutOptions),DownloadFile(ctx, storagePath, filepath)PutOptionsContentType,IfAbsent(refuse to replace an object) on every writeErrNotFound,ErrExists(errors.Is),ErrorWithStatusCode(errors.As)storage/objectsUploadData(usePutObjectwith a content type),DownloadData(GetObject),DeleteObject(DeleteObjects),GeneratePresignedUrl→PresignedURL,ErrObjectNotFound/ErrObjectExists→ErrNotFound/ErrExists,ProxyConfig.Url→URL,AssumeRoleArn/AssumeRoleExternalId→AssumeRoleARN/AssumeRoleExternalID(YAML tags unchanged),S3LoggerunexportedSemantics
S3's, wherever stores differ:
DeleteObjectsof a missing path succeeds; any path still holding an object fails the call, so nothing is reported deleted that is still there, and a retry after a partial failure is safe.ListObjectsis recursive, in byte order, on every backend.IfAbsentis atomic where the store honors the condition;ErrExistsonly ever comes from such a write.Improvements over main
DeleteObjectsignored per-key errors (AccessDenied reported as deleted); GCS/Azure/local failed a delete on a missing key, so a retry after a partial failure never finishedProxyConfigdroppedAWS_CA_BUNDLE; GCS timeouts never applied; GCS presign off GCE had no signerhttp.DefaultTransport, racing with every other request and sending them through its proxy; now its own transportProxyConfig/NO_PROXY; proxy credentials only went on CONNECT; EC2/ECS/GKE metadata endpoints went through the proxymax_retry_delaywas unset;MinRetryDelaywas unusedEndpointfailed (checksums dropped for every endpoint, now only non-AWS hosts)blob.core.windows.net):AzureConfig.Endpoint; a token-only Azure config now signs requestsStorageDir(../)OCI_CUSTOM_CLIENT_TIMEOUTstill honored)DownloadFilewrites through a temp file: a failed download leaves the destination as it was-raceBehavior changes callers can see
DeleteObjectsfails on per-key errors other thanNoSuchKeyDeleteObjectsof a missing path succeeds on every backendMaxRetriescounts retries (5 → 6 attempts)DownloadFilereturns bytes written, also for gzip-transcoded objects-1for thoseDownloadFilecreates the destination's directory, and replaces the file only on successStorageDirand directory keys are refused / not foundGOOGLE_API_USE_CLIENT_CERTIFICATE=truekeeps the SDK's transport (mTLS), without this package's proxy and timeoutsTested
IfAbsentIf-None-Matchon put and completeifGenerationMatch=0If-None-Matchon put and commitx-oss-forbid-overwrite(not on versioned buckets)Not supported, as on main: SSE-KMS/ACL/Requester Pays headers, GCS workload identity federation JSON (ADC works), an Azure proxy setting, AliOSS STS tokens.
Callers, merged in this order
cloudimports it)