mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-10 16:40:46 +02:00
c2cdefd06d89f4510dfe06508eee7ea5aa49d607
228
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chris Luandbruce-zzz
|
0f05957bc4 |
filer: self-heal chunk manifest reads when volume locations go stale (#11107)
* filer: self-heal fetchWholeChunk on stale volume locations Upstream #10156/#10800 wired cache invalidation into the buffer-based read paths, but manifest resolution still goes through fetchWholeChunk, which returns the raw error on failure. When cached volume locations are stale (volume tiered to remote storage, server rolled), resolving a large multipart file fails permanently even though other locations are healthy. Thread the ChunkGroup's cacheInvalidator through ResolveChunkManifest / ResolveOneChunkManifest / fetchWholeChunk, and on failure invalidate, re-lookup and retry once via the existing retryFetchWithFreshLocations helper. The streaming bytesBuffer is reset before the retry so partial bytes from the failed attempt cannot corrupt the manifest proto.Unmarshal. Non-mount callers pass nil and keep their semantics. * filer: move the manifest self-heal tests in with the other manifest tests Also make the stale server stream a prefix and then abort mid-body, which is what actually leaves partial bytes in the buffer: an HTTP error status returns before ReadUrlAsStream ever calls the writer, so a 500 never exercised the Reset the tests claimed to cover. Claude-Session: https://claude.ai/code/session_01FK3oGC5ZVeJYvNBWgb9JUD * filer: keep the cached volume locations when a manifest read is cancelled A cancelled or timed-out read says nothing about where the volume lives, so dropping the location and going back to the master only costs the next reader a round trip. PrepareStreamContentWithThrottler already guards its self-heal this way. The guard also goes inside retryFetchWithFreshLocations, since the caller can be cancelled between its own check and the invalidation, and that covers the reader cache and prefetch paths too. fetchWholeChunk returns the context error rather than the stream failure it provoked, and ResolveOneChunkManifest wraps with %w so errors.Is still sees it. That matters even where no invalidator is passed: volume.fsck resolves manifests with nil and tells its own abort from a corrupt manifest that way, so the cancellation check sits ahead of the nil-invalidator return. Claude-Session: https://claude.ai/code/session_01FK3oGC5ZVeJYvNBWgb9JUD * filer: self-heal manifest reads on the filer and s3 paths too Every caller that already holds the location cache backing its lookup function can hand it over: the filer's read, copy and deletion paths and the log cache have the MasterClient right there, and s3api has the FilerClient. MinusChunks takes one for the same reason, since the deletion path resolves manifests through it. Only the shell tools and the replication sinks, whose lookup functions cache privately with nothing to invalidate, keep passing nil. Claude-Session: https://claude.ai/code/session_01FK3oGC5ZVeJYvNBWgb9JUD --------- Co-authored-by: bruce-zzz <bruce.zou@hhy-data.com> |
||
|
Chris Lu
|
9ea52db219 |
s3: validate the version-id header used as a filer path segment (#11097)
* s3: reject a version-id header that is not a valid path segment
putToFiler stored the client-supplied Seaweed-X-Amz-Version-Id header
verbatim into object metadata. That value is later read back and used
as a filer path component when building the .versions/v_<id> path, so a
value containing "/", "\" or ".." could steer retention/legal-hold
writes and remote-cache reads outside the object's own bucket tree.
Validate the header with isValidVersionID before storing it, the same
check the versioned read paths already apply, and reject the request
otherwise. Server-set version ids ("null" and generated hex) pass.
Claude-Session: https://claude.ai/code/session_011QqNaxZwnpHgMoAZNp3RkY
* s3: validate a stored version-id before using it as a path
The retention and legal-hold sinks build a .versions/v_<id> path from a
version id read back out of object metadata, and the remote-cache path
builder does the same from either the request or the stored id, without
the isValidVersionID check the other version-id consumers apply. Guard
these so a value that is not a valid path segment falls back to the
regular / unversioned path instead of steering the write or read out of
the bucket tree.
Claude-Session: https://claude.ai/code/session_011QqNaxZwnpHgMoAZNp3RkY
|
||
|
Chris Lu
|
0b5fff2ccd |
filer, s3: reuse the volume server's guarded remote-storage client builder (#10990)
* volume: build the guarded remote storage client through a shared helper Fold the endpoint validation, credential check and rebinding-safe dialer that FetchAndWriteNeedle applies before dialing a caller-supplied remote storage endpoint into a single BuildGuardedRemoteStorageClient helper, so other callers that dial the same endpoints can reuse it. No behavior change on this path. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN * filer: build the remote-mount stream client through the guarded helper streamFromRemote serves a cold remote-only entry straight from its mounted origin. Build its client through BuildGuardedRemoteStorageClient so the same endpoint checks the volume server applies cover this read path too. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN * s3: build the remote-mount stream client through the guarded helper openRemoteStream serves a remote-mounted object straight from its origin when the local read cannot. Build its client through the same guarded helper so the endpoint checks apply here as well. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN |
||
|
Chris Lu
|
d8a189f07f |
s3: keep a missing object a 404 under If-Match and If-Unmodified-Since (#10985)
* s3: keep a missing object a 404 under If-Match and If-Unmodified-Since GET and HEAD resolved the target before evaluating the conditional headers, and a missing target failed If-Match and If-Unmodified-Since outright, so absence surfaced as 412 PreconditionFailed. AWS reports the missing object instead: 404 for HeadObject, NoSuchKey for GetObject, and 412 only when a live object fails the condition. Clients cannot tell absence from a stale precondition without an extra racy HEAD, so OpenDAL disabled its four conditional stat/read capabilities against SeaweedFS. A precondition now only fails against an object that exists; a missing one -- including a latest version that is a delete marker -- returns NoSuchKey. Claude-Session: https://claude.ai/code/session_01X4kEbuwxd9DFsTnSXjfjgv * s3: evaluate a conditional read against the version the request names GET and HEAD resolved the latest version before evaluating the conditional headers, so a request carrying versionId had its If-Match compared against a different version than the one it was asking for: a live version whose ETag the client held failed once a newer version -- or a delete marker -- became the latest. resolveObjectEntry now resolves the named version on a versioned bucket, the way DELETE already does. A named version that resolves to nothing is left to the handler, which alone knows whether the bucket is versioned and so whether it owes NoSuchVersion. Claude-Session: https://claude.ai/code/session_01X4kEbuwxd9DFsTnSXjfjgv |
||
|
Chris Lu
|
3431bdcb74 |
s3: fix UploadPartCopy with volume-data encryption (#10971)
* operation: give an encrypted chunk the plaintext ETag With -encryptVolumeData the volume server stores ciphertext, so it cannot echo a Content-MD5 back and the chunk lands with an empty ETag. Every ETag derived from those chunks then comes out empty for a single chunk, or d41d8cd98f00b204e9800998ecf8427e-N for several. The caller already hashes the plaintext to send as Content-MD5, so keep that digest as the chunk ETag instead of dropping it, and compute it for a WantMd5 caller under cipher too. * s3: re-encrypt a part copy from a volume-encrypted source UploadPartCopy raw-copies source chunks when neither side uses SSE, which also caught -encryptVolumeData sources. Those chunks are ciphertext a whole-chunk cipher key decrypts, so copying a byte range out of one and keeping the key leaves a destination that fails authentication on GET, and the copied chunks carry no ETag for the part result to report. Route them through the re-encrypting path already used for SSE: it reads the source as plaintext, hashes the part, and writes the destination under the gateway's own encryption. * s3: fetch only the range a part copy asked for The re-encrypting UploadPartCopy path opened the source at offset 0 and threw the prefix away, so assembling an object part by part read the source once per part. Now that volume-encrypted sources take this path too, that is the common case rather than an SSE corner. The chunk stream already seeks, so hand it the range. * s3: reject an unsatisfiable copy-source-range A part copy has no way to report a short part, so a range reaching past the source cannot be clamped the way a GET clamps one. The fast path silently produced a part shorter than asked for, or an empty one; the re-encrypting path pads with zeros, so a 2 MiB source copied as bytes=1048576-9999999 came back as 1 MiB of data followed by 7.5 MiB of nothing. Answer InvalidRange instead, which is what s3-tests' test_multipart_copy_invalid_range expects. |
||
|
Chris Lu
|
d2c470af1b |
S3: commit SSE GET status only after the first read succeeds (#10935)
The SSE streaming path kept writing 200/206 from filer metadata before fetching or decrypting anything, so a missing needle or failed decrypt setup surfaced as a broken 200 body. Same deferral as the plain path: the status commits on the first body write, and every failure before that returns to the handler for a clean S3 error response. |
||
|
Chris Lu
|
d9d5fab35b |
S3: commit GET status only after the first read succeeds (#10930)
streamFromVolumeServers wrote the 200/206 status from filer metadata before any byte had been fetched from a volume server, so a missing or corrupted needle surfaced as a broken 200 body and the request metrics recorded a success. Defer the status commit to the first body write: a failed first read now returns a clean 500 before headers, while the wire timing of successful responses is unchanged since net/http buffers the status line until body bytes arrive anyway. |
||
|
Chris Lu
|
863fec6c3f |
S3: let a key that is a prefix of other keys be an object (#10912)
* filer: keep the sentinel when CreateEntry reports an update failure CreateEntry flattened the error UpdateEntry wraps, so errors.Is stopped matching and ErrExistingIsDirectory and ErrExistingIsFile never reached the S3 mapper, which answered a retryable 500 instead. * s3: let a key that is a prefix of other keys be an object S3 keys are flat, so "a/b" and "a/b/c" are independent objects that coexist in either write order. The filer stores a key as a path, so one of them has to live on the directory the other is nested under. Writing the nested key first refused the prefix key outright. Writing it second promoted the file to a directory, which kept its data but lost the key: an empty object left nothing to recognise it by and disappeared, and one with data listed under a trailing slash it never had. Mark the directory that carries such a key, and write the object onto it when the path is already a directory. The mark makes an empty prefix object visible to listings and readable by GET and HEAD, keeps the empty folder cleaner off it, and lists it under the key it was written with. Deleting the key strips the mark back off along with the data. * filer: keep a TTL off a directory that stands for an object An expired entry is deleted a row at a time, so expiring a directory removes it and leaves everything under it unreachable. Promoting a file to a directory carried its TTL across, and a promoted file is exactly the one that has keys nested under it. Drop the TTL on promotion, and leave one an older build wrote alone. The lifecycle worker still expires the object, through the delete that leaves the directory behind. * s3: delete the null version of a key other keys are nested under The routed delete cannot remove an entry that other keys live under, and answered a retryable 500 rather than falling back to the lock path the unversioned delete already falls back to. That path then looked the entry up under the bucket with the whole key as its name, so the demote wrote it back one directory too high and failed as not found. Fall back on any non-precondition error, and split the key before deleting it. Trailing-slash directory markers with children reach the same delete. * filer: keep the sentinel when MkFile and Mkdir report a create failure Same flattening one layer out: every mkFile caller lost the sentinel, so a CopyObject onto a key that other keys are nested under answered a retryable 500 where a PutObject of the same key answers 409. * s3: copy and rename a key that other keys are nested under Such a key is stored on the directory those keys live in, and copy and rename both refused it: the source lookup maps every directory entry to NoSuchKey, so a key a plain GET serves could not be copied or moved, and the destination side refused it as a directory conflict. The source is read through a view of the entry as the object it names. The destination is written the way a PutObject of that key writes it. A rename at either end copies the object's own data across and strips it off the source key rather than going through AtomicRenameEntry, which moves a directory by moving everything under it - the nested keys are not part of what is being renamed. |
||
|
|
930603eb74 |
S3: optionally serve remote-mounted objects from remote when the local read fails (#10837)
* feat(s3): serve from remote on local read failure When a locally-cached chunk of a remote-mounted object becomes unreadable (volume server down/restarting, or an evicted needle 404ing under retry-backoff), fall back to serving the object from its mounted remote instead of erroring. A bounded pre-flight probe makes a stuck volume trip the timeout rather than stalling the request. Gated by -localReadFallbackToRemote (default off) with -localReadFallbackTimeout (2s default), so existing deployments are unaffected until they opt in. * fix(s3): register local-read-fallback flags for mini/server/filer The mini, server and filer launchers build S3Options directly and only populate the flag pointers they register. Without registering the two new flags there, startS3Server dereferenced nil pointers and crashed at boot, failing every integration suite that runs `weed mini`. * fix(s3): treat a zero-byte probe read as unreadable A read that returns no byte -- whether it reports io.EOF or no error at all -- means the offset is not locally readable, so the probe must fall back to the remote rather than proceeding to stream a truncated response. Only a returned byte (including the object's final byte with a trailing io.EOF) counts as readable. * s3: finish a mid-stream local read failure from the remote mount The pre-flight probe only proves the byte at the requested offset readable. A multi-chunk object can still lose a later chunk after the 200/206 and its Content-Length are committed, which truncated the body with no fallback. Resume from the mounted remote at the byte the local copy stopped at, so the response still carries the declared length. A short local read that surfaces as a clean EOF is treated the same way instead of silently truncating. * s3: fall back to the remote mount without a CLI switch Serving a remote-mounted object from its authoritative remote is what the read should have done all along -- the alternative is a 500 on an object the cluster can still reach -- so make it the behavior instead of two new flags, with the probe bounded by a constant. * s3: trim the comments on the fallback path * filer: report only the contiguous prefix when a parallel chunk read fails The parallel branch of doReadAt fans the chunk reads straight into their own windows of the output buffer, then sums every task's bytesRead. A middle chunk failing while a later one succeeds therefore returned a length covering a hole the reader never filled, handing the caller zeros in the middle of otherwise valid data. * s3: only splice the remote onto a local prefix while it is the cached generation Eligibility establishes a size match, not byte identity: a remote key overwritten with same-size content between the cache fill and the fallback would have finished the response with bytes from a second generation, under the first one's ETag. Stat the remote before resuming and keep the local error when it no longer matches -- a truncated body is a visible failure, a spliced one is not. --------- Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
db5a086d04 |
read cold remote objects straight from the origin while caching (#10731)
* refactor: extract remote mount resolution into shared helpers * refactor: share the adaptive remote cache wait policy * filer: stream cold remote reads from the origin while caching * s3: stream cold remote reads from the origin instead of 503 retries * test: cover the S3 origin stream-through path * remote mounts: match on path components and prefer the longest mount * fail short origin streams instead of silently truncating * s3: try the origin before failing a cold read on a local cache error * s3: gate origin streaming on the entry's resolved version * return the cache RPC's NotFound as a canonical status and classify it everywhere * filer: keep multipart-range cold reads on the retry path |
||
|
Chris Lu
|
7d6c83b126 |
s3: stop treating a directory marker as a versioned object (#10573)
* s3: delete a directory marker instead of versioning it The key "dir/" is stored as the filer directory itself, so a delete marker cannot stand in for it without hiding the children underneath, and its history has to sit inside the directory it describes, where listings keep meeting it. Delete it the way an unversioned bucket already does: remove the directory when nothing is left under it, demote it to a plain directory when children remain, and drop a history an older build recorded for it. * s3: stop resolving directory markers through a version history Nothing records one for them any more, so the lookups that read it are dead weight - and the one in the listing was a filer round trip per directory marker returned, which for a bucket that keeps a marker per directory is the whole listing cost. A listing reads what a directory stands for straight off the entry it already has; a unit test pins that N markers cost one ListEntries rather than N+1. The guard that keeps a history left inside a directory by an older build from surfacing as a key named after it stays. * s3: do not let deleting "dir/" destroy the object at "dir" Writing under an existing object turns that object's entry into a directory while it keeps its data, so the keys "m2" and "m2/" end up sharing one entry. Stripping the entry to delete "m2/" therefore wiped the object at "m2" - a different key, and in a versioned bucket one no delete marker records. Leave a directory holding uploaded data alone; "m2/" does not name it. * s3: make the directory-marker delete fail closed and take the write lock The guard that spares a promoted file only fired when the entry read succeeded, so a transient filer error fell through to the delete and could destroy the object at "dir" anyway. Fail the request instead, take the object write lock so the entry cannot change between the check and the delete, and report a stale history that cannot be removed rather than leaving it to keep naming the key in ListObjectVersions. * s3: check If-Match inside the directory-marker delete lock The lock belongs to the caller: taking it inside the delete nested it under the batch handler's own lock, and since every lock from a gateway shares one owner the inner release would have freed it while the outer caller still assumed it held it. Both callers now own the lock, the single-object path re-checks If-Match inside it the way the other delete paths do, and a batch delete of a trailing-slash key in an unversioned bucket goes through the same marker path instead of the raw delete. A history lookup that fails now fails the delete. |
||
|
Chris Lu
|
474a0713b0 |
s3: honor the version history of a directory marker (#10571)
* s3: stop listing a directory marker whose latest version is a delete marker A directory marker is stored as the filer directory itself, so deleting the key "dir/" writes its delete marker into dir/.versions while the directory keeps its mime and stays a key object. Every listing kept reporting the key. Consult that history before treating the entry as a key, and demote it in memory when it is delete-marked so live children still hold the prefix. Also skip the container's own .versions entry while listing inside it: the suffix match read it as the history of a nested object named "", which surfaces as a phantom dir/dir key as soon as a live directory version exists. * s3: a directory marker with version history is not also the latest null version The directory entry behind the key "dir/" is that key's null version, but list-object-versions reported it with IsLatest hardcoded true. After a delete the key came back twice, once as the delete marker and once as a null version, both claiming to be latest. Read the pointer under the directory instead. * s3: resolve directory markers through their version history on GET and HEAD GET and HEAD short-circuit any trailing-slash key straight to the filer directory, so a directory marker kept answering 200 after its delete marker was written. Resolve the key from dir/.versions first when the bucket is versioned: a delete-marked current version answers 404 with x-amz-delete-marker, a named delete-marker version answers 405, and a key with no history keeps today's directory-probe behavior untouched. * s3: re-creating a directory marker retires its delete marker PutObject on a trailing-slash key never looked at the bucket's versioning state, so re-creating a marker after a delete left the latest-version pointer on the delete marker and the key stayed invisible to every versioned read. Point the key back at the directory entry, which is its null version, and drop the null version .versions may still hold — the same two steps a suspended write already takes, now shared. * s3: fail a directory-marker request whose version history cannot be read Every lookup of dir/.versions treated any error as "no history", so a filer hiccup served the directory entry for a key whose current version may be a delete marker, reported a null version as latest over one, and let a PUT report success without retiring the delete marker it was meant to retire. Only a confirmed absence takes the no-history path now. * s3: cancel the directory-marker probe stream instead of abandoning it The probe answers off the first entry and returns, leaving the ListEntries stream open for the life of the parent context. Give it a context of its own. |
||
|
Lars Lehtonen
|
2bea4dd610 | chore(weed/s3api): prune dead code (#10462) | ||
|
Chris Lu
|
68a4e3347f |
S3: track manifest blob ownership through multipart completion (#10386)
* s3: track manifest blob ownership through multipart completion Manifest blobs made three orphan paths. A partial fold that failed midway kept its earlier batches on volume servers while the write fell back to flat chunks; the fold now records each saved blob and deletes them on error. A completion that failed after preparing left its fresh manifests behind on every retry; the completion state now owns them and deletes them unless a failed rollback left the version entry still holding them. And a completed upload removes its parts metadata-only, which stranded the part-manifest blobs superseded by flattening; those are collected during flattening and deleted once the completion commits. Two shared-chunk hazards nearby: the version-file rollback deleted its data, destroying the still-registered parts (worse once manifests resolve to inner chunks), and the idempotent-replay cleanup data-deleted leftover parts whose chunks the live object references. Both are metadata-only now. * s3: trim chunk manifest comments * s3: test manifest fold rollback and part range selection The fold-with-rollback and the boundary-to-byte-range logic were only exercised by hand against a live server; give both an injectable seam and cover the fold, the below-threshold and SSE no-ops, the midway failure deleting the blobs it saved, and offset-vs-legacy-index range selection including indexes that no longer address the chunk list. |
||
|
Chris Lu
|
542495f1f1 |
S3: fold large chunk lists into manifest chunks on the direct write path (#10383)
s3: fold large chunk lists into manifest chunks on the direct write path The S3 gateway uploads chunks itself and hands the filer a fully prepared entry. On the routed write path (ObjectTransaction) the filer stores that entry as-is, so a large PutObject or CompleteMultipartUpload persisted its whole flat chunk list - a 900GB object carries 120k chunk references in one entry. Manifestize on the gateway before the entry is written, the same way mount, WebDAV, and filer.copy prepare theirs. Multipart part boundaries also record byte offsets now: the stored chunk indexes stop matching the entry once the list is folded, and partNumber reads plus GetObjectAttributes prefer the offsets. Legacy index-only records still work, with bounds checks instead of a possible panic. Copy paths resolve a manifested source into data chunks before their per-chunk copy loops - copying a manifest chunk raw would store its blob as object data still pointing at the source - and a large copied list is folded again on the destination. Completion likewise resolves manifest chunks a part entry may carry (the filer folds an oversized UploadPartCopy range) before rebasing part offsets. |
||
|
Chris Lu
|
6f14be1138 |
stats: remote-mount bucket cache hit/miss metrics (#10352)
Reads of remote-backed entries now record hit or miss in
SeaweedFS_remote_cache_read_total{source,bucket,result} on the filer HTTP
path and the S3 gateway, so cache effectiveness of mounted buckets can be
graphed. Inline-content entries count as hits since they are served
locally without chunks. The filer purges the per-bucket series when the
bucket directory is deleted, so a standalone filer does not accumulate
series across bucket delete/recreate churn.
|
||
|
Cole Helbling
|
eefe634962 |
s3api: FULL_OBJECT checksums for CRC multipart uploads (#10236)
CRC64NVME multipart objects now emit a full-object checksum instead of a composite base64-N value, matching AWS (CRC64NVME supports full-object only). Adds x-amz-checksum-type handling (COMPOSITE/FULL_OBJECT) for CRC32/CRC32C/CRC64NVME via CRC combination, resolved at CreateMultipartUpload and applied at completion. |
||
|
Chris Lu
|
d0e47cf4da |
s3: answer directory-path probes like AWS so Flink savepoints restore (#10225)
* s3: answer application/x-directory for a directory without a stored mime A real directory reached via a trailing-slash GET/HEAD answered the octet-stream default when it had no stored mime, so Hadoop-style S3 filesystems (flink-s3-fs-presto and friends) classified the path as a 0-byte file instead of a directory and then failed reading it as one. Answer application/x-directory, the marker type those clients probe for. Stored mimes still echo verbatim, and a file promoted to a directory keeps octet-stream for its data. * s3: 404 GET and HEAD on a bare directory path consistently A directory with no object data of its own answered differently per path: plain GET gave an empty 200, ranged GET and non-versioned HEAD gave 404, and on versioned buckets the null-version fallback adopted the filer directory as a 0-byte object and answered 200. Clients that probe HEAD-then-GET took the 200s at face value, treated the path as an empty file, and never fell back to LIST-based directory discovery. Answer 404 for a bare directory path everywhere, which is what AWS returns for a prefix. A file promoted to a directory keeps its data and stays retrievable. |
||
|
Chris Lu
|
95427b5573 |
security: add BearerPrefix constant for Authorization headers (#10101)
Introduce security.BearerPrefix ("Bearer ", RFC 6750) and use it
everywhere an "Authorization: Bearer <token>" header is constructed,
replacing the scattered "BEARER "/"Bearer " string literals. SeaweedFS
matches the scheme case-insensitively when parsing (security.GetJwt), so
behavior is unchanged; this removes the magic string and settles the
casing on the standard form. The parser's upper-case comparison stays as
is on purpose.
|
||
|
Chris Lu
|
7abed4e517 |
s3: skip 503 when client disconnects during remote cache wait (#10071)
s3: don't write 503 to a disconnected client during remote cache wait When the remote-only cache poll returns without chunks, re-check the request context before emitting 503 + Retry-After. A client that disconnected during the wait surfaces as context.Canceled, which the caller already handles silently; writing to the closed connection only produced broken-pipe log noise. |
||
|
Chris Lu
|
63f2f0bef5 |
s3: keep a file promoted to a directory retrievable as an object (#10070)
* filer: treat a directory carrying object data as an S3 key object A file promoted to a directory by a child write keeps its chunks, inline content, or remote-tiered entry. Recognize that as a directory key object, not only when a Mime is set, so the object still lists, demotes on delete, and is not reclaimed by cleanup like the object it still is. * filer: keep the empty-folder cleaner from reclaiming a promoted object The cleaner skips directory key objects, but its check only looked at the Mime. Mirror the chunks/content/remote check so a file promoted to a directory is not deleted once its children are gone. * s3: serve ranged GET for a directory that carries object data Reject only zero-size directories so a file promoted to a directory streams range requests instead of returning 404, while empty directories still 404. * s3: return HEAD metadata for a directory that carries object data HEAD now 404s a directory only when it has no data, so a promoted object is retrievable while empty/implicit directories still fall back to LIST. |
||
|
patrick
|
d7860ddf24 | s3api: reject malformed Range offsets (#10034) | ||
|
Chris Lu
|
b763a5f6bf |
s3: improve TTFB for large remote objects (#10010)
* s3: add streaming reader interface for remote storage Add RemoteStorageStreamReader optional interface to support efficient streaming of large remote objects without buffering entire file in memory. This enables future stream-through caching where data can be served to clients while simultaneously writing to volume servers. Implement ReadFileAsStream() for S3, GCS, and Azure backends using their native streaming APIs. This provides the foundation for improving TTFB on large remote file access by serving data directly from remote storage while background cache operation populates local chunks. The streaming interface allows remote storage backends to return io.ReadCloser, enabling efficient memory usage for multi-GB objects compared to the current ReadFile() approach which buffers entire ranges in memory. * s3: adaptive timeout for remote object caching to improve TTFB Use size-aware cache polling timeout to balance cache-hit rate against time-to-first-byte: - Small files (<50MB): 10s timeout - more likely to complete caching before timeout, improving subsequent request performance - Medium files (50-500MB): 5s timeout - default balance - Large files (>500MB): 2s timeout - fail-fast to improve initial TTFB for very large downloads This reduces waiting time for large remote files while maintaining high cache-hit rate for smaller files that cache quickly. * s3: address code review feedback for stream-through cache - Move startBackgroundRemoteCache call after policy recheck to avoid cache side effects for denied requests (authorization first) - Make startBackgroundRemoteCache version-aware by accepting versionId parameter and using buildVersionedRemoteObjectPath - Add timeout (5 minutes) to background cache context to prevent goroutine pile-up if RPC stalls under load - Update cacheRemoteObjectForStreamingWithShortTimeout to return both entry and error, allowing callers to distinguish transient errors (timeout/cancellation) from permanent errors (not found, denied) - Update streamFromVolumeServers to handle permanent cache errors with appropriate HTTP status codes (404 for not found, 503 for transient) |
||
|
Chris Lu
|
b888cec106 |
s3: bound streaming remote-cache wait so large cold GetObject returns 503, not a hang (#9988)
* s3: bound streaming remote-cache wait so large cold GetObject returns 503, not a hang A remote-only object whose download outlasts the 30s dedup attempt fell through to cacheRemoteObjectForStreaming, which waited on the raw request context with no bound. For multi-GB blobs the client read timeout fires first, so the request was canceled and surfaced as InternalError rather than the 503 + Retry-After the handler already emits. The filer keeps caching on a detached context, so the retry would have streamed from the cached chunks. Bound the wait under common client read timeouts: the 503 fires while the client is still connected, and a retry picks up the finished cache. * s3: make the streaming remote-cache timeout atomic The bound is a package-level var so tests can shorten it. Hold it as an atomic int64 so a test that stores a short value can never race the request path that loads it. * s3: assert the streaming-cache timeout test waits on the bound The upper-bound-only check would also pass if the RPC returned immediately on a setup error. Add a lower bound so the test fails unless the bounded wait fired. |
||
|
Chris Lu
|
2d1b8be22b |
s3: route object reads to the key's owner filer (#9806)
* s3: route object reads to the key's owner filer Writes already route by key to the owner filer on the lock ring, where the entry is created. Reads went to the gateway's local filer and treated its NotFound as authoritative, so a GET on one gateway could miss an object another gateway had just written until the filers' metadata replication caught up. Resolve an object's entry from the key's owner first, failing over to the gateway's filer set only on transport errors. An owner NotFound stays authoritative: no fan-out across filers, and no resurrecting a peer's not-yet-replicated tombstone, so a delete routed to the owner is visible at once and a genuine miss costs one lookup. Keys owned by the local filer are unchanged. Objects written through the non-routed lock path land on a gateway's local filer, so they can still read as absent on the owner until they replicate. withFilerClientFailover takes a preferred start filer; the object-entry reads pass the owner, every other caller passes "" and keeps the current-filer fast path. * s3: consult the prior owner on a rebalance-window read miss Owner-first reads route a key to its current ring owner. When a filer joins, ~1/N of keys reassign to it, and the new owner may not have replicated a just-moved key yet, so an owner NotFound would surface a transient 404 for an object that already exists elsewhere. Retain the previous ring on the gateway's LockClient for a cooling-off window (PriorOwnerForKey, mirroring the master's LockRing.PriorOwner) and, on the owner's NotFound, probe the key's previous owner once before treating the miss as final. The probe is scoped to keys whose ownership actually moved and only within the window, so steady-state reads are untouched. This trades the transient scale-up 404 for a transient stale read if a delete routed to the new owner races the same window — the same authoritative-NotFound tradeoff, narrowed to the rebalance. * s3: try healthy filers before unhealthy ones on failover The candidate list probed its first entry (usually the current filer) unconditionally, so a health-flagged current filer cost a transport timeout on every ordinary call before failover reached a replica. Partition candidates into healthy and unhealthy, keep priority within each, and fall back to unhealthy ones only when all healthy ones fail. * reduce comments on the routed read and lock client paths * s3: skip a recently-unreachable owner on route-by-key reads The gateway's filer health tracking no-ops for an owner outside the static -filer list, so during a sustained owner outage every route-by-key read re-dials the dead owner before failing over. Flag an owner whose owner-first read hit a transport error and skip it (read local-first) for a short TTL, so reads pay one dead dial per TTL instead of one per request; the flag expires so owner-first reads resume once the owner or the ring recovers. * s3: always try the preferred owner first, health-order only the rest The healthy/unhealthy partition also demoted a health-flagged preferred owner behind healthy replicas, so a replica's authoritative NotFound could mask a write that had only reached the owner — the read-after-write race this routing exists to close. Pull preferred out of the partition and keep it first; the recently-unreachable gate already steers reads away from a genuinely dead owner. |
||
|
Chris Lu
|
0e35235908 |
s3: return NoSuchVersion (not NoSuchKey) for a missing versionId (#9749)
GET/HEAD object with an explicit versionId that does not exist returned NoSuchKey. AWS S3 returns NoSuchVersion (404) for this case; tools that distinguish "key gone" from "this version gone" rely on that code. Add the ErrNoSuchVersion error code and use it on the GET and HEAD specific-version lookups. Only a genuine not-found maps to NoSuchVersion; a transient or internal filer error now maps to InternalError (500) instead of a misleading 404. getSpecificObjectVersion wraps its lookup error with %w so callers can detect filer_pb.ErrNotFound. |
||
|
AlekseyandChris Lu
|
917a87928c |
fix(s3api/list): cancel ListEntries stream in hasChildren (#9617)
* fix(s3api/list): cancel ListEntries stream in hasChildren * fix(s3api): use filer_pb.List in hasChildren filer_pb.List already wraps the ListEntries stream in a cancellable context, so the single-entry probe needs no separate helper or manual context plumbing to avoid the leaked gRPC stream goroutine. * fix(s3api): propagate request context into hasChildren Thread r.Context() through listFilerEntries and hasChildren so the implicit-directory probe cancels when the client disconnects, instead of running on context.Background(). --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
3f1eaf9724 |
fix(s3/audit): emit audit log for successful GET/HEAD (#9467)
* fix(s3/audit): emit audit log for successful GET/HEAD Successful GET/HEAD object requests never produced a fluent audit entry because those handlers write the response directly (streaming for GET, WriteHeader for HEAD) and never reach a PostLog call site. The wiki advertises GET as an audited verb, so the asymmetry surprises operators who rely on the log for read-access auditing. Move the safety net into the track() middleware: tag each request with an audit-tracking flag, let PostLog/PostAccessLog (delete path) mark it, and emit a single fallback entry after the handler returns when nothing fired. The recorder's status flows into the fallback so the audit row still reflects 200/206 vs 404 etc. No double logging for handlers that already emit (write helpers, error paths, bulk delete). Refs #9463 * fix(s3/audit): defensive nil checks on audit-tracking helpers Address PR review: guard against nil request and nil *atomic.Bool stored under the audit-tracking key. The conditions are unreachable today (the key is private and we only ever store new(atomic.Bool)), but the checks are free and keep the helpers safe if a future caller misbehaves. * test(s3/audit): track() audit fallback coverage + stale comment cleanup (#9469) test(s3/audit): cover track() fallback wiring + cleanup Adds two unit tests in weed/s3api/stats_test.go that exercise the audit-tracking flag set up by track(): one verifies the fallback path fires when a handler writes the response directly (the GET/HEAD object regression in #9463), the other verifies the flag is set when a handler emits PostLog itself so the fallback is skipped. To make the wiring observable without standing up fluent, PostLog now marks the audit flag before short-circuiting on a nil Logger; production behavior is unchanged (no logger, no posting) but the flag stays consistent. Also drops two stale comments in s3api_object_handlers.go that still referenced proxyToFiler — that helper was removed when GET/HEAD started streaming from volume servers directly. Stacks on #9467. |
||
|
Chris Lu
|
6844ec067c |
fix(s3): cache remote-only source before CopyObject (#9304) (#9305)
* fix(s3): cache remote-only source before CopyObject (#9304) CopyObject from a remote.mount source whose object lives only upstream created a destination entry with FileSize > 0 but no chunks/content, because the resolved source entry has no local chunks and the copy path fell into the "inline/empty chunks" branch with empty entry.Content. A subsequent GET returned 500 with "data integrity error: size N reported but no content available". CopyObjectPart had the same shape via copyChunksForRange iterating an empty chunk list. Detect entry.IsInRemoteOnly() right after resolving the source in both CopyObjectHandler and CopyObjectPartHandler and cache the object to the local cluster first via a new cacheRemoteObjectForCopy helper (a copy-time analogue of cacheRemoteObjectForStreaming with a bounded 30s timeout and version-aware path resolution). If caching fails or produces no chunks, return 503 with Retry-After: 5 instead of writing a metadata-only destination, mirroring the GetObject behavior added in the #7817 cold-cache fix. Adds TestCopyObjectRemoteOnlySourceDetection pinning the four entry shapes the fix branches on plus the pre-fix broken-output shape. * address PR review on remote-only copy fix - Use the resolved entry's version id when srcVersionId is empty so a CopyObject reading the latest object in a versioning-enabled bucket caches the correct .versions/v_<id> path instead of getting stuck in a 503 retry loop. New helper resolvedSourceVersionId handles the fallback for both CopyObject and CopyObjectPart. - Drop the redundant cachedEntry.IsInRemoteOnly() recheck in both handlers; the cache helper now reports success based on local data presence, and IsInRemoteOnly does not look at inline Content so keeping the check would 503 on small inline-cached objects. - Treat inline Content as a successful cache result in both cacheRemoteObjectForStreaming and cacheRemoteObjectForCopy via a shared cachedEntryHasLocalData predicate. The CopyObject inline branch already handles entries that have Content but no chunks. - Extract buildVersionedRemoteObjectPath so the streaming and copy cache helpers share path construction. Adds TestResolvedSourceVersionId and TestCachedEntryHasLocalData to pin the new helpers' contracts. * narrow streaming cache contract back to chunks-only CodeRabbit flagged that cacheRemoteObjectForStreaming's caller in streamFromVolumeServers (lines 997-1002) still required non-empty chunks, so content-only cache hits would fall through to a 503 retry loop instead of being honored. Resolve by keeping the helper's contract chunks-only: the filer's caching code only ever writes chunks, the streaming downstream isn't wired to read inline Content from a cached entry, and a partial range- aware inline writer here would be overkill for a path that doesn't actually occur in practice. cacheRemoteObjectForCopy keeps the relaxed contract since the copy path's inline branch genuinely handles both chunked and content-only entries. Document the asymmetry on cachedEntryHasLocalData and on cacheRemoteObjectForStreaming so a future reader can see why the two helpers diverge. * extend version-id resolution to streaming cache path CodeRabbit flagged that GetObjectHandler still passed the raw query versionId to cacheRemoteObjectForStreaming. For latest-version reads in versioning-enabled buckets that stays empty even though the resolved entry lives at .versions/v_<id>, so remote-only GETs would keep caching the wrong path and 503-ing forever. Reuse the new resolvedSourceVersionId helper at the streaming call site too. Also document on cachedEntryHasLocalData that the zero-byte case flagged in the same review is handled upstream (IsInRemoteOnly requires RemoteSize > 0, so the cache helper is never invoked for empty remote objects -- CopyObject's pre-existing inline branch writes a correct empty destination directly). Pin this with a new test case. * trim verbose comments Drop tutorial-style and review-history comments. Keep only the WHY that isn't obvious from identifiers: the #9304 reference on the new branches in CopyObject / CopyObjectPart, the latest-version-fallback rationale on resolvedSourceVersionId, and the streaming/copy contract asymmetry on cachedEntryHasLocalData. * drop issue references from comments Issue numbers belong in PR descriptions and commit messages, not in source comments where they rot. Replace with the underlying invariant the code is preserving. * test: drive remote-object cache helpers through real gRPC Existing tests only re-enacted helper-function branching in test space, so they could not have caught a handler that consumed a remote-only entry without going through the cache. Stand up an in-process filer gRPC server (UnimplementedSeaweedFilerServer + configurable CacheRemoteObjectToLocalCluster response) and exercise the two cache helpers end-to-end. What's pinned: - cacheRemoteObjectForCopy returns nil when the cache makes no progress (response is still remote-only), lets gRPC errors through as nil, accepts both chunked and inline-content cache hits, and surfaces deadline-exceeded as nil so callers can 503 instead of holding the request open. - Versioned source paths route to .versions/v_<id>; non-versioned and "null" stay at the bucket-relative path. Captured by reading the request the stub server received. - cacheRemoteObjectForStreaming holds the stricter chunks-only contract: a content-only cache hit is not propagated, since streamFromVolumeServers' downstream isn't wired to read from inline Content there. Any current or future handler that calls these helpers exercises the same gRPC path under test, so the bug class is closed for helper-routed cache calls. * move remote-only copy test into the integration suite The previous gRPC-stub test in weed/s3api/ was integration-flavored but stubbed; relocate the coverage to the existing two-server suite under test/s3/remote_cache/, which already exercises the real remote.mount + remote.uncache flow against a primary SeaweedFS plus a secondary acting as remote storage. The new test/s3/remote_cache/remote_cache_copy_test.go drives: - TestRemoteCacheCopyObject: upload to primary, uncache (entry now remote-only), CopyObject to a new key, GET the destination. Pre- fix the GET returned 500 'data integrity error: size N reported but no content'; this pins the fixed behavior over real HTTP through the actual handler stack. - TestRemoteCacheCopyObjectPart: same shape via multipart UploadPartCopy on a 6 MiB object split into two parts, exercising CopyObjectPartHandler's range-copy path. Drop weed/s3api/s3api_remote_storage_grpc_test.go: the helper-level classification tests in s3api_remote_storage_test.go still cover the contract pieces (cachedEntryHasLocalData, resolvedSourceVersionId, the remote-only entry shape), and the integration suite covers the end-to-end behavior that those classifications enable. |
||
|
Chris Lu
|
a2639b533e |
fix(s3api): return 503 + Retry-After when remote object not cached yet (#9233)
* fix(s3api): return 503 with Retry-After when remote object not cached yet When a GET hits a remote-only object whose cache fill timed out or was canceled, the handler returned 500 InternalError. SDK clients treat 500 as a server bug and surface it as a fatal error (boto3 S3DownloadFailedError), even though the cache is often still filling in the background and the next request would succeed. Return 503 ServiceUnavailable with Retry-After: 5 instead, matching AWS S3's "try again later" semantics. AWS SDKs already classify 503 as retryable and apply exponential backoff transparently, so clients recover without changes. Refs https://github.com/seaweedfs/seaweedfs/discussions/9174 * treat client cancel as cancellation, not 503 If r.Context() is already canceled when the cache attempt returns no chunks, the cache failure is almost certainly a side-effect of the client disconnecting, not real backpressure. Surface the context error so GetObjectHandler logs at V(3) and skips writing a response, instead of synthesizing a 503 that nobody will read. Addresses Gemini review feedback on #9233. * simplify comments |
||
|
Chris Lu
|
4f628ff4e5 |
fix(s3api): stream multipart-SSE chunks lazily to avoid truncated GETs (#8908) (#9228)
* fix(s3api): stream multipart SSE-S3 chunks lazily to avoid truncated GETs (#8908) buildMultipartSSES3Reader opened a volume-server HTTP response for EVERY chunk upfront, then walked them with io.MultiReader. For a multipart SSE-S3 object with N internal chunks (e.g. a 200MB Docker Registry blob with 25+ chunks), N volume-server bodies sat live at once; chunks 1..N-1 were idle while io.MultiReader drained chunk 0. Under concurrent load the volume server's keep-alive logic closed those idle responses mid-flight, and the S3 client saw `unexpected EOF` partway through the GET. Truncated bytes hash to the wrong SHA-256, which is exactly the "Digest did not match" symptom Docker Registry reports in #8908 (and which persisted even after the per-chunk metadata fix in #9211 and the completion backfill in #9224). Introduce lazyMultipartChunkReader + preparedMultipartChunk{chunk, wrap}: a generic lazy chunk streamer with a per-chunk wrap closure for the SSE-specific decryption setup. Per-chunk metadata is still validated UPFRONT so a malformed chunk fails fast without opening any HTTP connection -- the eager validation contract callers and tests rely on is preserved. The volume-server GET and the SSE-specific decrypt wrap, however, fire LAZILY: at most one chunk body is live at any time, regardless of object size. This commit applies the new pattern to buildMultipartSSES3Reader only; the SSE-KMS and SSE-C multipart readers retain their eager form for now and will be migrated in follow-up commits, since the same shape exists there too. Tests: - TestBuildMultipartSSES3Reader_LazyChunkFetch pins the new contract: zero chunks opened at construction, peak liveness == 1, all closed after drain. - TestBuildMultipartSSES3Reader_RejectsBadChunkBeforeAnyFetch (replaces ClosesAppendedOnError) asserts a malformed chunk in position N causes zero fetches for chunks 0..N -- the previous test pinned a weaker contract (cleanup after eager open). - TestBuildMultipartSSES3Reader_InvalidIVLength updated for the same reason: the fetch callback must NOT be invoked at all on a bad-IV chunk. - TestMultipartSSES3RealisticEndToEnd round-trips multiple parts encrypted the way putToFiler writes them (shared DEK + baseIV, partOffset=0, post-completion global offsets) and walks them through buildMultipartSSES3Reader. * fix(s3api): stream multipart SSE-KMS chunks lazily Apply the same fix as the previous commit to createMultipartSSEKMSDecryptedReaderDirect: per-chunk SSE-KMS metadata is validated upfront, but volume-server GETs fire lazily through lazyMultipartChunkReader. At most one chunk body is live at any time. This is the same eager-open-all-chunks shape that produced #8908's truncated GETs for SSE-S3; SSE-KMS multipart objects with many chunks were exposed to the same idle-keepalive failure mode under concurrent load. The wire format on disk is unchanged (same per-chunk metadata, same encrypted bytes, same object Extended attributes). Existing SSE-KMS multipart objects read back identically -- only when the volume-server GETs fire changes. * fix(s3api): stream multipart SSE-C chunks lazily Apply the same fix as the previous two commits to createMultipartSSECDecryptedReaderDirect: per-chunk SSE-C metadata is validated upfront (IV decode, IV length check, non-negative PartOffset), but the volume-server GET and CreateSSECDecryptedReader- WithOffset wrap fire lazily through lazyMultipartChunkReader. At most one chunk body is live at any time. This is the same eager-open-all-chunks shape that produced #8908's truncated GETs for SSE-S3; SSE-C multipart objects with many chunks were exposed to the same idle-keepalive failure mode under concurrent load. The pre-existing TODO note about CopyObject SSE-C PartOffset handling is preserved verbatim. The wire format on disk is unchanged (same per-chunk metadata, same encrypted bytes); existing SSE-C multipart objects read back identically. After this commit all three multipart SSE read paths (SSE-S3, SSE-KMS, SSE-C) share lazyMultipartChunkReader as their streaming engine. * test(s3): add Docker Registry-shape multipart SSE-S3 GET regression Pin the end-to-end fix for #8908 with a test that mirrors what Docker Registry actually does on pull: a 25-part * 5MB upload with bucket- default SSE-S3, then a full GET, then SHA-256 over the streamed body must match SHA-256 over the uploaded bytes. The eager-multipart-reader bug was specifically a streaming truncation under load: the response status was 200 with a Content-Length matching the object size, but the body short-circuited mid-stream because later chunks' volume-server connections had already been closed by keepalive. The hash check is the symptom Docker Registry surfaces ("Digest did not match"), so this is the most faithful regression we can pin without spinning up a registry. uploadAndVerifyMultipartSSEObject already byte-compares the GET body, but hashing on top is intentionally explicit -- it documents WHY the test exists, and matches the failure mode reported in the issue. * test(s3): add range-read coverage matrix across SSE modes and sizes Existing range-read coverage in test/s3/sse was scoped to small (<= 1MB) single-chunk objects, with one ad-hoc range case per SSE mode and one 129-byte boundary-crossing case in TestSSEMultipartUploadIntegration. Nothing exercised: - Range reads on single-PUT objects whose content crosses the 8MB internal chunk boundary (medium size class). - Range reads on multipart objects whose parts each span multiple internal chunks (large size class) -- the shape #8908 originally surfaced for full-object GETs and the most likely site of any future regression in per-chunk IV / PartOffset plumbing for partial reads. - A consistent range-pattern set applied uniformly across SSE modes, so any divergence between modes (SSE-C uses random IV + PartOffset; SSE-S3/KMS use base IV + offset) is comparable at a glance. TestSSERangeReadCoverageMatrix introduces a parameterized matrix: modes: no_sse, sse_c, sse_kms, sse_s3 sizes: small (256KB single chunk), medium (12MB single PUT crossing one internal boundary), large (5x9MB multipart, ~10 internal chunks, every part itself spans an 8MB boundary) ranges: single byte at 0, prefix 512B, single byte at last, suffix bytes=-100, open-ended bytes=N-, whole object, AES-block boundary 15-31, mid straddling one internal boundary (medium+large), mid spanning many internal boundaries (large only) Per case it asserts: body bytes equal the expected slice, Content-Length matches the range length, Content-Range matches start-end/total, and the SSE response headers match the mode. The sse_kms branch probes once with a 1-byte SSE-KMS PUT and t.Skip's the remaining sse_kms subtests with a clear reason if the local server has no KMS provider configured -- the default `weed mini` setup lacks one; the Makefile target `test-with-kms` provides one via OpenBao. Other modes always run. Verified locally: 75 subtests pass under no_sse / sse_c / sse_s3 against weed mini, sse_kms cleanly skipped. * test(s3): conform new test names to TestSSE*Integration so CI runs them The two tests added in the previous commits had names that did NOT match the patterns the test/s3/sse Makefile and .github/workflows/s3-sse-tests.yml use to discover SSE integration tests: - test/s3/sse/Makefile `test` target: TestSSE.*Integration - test/s3/sse/Makefile `test-multipart`: TestSSEMultipartUploadIntegration - .github/workflows/s3-sse-tests.yml: ...|.*Multipart.*Integration|.*RangeRequestsServerBehavior Result: SSE-KMS coverage I added to TestSSERangeReadCoverageMatrix and the Docker-Registry-shape multipart regression in TestSSES3MultipartManyChunks_DockerRegistryShape were silently invisible to CI even though the underlying test setup (start-seaweedfs-ci using s3-config-template.json with the embedded `local` KMS provider) already has SSE-KMS configured. Renames: TestSSERangeReadCoverageMatrix -> TestSSERangeReadIntegration TestSSES3MultipartManyChunks_... -> TestSSEMultipartManyChunksIntegration Both names now match `TestSSE.*Integration` (Makefile `test` target) and TestSSEMultipartManyChunksIntegration additionally matches `.*Multipart.*Integration` (CI's comprehensive subset). No behavior change; only the function names move. Verified locally against `weed mini` with s3-config-template.json: TestSSERangeReadIntegration runs 96 leaf subtests across 4 SSE modes (none, SSE-C, SSE-KMS, SSE-S3) x 3 size classes x 7-9 range patterns, all passing, 0 skipped. The probe-and-skip in the SSE-KMS arm now only fires for ad-hoc local setups that don't load any KMS provider; the project's standard test setup loads the local provider, so CI has full SSE-KMS range coverage. * fix(s3api): validate SSE-KMS chunk IV during prep, before any fetch Addresses CodeRabbit review on PR #9228: in createMultipartSSEKMSDecryptedReaderDirect the per-chunk SSE-KMS metadata was deserialized in the prep loop but the IV length was only validated later, inside CreateSSEKMSDecryptedReader, which runs from the wrap closure -- AFTER the chunk's volume-server fetch has already started. That weakens the new "reject malformed chunks before any fetch" contract for SSE-KMS specifically: a chunk with a missing/short/long IV would fire its HTTP GET, then fail mid-stream during decrypt. The fix moves the existing ValidateIV check into the prep loop, matching the SSE-S3 and SSE-C paths. Drive-by: extract the SSE-KMS prep loop into a free buildMultipartSSEKMSReader helper that mirrors buildMultipartSSES3Reader, so the new contract is unit-testable without an S3ApiServer. The exported method (createMultipartSSEKMSDecryptedReaderDirect) stays a thin caller, so behavior for production callers is unchanged. New tests in weed/s3api/s3api_multipart_ssekms_test.go pin the contract: - TestBuildMultipartSSEKMSReader_RejectsBadIVBeforeAnyFetch covers missing IV, empty IV, short IV, long IV. Each case asserts both that an error is returned AND that the fetch callback is never invoked. - TestBuildMultipartSSEKMSReader_RejectsMissingMetadataBeforeAnyFetch pins the analogous behavior when SseMetadata is nil on a chunk in position N: chunks 0..N-1 must not be fetched (the earlier eager implementation depended on a closeAppendedReaders cleanup path; the new contract is stronger -- nothing is opened in the first place). - TestBuildMultipartSSEKMSReader_RejectsUnparseableMetadataBeforeAnyFetch covers the JSON-unmarshal failure branch. - TestBuildMultipartSSEKMSReader_SortsByOffset smoke-tests the documented sort-by-offset contract by recording the order in which fetch is invoked. All four pass under `go test ./weed/s3api/`. Existing weed/s3api unit suite + the SSE integration suite (with the local KMS provider enabled via s3-config-template.json) continue to pass. * test(s3): address CodeRabbit nitpicks on range coverage matrix Three small follow-ups on the range-read coverage matrix from the previous commit, per CodeRabbit nitpicks on PR #9228: 1. Promote the body-length check from `assert.Equal` to `require.Equal` so a truncation regression -- the canonical #8908 failure mode -- aborts the subtest immediately. Previously the assertion logged a length mismatch and then `assertDataEqual` ran on differently-sized slices, producing a noisy byte-diff on top of the actual symptom. The redundant trailing `t.Fatalf` block becomes dead and is removed. 2. Broaden the SSE-KMS probe-skip heuristic. The probe previously produced the friendly "KMS provider not configured" message only for 5xx responses; KMS-misconfig surfaces also include 501 NotImplemented, 4xx KMS.NotConfigured, and error messages containing "KMS.NotConfigured" / "NotImplemented" / "not configured". The behaviour change is purely cosmetic (the caller t.Skip's on any non-empty reason either way) but the new diagnostic is more useful in CI logs. 3. Add `t.Parallel()` at the mode and size-class levels of the matrix. Each (mode, size) writes an independent object key under the shared bucket, with no cross-talk, so parallel execution is safe. Local wall time on the full matrix dropped from ~2.0s to ~1.1s (~45%); the savings scale with chunk count and CI machine concurrency. Verified locally against `weed mini` with s3-config-template.json: - go test ./weed/s3api/ -count=1 PASS - TestSSERangeReadIntegration -v 112 PASS, 0 SKIP - TestSSEMultipartUploadIntegration etc. PASS * fix(s3api): tighten lazy reader error path; unify SSE IV validation Three CodeRabbit nitpicks on PR #9228: 1. lazyMultipartChunkReader: mark finished on non-EOF Read errors The Read loop's three earlier failure paths (chunk index past end, fetch error, wrap error) all set l.finished = true before returning. The non-EOF Read path -- where l.current.Read itself errors mid-chunk -- did not, leaving l.current/l.closer set and l.finished = false. A caller that retried Read after an error would re-enter the same broken stream instead of advancing or giving up. Set l.finished = true on non-EOF Read error so post-error state is consistent across all four failure sites; Close() (which the GetObjectHandler defers) still releases the chunk body. 2. Unify IV-length validation across SSE-S3, SSE-KMS, SSE-C prep paths The previous commit moved SSE-KMS to the shared ValidateIV helper but left SSE-S3 and SSE-C with bespoke inline `len(...) != AESBlockSize` checks. All three are enforcing the same invariant; inconsistency obscures the symmetry. Move SSE-S3 and SSE-C to ValidateIV too, with the same `<algo> chunk <fileId> IV` name convention. Error message wording shifts from "<algo> chunk X has invalid IV length N (expected 16)" to ValidateIV's "invalid <algo> chunk X IV length: expected 16 bytes, got N". The substring "IV length" is preserved across both, so the existing TestBuildMultipartSSES3Reader_InvalidIVLength substring assertion is loosened to match either form. 3. TestBuildMultipartSSEKMSReader_SortsByOffset: verify full ordering The test previously drove Read() to observe fetch-call order, but CreateSSEKMSDecryptedReader requires a live KMS provider to unwrap the encrypted DEK -- unavailable in unit tests -- so the wrap closure failed on the first chunk and only one fetch was ever recorded. The test asserted only fetchOrder[0] == "c0", which is weaker than the comment promised. Switch to a static check: type-assert the returned reader to *lazyMultipartChunkReader (same package so unexported fields are accessible) and inspect the prepared chunks slice directly. This pins the entire [c0, c1, c2] sort order in one place, doesn't depend on KMS, and runs in zero fetch calls. The fetch closure now asserts it is never invoked during preparation. All weed/s3api unit tests pass; integration suite (with KMS provider configured via s3-config-template.json) passes. * test(s3): switch range coverage cleanup to t.Cleanup; tighten KMS probe Two CodeRabbit comments on PR #9228, both about test/s3/sse/s3_sse_range_coverage_test.go: 1. CRITICAL: defer + t.Parallel() race in TestSSERangeReadIntegration The test creates one bucket up front, then runs subtests that call t.Parallel() at the mode and size levels (added in |
||
|
Chris Lu
|
525900dfe4 |
fix(s3api): backfill multipart SSE-S3 metadata at completion (#9224)
* fix(s3api): backfill missing per-chunk SSE-S3 metadata at completion When a part of an SSE-S3 multipart upload lands with SseType=NONE on its chunks (e.g. a transient failure to apply SSE-S3 setup in PutObjectPart), the completed object inherits NONE-tagged chunks and detectPrimarySSEType then misses the chunked SSE-S3 encryption. The read path falls through to the unencrypted serve and GET returns ciphertext, producing the SHA mismatch reported in #8908. Recover at completion using the base IV and key data the upload directory recorded at CreateMultipartUpload: - extractMultipartSSES3Info validates upload-entry metadata up front and hard-fails completion if the base IV or key data are malformed; serializing chunk metadata we then could not decrypt is worse than rejecting the upload. - completedMultipartChunk re-derives a per-chunk IV from baseIV + chunk.Offset (matching what putToFiler would have written) and serializes per-chunk SSE-S3 metadata when the chunk has no tag. Existing per-chunk metadata is left alone; we cannot recover an already-derived IV from the upload-entry alone. The IV formula intentionally has no partNumber term: putToFiler hardcodes partOffset=0 when it calls handleSSES3MultipartEncryption for every part, so each chunk's encryption IV is calculateIVWithOffset(baseIV, chunk.Offset_part_local). PartOffsetMultiplier is defined in s3_constants but is not consumed by the encryption path. Adopting (partNumber-1)*PartOffsetMultiplier + chunk.Offset would produce IVs that fail to decrypt the bytes on disk - a stronger failure mode than the bug being fixed. Tests pin this: - TestCompletedMultipartChunkBackfilledIVDecryptsActualCiphertext runs the round trip across the encryption boundary: encrypt parts with CreateSSES3EncryptedReaderWithBaseIV (the call putToFiler uses), drop chunk metadata to reproduce #8908, backfill, decrypt with backfilled IV, assert plaintext intact. - TestCompletedMultipartChunkRejectsPartNumberMultiplierFormula constructs the IV the partNumber formula would produce and shows it does not decrypt the actual ciphertext. This commit covers the chunk-level recovery only. The companion fix for the object-level Extended attributes (SeaweedFSSSES3Key / X-Amz-Server-Side-Encryption) follows separately. * fix(s3api): backfill canonical SSE-S3 attributes onto multipart object The previous commit ensures every chunk of an SSE-S3 multipart upload carries SseType=SSE_S3 with a per-chunk IV, so the multipart-direct read path can decrypt. The completed object's Extended map can still miss the canonical pair detectPrimarySSEType and IsSSES3EncryptedInternal look at: - X-Amz-Server-Side-Encryption (the AmzServerSideEncryption header detectPrimarySSEType reads on inline / small-object reads) - x-seaweedfs-sse-s3-key (SeaweedFSSSES3Key, required by IsSSES3EncryptedInternal and by the read-path key lookup) When a part of the upload was written by a path that did not set those (the same #8908 race that produced the NONE chunks), copySSEHeadersFromFirstPart finds nothing to copy and the final entry ends up with only the multipart-init keys (SeaweedFSSSES3Encryption / BaseIV / KeyData). The read path then mis-detects the object as unencrypted. applyMultipartSSES3HeadersFromUploadEntry writes the canonical pair from the multipart-init metadata in all three completion paths (versioned, suspended, non-versioned), only when the keys are missing so a healthy first part still wins. extractMultipartSSES3Info already ran in prepareMultipartCompletionState, so the data is reused without re-decoding. Tests: TestApplyMultipartSSES3HeadersFromUploadEntry covers backfill, do-not-clobber, and nil-info no-op cases. * fix(s3api): drop double IV adjustment in SSE-KMS chunk view decrypt decryptSSEKMSChunkView was pre-adjusting the SSE-KMS chunk IV (calculateIVWithOffset(baseIV, ChunkOffset)) and then handing the adjusted IV to CreateSSEKMSDecryptedReader, which itself runs calculateIVWithOffset(IV, ChunkOffset) on whatever it receives. The offset was being applied twice for any chunk with a non-zero ChunkOffset, corrupting the keystream for range reads that cross multipart chunk boundaries. Pass the raw SSE-KMS key (with base IV and the original ChunkOffset field) into CreateSSEKMSDecryptedReader so the offset is applied exactly once, and remove the now-dead intra-block skip that was compensating for the double adjustment. Add an anti-test inside TestSSEKMSDecryptChunkView_RequiresOffsetAdjustment that decrypts the same ciphertext with a deliberately double-adjusted IV and asserts the output is corrupted, so any regression that re-introduces the double application fails the unit test. * test(s3): cover multipart SSE across chunk-spanning parts and ranges Adds an integration subtest "Multipart Parts Larger Than Internal Chunks Across SSE Types" to TestSSEMultipartUploadIntegration that exercises the end-to-end S3 path for the bugs fixed in this branch: - Two-part multipart upload with each part larger than the 8MB internal SeaweedFS chunk, so each part itself spans multiple underlying chunks. - Subtests for SSE-C, SSE-KMS, explicit SSE-S3, and bucket-default SSE-S3 - the four paths multipart parts can take through the SSE pipeline. - Each subtest does a full GET (verifying every byte and the response Content-Length / SSE response headers) plus a 129-byte range read straddling the 8MB internal chunk boundary, which is the path that produced the SSE-KMS double-IV corruption (fix in the previous commit) and the SSE-S3 chunk-tag loss (fix in the earlier commits). Factored the request shape behind multipartSSEOptions / uploadAndVerifyMultipartSSEObject so all four SSE flavors share the same upload+verify code; only the SSE-specific input/output configuration differs per subtest. * test(s3): abort orphan multipart uploads on test failure Address coderabbit nitpick on uploadAndVerifyMultipartSSEObject. The helper used require.NoError after CreateMultipartUpload, UploadPart and CompleteMultipartUpload, so a failure in any of those (or in the later GET / range read on a still-incomplete upload) called t.Fatal without aborting the in-flight MPU, leaving an orphan upload in the bucket. Harmless in CI where the data dir is wiped on shutdown, but a real annoyance when iterating locally and a textbook AWS S3 caveat in production. Register a t.Cleanup that calls AbortMultipartUpload unless a "completed" flag was set right after a successful CompleteMultipartUpload. Use context.Background for the abort call since the parent ctx may already be cancelled at cleanup time, and t.Logf the abort error rather than failing the test so the original failure remains visible in the run output. |
||
|
Chris Lu
|
d65c568cbb |
fix(s3api): validate SSE-S3 chunk IV length; add multipart direct reader tests (#9218)
* fix(s3api): validate SSE-S3 chunk IV length; add multipart direct reader tests DeserializeSSES3Metadata does not require an IV, and a corrupted or legacy chunk without one would have flowed into cipher.NewCTR and panicked. Validate that each per-chunk IV is exactly AESBlockSize bytes before decryption, closing the current and any already-appended chunk readers on error. Factor the per-chunk decryption loop out of createMultipartSSES3DecryptedReaderDirect into buildMultipartSSES3Reader so it can be driven with a mock chunk fetcher, and add tests covering: the happy path with two parts (distinct per-chunk DEKs/IVs, out-of-order chunks) to lock in the fix from #9211; missing-IV and short-IV metadata rejection without panic; and reader cleanup when a later chunk fails. * address review: sort chunks copy; close encryptedStream on error - buildMultipartSSES3Reader now sorts a copy of the chunks slice so callers do not observe entry.Chunks reordered (other code paths, e.g. ETag computation, can rely on the original order). - createMultipartSSES3DecryptedReaderDirect now closes encryptedStream on the error path from buildMultipartSSES3Reader. All current callers pass nil, but this keeps cleanup symmetric with the success path. - Extend TestBuildMultipartSSES3Reader_PerChunkKeys to assert the input slice is not mutated. * address review: defer single close; extend chunk-copy + IV-guard pattern - createMultipartSSES3DecryptedReaderDirect: collapse the duplicated encryptedStream.Close() calls into a single nil-guarded defer so the error and success paths share cleanup. - createMultipartSSECDecryptedReaderDirect, createMultipartSSEKMSDecryptedReaderDirect: sort a copy of entry.Chunks instead of mutating the caller's slice, matching the SSE-S3 helper. - createMultipartSSECDecryptedReaderDirect: validate per-chunk IV length before handing it to cipher.NewCTR; a base64-decoded empty or short IV from malformed/corrupt metadata would otherwise panic. - SSE-KMS needs no IV guard: CreateSSEKMSDecryptedReader already calls ValidateIV before cipher.NewCTR. Note recorded in the sort comment. * address review: close appended readers on SSE-C/SSE-KMS error paths createMultipartSSECDecryptedReaderDirect and createMultipartSSEKMSDecryptedReaderDirect only closed the current chunk reader on error and leaked any chunk readers already appended to the local readers slice, mirroring the leak previously fixed in the SSE-S3 helper. Add the same closeAppendedReaders() closure pattern to both functions and invoke it on every error return inside the loop so failed requests do not leak volume-server HTTP connections. * address review: defer encryptedStream close in SSE-C/SSE-KMS; drop chunks reassignment - Move encryptedStream.Close() to a nil-guarded defer at the top of createMultipartSSECDecryptedReaderDirect and createMultipartSSEKMSDecryptedReaderDirect so the stream is closed on every return path (including error returns from inside the per-chunk loop), mirroring the SSE-S3 helper. - In buildMultipartSSES3Reader, iterate sortedChunks directly instead of reassigning chunks = sortedChunks. |
||
|
os-pradipbabarandChris Lu
|
8815844278 |
fix(s3api): correct SSE-S3 decryption key handling in multipart uploads (#9211)
* fix(s3api): correct SSE-S3 decryption key handling in multipart uploads * fix(s3api): preallocate readers and close on error in SSE-S3 direct path Address review feedback on createMultipartSSES3DecryptedReaderDirect: preallocate the readers slice with the known chunk count, and close any already-appended chunk readers on error returns so failed requests do not leak volume-server HTTP connections. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
228ed25a01 |
perf(s3): route GET through ChunkReadAt + per-request ReaderCache (#9068)
perf(s3): route GET through ChunkReadAt + shared ReaderCache
The S3 GET path previously used filer.PrepareStreamContentWithPrefetch,
which hands chunk bytes from the volume-server fetch goroutine to the
consumer through an io.Pipe. io.Pipe is a synchronous rendezvous, so
the prefetch=4 window only overlapped HTTP connection setup — the
actual data bytes still flowed one pipe at a time.
Switch to the same path WebDAV uses (server/webdav_server.go): build
a filer.ChunkReadAt backed by a server-wide filer.ReaderCache.
ReaderCache prefetches whole chunks into []byte buffers, so the
prefetch window translates into real in-flight bytes and the consumer
copies them out as memcpys.
The ReaderCache is server-wide (not per-request) for two reasons:
1. ChunkReadAt.Close() destroys the ReaderCache's downloader map.
With a per-request cache, the defer on the handler would wait for
background chunk downloads that run on context.Background() — so
a client disconnect would block handler cleanup on downloads that
the client no longer wants, tying up goroutines and memory.
2. Concurrent requests for the same object can share in-flight
downloads through the shared downloader map.
No persistent ChunkCache is added in this commit — the ReaderCache is
constructed with a nil *chunk_cache.TieredChunkCache (all its methods
are nil-receiver safe). A follow-up PR wires in an in-memory chunk
cache for cross-request warm hits.
JWT for volume-server requests is generated internally by
util_http.RetriedFetchChunkData from jwtSigningReadKey, so the new
path remains compatible with JWT-protected clusters — this is the
same mechanism the WebDAV and mount read paths have been using.
Measured on weed mini + 1 GiB random object over loopback, cold
cache, single-stream curl on a presigned URL:
before (io.Pipe): 2100-2200 MB/s
after (ChunkReadAt): 2900-3800 MB/s
|
||
|
Chris Lu
|
0798b274dd |
feat(s3): add concurrent chunk prefetch for large file downloads (#8917)
* feat(s3): add concurrent chunk prefetch for large file downloads Add a pipe-based prefetch pipeline that overlaps chunk fetching with response writing during S3 GetObject, SSE downloads, and filer proxy. While chunk N streams to the HTTP response, fetch goroutines for the next K chunks establish HTTP connections to volume servers ahead of time, eliminating the RTT gap between sequential chunk fetches. Uses io.Pipe for minimal memory overhead (~1MB per download regardless of chunk size, vs buffering entire chunks). Also increases the streaming read buffer from 64KB to 256KB to reduce syscall overhead. Benchmark results (64KB chunks, prefetch=4): - 0ms latency: 1058 → 2362 MB/s (2.2× faster) - 5ms latency: 11.0 → 41.7 MB/s (3.8× faster) - 10ms latency: 5.9 → 23.3 MB/s (4.0× faster) - 20ms latency: 3.1 → 12.1 MB/s (3.9× faster) * fix: address review feedback for prefetch pipeline - Fix data race: use *chunkPipeResult (pointer) on channel to avoid copying struct while fetch goroutines write to it. Confirmed clean with -race detector. - Remove concurrent map write: retryWithCacheInvalidation no longer updates fileId2Url map. Producer only reads it; consumer never writes. - Use mem.Allocate/mem.Free for copy buffer to reduce GC pressure. - Add local cancellable context so consumer errors (client disconnect) immediately stop the producer and all in-flight fetch goroutines. * fix(test): remove dead code and add Range header support in test server - Remove unused allData variable in makeChunksAndServer - Add Range header handling to createTestServer for partial chunk read coverage (206 Partial Content, 416 Range Not Satisfiable) * fix: correct retry condition and goroutine leak in prefetch pipeline - Fix retry condition: use result.fetchErr/result.written instead of copied to decide cache-invalidation retry. The old condition wrongly triggered retry when the fetch succeeded but the response writer failed on the first write (copied==0 despite fetcher having data). Now matches the sequential path (stream.go:197) which checks whether the fetcher itself wrote zero bytes. - Fix goroutine leak: when the producer's send to the results channel is interrupted by context cancellation, the fetch goroutine was already launched but the result was never sent to the channel. The drain loop couldn't handle it. Now waits on result.done before returning so every fetch goroutine is properly awaited. |
||
|
Chris Lu
|
3efe88c718 |
feat(s3): store and return checksum headers for additional checksum algorithms (#8914)
* feat(s3): store and return checksum headers for additional checksum algorithms When clients upload with --checksum-algorithm (SHA256, CRC32, etc.), SeaweedFS validated the checksum but discarded it. The checksum was never stored in metadata or returned in PUT/HEAD/GET responses. Now the checksum is computed alongside MD5 during upload, stored in entry extended attributes, and returned as the appropriate x-amz-checksum-* header in all responses. Fixes #8911 * fix(s3): address review feedback and CI failures for checksum support - Gate GET/HEAD checksum response headers on x-amz-checksum-mode: ENABLED per AWS S3 spec, fixing FlexibleChecksumError on ranged GETs and multipart copies - Verify computed checksum against client-provided header value for non-chunked uploads, returning BadDigest on mismatch - Add nil check for getCheckSumWriter to prevent panic - Handle comma-separated values in X-Amz-Trailer header - Use ordered slice instead of map for deterministic checksum header selection; extract shared mappings into package-level vars * fix(s3): skip checksum header for ranged GET responses The stored checksum covers the full object. Returning it for ranged (partial) responses causes SDK checksum validation failures because the SDK validates the header value against the partial content received. Skip emitting x-amz-checksum-* headers when a Range request header is present, fixing PyArrow large file read failures. * fix(s3): reject unsupported checksum algorithm with 400 detectRequestedChecksumAlgorithm now returns an error code when x-amz-sdk-checksum-algorithm or x-amz-checksum-algorithm contains an unsupported value, instead of silently ignoring it. * feat(s3): compute composite checksum for multipart uploads Store the checksum algorithm during CreateMultipartUpload, then during CompleteMultipartUpload compute a composite checksum from per-part checksums following the AWS S3 spec: concatenate raw per-part checksums, hash with the same algorithm, format as "base64-N" where N is part count. The composite checksum is persisted on the final object entry and returned in HEAD/GET responses (gated on x-amz-checksum-mode: ENABLED). Reuses existing per-part checksum storage from putToFiler and the getCheckSumWriter/checksumHeaders infrastructure. * fix(s3): validate checksum algorithm in CreateMultipartUpload, error on missing part checksums - Move detectRequestedChecksumAlgorithm call before mkdir callback so an unsupported algorithm returns 400 before the upload is created - Change computeCompositeChecksum to return an error when a part is missing its checksum (the upload was initiated with a checksum algorithm, so all parts must have checksums) - Propagate the error as ErrInvalidPart in CompleteMultipartUpload * fix(s3): return checksum header in CompleteMultipartUpload response, validate per-part algorithm - Add ChecksumHeaderName/ChecksumValue fields to CompleteMultipartUploadResult and set the x-amz-checksum-* HTTP response header in the handler, matching the AWS S3 CompleteMultipartUpload response spec - Validate that each part's stored checksum algorithm matches the upload's expected algorithm before assembling the composite checksum; return an error if a part was uploaded with a different algorithm |
||
|
Chris Lu
|
995dfc4d5d |
chore: remove ~50k lines of unreachable dead code (#8913)
* chore: remove unreachable dead code across the codebase Remove ~50,000 lines of unreachable code identified by static analysis. Major removals: - weed/filer/redis_lua: entire unused Redis Lua filer store implementation - weed/wdclient/net2, resource_pool: unused connection/resource pool packages - weed/plugin/worker/lifecycle: unused lifecycle plugin worker - weed/s3api: unused S3 policy templates, presigned URL IAM, streaming copy, multipart IAM, key rotation, and various SSE helper functions - weed/mq/kafka: unused partition mapping, compression, schema, and protocol functions - weed/mq/offset: unused SQL storage and migration code - weed/worker: unused registry, task, and monitoring functions - weed/query: unused SQL engine, parquet scanner, and type functions - weed/shell: unused EC proportional rebalance functions - weed/storage/erasure_coding/distribution: unused distribution analysis functions - Individual unreachable functions removed from 150+ files across admin, credential, filer, iam, kms, mount, mq, operation, pb, s3api, server, shell, storage, topology, and util packages * fix(s3): reset shared memory store in IAM test to prevent flaky failure TestLoadIAMManagerFromConfig_EmptyConfigWithFallbackKey was flaky because the MemoryStore credential backend is a singleton registered via init(). Earlier tests that create anonymous identities pollute the shared store, causing LookupAnonymous() to unexpectedly return true. Fix by calling Reset() on the memory store before the test runs. * style: run gofmt on changed files * fix: restore KMS functions used by integration tests * fix(plugin): prevent panic on send to closed worker session channel The Plugin.sendToWorker method could panic with "send on closed channel" when a worker disconnected while a message was being sent. The race was between streamSession.close() closing the outgoing channel and sendToWorker writing to it concurrently. Add a done channel to streamSession that is closed before the outgoing channel, and check it in sendToWorker's select to safely detect closed sessions without panicking. |
||
|
msementsovandChris Lu
|
4c13a9ce65 |
Client disconnects create context cancelled errors, 500x errors and Filer lookup failures (#8845)
* Update stream.go Client disconnects create context cancelled errors and Filer lookup failures * s3api: handle canceled stream requests cleanly * s3api: address canceled streaming review feedback --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Lars Lehtonen
|
5c5d377277 |
weed/s3api: prune test-only functions (#8840)
weed/s3api: prune functions that are referenced only from tests and the tests that exercise them. |
||
|
Lars Lehtonen
|
b01a74c6bb |
Prune Unused Functions from weed/s3api (#8815)
* weed/s3api: prune calculatePartOffset() * weed/s3api: prune clearCachedListMetadata() * weed/s3api: prune S3ApiServer.isObjectRetentionActive() * weed/s3api: prune S3ApiServer.ensureDirectoryAllEmpty() * weed/s3api: prune s3ApiServer.getEncryptionTypeString() * weed/s3api: prune newStreamError() * weed/s3api: prune S3ApiServer.rotateSSECKey() weed/s3api: prune S3ApiServer.rotateSSEKMSKey() weed/s3api: prune S3ApiServer.rotateSSEKMSMetadataOnly() weed/s3api: prune S3ApiServer.rotateSSECChunks() weed/s3api: prune S3ApiServer.rotateSSEKMSChunks() weed/s3api: prune S3ApiServer.rotateSSECChunk() weed/s3api: prune S3ApiServer.rotateSSEKMSChunk() * weed/s3api: prune addCounterToIV() * weed/s3api: prune minInt() * weed/s3api: prune isMethodActionMismatch() * weed/s3api: prune hasSpecificQueryParameters() * weed/s3api: prune handlePutToFilerError() weed/s3api: prune handlePutToFilerInternalError() weed/s3api: prune logErrorAndReturn() weed/s3api: prune logInternalError weed/s3api: prune handleSSEError() weed/s3api: prune handleSSEInternalError() * weed/s3api: prune encryptionConfigToProto() * weed/s3api: prune S3ApiServer.touch() |
||
|
Chris Lu
|
38e14a867b |
fix: cancel volume server requests on client disconnect during S3 downloads (#8373)
* fix: cancel volume server requests on client disconnect during S3 downloads - Use http.NewRequestWithContext in ReadUrlAsStream so in-flight volume server requests are properly aborted when the client disconnects and the request context is canceled - Distinguish context-canceled errors (client disconnect, expected) from real server errors in streamFromVolumeServers; log at V(3) instead of ERROR to reduce noise from client-side disconnects (e.g. Nginx upstream timeout, browser cancel, curl --max-time) Fixes: streamFromVolumeServers: streamFn failed...context canceled" * fixup: separate Canceled/DeadlineExceeded log severity in streamFromVolumeServers - context.Canceled → V(3) Infof "client disconnected" (expected, no noise) - context.DeadlineExceeded → Warningf "server-side deadline exceeded" (unexpected, needs attention) - all other errors → Errorf (unchanged)" |
||
|
Chris Lu
|
5a0204310c |
Add Iceberg admin UI (#8246)
* Add Iceberg table details view
* Enhance Iceberg catalog browsing UI
* Fix Iceberg UI security and logic issues
- Fix selectSchema() and partitionFieldsFromFullMetadata() to always search for matching IDs instead of checking != 0
- Fix snapshotsFromFullMetadata() to defensive-copy before sorting to prevent mutating caller's slice
- Fix XSS vulnerabilities in s3tables.js: replace innerHTML with textContent/createElement for user-controlled data
- Fix deleteIcebergTable() to redirect to namespace tables list on details page instead of reloading
- Fix data-bs-target in iceberg_namespaces.templ: remove templ.SafeURL for CSS selector
- Add catalogName to delete modal data attributes for proper redirect
- Remove unused hidden inputs from create table form (icebergTableBucketArn, icebergTableNamespace)
* Regenerate templ files for Iceberg UI updates
* Support complex Iceberg type objects in schema
Change Type field from string to json.RawMessage in both IcebergSchemaFieldInfo
and internal icebergSchemaField to properly handle Iceberg spec's complex type
objects (e.g. {"type": "struct", "fields": [...]}). Currently test data
only shows primitive string types, but this change makes the implementation
defensively robust for future complex types by preserving the exact JSON
representation. Add typeToString() helper and update schema extraction
functions to marshal string types as JSON. Update template to convert
json.RawMessage to string for display.
* Regenerate templ files for Type field changes
* templ
* Fix additional Iceberg UI issues from code review
- Fix lazy-load flag that was set before async operation completed, preventing retries
on error; now sets loaded flag only after successful load and throws error to caller
for proper error handling and UI updates
- Add zero-time guards for CreatedAt and ModifiedAt fields in table details to avoid
displaying Go zero-time values; render dash when time is zero
- Add URL path escaping for all catalog/namespace/table names in URLs to prevent
malformed URLs when names contain special characters like /, ?, or #
- Remove redundant innerHTML clear in loadIcebergNamespaceTables that cleared twice
before appending the table list
- Fix selectSnapshotForMetrics to remove != 0 guard for consistency with selectSchema
fix; now always searches for CurrentSnapshotID without zero-value gate
- Enhance typeToString() helper to display '(complex)' for non-primitive JSON types
* Regenerate templ files for Phase 3 updates
* Fix template generation to use correct file paths
Run templ generate from repo root instead of weed/admin directory to ensure
generated _templ.go files have correct absolute paths in error messages
(e.g., 'weed/admin/view/app/iceberg_table_details.templ' instead of
'app/iceberg_table_details.templ'). This ensures both 'make admin-generate'
at repo root and 'make generate' in weed/admin directory produce identical
output with consistent file path references.
* Regenerate template files with correct path references
* Validate S3 Tables names in UI
- Add client-side validation for table bucket and namespace names to surface
errors for invalid characters (dots/underscores) before submission
- Use HTML validity messages with reportValidity for immediate feedback
- Update namespace helper text to reflect actual constraints (single-level,
lowercase letters, numbers, and underscores)
* Regenerate templ files for namespace helper text
* Fix Iceberg catalog REST link and actions
* Disallow S3 object access on table buckets
* Validate Iceberg layout for table bucket objects
* Fix REST API link to /v1/config
* merge iceberg page with table bucket page
* Allowed Trino/Iceberg stats files in metadata validation
* fixes
- Backend/data handling:
- Normalized Iceberg type display and fallback handling in weed/admin/dash/s3tables_management.go.
- Fixed snapshot fallback pointer semantics in weed/admin/dash/s3tables_management.go.
- Added CSRF token generation/propagation/validation for namespace create/delete in:
- weed/admin/dash/csrf.go
- weed/admin/dash/auth_middleware.go
- weed/admin/dash/middleware.go
- weed/admin/dash/s3tables_management.go
- weed/admin/view/layout/layout.templ
- weed/admin/static/js/s3tables.js
- UI/template fixes:
- Zero-time guards for CreatedAt fields in:
- weed/admin/view/app/iceberg_namespaces.templ
- weed/admin/view/app/iceberg_tables.templ
- Fixed invalid templ-in-script interpolation and host/port rendering in:
- weed/admin/view/app/iceberg_catalog.templ
- weed/admin/view/app/s3tables_buckets.templ
- Added data-catalog-name consistency on Iceberg delete action in weed/admin/view/app/iceberg_tables.templ.
- Updated retry wording in weed/admin/static/js/s3tables.js.
- Regenerated all affected _templ.go files.
- S3 API/comment follow-ups:
- Reused cached table-bucket validator in weed/s3api/bucket_paths.go.
- Added validation-failure debug logging in weed/s3api/s3api_object_handlers_tagging.go.
- Added multipart path-validation design comment in weed/s3api/s3api_object_handlers_multipart.go.
- Build tooling:
- Fixed templ generate working directory issues in weed/admin/Makefile (watch + pattern rule).
* populate data
* test/s3tables: harden populate service checks
* admin: skip table buckets in object-store bucket list
* admin sidebar: move object store to top-level links
* admin iceberg catalog: guard zero times and escape links
* admin forms: add csrf/error handling and client-side name validation
* admin s3tables: fix namespace delete modal redeclaration
* admin: replace native confirm dialogs with modal helpers
* admin modal-alerts: remove noisy confirm usage console log
* reduce logs
* test/s3tables: use partitioned tables in trino and spark populate
* admin file browser: normalize filer ServerAddress for HTTP parsing
|
||
|
Chris Luandgemini-code-assist[bot]
|
e6ee293c17 |
Add table operations test (#8241)
* Add Trino blog operations test * Update test/s3tables/catalog_trino/trino_blog_operations_test.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * feat: add table bucket path helpers and filer operations - Add table object root and table location mapping directories - Implement ensureDirectory, upsertFile, deleteEntryIfExists helpers - Support table location bucket mapping for S3 access * feat: manage table bucket object roots on creation/deletion - Create .objects directory for table buckets on creation - Clean up table object bucket paths on deletion - Enable S3 operations on table bucket object roots * feat: add table location mapping for Iceberg REST - Track table location bucket mappings when tables are created/updated/deleted - Enable location-based routing for S3 operations on table data * feat: route S3 operations to table bucket object roots - Route table-s3 bucket names to mapped table paths - Route table buckets to object root directories - Support table location bucket mapping lookup * feat: emit table-s3 locations from Iceberg REST - Generate unique table-s3 bucket names with UUID suffix - Store table metadata under table bucket paths - Return table-s3 locations for Trino compatibility * fix: handle missing directories in S3 list operations - Propagate ErrNotFound from ListEntries for non-existent directories - Treat missing directories as empty results for list operations - Fixes Trino non-empty location checks on table creation * test: improve Trino CSV parsing for single-value results - Sanitize Trino output to skip jline warnings - Handle single-value CSV results without header rows - Strip quotes from numeric values in tests * refactor: use bucket path helpers throughout S3 API - Replace direct bucket path operations with helper functions - Leverage centralized table bucket routing logic - Improve maintainability with consistent path resolution * fix: add table bucket cache and improve filer error handling - Cache table bucket lookups to reduce filer overhead on repeated checks - Use filer_pb.CreateEntry and filer_pb.UpdateEntry helpers to check resp.Error - Fix delete order in handler_bucket_get_list_delete: delete table object before directory - Make location mapping errors best-effort: log and continue, don't fail API - Update table location mappings to delete stale prior bucket mappings on update - Add 1-second sleep before timestamp time travel query to ensure timestamps are in past - Fix CSV parsing: examine all lines, not skip first; handle single-value rows * fix: properly handle stale metadata location mapping cleanup - Capture oldMetadataLocation before mutation in handleUpdateTable - Update updateTableLocationMapping to accept both old and new locations - Use passed-in oldMetadataLocation to detect location changes - Delete stale mapping only when location actually changes - Pass empty string for oldLocation in handleCreateTable (new tables have no prior mapping) - Improve logging to show old -> new location transitions * refactor: cleanup imports and cache design - Remove unused 'sync' import from bucket_paths.go - Use filer_pb.UpdateEntry helper in setExtendedAttribute and deleteExtendedAttribute for consistent error handling - Add dedicated tableBucketCache map[string]bool to BucketRegistry instead of mixing concerns with metadataCache - Improve cache separation: table buckets cache is now separate from bucket metadata cache * fix: improve cache invalidation and add transient error handling Cache invalidation (critical fix): - Add tableLocationCache to BucketRegistry for location mapping lookups - Clear tableBucketCache and tableLocationCache in RemoveBucketMetadata - Prevents stale cache entries when buckets are deleted/recreated Transient error handling: - Only cache table bucket lookups when conclusive (found or ErrNotFound) - Skip caching on transient errors (network, permission, etc) - Prevents marking real table buckets as non-table due to transient failures Performance optimization: - Cache tableLocationDir results to avoid repeated filer RPCs on hot paths - tableLocationDir now checks cache before making expensive filer lookups - Cache stores empty string for 'not found' to avoid redundant lookups Code clarity: - Add comment to deleteDirectory explaining DeleteEntry response lacks Error field * go fmt * fix: mirror transient error handling in tableLocationDir and optimize bucketDir Transient error handling: - tableLocationDir now only caches definitive results - Mirrors isTableBucket behavior to prevent treating transient errors as permanent misses - Improves reliability on flaky systems or during recovery Performance optimization: - bucketDir avoids redundant isTableBucket call via bucketRoot - Directly use s3a.option.BucketsPath for regular buckets - Saves one cache lookup for every non-table bucket operation * fix: revert bucketDir optimization to preserve bucketRoot logic The optimization to directly use BucketsPath bypassed bucketRoot's logic and caused issues with S3 list operations on delimiter+prefix cases. Revert to using path.Join(s3a.bucketRoot(bucket), bucket) which properly handles all bucket types and ensures consistent path resolution across the codebase. The slight performance cost of an extra cache lookup is worth the correctness and consistency benefits. * feat: move table buckets under /buckets Add a table-bucket marker attribute, reuse bucket metadata cache for table bucket detection, and update list/validation/UI/test paths to treat table buckets as /buckets entries. * Fix S3 Tables code review issues - handler_bucket_create.go: Fix bucket existence check to properly validate entryResp.Entry before setting s3BucketExists flag (nil Entry should not indicate existing bucket) - bucket_paths.go: Add clarifying comment to bucketRoot() explaining unified buckets root path for all bucket types - file_browser_data.go: Optimize by extracting table bucket check early to avoid redundant WithFilerClient call * Fix list prefix delimiter handling * Handle list errors conservatively * Fix Trino FOR TIMESTAMP query - use past timestamp Iceberg requires the timestamp to be strictly in the past. Use current_timestamp - interval '1' second instead of current_timestamp. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> |
||
|
Chris Lu
|
c284e51d20 |
fix: multipart upload ETag calculation (#8238)
* fix multipart etag * address comments * clean up * clean up * optimization * address comments * unquoted etag * dedup * upgrade * clean * etag * return quoted tag * quoted etag * debug * s3api: unify ETag retrieval and quoting across handlers Refactor newListEntry to take *S3ApiServer and use getObjectETag, and update setResponseHeaders to use the same logic. This ensures consistent ETags are returned for both listing and direct access. * s3api: implement ListObjects deduplication for versioned buckets Handle duplicate entries between the main path and the .versions directory by prioritizing the latest version when bucket versioning is enabled. * s3api: cleanup stale main file entries during versioned uploads Add explicit deletion of pre-existing "main" files when creating new versions in versioned buckets. This prevents stale entries from appearing in bucket listings and ensures consistency. * s3api: fix cleanup code placement in versioned uploads Correct the placement of rm calls in completeMultipartUpload and putVersionedObject to ensure stale main files are properly deleted during versioned uploads. * s3api: improve getObjectETag fallback for empty ExtETagKey Ensure that when ExtETagKey exists but contains an empty value, the function falls through to MD5/chunk-based calculation instead of returning an empty string. * s3api: fix test files for new newListEntry signature Update test files to use the new newListEntry signature where the first parameter is *S3ApiServer. Created mockS3ApiServer to properly test owner display name lookup functionality. * s3api: use filer.ETag for consistent Md5 handling in getEtagFromEntry Change getEtagFromEntry fallback to use filer.ETag(entry) instead of filer.ETagChunks to ensure legacy entries with Attributes.Md5 are handled consistently with the rest of the codebase. * s3api: optimize list logic and fix conditional header logging - Hoist bucket versioning check out of per-entry callback to avoid repeated getVersioningState calls - Extract appendOrDedup helper function to eliminate duplicate dedup/append logic across multiple code paths - Change If-Match mismatch logging from glog.Errorf to glog.V(3).Infof and remove DEBUG prefix for consistency * s3api: fix test mock to properly initialize IAM accounts Fixed nil pointer dereference in TestNewListEntryOwnerDisplayName by directly initializing the IdentityAccessManagement.accounts map in the test setup. This ensures newListEntry can properly look up account display names without panicking. * cleanup * s3api: remove premature main file cleanup in versioned uploads Removed incorrect cleanup logic that was deleting main files during versioned uploads. This was causing test failures because it deleted objects that should have been preserved as null versions when versioning was first enabled. The deduplication logic in listing is sufficient to handle duplicate entries without deleting files during upload. * s3api: add empty-value guard to getEtagFromEntry Added the same empty-value guard used in getObjectETag to prevent returning quoted empty strings. When ExtETagKey exists but is empty, the function now falls through to filer.ETag calculation instead of returning "". * s3api: fix listing of directory key objects with matching prefix Revert prefix handling logic to use strings.TrimPrefix instead of checking HasPrefix with empty string result. This ensures that when a directory key object exactly matches the prefix (e.g. prefix="dir/", object="dir/"), it is correctly handled as a regular entry instead of being skipped or incorrectly processed as a common prefix. Also fixed missing variable definition. * s3api: refactor list inline dedup to use appendOrDedup helper Refactored the inline deduplication logic in listFilerEntries to use the shared appendOrDedup helper function. This ensures consistent behavior and reduces code duplication. * test: fix port allocation race in s3tables integration test Updated startMiniCluster to find all required ports simultaneously using findAvailablePorts instead of sequentially. This prevents race conditions where the OS reallocates a port that was just released, causing multiple services (e.g. Filer and Volume) to be assigned the same port and fail to start. |
||
|
Chris Lu
|
c2bfd7b524 |
fix: honor SSE-C chunk offsets in decryption for large chunked uploads (#8216)
* fix: honor SSE-C chunk offsets in decryption for large chunked uploads Fixes issue #8215 where SSE-C decryption for large objects could corrupt data by ignoring per-chunk PartOffset values. Changes: - Add TestSSECLargeObjectChunkReassembly unit test to verify correct decryption of 19MB object split into 8MB chunks using PartOffset - Update decryptSSECChunkView and createMultipartSSECDecryptedReaderDirect to extract PartOffset from SSE-C metadata and pass to CreateSSECDecryptedReaderWithOffset for offset-aware decryption - Fix createCTRStreamWithOffset to use calculateIVWithOffset for proper block-aligned counter advancement, matching SSE-KMS/S3 behavior - Update comments to clarify SSE-C IV handling uses per-chunk offsets (unlike base IV approach used by KMS/S3) All tests pass: go test ./weed/s3api ✓ * fix: close chunkReader on error paths in createMultipartSSECDecryptedReader Address resource leak issue reported in PR #8216: ensure chunkReader is properly closed before returning on all error paths, including: - DeserializeSSECMetadata failures - IV decoding errors - Invalid PartOffset values - SSE-C reader creation failures - Missing per-chunk metadata This prevents leaking network connections and file handles during SSE-C multipart decryption error scenarios. * docs: clarify SSE-C IV handling in decryptSSECChunkView comment Replace misleading warning 'Do NOT call calculateIVWithOffset' with accurate explanation that: - CreateSSECDecryptedReaderWithOffset internally uses calculateIVWithOffset to advance the CTR counter to reach PartOffset - calculateIVWithOffset is applied only to the per-part IV, NOT to derive a global base IV for all parts - This differs fundamentally from SSE-KMS/SSE-S3 which use base IV + calculateIVWithOffset(ChunkOffset) This clarifies the IV advancement mechanism while contrasting it with the base IV approach used by other encryption schemes. |
||
|
Chris Lu
|
066410dbd0 |
Fix S3 Gateway Read Failover #8076 (#8087)
* fix s3 read failover #8076 - Implement cache invalidation in vidMapClient - Add retry logic in shared PrepareStreamContentWithThrottler - Update S3 Gateway to use FilerClient directly for invalidation support - Remove obsolete simpleMasterClient struct * improve observability for chunk re-lookup failures Added a warning log when volume location re-lookup fails after cache invalidation in PrepareStreamContentWithThrottler. * address code review feedback - Prevent infinite retry loops by comparing old/new URLs before retry - Update fileId2Url map after successful re-lookup for subsequent references - Add comprehensive test coverage for failover logic - Add tests for InvalidateCache method * Fix: prevent data duplication in stream retry and improve VidMap robustness * Cleanup: remove redundant check in InvalidateCache |
||
|
Chris Lu
|
51735e667c |
Fix S3 conditional writes with versioning (Issue #8073) (#8080)
* Fix S3 conditional writes with versioning (Issue #8073) Refactors conditional header checks to properly resolve the latest object version when versioning is enabled. This prevents incorrect validation against non-versioned root objects. * Add integration test for S3 conditional writes with versioning (Issue #8073) * Refactor: Propagate internal errors in conditional header checks - Make resolveObjectEntry return errors from isVersioningConfigured - Update checkConditionalHeaders checks to return 500 on internal resolve errors * Refactor: Stricter error handling and test assertions - Propagate internal errors in checkConditionalHeaders*WithGetter functions - Enforce strict 412 PreconditionFailed check in integration test * Perf: Add early return for conditional headers + safety improvements - Add fast path to skip resolveObjectEntry when no conditional headers present - Avoids expensive getLatestObjectVersion retries in common case - Add nil checks before dereferencing pointers in integration test - Fix grammar in test comments - Remove duplicate comment in resolveObjectEntry * Refactor: Use errors.Is for robust ErrNotFound checking - Update checkConditionalHeaders* to use errors.Is(err, filer_pb.ErrNotFound) - Update resolveObjectEntry to use errors.Is for wrapped error compatibility - Remove duplicate comment lines in s3api handlers * Perf: Optimize resolveObjectEntry for conditional checks - Refactor getLatestObjectVersion to doGetLatestObjectVersion supporting variable retries - Use 1-retry path in resolveObjectEntry to avoid exponential backoff latency * Test: Enhance integration test with content verification - Verify actual object content equals expected content after successful conditional write - Add missing io and errors imports to test file * Refactor: Final refinements based on feedback - Optimize header validation by passing parsed headers to avoid redundant parsing - Simplify integration test assertions using require.Error and assert.True - Fix build errors in s3api handler and test imports * Test: Use smithy.APIError for robust error code checking - Replace string-based error checking with structured API error - Add smithy-go import for AWS SDK v2 error handling * Test: Use types.PreconditionFailed and handle io.ReadAll error - Replace smithy.APIError with more specific types.PreconditionFailed - Add proper error handling for io.ReadAll in content verification * Refactor: Use combined error checking and add nil guards - Use smithy.APIError with ErrorCode() for robust error checking - Add nil guards for entry.Attributes before accessing Mtime - Prevents potential panics when Attributes is uninitialized |
||
|
Chris Lu
|
b8dc8d12f2 | ErrNoSuchKey should not be reported as an error in the logs | ||
|
Chris Lu
|
5a3aade445 | less logs |