mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-18 04:20:53 +02:00
bd6bcd47e38806a2e763bbf4381bfdf2ca6a9303
9888
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Bruce Zou
|
eb6a7e93ca |
Fix mount eio on manifest resolve failure (#11287)
* mount: fail reads with error when chunk manifest resolution fails When SetChunks fails to resolve a chunk manifest (e.g. the volume is on a remote tier with reads disabled), the sections map stays empty and readDataAtSequential/readDataAtParallel zero-fill every missing section as if it were a sparse hole. Reads then return all-zero data with no error, so a plain cp of a large manifest-based file silently produces a completely zero-filled file. Remember the resolve error in ChunkGroup (guarded by sectionsLock) and return it from ReadDataAt. A later successful SetChunks clears it. Fixes the mount path of #11286. * filer: propagate manifest resolve errors in streaming read paths ViewFromChunks discards the chunk manifest resolve error returned by NonOverlappingVisibleIntervals. On failure the chunk views come back empty, and the streaming paths zero-fill the entire requested range, serving HTTP 200 / WebDAV 200 responses whose body is all zeros. Propagate the error in PrepareStreamContentWithThrottler, PrepareStreamContentWithPrefetch and the WebDAV read path so these requests fail with 500 instead. Fixes the filer HTTP and WebDAV paths of #11286. * mount: fail lseek with EIO when chunk manifest resolution fails SearchChunks still consulted the stale section map after SetChunks recorded a manifest resolution failure, so SEEK_DATA/SEEK_HOLE would describe the unresolved regions as sparse holes or return ENXIO. Return the recorded error from SearchChunks and map it to EIO in Lseek. Also add regression tests for the stream preparation error paths. Addresses review feedback on #11287. |
||
|
|
5b2fe374fc |
[Volume] Scrub every disk's EC shards for a volume id, not just the first (#11258)
* storage: add Store::find_all_ec_volumes for split-disk EC lookups Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: add merge_ec_runtimes to resolve a vid's per-disk shard set Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: replace dead slots.get(14) assertion with a width-14 pin Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: build the checksum scrub plan from every per-disk runtime Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: build the local scrub plan from every per-disk runtime Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: prove the local scrub plan reaches every runtime's slots Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: make the scrub plan tests falsifiable Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: report unverifiable protection when the sidecar predates the scrubbed encode Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: commit sidecar provenance with the sidecar it describes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: scrub every disk's EC shards for CHECKSUM and LOCAL Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: run the FULL/READS parity check across split-disk shards Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: report fenced-out runtimes in FULL/READS scrubs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: tighten verify_ec_shards ordering and missing-shard coverage Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: visit each EC volume id once in node-wide scrubs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: cover split-disk scrub aggregation end to end Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: pin fenced-out disks and sibling-disk shards in EC scrubs Three scrub behaviors shipped without a test at the RPC seam. Task 8 showed the seam exists, so close them here. FULL/READS (mode 2|5) now marks a volume broken when the identity fence excludes a runtime, where it previously reported clean. Pinned against a control fixture whose two disks AGREE and scrub clean, so the test fails on the clean->broken transition, not only on the message text. That needs a structurally valid, tombstone-only .ecx (so the needle walk finds nothing to complain about) and a seeded shard-location cache (so the absent master does not short-circuit the scrub with an error of its own). LOCAL (mode 3) and CHECKSUM (mode 4) now build their plans from every per-disk runtime. Made observable by moving shard 0 -- the shard the volume's single needle spans and the one the checksum sidecar is checked against -- to the SIBLING disk, leaving shard 5 on the disk the singular find_ec_volume lookup returns. Built from that disk alone, neither scrub ever looks at shard 0. The split-disk fixture grows a config struct rather than more positional arguments; its defaults reproduce the existing layout byte for byte, so the node-wide dedupe test is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: report fenced-out disks on a malformed sidecar too `errors.extend(self.skipped)` sat below the whole status match, so only `(Some(p), On)` ever reached it. The Invalid arm already returns a non-empty error vector of its own, so the Go-parity contract that silences the Off arm (`case BitrotOff: return 0, nil, nil`) does not reach it -- appending the fence lines there costs nothing that contract protects. A volume with BOTH a malformed sidecar and a disk the identity fence excluded reported only the sidecar, hiding the unscanned disk behind an unrelated integrity error. Off stays byte-identical, and so does the `(None, On)` arm that is documented as treating a missing payload defensively as protection off. Off is now the ONLY status that drops the report, and the comment at the On-path copy says so: that is the one place the parity constraint costs us coverage. Also corrects a false claim in the FULL/READS test's doc comment. It said a fenced-out disk "is a disk this scrub did NOT read", which is true only of the merge-driven parity half. The per-needle walk still resolves `store.find_ec_volume` (store_ec.rs:281) and binds `expected_encode_ts_ns` to that runtime (:311) -- position 0, the EXCLUDED one on that fixture -- so `read_local_intervals`' generation filter (:1204) makes it read the excluded disk and treat the anchor's shards as non-local, the inverse of what `skipped` reports. The fixture's tombstone-only .ecx walks nothing, so the test cannot tell the two apart; the comment now says that rather than implying coverage it does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: take CHECKSUM's bitrot protection from the disk that has the sidecar `EcChecksumScrubPlan::for_volumes` read `(prot, status)` off the ANCHOR. The anchor is the first shard-bearing runtime at the maximum `encode_ts_ns`, chosen with no regard for which disk holds the `.ecsum`. That sidecar is deliberately NOT mirrored across disks -- `ec_metadata_dirs()` exists so one authoritative copy stays reachable rather than being duplicated -- and at mount `EcVolume::new` resolves it via `load_active_bitrot_sidecar(&[])` with no sibling directories at all; only the `VolumeEcShardsMount` RPC ever passes `ec_metadata_dirs()`. So after EVERY volume-server restart, the split-disk runtime that does not physically hold the sidecar mounts `BitrotStatus::Off`. When the one copy lives on disk 1 and the anchor is disk 0, `run()` hit `case BitrotOff` and returned `(0, [], [])`: the whole volume scrubbed clean, silently. That is the steady state for roughly half of all mirrored split-disk layouts, and it is the exact failure this branch exists to remove. Source protection from the first MERGED runtime that has any -- `On` if one does, else `Invalid`, else the anchor's `Off`. Two facts make that safe, and both are load-bearing: - Every runtime that mounted `On` already passed the `geometry_matches` gate in `load_bitrot_for_generation`, so its manifest agrees with the volume's layout. A sidecar that contradicted it would have failed the mount. - All merged runtimes share the same `encode_ts_ns` by construction of the identity fence, so a sidecar from any of them describes the same encode run. The `unverifiable_sidecar` provenance rule four lines down read `anchor.bitrot_source_dir`; it now reads the SAME runtime `prot` came from. Otherwise the two would describe different sidecars and the rule would vouch for a manifest nobody is scanning against. One consequence worth naming: that source dir is now non-empty by construction (a runtime with protection found a file), where the anchor's was often "" and short-circuited the rule -- so on a fenced volume whose anchor had no sidecar, an unverifiable-protection note now surfaces where previously nothing was reported at all. `run()` is untouched, and the `BitrotStatus::Off` arm still returns `(0, [], [])` exactly, for Go parity with `case BitrotOff: return 0, nil, nil`. `parity_shards` still comes from the anchor while `prot` may come from a sibling; the geometry gate above makes them agree, and slot-width agreement is handled separately. The test drives mode 4 through the real RPC against a split-disk volume whose sidecar exists only on dir1, and asserts up front that the anchor mounted `Off` and the sibling `On` -- otherwise it would prove nothing. Reverting this commit's one-line source change makes it report `[]` instead of `[0, 5]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: pin the slot width, contain the shard-size fallback, and cover multi-disk FULL Five findings from the whole-branch review, none of which changes what a healthy volume reports. Slot width was undefined and the two consumers disagreed (ec_volume.rs). `merge_ec_runtimes` sizes `slots` to the WIDEST merged runtime, but the identity fence keys on `encode_ts_ns` alone and never on geometry -- so two same-generation runtimes whose `.vif`s disagree do merge. The mode 2|5 arm truncates to the anchor's `data+parity` and silently drops the surplus slots, while `EcChecksumScrubPlan::for_volumes` iterated the full width and emitted "present but missing from sidecar manifest" for exactly those ids. Nothing in the volume describes them -- the sidecar manifest and the Reed-Solomon matrix are both the anchor's -- so that message was the width disagreement talking, not a finding. The `slots` field doc now states the contract (the range is the anchor's geometry; every consumer truncates to it) and CHECKSUM truncates. The LOCAL `shard_size` fallback had grown a node-wide blast radius (ec_volume.rs). `anchor.shard_file_size()` returns the anchor's FIRST held shard, not a maximum. Before aggregation the plan read only that runtime's own shards, so a truncated shard was contained to its disk; now that one value sizes every merged sibling's shards, mis-offsetting `locate_data` and manufacturing needle corruption across the node. Take the max over the merged slots, which is how `verify_ec_shards` already answers the same question (`if size > shard_size { shard_size = size }`). Only on the legacy `dat_file_size == 0` path. Multi-disk `all_local` had no end-to-end test (grpc_server.rs). The parity check is gated on every shard being present, and the one all-local fixture keeps them in a single directory, so every entry of `dirs` is the same string and a permutation or off-by-one in the `slots` -> `dirs` mapping is invisible; `test_verify_ec_shards_reads_shards_from_multiple_dirs` builds its `dirs` by hand and never goes through `merge_ec_runtimes`. The new fixture is a real 10+4 encode split 0..=6 / 7..=13 across two store locations (the `.dat`/`.idx` stay outside both, so `prune_incomplete_ec_with_sibling_dat` has nothing to act on), driven through the real RPC: clean first, then a corrupted PARITY shard on the SECOND disk -- which only the parity half can see, and only through a correct mapping. Shifting that mapping by one, or computing `all_local` from the anchor alone, both make it report `[]` instead of `[13]`. Deleted `test_ec_volume_enumeration_is_deduped` (store_ec_reconcile.rs). It built `raw` from `store.locations` and then applied its OWN inline `filter(|v| seen.insert(*v))`, asserting on that -- a property of `HashSet::insert`, never reaching the production dedupe. That path is covered by `test_scrub_ec_volume_node_wide_dedupes_a_split_disk_volume`, which does fail (2 != 1) when the dedupe is removed. Corrected `test_verify_ec_shards_treats_a_none_dir_as_missing`'s docstring (ec_encoder.rs). It claimed the unmounted shard "must not drag the shards that ARE mounted down with it", but `dirs[5] = None` puts shard 5 in `broken_shards` before the block loop, so every iteration takes the `read_failed` arm and the parity comparison never runs: corrupting a mounted shard in that fixture changes nothing about the result. The assertions are unchanged; the docstring now states what they actually establish. Also refreshed two comments that cited `shard_file_size() - 1` as the reason `merge_ec_runtimes` prefers a shard-bearing anchor -- true before this commit, stale after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: correct the Fix 1 rationale and truncate the shard-size scan The safety argument attached to `EcChecksumScrubPlan::for_volumes`'s protection selection was false as written, and it is the argument a reviewer reads first. `geometry_matches` compares a sidecar against the MOUNTING runtime's own data/parity/block size, not the anchor's, and returns true vacuously when `ec_shard_config` is `None` -- so it establishes agreement only when all merged runtimes share one geometry, which an `encode_ts_ns`-only fence does not guarantee and which `test_checksum_scrub_truncates_slots_to_the_anchors_geometry` constructs a counterexample to. The second clause was weaker than stated too: a `.ecsum` records no encode identity at all, so merged runtimes agreeing on `encode_ts_ns` does not transfer to the sidecar. Replace it with the property that is true, checkable from the selection itself, and stronger for what actually matters. `anchor` is an element of `merged`, so the `.unwrap_or(anchor)` fallback is reached only when no merged runtime is `On` and none is `Invalid` -- in which case the anchor is necessarily `Off`. The status can therefore only move `Off -> On`, `Off -> Invalid` or `Invalid -> On`; never `On -> Off`, never `Invalid -> Off`. This selection cannot stop a volume that was being scanned from being scanned, and cannot turn a reported integrity error into silence: every change it makes is toward more verification. The comment now also states what it does NOT establish -- geometry agreement is not guaranteed -- and names geometry fencing as the follow-up that would close it. Second, `EcLocalScrubPlan::for_volumes`'s `shard_size` max scanned the FULL slot width, violating the `slots` contract documented in the same commit that introduced the max: the volume's shard-id range is the anchor's geometry and every consumer must truncate to it. Pre-fix that input could not exist, because `anchor.shard_file_size()` read only the anchor's own anchor-sized vector -- so the max opened a new, narrow path to the same node-wide mis-sizing it exists to close (same-generation runtimes with disagreeing `.vif`s, the wider one holding an out-of-geometry shard larger than the in-geometry ones, `dat_file_size == 0`). `.take(anchor.data_shards + anchor.parity_shards)` mirrors the truncation already applied to the CHECKSUM shard scan. The sibling `shards:` vector is left untruncated on purpose: every access in `EcLocalScrubPlan::run` is `shards.get(sid)` with `sid < data_shards`, so the surplus entries are inert. No behavior change for any healthy volume, and no test added -- the suite is unchanged at 575 passing, 0 failing, 0 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: aggregate split-disk runtimes in Go scrubs, mirroring Rust Go volume scrubs previously used FindEcVolume (first runtime only), so a volume whose EC shards are split across multiple disks was scrubbed against just one disk's shards and the others were silently skipped. Node-wide ScrubEcVolume also appended each disk's EcVolumeIds without deduplication, scrubbing a split-disk volume once per disk. Add MergedEcRuntimes/MergeEcRuntimes (Go counterpart to Rust's merge_ec_runtimes): select the maximum EncodeTsNs as the anchor generation, fence out runtimes whose encode generation or geometry (DataShards, ParityShards, BlockSize) disagrees with the anchor, merge shard handles by shard ID, and report excluded runtimes rather than dropping them. Wire it into every scrub mode: - INDEX: scrub the anchor's index, report skipped runtimes. - LOCAL: aggregate local shards across all merged runtimes via a synthetic EcVolume built from the merged shard slots. - FULL/READS: resolve the runtime matching the anchor's encode generation (not the first match) so the needle walk and parity phase inspect one encode run; report skipped runtimes. - CHECKSUM: take bitrot protection from the first merged runtime that has a valid sidecar (On, else Invalid, else anchor's Off), preserve invalid sidecar errors from every other merged runtime, and report skipped runtimes. Deduplicate EC volume IDs in node-wide ScrubEcVolume so each volume is scrubbed exactly once. Refactor ScrubEcVolume to share the per-needle walk via scrubEcVolumeWalk, called by both the legacy first-runtime path and the new merged path. Add Go regression tests covering split-disk deduplication, encode-generation fencing, geometry fencing, sibling-disk LOCAL reach, and merge anchor selection. Rust: keep the previously-landed merge/fence/checksum changes intact; revert incidental cargo-fmt drift from unrelated files so the diff stays focused. * ec: fence merged CHECKSUM on sidecar encode generation and fix legacy shard size Address two review findings on the Go merged-runtime scrub: 1. Sidecar provenance: a merged runtime can load a bitrot sidecar from a sibling metadata directory (ReloadBitrotSidecar), and the merge fence may then exclude the runtime owning that directory. Generation-0 sidecars do not identify the encode run, so geometry validation alone cannot prove the borrowed manifest describes the anchor shards. If the sidecar records a non-zero EncodeTsNs that disagrees with the anchor, refuse the scan instead of applying stale checksums to current shards and reporting false corruption. 2. Legacy shard size: for volumes without datFileSize in .vif, LocateEcShardNeedleInterval derives the shard size from Shards[0].ecdFileSize. The merged shard set is compacted in shard-ID order, so a truncated lowest-ID shard would shrink every interval and misread intact sibling shards. Synthesize a datFileSize from the maximum mounted shard size when the anchor lacks one, so the datFileSize>0 path uses the largest shard size across all merged runtimes. * ec: fix copylocks, legacy shard boundary, and encode-aware Rust lookups Address review findings from CodeRabbit and Devin: Go (ec_volume_merge.go): - Remove bitrotLock copy from the synthetic EcVolume: copying a sync.RWMutex is a go vet copylocks error. The synthetic volume uses its own zero-value mutex; bitrot/bitrotStatus are set directly before ChecksumScrub reads them via BitrotProtection(), so no concurrent access occurs. - Fix legacy shard-size boundary: synthesize datFileSize from (maxShardSize - 1) * DataShards, not maxShardSize * DataShards, to match the legacy fallback in LocateEcShardNeedleInterval (ecdFileSize - 1). An exact large-block boundary is ambiguous; the unadjusted size would select an extra large row and misread intact sibling shards. Rust (store_ec.rs): - Add find_ec_volume_for_scrub helper that resolves by encode generation (not first-match find_ec_volume) and use it in scrub_snapshot_under_lock, write_back_shard_locations, and the post-refresh shard-location read. Previously the encode-aware lookup was only used for the initial runtime selection; the cache write-back and per-needle snapshot still used first-match, so a split-disk volume whose first runtime was from an older encode run would write to and read from the wrong runtime's shard-location cache and falsely abort with 'remounted as a different encode run'. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
c46f82d29a |
fix(master): stop goraft server on shutdown and bump raft to v1.2.1 (#11284)
MasterServer.Shutdown only stopped the Hashicorp raft implementation; when using the default goraft backend, the raft event-loop goroutine (leaderLoop/followerLoop) kept running after the master shut down. In the in-process test harness this leaked goroutines across sequential test runs, and a stale event occasionally reached a leader at term 0 and tripped the goraft "leader.elected.at.same.term" assertion, crashing the whole test binary (CI run 34670959967, PR 11279). Stop the goraft server in Shutdown() so its goroutines exit cleanly, and bump seaweedfs/raft to v1.2.1 which replaces that assertion with a graceful step-down to Follower instead of a panic. |
||
|
Chris Lu
|
5a0e017457 |
s3: reject virtual-host bucket retargeting via X-Forwarded-Host (#11281)
* s3: reject virtual-host bucket retargeting via X-Forwarded-Host SigV4 verification tries the client-supplied X-Forwarded-Host as a signed host candidate, while routing and IAM select the bucket from the actual Host header. A presigned URL for one virtual-host bucket could therefore be retargeted to another bucket accessible to the same signing identity by changing Host and adding X-Forwarded-Host. After the signature matches a host candidate, extract the bucket that the candidate implies (via the configured virtual-host domains) and compare it with the bucket the router selected. Reject when they differ, before returning success. * test(s3api): cover virtual-host presigned URL retargeting Add unit tests for bucketFromVirtualHost and end-to-end tests that reproduce the X-Forwarded-Host retargeting attack for both presigned and signed requests, plus a negative test confirming the legitimate same-bucket case still verifies. * s3: harden bucketFromVirtualHost for case and overlapping domains Compare host and domain suffixes case-insensitively so a mixed-case X-Forwarded-Host cannot bypass the consistency check. Only treat the exact path-style domain as non-virtual-host; subdomains of a path-style domain still match the virtual-host router pattern and must be checked. |
||
|
Chris Lu
|
210afacd12 |
s3: close list-type / ownership-controls routing mismatch (#11280)
* s3: reject list-type paired with another operation subresource ?list-type=2&ownershipControls= routes to ListObjectsV2 (the list-type route is registered first) while the IAM action resolver resolves the ownershipControls selector to s3:GetBucketOwnershipControls. A principal denied s3:ListBucket but allowed s3:GetBucketOwnershipControls would therefore list the bucket. list-type selects an operation just like the other keys in operationSubresources, so add it there and reject the combination before routing, matching the fix for policy&tagging (#10987). * s3: resolve list-type to s3:ListBucket ahead of bucket subresources The router registers the ListObjectsV2 route ahead of the bucket subresource routes, so the action resolver should resolve list-type the same way. Without this, a request carrying list-type and another operation selector resolves to the subresource action (e.g. s3:GetBucketOwnershipControls) while being served by ListObjectsV2. The ambiguity guard rejects such combinations before routing, but resolving list-type to s3:ListBucket keeps the resolver aligned with the router, mirroring how versions is handled. * s3: match list-type=2 exactly in action resolver The router selects ListObjectsV2 only for list-type=2; other values fall through to the subresource routes. Resolve the same way so the action matches the handler for every list-type value, not just 2. |
||
|
Chris Lu
|
9f6feef299 |
feat(s3api): add bucket quota S3 extension via ?seaweedfs-quota (#11279)
* feat(s3api): add bucket quota S3 extension via ?seaweedfs-quota
Add a SeaweedFS-specific S3 subresource for bucket quota management:
PUT /{bucket}?seaweedfs-quota — set bucket quota (s3:PutBucketQuota)
GET /{bucket}?seaweedfs-quota — get bucket quota (s3:GetBucketQuota)
The request/response body is JSON:
{"quota_size": 100, "quota_unit": "GB", "quota_enabled": true}
Quota is stored on the bucket's filer entry (positive = enabled,
negative = disabled but retained, zero = no quota), matching the
existing admin REST API behavior. When quota is cleared, the bucket's
read-only flag is also lifted.
Authentication uses the existing S3 SigV4 flow — no new global secret
is needed. Authorization uses two new dedicated IAM permissions:
s3:PutBucketQuota
s3:GetBucketQuota
This allows integrations like Apache CloudStack to manage per-bucket
quotas through the S3 endpoint with a scoped credential, without
exposing the broad admin REST API or requiring a separate admin token.
The credential can be limited to s3:PutBucketQuota/s3:GetBucketQuota
only, preventing bucket deletion, user management, or cluster topology
changes.
The coarse-grained ACTION_PUT_BUCKET_QUOTA/ACTION_GET_BUCKET_QUOTA
constants are added to s3_constants, and the action resolver maps the
seaweedfs-quota query parameter to the fine-grained s3: actions for
policy evaluation.
* docs: update design for S3 ?seaweedfs-quota extension approach
Replace the broad admin REST API + bearer-token design with the narrow,
scoped S3 ?seaweedfs-quota extension. Update quota, usage reporting, and
SeaweedFS-side changes sections to reflect PR #11279.
* fix(s3api): address review comments on quota handler
Fix four issues identified by Devin, Greptile, and CodeRabbit reviews:
1. Integer overflow in convertQuotaToBytes: large quota_size values
(e.g. 8388608 TB) could overflow int64, wrapping to negative and
being silently treated as zero quota. Now returns an error when
size * multiplier would exceed math.MaxInt64.
2. Disabled quotas returned negative sizes in GET: the GET handler
returned entry.Quota directly, which is negative for disabled-but-
retained quotas. Now returns the absolute magnitude as quota_size
and derives quota_enabled from the sign, making the response
round-trippable.
3. Missing buckets returned 500 instead of NoSuchBucket: the PUT
handler treated all lookup failures as internal errors. Now
distinguishes filer_pb.ErrNotFound and returns ErrNoSuchBucket.
4. Trailing JSON was silently accepted: the decoder read only the
first JSON object without checking for trailing data. Now
requires EOF after the object, rejecting malformed payloads.
Also add tests for overflow detection and trailing data rejection.
* fix(s3api): cast math.MaxInt64 to int64 for 32-bit vet
On 32-bit platforms, math.MaxInt64 is an untyped int constant that
overflows int (32-bit) when used directly in fmt.Errorf with %d.
Cast to int64 explicitly to fix Go Vet 32-bit.
* docs: reconcile design doc with implementation and add AWS tools note
- Resolve open question about IAM endpoint path: driver accepts optional
iamUrl and defaults to <s3Url>/iam
- Add note explaining ?seaweedfs-quota is not callable by standard AWS tools
(aws s3api, s3cmd, rclone), and how this compares to MinIO and Ceph quota
APIs which also live outside the standard S3 API
* docs: fix IAM endpoint default — SeaweedFS IAM is at POST / on S3 endpoint
SeaweedFS registers its embedded IAM API at POST / on the same S3
endpoint (UnifiedPostHandler), not under /iam. The design doc
previously said the driver defaults iamUrl to <s3Url>/iam, which would
send IAM operations to an unregistered path. Correct the default to
s3Url.
Found by Greptile review on PR #11279.
* docs: fix credential model, signer, and GET response shape in design doc
Three issues found by CodeRabbit review on PR #11279:
1. Credential-scope contradiction: the doc claimed the service credential
is scoped to only s3:PutBucketQuota/s3:GetBucketQuota, but the
implementation uses it as the admin credential for all operations
(bucket CRUD, IAM user provisioning, quota). Document the actual
model.
2. S3Signer -> AWSS3V4Signer: the doc said 'S3Signer for SigV4 signing'
but S3Signer is legacy SigV2. Correct to AWSS3V4Signer.
3. GET response shape: the doc showed a single JSON example with 'GB'
for both PUT and GET, but GET always returns quota_unit 'B' and the
absolute byte count. Document PUT input and GET response separately.
|
||
|
Chris Lu
|
79994b69af |
s3: fail closed on unsupported bucket-policy condition operators (#11283)
* s3: support StringEqualsIgnoreCase and related condition operators The S3 bucket-policy condition engine rejected StringEqualsIgnoreCase (and StringNotEqualsIgnoreCase, StringLikeIgnoreCase, StringNotLikeIgnoreCase), which AWS and the IAM policy engine both accept. Add evaluators and register them in GetConditionEvaluator so valid policies using these operators evaluate correctly instead of being skipped. * s3: reject bucket policies with unsupported condition operators validateStatement did not check Condition operators, so a policy with an unknown operator (e.g. a typo or unsupported key) was accepted at upload time and only surfaced at evaluation, where it was silently skipped. Reuse GetConditionEvaluator to reject unknown operators when a policy is parsed or stored, failing closed at the entry point instead of relying on evaluation-time handling. * s3: fail closed on unsupported condition operators at evaluation EvaluateConditions skipped statements whose condition operator was unsupported, logging a warning and continuing. With no remaining conditions to fail, the function returned true, so an Allow statement conditioned on an unrecognized operator became unconditional and granted access to private objects. Return false instead so an unrecognized operator fails the condition block and the statement does not match, matching the fail-closed behavior of the IAM policy engine. * s3: validate condition operators at upload time only, not load time Validating condition operators in validateStatement rejected the whole policy document from ParsePolicy, which SetBucketPolicy uses when loading stored bucket policies. A legacy policy saved before this change could contain an unsupported operator, and rejecting it at load time dropped the entire policy - including unrelated explicit Deny statements - so the bucket lost its protections. Move the operator check into ValidateBucketPolicy, which only the PutBucketPolicy handler and admin UI run at upload time, so legacy policies still load and EvaluateConditions fails the unsupported statement closed instead. * s3: drop non-AWS StringLikeIgnoreCase and StringNotLikeIgnoreCase operators AWS defines StringEqualsIgnoreCase and StringNotEqualsIgnoreCase but not StringLikeIgnoreCase or StringNotLikeIgnoreCase (StringLike and StringNotLike are case-sensitive only). Registering the wildcard IgnoreCase variants made the engine accept operators AWS rejects. Keep only the two AWS-defined IgnoreCase operators and add a test asserting the wildcard IgnoreCase names are unsupported. |
||
|
Konstantin AdzerandChris Lu
|
42b0ca7850 |
s3 sink: report the source read error the SDK hides (#11277)
* s3 sink: report the source read error the SDK hides filer.backup stops for good on an event whose chunks are gone from the volume servers: the uploader reads the body, the read fails with the volume's 404, and the AWS SDK returns "ContentLength=N with Body length 0" without the cause. isIgnorable404 would skip such an event, but it never sees the 404, so the event is retried forever and the checkpoint never advances. ChunkStreamReader keeps its first source failure and the s3 sink returns it when the upload fails. * s3 sink: trim verbose comments on source error propagation --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Da.Sanchezandchrislusf
|
d8aa7ecf04 |
fix(vacuum): stop comparing compact size against the live needle map (#11263)
* fix(vacuum): stop comparing compact size against the live needle map CompactByIndex's post-copy integrity check compared bytes written to the .cpd against v.nm.ContentSize()-DeletedSize(), the live map that keeps mutating for as long as the volume stays writable during the copy. Any write landing after the point-in-time index snapshot was loaded made the live map's tally exceed what got copied, aborting compaction with "unexpected new data size" — even though CommitCompact's makeupDiff exists specifically to reconcile writes that land mid-copy. On a busy volume this can fail every vacuum cycle. Tally the expected live size from oldNm, the same frozen snapshot the copy loop reads from, instead of the live map. This keeps the check's original protection (destination smaller than what should have been copied signals real data loss) while removing the false positive from ordinary concurrent traffic. * fix(vacuum): stop double-subtracting skipped bytes from the size check Unreadable needles return before reaching the expectedLiveBytes tally, so it already excludes them. Subtracting skippedDataBytes again on top loosened the integrity check's margin by that same amount, letting a .cpd short of the true expected size slip past undetected — the exact failure mode the check exists to catch. Flagged independently by three automated PR reviewers (Devin, Greptile, CodeRabbit). Extract the comparison into exceedsExpectedCompactedSize and drop the subtraction entirely; add TestExceedsExpectedCompactedSize to pin the threshold to expectedLiveBytes alone. * fix(vacuum): trim verbose integrity-check comment Reduce the 8-line block comment to a concise 3-line rationale. No behavior change. * fix(vacuum): mirror compact integrity check in Rust volume server Mirror the Go fix in the Rust volume server's do_compact_by_index: tally expected_live_bytes from the frozen index snapshot (not the live needle map) and compare the compacted .dat against it after the copy. Unreadable needles already return before the tally, so no skipped-byte adjustment is needed. Adds exceeds_expected_compacted_size and two regression tests. * fix(vacuum): exercise makeup_diff in Rust concurrent-write test Address CodeRabbit review: write a needle after compaction (before commit), then call commit_compact() and assert the late write survives via makeup_diff. This actually exercises the concurrent-write path rather than just confirming the integrity check passes. --------- Co-authored-by: chrislusf <chris.lu@gmail.com> |
||
|
Chris Lu
|
2ebfeabfce |
mount: rebuild expired directory cache on entry lookup (#11268)
* test: reproduce expired directory cache degrading lookup to N RPCs After cacheMetaTtlSec elapses the kernel can still serve a directory listing from its page cache, so ReadDir never runs and EnsureVisited is not called. Metadata lookups then fall through to one LookupEntry RPC per entry instead of rebuilding the directory cache once. Issue #11262 * mount: add expired-directory rebuild predicate with cooldown to InodeToPath ShouldRebuildExpiredDir distinguishes a TTL-expired cached directory from a never-cached, invalidated, evicted, or read-through one (those clear isChildrenCached, while a plain TTL expiry keeps it set). It also gates retries on a cooldown since the last failed rebuild attempt, recorded by MarkRebuildAttempt, so a transient listing failure does not trigger a full rebuild on every later lookup. Issue #11262 * mount: rebuild expired directory cache on entry lookup When the kernel still serves a directory listing from its page cache past cacheMetaTtlSec, ReadDir never runs and EnsureVisited is not called, so lookupEntry issues one LookupEntry RPC per entry. Rebuild the expired directory once via ensureDirectoryVisited before the cache-hit check so later lookups are served locally. The EnsureVisited singleflight deduplicates concurrent rebuilds. On a non-oversized rebuild failure, record the attempt so the cooldown suppresses repeated rebuilds while the listing keeps failing; once it elapses a later lookup retries, recovering without waiting for ReadDir. Oversized dirs are already marked read-through by ensureDirectoryVisited. Issue #11262 * test: cover concurrent rebuild dedup and rebuild-cooldown fallback Add a test that runs concurrent lookups into the same expired directory behind a gated listing, asserting they share one rebuild via the EnsureVisited singleflight. Add a test that a failed rebuild records the attempt so an immediate retry is suppressed (per-entry RPC fallback), and that once the cooldown elapses and the filer recovers a later lookup rebuilds the cache. Issue #11262 * mount: wait for pending async flush before rebuilding parent cache The rebuild lists the parent directory from the filer, so a pending async flush of the target entry must land first; otherwise the rebuilt cache captures pre-flush metadata and the cache-hit path returns it without the wait that guards the filer-fallback path. waitForPendingAsync Flush is a no-op when no flush is pending, so the common case is unaffected. Issue #11262 |
||
|
Chris Lu
|
a3638e479e |
fix(s3api/audit): surface OIDC identity claim in audit log for STS sessions (#11269)
* Add ResolveIdentityClaim helper for OIDC audit identity ComputeParentUser derives a stable per-identity hash from (sub, iss) for internal keying, but it is opaque and not human-readable. Audit logs for STS-assumed OIDC sessions currently surface that opaque value (or the random session id) as the requester, leaving no authoritative trace of the federated user. Add ResolveIdentityClaim next to ComputeParentUser to recover a human-readable, server-asserted identity attribute from the STS request context populated at federation time. It walks a priority list (preferred_username, email, name, sub) so a federated session always audits against a stable OIDC claim rather than a client-supplied role session name. For #11264 * Surface authoritative OIDC identity claim in S3 audit log For STS-assumed sessions minted from an OIDC web identity, the audit log requester field is the opaque session subject, which cannot be traced back to the federated user who performed the operation. The OIDC identity claims (preferred_username, email, sub) are already carried in the session request context and reach the auth layer as identity.Claims, but they were never surfaced to the audit log. Add a requester_identity field to the S3 access audit log, populated from the authoritative OIDC identity claim resolved via ResolveIdentityClaim. The claim is propagated through the shared identity holder (the same mechanism the requester name and principal ARN already use) so it survives the request-context copy that hides auth-set values from the outer audit middleware. The existing requester field is left unchanged for backward compatibility; requester_identity is empty for non-federated sessions, where requester already carries the real username. For #11264 * Gate OIDC audit identity on federation marker and harden resolver Address review feedback (Devin Review, Greptile) on the initial implementation: - Non-federated STS sessions no longer gain a false requester_identity. ValidateJWTWithClaims merges the JWT registered sub claim (the opaque session id) into RequestContext for sessions without an explicit request context, so the previous ResolveIdentityClaim fallback to sub surfaced that session id as an authoritative identity. Resolution is now gated on SessionInfo.ParentUser, which is set only for OIDC-federated sessions in AssumeRoleWithWebIdentity. The claim is resolved from the original sessionInfo.RequestContext (not the local claims map, whose sub the bearer path overwrites with the session subject) so SigV4 and bearer sessions surface the same identity. - ResolveIdentityClaim now trims whitespace and treats whitespace-only claims as absent, so a blank preferred_username no longer masks a usable email or sub. The resolved claim is carried on Identity.IdentityClaim (and IAMIdentity for the bearer path) rather than re-derived in recordIdentityInContext, making the federation gate explicit at the auth boundary. For #11264 * Resolve OIDC identity claim for external bearer tokens The external OIDC bearer path (a raw OIDC JWT presented directly, not via STS) populates Claims with preferred_username/email/name/sub from the validated token but did not set IdentityClaim, so requester_identity stayed blank for that authentication path. Resolve the claim there too — sub is the real OIDC subject on this path (not an STS session id), so no federation gate is needed. Also drop an ineffectual ctx assignment flagged by ineffassign in the audit test. For #11264 |
||
|
Chris Lu
|
80dae68dbf |
fix: write the new key when a remote-synced file is renamed (#11270)
* refactor: extract update event handling into processUpdateEvent Pull the OldEntry/NewEntry update branch of the remote sync event processor into its own function so the rename skip logic can be exercised by tests with stub clients. No behavior change. * test: reproduce remote sync rename dropping the new key A rename under a remote mount arrives as an update whose NewEntry inherits the source RemoteEntry. shouldSendToRemote returns false for it, so processUpdateEvent skipped the event without writing the new key, while the filer had already deleted the old object. The test runs such an event through processUpdateEvent and expects both a delete of the old key and a write of the new one. Fails before the fix. See #11261. * fix: write the new key when a remote-synced file is renamed A rename under a remote mount arrives as an update whose NewEntry inherits the source RemoteEntry, so shouldSendToRemote returns false (RemoteMtime >= Mtime) and processUpdateEvent skipped the event. That skip is only valid when the destination key is unchanged; a path change always needs a write, and the delete-old/write-new handling below the early return is exactly what a rename needs. Guard the skip with proto.Equal(oldDest, dest) so a rename falls through to it. Fixes #11261. * fix: skip empty upload when renaming a remote-only entry A remote-only entry (no local chunks or content, data lives only on the remote object) carries a positive RemoteSize but nothing for NewFileReader to read. After the previous commit lets a rename fall through to the delete-old/write-new path, such a rename would upload EOF and create a zero-byte object at the new key, then stamp it as synced. Guard the write so a path change on a remote-only entry skips the upload instead of replacing the file with zero bytes. The filer has already deleted the old object, so the data is gone regardless; this avoids leaving a misleading empty object behind. * fix: propagate old-key delete errors except already-deleted When deleting the old key on a rename fails for a non-multipart entry, the error was swallowed and the write proceeded, which could leave both remote keys. Return the error so MetadataProcessor retries the event. The filer deletes the source remote object synchronously during the rename, so the sync delete is redundant and the object may already be gone. GCS reports that as ErrRemoteObjectNotFound (unlike S3/Azure, whose deletes are idempotent), so treat it as a successful deletion and continue to retriedWriteFile rather than pinning the sync offset. |
||
|
Chris Lu
|
5ff49909a0 |
fix(s3api/iam): avoid transient AccessDenied from full reloads on single IAM file changes (#11271)
* fix(s3api/iam): fail config snapshot on empty or malformed IAM files A full IAM reload reads every identity/policy/service-account/group file from the filer. When an external secrets tool rewrites a file, a reload that reads it mid-rewrite sees empty or partially-written content. The identity, policy and service-account loaders silently skipped such files (``continue``), so the snapshot was missing entries that still existed on disk. The atomic swap then installed an incomplete identity set while ``isAuthEnabled`` stayed on, denying unrelated clients mid-reload (#11259). The group loader and the read-error paths already fail the snapshot in this situation (a skipped entry reads as deleted). Apply the same behavior to empty content and unmarshal failures across the identity, policy, service-account and group loaders, so a transient mid-rewrite fails the reload (preserving the last known-good state) instead of silently dropping entries. * fix(s3api/iam): coalesce burst IAM config reloads through the reload queue onIamConfigChange did a full synchronous reload for every identity/policy file change event. When several independently-refreshing credentials rewrite their files within the same second, that produced a burst of dozens of back-to-back full reloads, each reading the whole store and widening the window where a mid-rewrite file is observed (#11259). Route every IAM config change through the existing coalescing reload queue (scheduleReload/reloadRetryLoop) instead. A burst of N events now collapses into a single reload (plus one tail reload for events that arrived while one was in flight). scheduleReload gains a reason argument for the existing log line; the reloadRetryLoop already retries failed reloads, so the per-event failure handoff is no longer needed. Tests that asserted on the synchronous reload now wire up the queue (centralized in newTestS3ApiServerWithMemoryIAM) and poll via waitForIdentity/waitForIdentityGone. Adds TestOnIamConfigChangeCoalescesBurstReloads showing 50 events coalesce into <=3 reloads. * fix(s3api/iam): skip non-JSON auxiliary files before failing IAM snapshot Per review: the multi-file loaders unmarshal every entry in an IAM directory, so a non-JSON auxiliary file (README, .DS_Store, a migration backup such as identity.json.old) would hit the new empty/malformed errors and reject the whole snapshot, blocking all later IAM reloads. Only *.json files are IAM objects (SeaweedFS writes identities, policies, service accounts and groups as <name>.json, and other call sites already gate on the .json suffix). Skip non-.json entries at the top of each loader loop, before reading content, so auxiliary files are ignored while empty/malformed .json files still fail the snapshot. Adds TestLoadConfigurationIgnoresNonJsonAuxiliaryFiles. * fix(s3api/iam): reject IAM files with empty identifiers and skip aux in listing Per review: - ListPolicyNames listed every regular entry in the policies directory as a policy name, including non-JSON auxiliary files, but GetPolicy cannot retrieve them. Apply the same .json suffix filter used by the loader so the list only exposes retrievable policies. - json.Unmarshal accepts `{}` and unknown fields. The identity and group loaders merge by the decoded Name (not the file name), so a `{}` file could install an empty-key record and displace a real one; the service-account loader accepted an empty Id. Validate Identity.Name, Group.Name and ServiceAccount.Id (via validateServiceAccountId) after unmarshal and fail the snapshot on empty identifiers. Adds TestFilerEtcStoreListPolicyNamesSkipsNonJsonAuxiliary and empty-identifier regression tests for identity, group and service-account files. |
||
|
Chris Lu
|
0de9c1f231 |
fix(s3api/sts): respect MaxSessionLength config in DurationSeconds validation (#11267)
* Refactor parseDurationSeconds into a STSHandlers method Convert the parseDurationSeconds wrapper from a package-level function into a method on STSHandlers so it can reach the configured STS service. No behavior change; the three AssumeRole* handlers now invoke it via their receiver. * Respect MaxSessionLength config in STS DurationSeconds validation parseDurationSeconds validated DurationSeconds against a hardcoded 43200s (12h) ceiling, so raising maxSessionLength in iam.json above 12h had no effect on AssumeRole, AssumeRoleWithWebIdentity, or AssumeRoleWithLDAPIdentity — requests were rejected at the handler before reaching the service layer. Derive the upper bound from the configured STS MaxSessionLength, falling back to maxDurationSeconds (43200s) when unset. The service layer (calculateSessionDuration) already caps the issued duration at MaxSessionLength, so this only relaxes the input-validation gate. * Add tests for STS DurationSeconds MaxSessionLength bound Cover the configured MaxSessionLength upper bound, rejection above it, fallback to the 43200s default when STS config is unset, the 900s minimum, and the empty-parameter nil path. * Refactor validateSessionDurationSeconds into a STSService method Convert validateSessionDurationSeconds from a package-level function into a method on STSService so it can reach the configured STS config. No behavior change; the three assume-role entry points in the service (AssumeRoleForPrincipal, validateAssumeRoleWithWebIdentityRequest, validateAssumeRoleWithCredentialsRequest) now invoke it via their receiver. * Respect MaxSessionLength config in STS service DurationSeconds validation The STS service validateSessionDurationSeconds rejected DurationSeconds above a hardcoded 43200s (12h) ceiling, so even after the handler accepted a longer duration it was rejected again in the service layer for AssumeRoleForPrincipal, AssumeRoleWithWebIdentity, and AssumeRoleWithCredentials. Derive the upper bound from the configured MaxSessionLength, falling back to DefaultMaxSessionLength (43200s) when unset. The issued duration is still capped at MaxSessionLength by calculateSessionDuration. * Add tests for STS service DurationSeconds MaxSessionLength bound Cover the configured MaxSessionLength upper bound, rejection above it, fallback to the 43200s default when STS config is unset, the 900s minimum, and the nil DurationSeconds path. * Preserve capping when MaxSessionLength is below the API minimum Deriving the DurationSeconds upper bound directly from MaxSessionLength created an empty valid range when MaxSessionLength is configured below the 900s API minimum, rejecting every explicit DurationSeconds that the old code silently capped via calculateSessionDuration. Only apply the configured MaxSessionLength as the upper bound when it is at least minDurationSeconds; otherwise keep the default bound and let calculateSessionDuration enforce the shorter configured limit. * Add tests for sub-minimum MaxSessionLength capping behavior Verify that a MaxSessionLength below the 900s API minimum keeps the default upper bound so explicit DurationSeconds within the default range are still accepted (and later capped by calculateSessionDuration). |
||
|
ssshr-66andChris Lu
|
e919bec9d1 |
fix(volume): Harden Volume Copy Validation and Failure Handling (#11252)
* fix(volume): harden volume copy validation * fix(volume): use stream context for ReadVolumeFileStatus in VolumeCopy ReadVolumeFileStatus ran on context.Background() while the adjacent VolumeStatus call used stream.Context(), an inconsistency left over from the context revert in #11252. Use stream.Context() consistently so the source status check is cancelled with the VolumeCopy stream. * fix(volume): reserve destination before deleting existing replica FindFreeLocation now runs before DeleteVolume so a full target fails without destroying the existing replica. Previously, when the initial VolumeStatus check failed (advisory) but ReadVolumeFileStatus succeeded, the existing replica was deleted before a destination was reserved, risking data loss if no location had enough free space. Add a regression test verifying the existing replica survives when the destination is full and the initial status check fails. * fix(volume): count replaced replica slot in FindFreeLocation FindFreeLocation now accepts the volume being replaced so its slot is treated as available. Without this, a location at its MaxVolumeCount limit could not replace its sole replica even though deleting it would free the slot. VolumeCopy passes the volume ID so destination selection succeeds before the existing replica is deleted. Add TestVolumeCopyReplacesReplicaAtSlotLimit covering a single-slot location that must replace its only replica. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
|
2cd6c36c54 |
filer: end local-only metadata subscriptions when remote peers appear (#11251)
* filer: end local-only metadata subscriptions when remote peers appear SubscribeMetadata delegates to SubscribeLocalMetadata whenever the MetaAggregator knows no remote peers at stream setup. Peer discovery is asynchronous with the gRPC server accepting streams: the master announces filers after Filer.Init, via ListExistingPeerUpdates and OnPeerUpdate. A subscriber that connects inside that window is pinned to a filer-local stream for its whole life, silently missing every other filer's writes. For filer.remote.sync in a multi-filer cluster this means the remote tier permanently stops receiving writes served by other filers (#11247). End the delegated local stream when the first remote peer appears, so the client reconnects into the aggregated stream. The end surfaces as an error, not a clean EOF: RetryUntil-driven followers (mount meta cache, s3api IAM) treat a clean end as following finished and stop reconnecting. The arrival channel is armed under the same lock as the peer check in RemotePeerArrivedChan, so a peer learned in between sends the stream straight to the aggregated path instead of parking on a channel that would never fire. A standalone filer is unaffected: no peer ever appears, the channel never fires, and the local stream serves indefinitely. Fixes #11247 * filer: interrupt disk replay on peer arrival, trim comments Check upgradeOnRemotePeer inside eachLogEntryFn and chunkDiskPass so a peer arriving during a backlog replay stops the stream before the cursor advances past older remote events. Wrap errAggregationUpgrade with StopReadingError so LoopProcessLogData does not log it. Remove issue references from comments and trim verbose commentary. * filer: check upgrade signal between ref batches Pass upgradeOnRemotePeer to sendRefsBatched so a peer arriving while refs are shipped to a slow client is detected between batches, not only after the full batch completes. * filer: interrupt gap park on peer arrival Pass upgradeOnRemotePeer through gapPass to parkOnGap so a peer arriving during a gap park ends the stream immediately instead of waiting for the retry timer (up to one minute). --------- Co-authored-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
7fa2f75f30 |
s3: bucket-policy Allow must not override an identity explicit Deny (#11256)
* s3: add isActionExplicitlyDeniedByApplicablePolicies helper Add a helper that reports whether any applicable identity-side policy (attached IAM policies, enabled-group policies, or the IAM-integration session policy) explicitly denies an action. It reuses the existing evaluateAttachedIAMPolicies, resolveS3AuthTarget, buildPrincipalARN, and isActionExplicitlyDeniedByIAM helpers, and fails closed on evaluation errors. A nil identity has no identity-side policy plane, so the helper returns false to keep the bucket policy authoritative for anonymous access. No behavior change yet; the next commits apply it to the two bucket-policy Allow short-circuits. * s3: enforce identity explicit Deny before bucket-policy Allow authRequestWithAuthType short-circuits on a matching bucket-policy Allow and skips VerifyActionPermission, so an explicit Deny in an authenticated identity attached, group, or session policy is bypassed. A non-admin principal with s3:PutBucketPolicy can install a bucket-policy Allow for itself and read an object its identity policy explicitly denies. Before honoring a bucket-policy Allow, check the applicable identity-side policies for a matching explicit Deny via the new isActionExplicitlyDeniedByApplicablePolicies helper, and fail closed. The cross-account behavior is preserved: a bucket Allow still supplies the Allow an identity policy omits (implicit denial), and a nil identity keeps the bucket policy authoritative for anonymous access. Regression tests cover the explicit-Deny override, the implicit-deny Allow preservation, and the unmatched-key fall-through control. * s3: enforce identity explicit Deny in secondary object-key auth authorizeObjectKeyAction authorizes keys the request URL does not name (CopySource, DeleteObjects body keys, POST Object form keys) and shares the same bucket-policy Allow short-circuit as the primary path, so an explicit Deny in the identity, group, or session policy is bypassed the same way when a bucket policy allows the secondary key. Apply isActionExplicitlyDeniedByApplicablePolicies before accepting the bucket-policy Allow, mirroring the primary path. A regression test covers AuthorizeCopySource for both the explicit-Deny override and the implicit-deny Allow preservation. |
||
|
Feng Shao
|
13bf056a15 |
Mount req with collection (#11249)
* volume mount req support specify collection * rust mirror change |
||
|
Nguyễn Đăng Minh Lực
|
c968084b34 |
iceberg: fix OAuth token expiry handling (401 + token-exchange + configurable TTL) (#11242)
* iceberg: return 401 for invalid or expired Bearer tokens BUG-0001: when the OAuth JWT expired, Server.Auth fell through to the S3 SigV4 authenticator, which rejects the "Authorization: Bearer" scheme with NotImplemented — a 501. Iceberg clients (Java OAuth2Manager, pyiceberg) only refresh tokens on 401, so they retried the dead token forever: RisingWave sinks stalled and Doris catalog queries failed every token TTL (1h) until the client process was restarted. A request carrying a Bearer header is an Iceberg REST client: answer 401 (+ WWW-Authenticate: Bearer, RFC 6750) when the token fails, and only fall through to the S3 authenticator when no Bearer header is present. * iceberg: make OAuth token TTL configurable via ICEBERG_OAUTH_TOKEN_EXPIRY BUG-0001 follow-up: production evidence shows Iceberg Java 1.10.x clients (RisingWave connector node, Doris FE) never re-fetch tokens on 401 — the sink stalled again on token expiry even with the 501→401 fix, and no POST /v1/oauth/tokens appeared in server logs across dozens of retries. 401 is necessary but not sufficient for these clients. The TTL was hardcoded to 3600 with no knob. Read the expiry (seconds) from ICEBERG_OAUTH_TOKEN_EXPIRY, defaulting to 3600, so deployments can issue longer-lived tokens (e.g. 86400) to survive client restart cycles. * iceberg: support OAuth token exchange (RFC 8693) for client refresh Decompiling the Iceberg Java 1.10.1 client bundled with Doris FE showed the missing half of BUG-0001: OAuth2Manager refreshes via token-exchange (AuthConfig.exchangeEnabled defaults to true — the client_credentials re-fetch branch only runs with exchange disabled), so a server that only accepts client_credentials leaves Iceberg clients unable to ever refresh their token, regardless of 401 correctness. Accept grant_type=urn:ietf:params:oauth:grant-type:token-exchange on POST /v1/oauth/tokens: verify the subject_token signature against the issuing credential, allow exchange within a recovery grace window (max(2*TTL, 1h), capped 24h) so clients holding tokens that expired while the grant was unsupported recover without a restart, and mint a fresh access token with the configured TTL. * iceberg: harden OAuth token exchange and Bearer matching per review - match the Bearer scheme case-insensitively (RFC 7235), like authenticateBearer already does - accept optional client authentication on the token-exchange grant (Basic or form credentials, bound to the subject token's client); expired subject tokens now require it. Iceberg Java's proactive refresh sends Bearer-only headers, so the grant cannot require it - reject subject tokens without an exp claim, and re-check the issuer on the verified claims - unauthenticated exchange cannot extend the lifetime past the subject token's own expiry (no chain-refresh from a leaked token) - return 400 invalid_grant per RFC 6749 §5.2 (was 401) - include issued_token_type on exchange responses (RFC 8693) - clamp ICEBERG_OAUTH_TOKEN_EXPIRY to 365d so Duration math cannot overflow into already-expired tokens * iceberg: give authenticated token exchanges a fresh full TTL The remaining-lifetime cap only guards unauthenticated (Bearer-only) exchanges; an authenticated client renewing a live token must get the full configured TTL, matching client_credentials. * iceberg: reject token exchange when no lifetime remains A Bearer-only exchange with under a second of subject lifetime would mint a token with expires_in: 0. Reject with invalid_grant instead. * iceberg: pin near-expiry test token to the next second boundary jwt/v5 serializes exp at one-second precision, so a 300 ms offset can round into the current second and route the test through the expired branch instead of the ttlSeconds<=0 guard. Mint the subject with the next whole-second expiry: live at exchange time, deterministically under a second of remaining lifetime. * iceberg: drop internal ticket reference from comments * iceberg: clamp oversized OAuth TTLs on 32-bit platforms strconv.Atoi on an int-sized value fails with ErrRange on 386, so an oversized ICEBERG_OAUTH_TOKEN_EXPIRY silently fell back to the default instead of clamping. Parse in 64-bit space and clamp, then narrow. * iceberg: make OAuth TTL narrowing explicit * iceberg: disable legacy OAuth in PyIceberg integration tests |
||
|
Chris Lu
|
01fc31cb71 |
fix(filer): use path.Split instead of filepath.Split for filer paths (#11246)
FullPath.DirAndName() and FullPath.Name() used filepath.Split, which is OS-dependent: on Windows it treats backslash as a path separator, corrupting filer paths that contain literal backslashes. Filer paths always use "/" as the separator, so switch to path.Split and path.Join which only split on "/" regardless of the host OS. This fixes the backslash case from #11243 where a file saved as /test/special\reverseslash4.jpg was stored with a corrupted path on Windows filer builds. The #, ?, and % cases from the same issue are client-side URL-encoding problems (the server never receives the raw characters), but once the client properly percent-encodes them the server now handles the decoded path correctly on all platforms. |
||
|
ssshr-66
|
966692fa23 |
[Volume] Validate record counts after volume copy (#11238)
* validate Volume Copy record counts * Delete s3api_object_versioning_bench_test.go * reply ai comments |
||
|
Chris Lu
|
2ffa696809 |
fix(volume): handle faulty storage media (Go + Rust) (#11233)
* fix(volume): track EC shard read errors and unmount on faulty media Extract the volume EIO tracker into a reusable IoErrorTracker and add the same tracking to EcVolume. Sustained EIO on .ecx lookups or .ecd shard reads now unmounts the EC volume in the heartbeat (without deleting files) so the master re-replicates from healthy peers, mirroring the existing volume replica quarantine. Closes #11227 (EC shard unmount). * rust(volume): mirror EC shard read error tracking and unmount Add EIO tracking to the Rust EcVolume mirroring Go: a streak counter with IO_ERROR_TOLERANCE, a sticky quarantine flag, and unmount (not file deletion) in the heartbeat so the master re-replicates from healthy peers. * feat(metrics): expose storage IO error counter and quarantine gauge Add a storage_io_error_total counter incremented on every EIO recorded by the volume or EC shard tracker, and an io_quarantine gauge labelled by kind (volume/ec_shard) reflecting the count of replicas suppressed in the heartbeat. Mirrored in Go and Rust. * feat(healthz): report 503 when local replicas are IO-quarantined Add Store.HasIoQuarantine (Go) / Store::has_io_quarantine (Rust) and have /healthz return 503 when any local volume or EC shard is quarantined due to sustained storage-media EIO, so a load balancer can drain a server whose underlying media is faulty. Mirrored in Go and Rust. * fix(volume): keep quarantined EC volumes in memory and reset EIO on success Address review feedback: instead of unloading quarantined EC volumes (which discards the quarantine state healthz needs), keep them in memory and just skip them from heartbeat reporting, mirroring the regular volume quarantine. Also clear the EIO streak on successful .ecx reads in Rust so a transient error does not accumulate, and add an ec_shard label to the io_quarantine gauge in both Go and Rust. * fix(volume): exclude quarantined EC shards from heartbeat and add Rust volume tolerance Address review feedback: - Filter quarantined EC volumes from CollectErasureCodingHeartbeat (Go) and collect_ec_shard_delta_messages / collect_live_ec_shards (Rust) so the master stops advertising faulty shards and re-replicates from healthy peers. - Add consecutive EIO count and sticky quarantine to the Rust regular Volume, mirroring Go IoErrorTracker: a single EIO no longer deletes the replica; the heartbeat quarantines after the tolerance threshold and keeps the volume in memory. - Use the quarantine flag (not last_io_error) in has_io_quarantine so /healthz reflects sustained, not transient, failures. * fix(volume): make Rust quarantined volumes read-only and wire recovery Address Devin review: - Set no_write_or_delete on Rust volumes when quarantined in the heartbeat, so cached or direct clients cannot mutate a faulty replica after the master removes it (mirrors Go). - Wire reset_io_error_state into Volume::set_writable so an operator making a volume writable again clears the sticky quarantine and the volume re-enters heartbeat rotation. * fix(volume): clear EC quarantine on shard re-mount for operator recovery Address Greptile review: re-mounting EC shards (Go loadEcShardWithIdxDir / Rust mount_ec_shards_with_idx_dir) now calls ResetIoErrorState on the existing EcVolume, giving operators a documented recovery path that clears the sticky quarantine and returns the EC volume to heartbeat rotation. Mirrored in Go and Rust. * fix(volume): do not clear EC quarantine on routine shard mounts Address review feedback: clearing the EC IO quarantine on every mount (including duplicate, retry, sibling-shard, and reconciliation mounts) is too aggressive and can re-advertise known-bad shards before the storage media has been validated. Remove the automatic reset from the mount path; quarantine clears naturally on restart or full unmount when a fresh EcVolume is created with clean state. * test(volume): update Rust IO error test for quarantine semantics The heartbeat now quarantines a volume with sustained EIO (keeps it mounted, makes it read-only, omits it from heartbeat) instead of deleting it. Update test_collect_heartbeat_deletes_io_error_volume to assert the volume stays in the store with no_write_or_delete set, and update set_last_io_error_for_test to set the consecutive error count at the tolerance threshold so the test reflects a sustained error. * fix(volume): reset EIO streak after full write and match Windows media errors Move the success-side EIO reset from append_needle (after write_all only) to the end of do_write_request, after flush_dat/flush_idx complete, so a successful write_all followed by a failed fsync no longer resets the counter before the EIO is recorded. Repeated fsync EIOs now accumulate toward the quarantine threshold as intended. Recognize Windows storage-media failure codes ERROR_CRC (23) and ERROR_IO_DEVICE (1117) in addition to Unix EIO (errno 5), so quarantined heartbeat behavior is preserved on Windows. Mirrors the change in both Go and Rust volume servers. * fix(volume): preserve checkpoint EIO and clear streak on successful delete maybe_checkpoint_index now returns whether the checkpoint succeeded; the success-side EIO reset in do_write_request and do_delete_request only fires when it did, so a checkpoint media failure is no longer erased by the unconditional reset that followed it. do_delete_request also gains the success reset that was lost when append_needle stopped clearing the streak, so a successful delete still clears an earlier failure streak. is_storage_io_error now uses libc::EIO on Unix instead of a hard-coded 5, and the ECX binary-search read path gains a Windows fallback (seek + read_exact) so the buffer is no longer zeroed on non-Unix targets. |
||
|
|
9b12d13934 |
volume server: release the store lock before scrubbing EC volumes (#11235)
* volume server: release the store lock before scrubbing EC volumes
`ec.scrub` makes a Rust volume server stop serving for the duration of the
scrub, and then kills its own gRPC connection:
error: rpc error: code = Unavailable desc = keepalive ping failed to
receive ACK within timeout
Measured on a 4.46 cluster (17 Rust volume servers on one host, ~520 volumes
and 53 EC volumes, --index=redb, EC 10+4). It reproduces against a SINGLE
node in 30-70s, in checksum, index and local modes, at -maxParallelization 1.
## Cause
The CHECKSUM arm of scrub_ec_volume reads every byte of every local shard
while holding the caller's store.read() guard:
let store = self.state.store.read().unwrap();
let ecv = store.find_ec_volume(vid)...?;
let (blocks, broken, errs) = ecv.checksum_scrub(); // GBs of I/O, lock held
VolumeServerState::store is a std::sync::RwLock, which is write-preferring.
The periodic heartbeat's collect_heartbeat_with_snapshot takes store.write()
and blocks; once that writer is pending, every later store.read() queues
behind it. Every HTTP handler takes store.read(), so the node serves nothing,
stops heart-beating, and cannot answer the scrub RPC's own keepalive - the
scrub kills the connection it is running on.
The INDEX and LOCAL arms have the same shape, and the node-wide scrub_volume
loop is worse: it held ONE guard across every volume on the node.
## Evidence
offcputime, off-CPU stacks >1s in a 30s window during a scrub:
futex_wait
seaweed_volume::server::heartbeat::collect_heartbeat_with_snapshot
- tokio-rt-worker
27967020 <- 27.97s blocked, of a 30s window
A single HTTP /status request issued 12s into a scrub, with 180s of patience,
was accepted and queued for 120 seconds, then served once the scrub released.
Thread states throughout: 1 D + 48 S. One thread working, 48 idle - not
executor starvation and no thread pileup, which is what a single lock holder
looks like.
Memory was tested and ruled out as the cause: the same scrub was run at
MemoryMax 3G, 8G and unlimited. With no limit there is no reclaim at all,
page cache grows freely to 22 GB, and the node still goes unresponsive at
t+30s. anon stays flat at 48-86 MB in every run.
## Fix
checksum_scrub, scrub_index and scrub_local gain plan types -
EcChecksumScrubPlan, EcIndexScrubPlan and EcLocalScrubPlan - snapshotted from
the volume under a brief guard. The handler builds a plan, drops the guard,
and runs the scan in spawn_blocking, off the async workers, since it is
synchronous CPU + file I/O either way.
A plan captures DESCRIPTORS, not paths. Resolving a path again after the
guard is dropped would let a writer that legitimately unlinks the files - the
heartbeat's delete_expired_ec_volumes, which reaches EcVolume::destroy(), or
volume_ec_shards_delete - surface an intentional removal as "scrub read
error: No such file or directory" and put the volume in broken_volume_ids. A
descriptor outlives the name.
For the shards it duplicates the handle the mounted EcVolumeShard already
holds (try_clone_file), which is what Go does: ChecksumScrub reads through
shard.ReadAt (weed/storage/erasure_coding/ec_volume_scrub.go:71), never
through a path. That also inherits open_volume_file's O_NOATIME and drops a
dead branch - the old code built {base}.ec{id}.v{gen} for a non-zero
generation, a name nothing in this tree writes. dup shares the kernel offset,
so shard reads stay positional; the .ecx gets a fresh open instead, since
check_index_file seeks.
FULL/READS is unchanged here: it already released the guard across the index
walk, and still re-takes it per needle in store_ec::scrub_snapshot_under_lock
for that needle's local shard intervals - short holds, many of them.
scrub_volume now takes the read guard PER VOLUME instead of across the whole
loop, so the heartbeat can land between volumes. Its per-volume work still
runs under the guard; Volume needs an equivalent plan to fix that properly,
left as a follow-up and noted in the code.
## A failed scrub task must not take the whole RPC down
Moving the scans into spawn_blocking changed where a panic lands. It no
longer unwinds inside the handler's own future; it comes back as a JoinError
at the .await, and all four join points sat behind a `?`. So one bad volume
out of six hundred returned Err from the entire handler: the
broken_volume_ids, broken_shard_infos and details already gathered for the
other 599 were dropped, and emit_scrub_metrics - the only writer of
SCRUB_LAST_TIME_SECONDS, SCRUB_VOLUME_FAILURES and SCRUB_SHARD_FAILURES - was
never reached, so the staleness alert kept firing while real corruption went
unreported.
And there is a reachable panic behind it. EcLocalScrubPlan::run() sized its
reassembly buffer with
Vec::with_capacity(get_actual_size(size, version) as usize)
which for any negative size that is not the -1 tombstone skipped above is a
capacity-overflow abort. Mode 3 (LOCAL) is the default of `weed shell
ec.scrub`, and a scrub is what you point at an index you already suspect, so
an arbitrary i32 in a .ecx size field is in-scope input. The buffer is
Rust-only - Go appends to a nil slice and has no capacity hint here. Guard on
`want <= 0` and fall through with an empty buffer: locate_data returns no
intervals for a non-positive size, read stays 0, and the existing
`read != want` error reports the row exactly as Go does.
Each join point now records the failure against its own volume and continues.
A panic is evidence about the volume and counts as broken; a non-panic
JoinError is not - spawn_blocking only reports one when the runtime is going
down, the volume was never scanned, and counting it would put a false
corruption into SCRUB_VOLUME_FAILURES. total_volumes moves before the join in
modes 1, 3 and 4 (2|5 already counted there) so a failed join cannot silently
shrink it. Mode 2|5's verify_ec_shards join is the one that must not
`continue`: the needle walk above has already produced findings for that
volume.
The tombstone guard stays is_tombstone() on purpose. ScrubLocal in
ec_volume_scrub.go:228 skips only IsTombstone(), while the distributed walk
in store_ec.go:516 skips all IsDeleted() - the asymmetry is Go's, and both
Rust walks mirror their own counterpart.
## Both servers: a node-wide scrub skips a volume that vanished mid-run
Releasing the lock makes the volume set legitimately mutable during a scrub,
so a node-wide run can reach a volume that has since been unmounted. That is
not a scrub failure. A node-wide run now logs and skips it; an explicitly
requested volume id still returns NotFound. The Go server is changed the same
way, so both implementations answer the same shell command identically.
mark_broken_volumes_readonly tolerates the same teardown one step later,
instead of throwing away the whole scrub report.
## Test
test_scrub_plans_are_self_contained_and_match_direct_call drops the EcVolume
and runs both plans on another thread, asserting the results match the direct
calls. A plan that borrowed from EcVolume could do neither, so the test stops
compiling if the snapshot regresses to a borrow.
test_scrub_plans_survive_files_removed_after_snapshot unlinks every shard and
the .ecx after the plans are built, then asserts the results still equal the
direct call. Against a path-resolving version it fails with all 14 shards
reported as "No such file or directory".
test_local_scrub_plan_reports_negative_size_ecx_row rewrites a .ecx row's
size to -1000 and runs the local plan on another thread, so the join is the
assertion - that thread is the spawn_blocking whose panic used to fail the
RPC. Without the capacity guard it fails with "capacity overflow"; with it,
the row is reported.
The Go tests cover both halves of the vanished-volume rule for volumes and EC
volumes.
517 lib tests pass, plus 34 across the other targets (`cargo test`).
`go test ./weed/server -run Scrub` passes.
## Known remaining, not fixed here
`ec.scrub -volumeId=N` is still fanned out to every node, and a node that
holds no shard of N returns NotFound, so the shell command errors even when
the nodes that do hold shards scrub cleanly. That is a shell-side fan-out
question rather than a volume-server one, and both servers keep the existing
behaviour for an explicitly requested id.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DvHoW85w6SNKNBPvrqLMmK
* scrub: discard checksum block count from total_files; capture .ecx fd for FULL walk
Two review fixes:
1. CHECKSUM arm: plan.run() returns blocks scanned, not a file count.
Go discards it (_, shardInfos, serrs = v.ChecksumScrub()) so TotalFiles
stays a needle/file count. The Rust arm was adding it to total_files,
inflating the count. Discard it to match Go.
2. FULL/READS (scrub_ec_volume_distributed): the needle walk reopened the
.ecx by PATH after the store guard was released, so a concurrent teardown
that unlinks or replaces the .ecx (heartbeat delete_expired_ec_volumes,
volume_ec_shards_delete) could surface an intentional removal as a scrub
error or mix index generations within one scrub. Capture a second .ecx
descriptor under the guard (the index plan handle is consumed by its own
structural walk, and both seek) and read through it instead -- the same
descriptor-outlives-name invariant the checksum plan shard handles use.
* scrub: bind FULL/READS walk to one encode generation
Address Devin review: after capturing the .ecx descriptor under the guard,
scrub_snapshot_under_lock still re-resolves the volume by id per needle, so
a teardown-and-remount of the same vid between two rows would apply the
captured .ecx offsets to a replacement volume's shards -- falsely reporting
corruption.
Capture the volume's encode_ts_ns (encode-run identity) in Phase A and pass
it to scrub_snapshot_under_lock. If the mounted volume's encode_ts_ns no
longer matches, abort the walk like a mid-scan unmount instead of mixing
generations within one scrub.
* scrub: run FULL/READS index scan in the blocking pool
Address CodeRabbit review (5147767192): index_plan.run() reads the whole
.ecx synchronously, so running it on the async executor worker could block
unrelated RPC work handled on the same executor. Move it into spawn_blocking,
matching the treatment the CHECKSUM/LOCAL arms already give their plans. A
join failure (panic/cancellation) is reported as a seed error so the
per-volume findings below are not silently dropped.
* scrub: move ecx walk to blocking pool, classify join errors, guard encode_ts_ns==0
Three CodeRabbit review fixes (5148034447):
1. Move the FULL/READS needle walk (walk_index_file over the captured ecx
descriptor) into spawn_blocking. It reads the full .ecx synchronously and
was still running on the async executor worker, the same blocker the
index_plan.run() fix in the previous commit addressed.
2. Preserve JoinError classification in both spawn_blocking join points in
scrub_ec_volume_distributed. A panic is evidence about the volume and
counts as broken; a cancellation only happens at runtime shutdown, the
volume was never scanned, and returning it as an error would put a false
corruption into broken_volume_ids (the FULL/READS arm marks the volume
broken on any non-empty errs). Panics return an error; cancellations
return clean.
3. Do not treat encode_ts_ns == 0 as a verified generation match. The .vif
assigns 0 when it carries no encode-run identity (legacy/pre-feature
volumes), so 0 == 0 would accept a teardown-and-remount and apply the old
.ecx offsets to the replacement volume's shards. Only enforce the
generation check when the captured identity is non-zero; when it is zero,
fall back to the pre-check behavior (no generation binding) rather than
aborting a scrub that was already running without the guard.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Chris Lu <chris.lu@gmail.com>
|
||
|
Chris Lu
|
723f473f02 |
filer: widen metadata-subscription readahead buffers (#11237)
The metadata-subscription readahead channels were sized for a low-throughput era and now bottleneck replay catch-up: - ReadPersistedLogBuffer's readaheadSize was 1024 entries: the background visitor fills the channel, then blocks on the consumer's gRPC Send, so volume-server I/O for the next log file never overlaps with delivery of the current one. Each disk pass takes longer, and the subscribe loop re-lists log files (ListDirectoryEntries on the filer store) more often to drain the same backlog. Raised to 8192 so the reader stays ahead of the consumer through a full log file's worth of entries. - readFilersMerged's logEntryChannelSize was 512 entries per filer stream: the same serialization on the client side, where weed mount (chunk mode) reads persisted log chunks directly from volume servers. A small channel means the producer stalls on the merge consumer's processEventFn, and the next log file's chunks are never fetched ahead. Raised to 4096 so volume I/O overlaps with event delivery. The wider buffers keep the producer goroutines reading through a full log file while the consumer is still processing the previous one, turning serial read→process→read into pipelined read∥process. This cuts the per-pass wall time that drives filer store listings and volume-server round-trips, reducing filer workload under backlog catch-up (e.g. CSI deployments where ~200 mounts reconnect on filer restart). |
||
|
Alessandro ZanattaandChris Lu
|
5f77a0b67e |
admin: allow insecurely binding to any IP if -allowInsecureNoAuth is set (#11228)
* admin: allow insecurely binding to any IP if -allowInsecureNoAuth is set * admin: rename -allowInsecureNoAuth to -allowInsecureBind The new flag name is shorter and clearer: it describes what is being allowed (an insecure bind to a non-loopback address) without the redundant "NoAuth" suffix. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
cb9fcd39d2 |
filer/postgres: create filemeta table on startup via createTable config (#11229)
* filer/postgres: create default filemeta table on startup The postgres filer store hardcoded CreateTableSqlTemplate to empty and never created the filemeta table, unlike postgres2/mysql2/sqlite which all create it during Initialize. Users had to create the table manually or the filer would crash loop with "relation filemeta does not exist". Read the createTable config option (same as postgres2), default to DefaultCreateTableQuery when unset, and execute CREATE TABLE IF NOT EXISTS on the default table after the connection pool is established. SupportBucketTable stays false so per-bucket table creation remains a no-op; only the shared filemeta table is created, via a direct ExecContext since AbstractSqlStore.CreateTable short-circuits without bucket support. * filer/postgres: accept boolean createTable = true/false viper reads a TOML boolean as the string "true"/"false" via GetString, so createTable = true was being used as a SQL template and failed. Add ResolveCreateTableQuery to normalize the value: true and empty select the default template, false disables table creation, anything else is a custom template. Both postgres and postgres2 now use it, and both skip the CREATE TABLE call when the resolved template is empty. * scaffold: document createTable option for postgres filer store Replace the commented-out CREATE TABLE SQL in the [postgres] scaffold with a createTable config hint, matching the [postgres2] section. Users no longer need to manually create the filemeta table before starting the filer. * filer/postgres: make createTable opt-in for postgres, keep postgres2 default The previous commit defaulted postgres to create the filemeta table even when createTable was unset, which could break existing deployments whose DB user lacks CREATE TABLE privileges. ResolveCreateTableQuery now returns empty for an unset value so postgres only creates the table when createTable is explicitly true or a custom template — preserving the prior no-DDL behaviour for existing configurations. postgres2 keeps its existing always-create default: it defaults an empty resolved value to DefaultCreateTableQuery, and only skips when createTable is explicitly false. * filer/postgres2: simplify createTable handling, document all modes Drop the false opt-out from postgres2 — it only skipped the default table while per-bucket CreateTable still ran, leaving restricted DB roles broken on bucket access. postgres2 now accepts true the same way (defaulting to DefaultCreateTableQuery) and keeps its existing always-create behaviour for every other value, matching the original semantics. The scaffold comment now documents true/false/custom for the postgres section so users know false (or unset) is the backward-compatible default. * filer/postgres2: normalize false via ResolveCreateTableQuery postgres2 only handled "" and "true", leaving createTable = false as the literal string "false" which CreateTable then executed as invalid SQL. Route it through ResolveCreateTableQuery (which maps false to empty) and default the empty result to DefaultCreateTableQuery, so false is treated the same as unset for the bucket-aware store. * filer/postgres2: honor createTable = false for default table postgres2 treated false the same as unset and always created the default filemeta table, failing startup for restricted DB roles that explicitly opted out. Track the original false value before ResolveCreateTableQuery collapses it to empty, and skip the default CreateTable call when set. Per-bucket table creation is unaffected — it is a runtime requirement of the bucket-aware store. Users who need to suppress all DDL should use the postgres (non-bucket) store with createTable unset. * filer/postgres2: disable bucket tables when createTable = false Setting SupportBucketTable = false when createTable is explicitly false makes AbstractSqlStore.CreateTable a no-op (it already returns nil when SupportBucketTable is false), so neither the default filemeta table nor per-bucket tables are created. The template stays empty and no DDL runs, honouring the opt-out for restricted DB roles. All data routes to the pre-provisioned filemeta table, matching the postgres (non-bucket) store. * filer: suppress DDL without disabling bucket routing Setting SupportBucketTable = false when createTable = false also disabled per-bucket routing, hiding objects in pre-provisioned per-bucket tables. Keep SupportBucketTable true and instead skip the CREATE TABLE execution when the resolved template is empty. GetSqlCreateTable now returns empty for both postgres and mysql SQL generators when CreateTableSqlTemplate is empty, and AbstractSqlStore.CreateTable skips the ExecContext call when the SQL is empty. This preserves bucket routing while suppressing all DDL for users who explicitly set createTable = false and pre-provision their tables. * filer: add SkipDDL to suppress CREATE and DROP without disabling routing createTable = false with SupportBucketTable = true preserved bucket routing but deleteTable still executed DROP TABLE on bucket deletion, dropping externally managed tables. CanDropWholeBucket also returned true, so the S3 layer tried whole-table drops instead of row-by-row deletes. Add a SkipDDL flag to AbstractSqlStore, independent of SupportBucketTable. CreateTable and deleteTable both skip when SkipDDL is set, and CanDropWholeBucket returns false so bucket deletion falls back to row-by-row metadata deletes. postgres2 sets SkipDDL when createTable is explicitly false — bucket routing is preserved, no DDL runs. * filer: fall back to row-by-row delete when CanDropWholeBucket is false DeleteFolderChildren took the whole-table drop path whenever the path was a bucket root, even when SkipDDL made deleteTable a no-op. The no-op returned nil, the caller returned early, and rows inserted after the recursive enumeration survived the bucket deletion. Gate the whole-table drop on CanDropWholeBucket so the row-by-row DeleteFolderChildren SQL runs when SkipDDL is set, removing all metadata without issuing DROP TABLE. |
||
|
Chris Lu
|
8782749f26 |
admin: reject IAM policy deletion while still attached to a user/group (#11230)
* admin: add IsPolicyAttached helper to detect user/group attachments Introduces AdminServer.IsPolicyAttached, which lists the users and groups that still have a managed policy attached, reusing the existing credential manager ListUsers / ListAttachedUserPolicies / ListGroups / GetGroup methods. This is the building block for rejecting policy deletion while a policy is still referenced, so deleted policy names stop lingering in a user attached policy names list (issue #11225). * admin: reject IAM policy deletion while still attached Guards AdminServer.DeletePolicy with the new IsPolicyAttached check and returns the typed ErrPolicyStillAttached error when the policy is still referenced by a user or group. This matches AWS IAM and the existing IAM API handler behavior, fixing the stale reference where a deleted policy name kept showing up in a user attached policy names list (issue #11225). * admin: return 409 Conflict when deleting an attached IAM policy The admin UI DeletePolicy handler now maps ErrPolicyStillAttached to HTTP 409 Conflict instead of 500, so the dashboard can surface the attachment conflict to the user rather than reporting a generic server error. * admin: skip vanished groups when checking policy attachments IsPolicyAttached now treats a group that disappears between ListGroups and GetGroup (credential.ErrGroupNotFound) as no longer attached instead of failing the whole deletion with HTTP 500, matching the IAM API handler which skips vanished groups. * test: assert policy state after deletion paths Strengthen GetPolicy assertions in the policy deletion tests to check the returned policy is non-nil after a rejected deletion and nil after a successful one, not just that no lookup error occurred (GetPolicy returns nil, nil when a policy is absent). |
||
|
Chris Lu
|
b88156fe6b |
fix(s3api): evaluate aws:SourceIp from the direct TCP peer, not forwarded headers (#11231)
* fix(s3api): use direct peer IP for aws:SourceIp in bucket policy engine extractSourceIP in the bucket-policy engine trusted X-Forwarded-For and X-Real-Ip whenever the TCP peer looked private (loopback/RFC1918/link-local), with no configurable trusted-proxy allowlist. In containerized deployments the gateway peer is almost always private, so any caller reaching it directly or from a co-located workload could spoof aws:SourceIp and bypass IpAddress/NotIpAddress bucket-policy restrictions. Always return the direct peer address (r.RemoteAddr), matching AWS S3 semantics. Remove the now-unused isPrivateIP helper and header-trust branch. Update TestExtractConditionValuesFromRequestSourceIPPrecedence to assert the peer IP is used regardless of forwarding headers, and add regression tests TestExtractSourceIP_IgnoresForwardedHeaders and TestExtractSourceIP_EnforcesIPRestrictionPolicy. * fix(s3api): use direct peer IP for aws:SourceIp in IAM role/session policies The IAM middleware's extractSourceIP trusted X-Forwarded-For and X-Real-IP whenever the TCP peer looked private (loopback/RFC1918/link-local), with no configurable trusted-proxy allowlist. In containerized deployments the gateway peer is almost always private, so any caller reaching it directly or from a co-located workload could spoof aws:SourceIp and bypass IpAddress/NotIpAddress conditions on role and session policies (IsPrincipalActionExplicitlyDenied). Always return the direct peer address (r.RemoteAddr), matching AWS S3 semantics. Remove the now-unused isPrivateIP helper, privateNetworks table, and its init(). Update TestRequestContextExtraction and TestIPBasedPolicyEnforcement to assert the peer IP is enforced regardless of forwarding headers, and add regression test TestUserInlinePolicySourceIpCondition_IgnoresForwardedHeaders. |
||
|
Chris Lu
|
557fffa350 |
iam: preserve native Admin when IAM policies are attached (#11226) (#11232)
* iam: expose tri-state result from attached policy evaluation evaluateIAMPolicies returned a bool that collapsed explicit Deny and no-match into a single false, so the authorization path could not tell "policies forbid this" from "policies say nothing". Introduce evaluateAttachedIAMPolicies returning Allow/Deny/NoMatch and keep evaluateIAMPolicies as a bool projection for existing callers. This is preparation for unioning native permissions with attached policies while preserving deny-always-wins. * iam: preserve native Admin when IAM policies are attached Attaching an IAM policy routed authorization exclusively to the attached policies, dropping the identity native permissions. A user with native Admin lost all access after attaching a non-granting policy, and stayed locked out if that policy was deleted without being detached first (#11226). Treat a native bare Admin grant as a permission floor that survives attached policies: when the attached policies do not explicitly allow, fall back to isAdmin() on the attached-policy path, and on the IAM integration path allow unless an attached policy explicitly denies. Explicit Deny still wins on both paths. Only bare Admin is consulted because inline policies flatten lossily into Actions (dropping conditions), so scoped actions are not unambiguously native and must keep flowing through the policy engine. * iam: regression tests for native Admin surviving attached policies Reproduces issue #11226: - TestNativeAdminSurvivesAttachedPolicy: a user with native Admin keeps Write access after attaching a policy that does not grant it. - TestNativeAdminSurvivesDeletedPolicy: the same user keeps Write access after the attached policy is deleted without being detached. - TestAttachedPolicyExplicitDenyOverridesNativeAdmin: an explicit Deny in an attached policy still constrains a native admin (deny-always-wins). * iam: apply native Admin floor before IAM principal validation The native Admin floor in authorizeWithIAM ran after the auth-path switch, which denies when no session principal or PrincipalArn is present. An Admin identity without a PrincipalArn (no session token) was therefore denied before the floor executed. Move the floor ahead of the switch and derive the principal for its explicit-deny check with buildPrincipalARN, which already handles identities without a PrincipalArn. Adds a regression case for an Admin identity with an empty PrincipalArn. Addresses CodeRabbit review feedback on PR #11232. |
||
|
Chris Lu
|
4a1d65939f |
fix(mount): bound reader cache memory across open files (#11220)
* fix(filer): bound retained reader cache buffers by bytes * test(filer): keep in-flight downloads during cache trimming * feat(mem): expose pooled allocation capacity for byte reservations * fix(mount): share a configurable reader buffer budget across files * fix(filer): release failed prefetch slots and memory reservations * feat(mount): expose a soft Go runtime memory limit * docs(filer): restore shared-download rationale in startCaching The one-line comment replacing the original context.Background() explanation was too thin for readChunkAt to cross-reference shared resource semantics. Restore a concise note on why request cancellation must not abort a download shared by concurrent readers. * test(filer): loosen reader cache test deadlines to 5s Three tests used 1-second deadlines that can flake on CI under load: TestReaderCacheBudgetInFlight, TestReaderCacheEvictionDoesNotHoldCacheLock, and TestReaderCacheFailedPrefetchReleasesBudget. Increase to 5 seconds. * test(filer): cover re-read after reader cache eviction Add TestReaderCacheReReadAfterEviction: reads chunk 'a', reads chunk 'b' (evicting 'a' via budget pressure), then re-reads 'a' and asserts a fresh download returns correct data. Verifies the core correctness property that eviction never exposes missing or stale data to readers. |
||
|
ssshr-66
|
c6b330be2b |
[Mount] Add ChunkGroup seeking tests and fix boundary handling (#11223)
* fix issue 11221 * reply ai comments |
||
|
github-actions[bot]
|
d997fba157 | 4.46 | ||
|
Chris Lu
|
c0a7dbb2bb |
iam: bind CreateServiceAccount ParentUser to the caller (#11218)
* iam: bind CreateServiceAccount target to caller in AuthorizeIamAction A non-admin holding iam:CreateServiceAccount could pass an arbitrary ParentUser and mint a service account for any identity, inheriting that identity permissions. Add a self-target category so a granted non-admin may only target their own identity; admins remain unrestricted. * iam: authorize CreateServiceAccount against its ParentUser target AuthIamManagement passed UserName as the authorization target for every action, so CreateServiceAccount was authorized with an empty target and the self-target binding never saw the caller-supplied ParentUser. Pass ParentUser for that action so the binding takes effect on the live path. * iam: test CreateServiceAccount binds target to caller Regression test: a non-admin holding iam:CreateServiceAccount may target itself but is denied targeting another identity; admins remain unrestricted. * iam: authorize CreateServiceAccount against ParentUser on the S3 port UnifiedPostHandler passed UserName as the authorization target for every IAM action, so CreateServiceAccount was authorized with an empty target on the S3-port route and the self-target binding never saw the caller ParentUser. Extract iamTargetUserName (ParentUser for CreateServiceAccount, UserName otherwise) and use it from both IAM dispatch surfaces so the binding applies on the live S3-port path as well as the standalone iam server. * iam: test CreateServiceAccount ParentUser binding on the S3 port End-to-end regression test through UnifiedPostHandler: a non-admin holding iam:CreateServiceAccount is denied (403) when targeting another identity and passes authorization when targeting itself. |
||
|
ssshr-66andChris Lu
|
361fd6b263 |
[Filer] Parallelize Chunk Manifest Resolution to Reduce Large File Read Latency (#11215)
* fix issue-11214 * fix(filer): cancel sibling manifest reads on failure * fix(filer): scope manifest cancellation to read batch and propagate context to encrypted reads Address PR review comments on #11215: - Scope cancellation to each parallel read batch instead of the resolver-wide context, so a later manifest failure does not cancel recursive work for an earlier successful manifest (CodeRabbit #3952446554). - Propagate the resolver context through GetAuthenticatedWithContext so encrypted sibling reads observe cancellation and stop promptly when another manifest fails (Greptile #3952422343). - Use net.ListenConfig.Listen with an explicit context in the test fixture to satisfy the noctx linter (CodeRabbit #3952111696). - Add regression tests for encrypted sibling cancellation and for preserving earlier manifest children on later failure. * fix(filer): return known manifest errors without blocking on earlier children Address Greptile review comment on #11215: After all parallel reads complete, pre-scan slots for the first real (non-internal-cancel) error before recursing into earlier manifests' children. If a later manifest already failed, return its error promptly with data chunks already in hand, instead of blocking on recursive network reads of earlier manifests' children. Updated the regression test to verify the error returns within 1 second when an earlier manifest's child has a 2-second delay, and that the child is never loaded. * fix(filer): filter partial child chunks by requested range on error path Address CodeRabbit review comment on #11215: The pre-scan error path appended non-manifest child chunks from earlier manifests without applying the [startOffset, stopOffset) overlap check used for top-level chunks. A child outside the requested range could be returned in dataChunks alongside the later manifest's error. Apply the same range predicate before appending. Add regression test with an out-of-range child chunk. * fix(filer): buffer job channel and abort submission on batch cancellation Address Greptile review comment on #11215: - Use a buffered job channel (capacity 128) so submission does not block when all workers are busy. This ensures a promptly-failing manifest is always queued and can cancel stalled sibling reads once a worker picks it up, instead of blocking the caller on the unbuffered channel send. - Add batchCtx.Done() to the submit select so submission aborts promptly when the batch is already cancelled by a sibling failure. - Add regression test with 5 manifests (4 stalled + 1 failing) verifying the failing job is queued and picked up after a stalled worker is freed. * fix(filer): avoid double WaitGroup decrement on batch cancellation in submit Address Devin review comment on #11215: When batchCtx.Done() fired in submit, it called job.done.Done() and returned false. The caller in resolve also called reads.Done() on the same WaitGroup, causing a double decrement that would panic with a negative counter. Fix: submit sets the result error but does not decrement the WaitGroup. The caller always owns the decrement and skips overwriting the result when submit already set it. * fix(filer): overflow execution when job queue buffer is full Address Greptile follow-up review comment on #11215: With a 128-entry buffer, if more than 132 in-range manifests (4 workers + 128 buffer) stall at one level, a promptly-failing manifest beyond the buffer cannot be submitted and cannot cancel the stalled reads. Fix: when the buffer is full, run the job directly in a goroutine instead of blocking on the channel send. This only triggers for >132 manifests at one level (exceedingly rare), so the bounded concurrency guarantee (4 workers) holds for all normal workloads. Extracted executeJob method shared by both workers and overflow goroutines. * fix(filer): bound overflow execution with a semaphore Address Devin review comment on #11215: The unbounded overflow goroutines could create thousands of concurrent reads for large files, defeating the four-worker resource bound. Fix: add a semaphore (capacity = maxChunkManifestResolveWorkers) that overflow goroutines must acquire before doing the read. While waiting for the semaphore, they also watch batchCtx and r.ctx so they exit promptly on cancellation. Total concurrency is now bounded to 2 * workers (4 workers + 4 overflow) in the degenerate case. * refactor(http): add ctx to GetAuthenticated signature instead of new function Reuse the existing GetAuthenticated name by adding ctx as the first parameter, matching the pattern of ReadUrl, ReadUrlAsStream, and RetriedFetchChunkData. Removes the GetAuthenticatedWithContext wrapper. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
75ec5ec193 |
admin: allow setting volume read-only and read/write modes (#11217)
* admin: support setting volume read-only and read/write modes
* admin: address PR review on volume access-mode persistence
Reject trailing JSON values in the SetVolumeReadOnly handler so
requests like {"read_only":true}{} no longer pass validation, and add
a trailing-value case to the invalid-request test.
Propagate .vif persistence failures through the access-mode chain.
PersistReadOnly now returns the SaveVolumeInfo error and rolls back
the in-memory volumeInfo on failure; Store.MarkVolumeReadonly and
Store.MarkVolumeWritable propagate that error and roll back their
noWrite flags, so the API reports failure instead of success while
restart would revert the mode.
* admin: make .vif persistence atomic and preserve error chain
SaveVolumeInfo now writes to a .vif.tmp file, syncs it, renames it
over the target, and fsyncs the directory. A write/sync/close failure
leaves the existing .vif intact, so the PersistReadOnly in-memory
rollback matches the durable state instead of diverging from a
partially written file that restart would apply.
Switch the error wrappers in PersistReadOnly, MarkVolumeReadonly, and
MarkVolumeWritable from %v to %w so callers can use errors.Is and
errors.As to classify persistence failures.
* admin: treat post-rename dir fsync failure as a warning
After os.Rename commits the new .vif, the on-disk file already holds
the requested mode. A directory fsync failure only risks losing the
rename across a crash; returning an error here would make
PersistReadOnly roll back in-memory state while the durable file keeps
the new mode, splitting the replica. Log the failure as a warning
instead, matching the best-effort nature of FsyncDir (already skipped
on Windows).
* admin: distinguish post-rename durability failures and use unique temp files
SaveVolumeInfo now uses os.CreateTemp for the staging file, preventing
concurrent saves for the same volume from colliding on a shared .tmp
path.
A directory fsync failure after os.Rename returns a
NotCrashDurableError instead of being silently swallowed. The rename
already committed the new metadata to disk, so PersistReadOnly,
MarkVolumeReadonly, and MarkVolumeWritable skip the in-memory rollback
for this error type (keeping state aligned with the durable file) while
still propagating the failure to the API. Pre-commit failures continue
to roll back as before.
* admin: continue post-commit work after NotCrashDurableError
MarkVolumeWritable now clears the EIO quarantine and the gRPC handlers
(makeVolumeReadonly step 3, makeVolumeWritable master notification)
proceed with their post-commit work when SaveVolumeInfo returns a
NotCrashDurableError, instead of aborting and leaving the volume
unavailable or the master unaware of the mode change. The durability
warning is still propagated to the API caller. Pre-commit failures
continue to abort early as before.
* admin: handle NotCrashDurableError in tier and EC callers
VolumeTierMoveDatFromRemote and VolumeEcShardsGenerate now check for
NotCrashDurableError from SaveVolumeInfo. When the rename has already
committed the new .vif, they continue with their post-commit work
(backend switch, remote deletion, keeping generated EC shards) instead
of aborting and leaving the on-disk metadata inconsistent with the
file layout. The durability warning is logged for the operator.
|
||
|
dependabot[bot]andChris Lu
|
2d4b730a2f |
build(deps): bump github.com/twmb/avro from 1.7.2 to 1.8.0 (#11210)
* build(deps): bump github.com/twmb/avro from 1.7.2 to 1.8.0 Bumps [github.com/twmb/avro](https://github.com/twmb/avro) from 1.7.2 to 1.8.0. - [Commits](https://github.com/twmb/avro/compare/v1.7.2...v1.8.0) --- updated-dependencies: - dependency-name: github.com/twmb/avro dependency-version: 1.8.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * iceberg: adapt to twmb/avro v1.8.0 and iceberg-go defensive copies avro v1.8.0 changes Schema.Root() to return *SchemaNode, which breaks iceberg-go v0.6.0's internal avro_schemas.go. The fix (apache/iceberg-go#1843) is only on iceberg-go's main branch, unreleased, so bump iceberg-go to that commit (c210509) alongside the avro bump. That iceberg-go revision also changes two behaviors seaweedfs worked around: - It now infers a manifest list's format version from the embedded writer schema, so a list missing the "format-version" header entry (DuckDB's shape is read as v2, not v1. ReadManifestList's header patching is now a redundant safety net; tests updated to expect v2. - It returns defensive copies from DataFile.Partition(), so the ReadManifest shim's in-place partition normalization was silently discarded. Rebuild the entry through NewDataFileBuilder when any partition value is normalized, copying every other DataFile field so manifest round-trips are preserved. - It converts day-transform partitions to iceberg.Date on read (applyDayTransformDates), so the day-partition cases the shim and tests guarded now convert without help; tests updated to expect iceberg.Date from the raw read. EOF ) * iceberg: accept assert-ref-snapshot-id without snapshot-id iceberg-go's new nullableInt64 parser rejects an assert-ref-snapshot-id requirement whose "snapshot-id" field is absent from the JSON, even though the Iceberg REST spec makes it optional (null means the ref must not already exist). v0.6.0 used a plain *int64, so absent was nil and accepted. ClickHouse sends the requirement without snapshot-id when asserting a branch does not yet exist, so its writes fail with "missing required field \"snapshot-id\"". normalizeRequirements splices an explicit null into any assert-ref-snapshot-id requirement missing the field before handing the JSON to iceberg-go's parser, restoring the v0.6.0 behavior across both iceberg-go versions. * iceberg: fix v1 block_size_in_bytes default in rebuilt manifest entries rebuildManifestEntry set block_size_in_bytes to 0, but the v1 manifest schema requires the default of 64 MiB ("Always write default in v1"). The original value is not exposed on the DataFile interface, so use the spec default. Also clarify the fallback comment to note that empty (zero-record / zero-byte) files also trigger it, not just a nil spec. Add a round-trip test that writes a rebuilt entry as v1 and verifies block_size_in_bytes is 64 MiB via Avro decoding. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
5d5ea18287 |
topology: fix fatal concurrent map read/write on VolumeLayout.crowded (#11216)
SetVolumeCrowded mutated the crowded map under accessLock.RLock(), while GetWritableVolumeCount reads the same map under RLock() on the Assign hot path. Two concurrent RLock holders with one writing and one reading the map triggers a fatal "concurrent map read and map write" that kills the master process (unrecoverable, bypasses recover). Take the write lock in SetVolumeCrowded instead. This event path is a low-frequency single consumer driven by the crowded-volume event loop, and every other mutation of crowded already holds Lock(); setVolumeCrowded takes no nested locks, so there is no deadlock path. The hot readers (GetWritableVolumeCount, CloneWritableVolumes) keep using RLock. Adds a -race regression test that fails (race detected) on the old RLock and passes with the write lock. Fixes #11211 |
||
|
Junker der ProvinzandJunker der Provinz
|
a4885b7975 |
log_buffer: stop closing a notification channel another reader still holds (#11177)
* fix(log_buffer): stop closing a notification channel another reader still holds - #10810 The report blames the polling loop for the busy spin, but that loop is not what burns the core. LogBuffer keeps one notification channel per subscriberID, and UnregisterSubscriber closes it. Two registrations that share a subscriberID share that channel, which happens whenever a client opens a second stream or an old stream has not yet noticed it was replaced, so the first unregister closes a channel the other reader is parked on. A closed channel makes every receive in awaitNotificationOrTimeoutFor return instantly, and that reader then spins at full speed for the rest of its life. Subscriptions are now reference counted. Registering an existing subscriberID hands back the same channel and raises the count; the channel is only closed when the last holder unregisters. * test: fail if the surviving reader stops instead of keeps reading Review caught that the iteration count alone proves nothing: had LoopProcessLogData returned when the duplicate reader unregistered, the counter would sit at 0 and the assertion would pass without a reader ever having been there to spin. Check the reader is still running before trusting its low count. --------- Co-authored-by: Junker der Provinz <jdp@braethoria.com> |
||
|
Chris Lu
|
15e4da65f7 |
volume: avoid read-only replica write targets (#11195)
* master: carry replica read-only state in volume lookups * volume: refresh writable replica targets * volume: preserve read-only replicas for deletes * master: propagate read-only delete capability * volume: target delete-capable replicas * volume: honor configured HTTPS for replica deletes * volume: reject insecure delete authorization forwarding * master: broadcast delete capability changes * volume: align Rust replica routing * http: protect credentialed replica redirects * master: preserve digest compatibility for delete capability * volume: propagate read-only state in short heartbeats * volume: report changed short volume state * http: guard TLS client redirects * master: announce mounted volume read-only state * volume: replace changed identity deltas * master: replace incremental volume layouts in order * master: keep moved volume lookup available * volume: announce read-only mounts |
||
|
Tim Olowandtimolow
|
331c6c3642 |
shell: volume.check.disk — actionable verdict for diverged vacuumed replicas (#11197)
* shell: volume.check.disk — actionable verdict for diverged vacuumed replicas
When -resurrectMissingNeedles is gated off because both replicas have been
vacuumed (compaction revision > 0) — the normal state of any production
cluster — check.disk previously stopped at 'cannot prove they are missing
writes vs vacuumed deletes' and did nothing, leaving a diverged replica with
no repair path. volume.fix.replication does not catch it either: it only
acts when the replica COUNT is below the expected replication, never when
two replicas are both present but hold different live data.
Classify the divergence instead of dead-ending:
liveDivergence() counts live (non-deleted) needles present on one replica's
index but entirely absent from the other, in both directions. Tombstones
are excluded, so vacuum asymmetry (a compacted replica that dropped deleted
entries) is not mistaken for divergence.
reportDivergenceVerdict() turns the count into an operator action:
- one-sided (one replica has all the live data, the other has no unique
live needles) -> print the exact safe repair:
volume.copy -source <complete> -target <lagging> -volumeId <id>
Re-copying the complete replica is safe precisely because the lagging
side holds no unique live data; VolumeCopy's verify-before-destroy gate
independently confirms the source holds the volume before deleting the
target.
- two-sided (split-brain, both sides have unique live data) -> warn and
do NOT emit an auto repair; point to volume.fsck -findMissingChunksInFiler
to confirm the 'missing' needles are orphans before converging.
Report-only: no data is modified and the resurrection safety gate is
untouched. This is what lets a 13-volume diverged cluster be diagnosed and
repaired in minutes instead of by hand-diffing every index.
Observed motivating case: home SeaweedFS 4.45 cluster, 13 010 cross-rack
volumes diverged after failed replicate writes (ReplicatedWrite MaxAttempts=1
fire-and-forget), 11 one-sided + 2 two-sided, all repaired via volume.copy.
* shell: volume.check.disk — address review nits on divergence verdict
- Use pb.NewServerAddressFromDataNode (dialable ip:port, Address with Id
fallback) for the advertised volume.copy -source/-target instead of the
logical node Id, which may not be dialable.
- Make the one-sided verdict tombstone-aware: when the lagging replica has
been vacuumed, absent live needles may be valid deletions whose tombstones
were dropped, so a whole-volume re-copy would resurrect them. The command
is only advertised as safe when the lagging side is proven never-vacuumed
(compaction revision 0 read under -resurrectMissingNeedles); otherwise a
caveat is printed pointing at fsck/needle-level repair.
- Fix reversed copy direction when the source replica is the lagging one
(must copy complete -> lagging in both cases).
- Test: real tombstone (negative size) with the correct 0/0 expectation and
|| assertion; verdict test now covers dialable address, corrected
direction, and caveat on/off.
* shell: volume.check.disk — per-replica revision knowledge, no copy command for vacuumed lagging side
- Track srcRevKnown/tgtRevKnown separately: in unidirectional mode the
target revision IS read, so a proven never-vacuumed target no longer
gets a false resurrection warning (regression: bidi=false, target rev 0).
- A one-sided verdict now only emits the volume.copy command when the
lagging side is proven never-vacuumed; when it is vacuumed (or unproven)
the verdict refuses to print the destructive command and points at
fsck/needle-level repair instead — an appended caveat next to a ready-to-
paste copy command was still inviting the resurrection.
- deletionCaveat now returns the boolean safety decision.
* shell: volume.check.disk — preserve gRPC port, proven two-sided is not split-brain
- Emit the raw ServerAddress string (host:port.grpcPort) instead of
String()/ToHttpAddress(), which drops the custom gRPC port and would
make the suggested volume.copy dial the default port and fail.
- Two-sided divergence with both replicas proven never-vacuumed under
-resurrectMissingNeedles is mutually missed writes, not split-brain:
recommend re-running with -apply (in-place resurrection both
directions) instead of the split-brain no-auto-repair warning.
- Tests: case F (proven two-sided -> -apply, no split-brain warning),
case G (custom gRPC ports preserved in emitted addresses).
---------
Co-authored-by: timolow <tim@timolow.dev>
|
||
|
Chris Lu
|
70a26cb5d2 |
s3: gate IAM-cache gRPC RPCs behind admin Bearer auth (#11190)
* s3: gate IAM-cache gRPC RPCs behind admin Bearer auth The SeaweedS3IamCacheServer registered on the S3 gateway's internal gRPC port (default 0.0.0.0:18333) accepted PutIdentity/RemoveIdentity/PutPolicy/ DeletePolicy/GetPolicy/ListPolicies/PutGroup/RemoveGroup with no per-RPC authentication. An unauthenticated network peer could call PutIdentity with Actions:[Admin] and write straight into the live accessKeyIdent map that the SigV4 path reads, bypassing S3 authentication entirely. Mirror the filer's IamGrpcServer.checkAdminAuth: require a Bearer token signed with jwt.filer_signing.key (read from the existing s3a.filerGuard) at the top of every IAM-cache RPC. With no key configured the check is a no-op, matching the rest of SeaweedFS's gRPC surface. * credential: attach admin Bearer token to S3 IAM-cache propagation The filer's PropagatingCredentialStore fans IAM mutations out to peer S3 servers over the SeaweedS3IamCache gRPC service. Now that the S3 handlers require a Bearer token signed with jwt.filer_signing.key, attach one to the outgoing propagation context (mirroring shell/iamAdminAuthContext). With no key configured it is a no-op, so deployments that run without the signing key keep working. * credential: mint IAM-cache admin token after master discovery propagateChange attached the admin Bearer token before ListClusterNodes, so master-client retries could run down the (default 10s) token lifetime before the peer S3 fan-out began, leaving peers to reject an expired token and IAM caches stale. Move withIamCacheAdminAuth to after discovery succeeds, immediately before the propagation timeout is derived. * credential: cap IAM-cache propagation timeout below JWT lifetime The propagation fan-out used a fixed 10s timeout. If an operator configures jwt.filer_signing.expires_after_seconds below 10, the admin token can expire while slower S3 peers are still being contacted, leaving their IAM caches stale. Derive the propagation deadline as min(10s, tokenTTL) so it never outlives the token. withIamCacheAdminAuth now returns the token's lifetime (0 = no expiry) for this purpose. |
||
|
Chris Lu
|
e6f2386a0f |
admin: redact S3 secret keys for read-only sessions (#11189)
The Admin UI documents its read-only account as view-only and blocks its
write requests, but the authenticated read routes returned object-store
users with plaintext access and secret keys. A read-only admin user could
retrieve another user's live S3 credential pair from GET /api/users and
GET /api/users/{username} and use it directly against the S3 endpoint,
converting view-only access into the victim identity's object-store
authority.
Redact the reusable secret_key in GetUsers, GetUserDetails, and the
rendered users page whenever the requesting session has the read-only
role. The public access_key identifier is retained so identities remain
browsable; only the reusable secret is stripped. Admin and no-auth
sessions are unaffected.
|
||
|
Junker der Provinz
|
78f79a3919 |
master: honour -volume.fileSizeLimitMB on the master's /submit (#11176)
* fix(master): honour -volume.fileSizeLimitMB on the master's /submit - #6748 `weed server -volume.fileSizeLimitMB=2048` still refused anything over 256MB, and the reason is not the one the report assumes: the option does reach the volume server. The master does not use it. Uploads through the master's /submit are buffered by submitForClientHandler, which passed a hardcoded 256MB to needle.ParseUpload, so the master rejected what the volume server it started would have accepted. The limit is now passed in. `weed master` gains its own -fileSizeLimitMB with the same 256 default, so a standalone master behaves exactly as before, and `weed server` and `weed mini` hand it the value their volume server already got. * master.follower: take the same upload limit, and say which flag to match Review found the follower left behind. It serves /submit like the leader and buffers uploads under the same limit, but kept the fixed 256MB, so a cluster raised above that would accept an upload through the leader and refuse the identical one through a follower. Two smaller points from the same review: the master's flag description named only the standalone volume server's spelling, and now names the weed server and weed mini form too; and the under-limit test asserted on the error message alone, so it would have passed had the limit rejected that payload with different wording. It now requires the request to get past parsing. |
||
|
yanglongwei
|
eb717199d0 |
master: delete replica_placement_mismatch labels when volumes leave topology (#11062)
* master: delete replica_placement_mismatch labels when volumes leave topology Fixes #10804. Setting the gauge to 0 left stale Prometheus time series that grew unbounded with volume churn; remove the label set on unregister instead. * master: delete replica_placement_mismatch only after last placement leaves Unconditional DeleteLabelValues on UnRegisterVolumeLayout dropped the series while other data nodes still held the volume, hiding under-replication until the next collect cycle. Delete only when Lookup is empty, and cover the two-copy case in a regression test. |
||
|
Chris Lu
|
3e85d9ec8e |
admin: bind to loopback by default, guard public unauthenticated bind (#11185)
admin: bind to loopback by default, refuse public unauthenticated bind The admin HTTP server (port 23646) defaulted to binding 0.0.0.0 with authentication disabled when -adminPassword was not supplied, exposing the full admin REST API (user creation, credential issuance, bucket deletion, filer deletion) unauthenticated on the network. This is the footgun described in GHSA-m3m8-mrgq-hf9h. Keep the no-auth mode for local dev, but remove the network exposure: - Add -ip flag (default 127.0.0.1) so the server binds loopback only unless the operator explicitly chooses a public address. - Refuse to start when binding a non-loopback address with no -adminPassword and no [https.admin] mTLS. The operator must enable auth or use loopback. - weed mini sets -ip from its existing -ip.bind; the guard does not apply because mini calls startAdminServer directly, not runAdmin. Addresses GHSA-m3m8-mrgq-hf9h. |
||
|
Chris Lu
|
f35e2ccf21 |
s3: warn when a lifecycle change leaves fast-path-stamped objects on their old TTL (#11184)
* s3: warn when a lifecycle change leaves fast-path-stamped objects on their old TTL The per-write TTL fast path (opt-in via s3.bucket.lifecycle.fastpath) stamps a volume TTL at PutObject time that can't be taken back. When an operator lengthens or removes an Expiration.Days rule (or deletes the bucket lifecycle) on a fast-path-enabled bucket, objects already written keep their baked-in TTL and won't be rescued by the change — unlike the default worker-driven path, which re-evaluates the current rules each pass. This is the data-loss direction described in #11183. Surface it: Put/DeleteBucketLifecycle now emit a glog warning and set X-Seaweed-Lifecycle-Fastpath-Warning on the response when the change removes, disables, lengthens, or re-scopes a fast-path-eligible rule. Shortening a rule does not warn (old objects simply expire later, not data loss). Tag-only and overflow-day rules are never on the fast path and never warn. Addresses the warning half of option 2 in #11183. * s3: address review — emit warning after mutation succeeds, fix ID-rename false positive Two issues raised by CodeRabbit, Greptile, and Devin reviews: 1. Failed mutations retained the warning header. The warning was set on the ResponseWriter before storeBucketLifecycleConfiguration / clearStoredBucketLifecycleConfiguration was called; if that failed, the error response carried a warning for a change that was never applied. Now the reason is computed before the mutation but the log and header are emitted only after it succeeds. 2. Rule renames produced false "removed" warnings. fastpathRuleKey used Rule.ID as the sole identity when present, so renaming a rule (same prefix/size/days, different ID) treated the old rule as removed. Replaced with two-pass matching: first by ID, then by fast-path predicates (prefix + size). An ID-only rename with unchanged predicates and days no longer warns. Greedy matching ensures each new rule is consumed by at most one old rule. Added regression tests: ID-only rename (no warn), rename + lengthen (warn), rename + shorten (no warn). |
||
|
|
f99c4a1f14 |
s3: make RenameObject idempotent for a retried request (#11178)
* feat(s3): make RenameObject idempotent for a retried request - #10661 A rename that succeeds but whose response is lost leaves the client with no safe move: retrying returned 404, because the source is already gone, so a retry was indistinguishable from a rename that never happened. The destination now carries what the rename that created it was, under x-seaweedfs-rename-token: the client's token, the source key and the time. A retry that names the same token and the same source and destination is answered 200 without touching anything. The same token sent for a different rename is refused with 409 rather than silently answered, and a token older than 24 hours is treated as unrelated so a key cannot answer for a request indefinitely. Requests without the header behave exactly as before. * s3: answer a reused rename token with 409, not 400 The PR promised Conflict and the code returned Bad Request. 400 tells a client its request was malformed and invites it to give up; this request is well formed and resending it unchanged will not help, because what it collides with is a rename the same token already stands for. The status code is now asserted in a test, since it is the part of this behaviour a client actually acts on. * Update weed/s3api/s3err/s3api_errors.go Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: fix rename token review notes - ErrIdempotentParameterMismatch returns 409 Conflict, not 400. The comment and TestRenameTokenReuseAnswersConflict both expect 409; the code regressed to 400 in a later commit. - stampRenameToken: clarify that markRenameToken mutates srcEntry in place, so the token reaches the destination via the move regardless of whether the UpdateEntry succeeds. The precondition only guards the pre-move write, not the move itself. - Extract the handler retry branch into retryRenameDecision and add TestRetryRenameDecision, covering the source-still-exists fallthrough that was previously reasoned about but not tested. * s3: IdempotentParameterMismatch returns 400, matching AWS docs The AWS S3 RenameObject API documentation specifies HTTP Status Code: 400 for IdempotencyParameterMismatch. Revert the previous 409 change and align the comment and test with the documented behavior. --------- Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
8a68337256 |
filer: pack SSE chunks into manifests (#11175)
* filer: pack SSE chunks into manifests * s3: resolve encrypted manifests before reads * s3: scope encrypted manifest resolution to ranges |
||
|
Chris Luanddevin-ai-integration[bot]
|
811b8b5734 |
make the remote-mount cache wait configurable per mount (#11168)
* add a per-mount cache_wait_ms to the remote storage mount mapping A read of an uncached remote-only object waits on a hardcoded size tier before it can fall back to the origin, so every ranged read of a large remote-only object pays that wait. Carry the wait in the mount mapping so it can be tuned, or set to zero, per mount. * resolve the cache wait of an uncached remote-only read from its mount The wait came only from the object size, so an operator could not trade cache hits for time to first byte. Both read paths now resolve the mount covering the object and let its cache_wait_ms replace the size tiers. * read straight from the remote when a mount waits zero for its cache A mount used as a streaming source pays the cache wait on every ranged read of an object too large to finish caching, and the caching itself is wasted work. A zero wait now skips the cache call, so both read paths go to the origin immediately. * let remote.mount set the cache wait of a mount remote.mount -cacheWait=0 turns a mount into a streaming source, and any other duration trades cache hits against time to first byte. * keep the size based wait for a version-specific read A read pinned to a version cannot fall back to the origin, since the mounted remote only holds the current key, so a mount that opts out of caching would leave it on the 503 retry loop forever. * let the operator allow a remote-only read to dial an internal endpoint The remote-mount read paths in the filer and the S3 gateway always refused an endpoint resolving to a loopback or private host, so a mount backed by an internal S3 could never be read from its origin, only through the local cache. Both now take the allowance the volume server already has, still off by default. * skip the background cache of a mount that waits zero for its cache GetObjectHandler kicks off caching for every remote-only read, so a mount serving as a streaming source kept downloading whole objects even though no read ever waited for them. * cover a zero cache wait end to end The read has to reach a real origin, so the harness also opts the filer and the S3 gateway into dialing the loopback remote it already allows for the volume server. * resolve the S3 cache wait once so the background cache follows it too The background cache that GetObjectHandler starts read the mount on its own, so it skipped a version-specific read that the foreground path still waits for. Both now ask the same resolver. * answer 404 when the origin of a zero-wait read is gone Metadata can outlive the object it points at, and with no cache to fill the read would sit on the 503 retry path forever. The remote backends already report a missing object as ErrRemoteObjectNotFound. * open the origin at write time for a multipart range Every part of a multipart Range is prepared before any is written, so opening eagerly would hold one origin connection per part and leak the ones already opened when a later part fails to open. * reject a cache wait shorter than a millisecond The mapping stores milliseconds, so -cacheWait=500us truncated to zero and silently turned caching off instead of waiting. * restore the doc comment of cacheRemoteObjectForStreamingWithShortTimeout Extracting the wait resolver left its comment on the new function. * stat the origin before committing a multipart range Opening at write time keeps no connection through the preparation, but it also moved a failure past the point where the multipart body picks the response status, so a gone origin truncated a 206 instead of answering 404. One stat up front puts the status back. * stat the origin once per request Every part of a multipart Range is prepared on its own, so the preflight ran once per range instead of once per read. * map Azure and GCS stream not-found to ErrRemoteObjectNotFound ReadFileAsStream on Azure and GCS returned provider-specific not-found errors instead of ErrRemoteObjectNotFound, so a zero-wait read of a deleted object was misclassified as a transient cache failure and retried indefinitely. Map BlobNotFound and ErrObjectNotExist the same way StatFile already does. * Update weed/remote_storage/gcs/gcs_storage_client.go Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
Chris Lu
|
f79d83abf4 |
volume: expire TTL volumes whose only traffic is deletes (#11167)
* volume: count a TTL volume's age from its last write, not the .dat mtime A delete appends a tombstone needle and vacuum rewrites the .dat wholesale, so the file's mtime moves without any write ever landing. The loader read lastModifiedTsSeconds back from that mtime, so every restart of a volume taking delete traffic re-armed expired() for another full TTL: an overwrite-heavy collection kept growing until it hit the max-volume cap. Recover the clock from the newest .idx entry that is not a tombstone and read that needle's append timestamp, falling back to the mtime when no write is recoverable. Only TTL volumes pay for the scan. Fixes #11160 * volume: count the .vif destroy time from the last write too ExpireAtSec is what an EC volume is reclaimed on, and it was recomputed as now+TTL every time the .vif was written. A read-only mark, a tier upload or an EC encode therefore handed an already expiring volume another full TTL, the same way the .dat mtime did. Derive it from the volume's last write, falling back to now for a volume that has not taken one yet so a fresh volume is not born expired. * volume: mirror the last-write TTL clock in the Rust volume server Same recovery as the Go loader: scan the .idx backwards for the newest entry that is not a tombstone and take that needle's append timestamp, leaving the clock on the .dat mtime when no write is recoverable. * volume: mirror the last-write destroy time in the Rust volume server Both .vif writers and the EC encode computed ExpireAtSec as now+TTL, the same way Go did, so the destroy time moved every time the sidecar was rewritten. Route all three through the volume's last write. * volume: report the .dat mtime in the Rust heartbeat, like Go does The Rust server reported its TTL clock as ModifiedAtSecond while Go reports the .dat mtime. The shell's quiet-period gates (volume.tier.move, volume.delete_empty) read that field as "last touched", which a delete has to count towards even though the TTL clock deliberately ignores it -- and with the clock now recovered from the last write, the two drift further apart. * volume: take the newest write by timestamp on a vacuumed volume The reverse .idx scan trusted position, which holds only while the .dat is append ordered. Vacuum rewrites it in key order, and since an overwrite keeps its original key, the highest-key survivor is not necessarily the newest write -- the recovered clock could land up to a TTL early and take the volume with data still inside its TTL. A volume that has been vacuumed (CompactionRevision > 0) now takes the maximum append timestamp over a bounded window of write entries instead. An append-ordered volume still answers in one read. * volume: never guess a vacuumed volume's last write, and resolve wrapped offsets Two holes in the reverse scan, both from review: A vacuumed volume's writes are ordered by key, so any of them can hold the newest timestamp. Reading a capped window sampled the highest keys, which could still miss a recently overwritten low-key needle and expire data inside its TTL. The scan now covers every write a vacuumed volume indexes, and a volume too large to scan keeps the .dat mtime rather than report a partial maximum -- late is recoverable, early is not. A .dat past MaxPossibleVolumeSize wraps the offsets in its .idx, so reading a timestamp at the unwrapped offset picks up an unrelated needle. Resolve the entry against the needle header first and retry one volume size in, the way doCheckAndFixVolumeData already does. * volume: drop GitHub issue references from TTL comments |