mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-07-28 19:13:17 +00:00
49e83a26cb4e1a2d8fd64862efc1097571e01255
13630
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  Chris LuandGitHub
|
49e83a26cb |
feat(seaweed-volume): auto-load EC shards on startup (#9212) (#9251)
* feat(seaweed-volume): auto-load EC shards on startup
The Rust volume server's load_existing_volumes only scanned .dat
files; EC shards on disk stayed invisible until something explicitly
issued VolumeEcShardsMount. Strict superset of the issue
seaweedfs/seaweedfs#9212 reports for Go: after a fresh restart, every
local EC shard was missing from the master's view.
Port loadAllEcShards from weed/storage/disk_location_ec.go:
- DiskLocation::load_all_ec_shards walks Directory (and IdxDirectory
if separate) sorted, groups .ec?? shard files by (collection, vid),
validates and mounts each group when its matching .ecx is found.
- handle_found_ecx_file: validate_ec_volume + mount_ec_shards path,
with cleanup when .dat exists and validation fails (incomplete
encoding) or load fails.
- check_orphaned_shards: cleans up shard remnants whose .ecx never
arrived AND whose stale .dat is still present (interrupted
encoding); leaves them on disk otherwise so cross-disk
reconciliation / operator recovery can find them.
- check_dat_file_exists / parse_collection_volume_id /
parse_ec_shard_extension: small helpers mirroring Go's checkDatFileExists,
parseCollectionVolumeId, and the `\.ec\d{2,3}` regex.
- Wire through load_existing_volumes after the .dat scan; failures
log but don't fail the disk's startup.
Tests:
- test_parse_ec_shard_extension covers .ec00–.ec255 and the rejection
of .ec0, .ec999, .ecx, .ecj, .dat, and missing leading dot.
- test_load_all_ec_shards_mounts_pairs_with_ecx: shards + .ecx + .vif
on disk get mounted into ec_volumes after load_existing_volumes.
- test_load_all_ec_shards_keeps_orphan_shards_when_no_dat: orphan
shards (no .ecx, no .dat) stay on disk untouched
(distributed-EC scenario).
- test_load_all_ec_shards_cleans_orphan_shards_when_dat_exists:
orphan shards alongside a stale .dat get cleaned up
(interrupted-encoding scenario).
Prerequisite for porting the cross-disk orphan-shard reconciliation
in seaweedfs/seaweedfs#9244 to Rust.
* fix(seaweed-volume): dedupe filenames when scanning data + idx dirs
load_all_ec_shards scans both `directory` and `idx_directory` (when
they differ) so the loop can pair `.ec??` shards with their `.ecx`
regardless of which dir owns the index. If the same filename is
present in both — possible in idempotent legacy layouts that
pre-date `-dir.idx` — the previous implementation processed it
twice. mount_ec_shards increments the per-shard `ec_shards` metric
inside the loop, so a duplicated `.ec??` entry would double-count
the gauge.
Use a HashSet<String> while accumulating entries so each filename
is processed exactly once.
Reported in PR #9251 review by @gemini-code-assist.
* fix(seaweed-volume): drive partial-mount cleanup through unmount_ec_shards
handle_found_ecx_file calls mount_ec_shards which adds shards one at
a time. mount_ec_shards increments the `ec_shards` gauge per shard
that successfully attaches. If mount fails halfway, plain
ec_volumes.remove(vid) drops the EcVolume but leaves the gauge
incremented for whatever did mount.
Drive the cleanup branches through unmount_ec_shards instead — it
mirror-decrements the gauge per shard and only then drops the
EcVolume. Same shape applied to both .dat-exists and distributed-EC
fallbacks.
Reported in PR #9251 review by @gemini-code-assist.
* docs(seaweed-volume): clarify parse_ec_shard_extension shard-id range
Doc previously said `.ec00`–`.ec999` but the implementation rejects
any shard id > 255 (matches the `EcVolumeShard` u8 typed shard id
and Go's `strconv.ParseInt(... 10, 64)` + `> 255` guard). Fix the
doc to say `.ec00`–`.ec255` and explain why the 3-digit form is
still recognised.
Reported in PR #9251 review by @coderabbitai.
|
||
|
Chris LuandGitHub
|
933ae6e386 |
fix(seaweed-volume): port EC shard placement fix to Rust (#9212, mirrors #9245) (#9250)
* feat(seaweed-volume): add DiskLocation::has_ecx_file_on_disk Mirrors `DiskLocation.HasEcxFileOnDisk` from the Go side (seaweedfs/seaweedfs#9245). Reports whether this disk has a sealed .ecx index file for (collection, vid) by stat'ing the IdxDirectory first, then falling back to Directory if different — covers the legacy "written before -dir.idx was set" layout. Skips entries that are directories so a stray dir named `<col>_<vid>.ecx` doesn't register as a present index file. Unlike has_ec_volume() this does not require the EC volume to be mounted in memory, which makes it the right primitive for placement decisions during ec.balance / ec.rebuild flows where shards may arrive before any VolumeEcShardsMount has happened on the receiving disk. Wiring + tests in follow-up commits. * feat(seaweed-volume): add Store::find_ec_shard_target_location Mirrors `Store.FindEcShardTargetLocation` from the Go side (seaweedfs/seaweedfs#9245). Single canonical placement primitive for new EC shard / index files. Selection order: 1. a disk that already has the EC volume mounted (in-memory), 2. a disk that owns the .ecx file on disk (volume not yet mounted), 3. any HDD with free space, 4. any disk with free space. Step 2 is the missing primitive that pinned subsequent shards to the first-shard disk during ec.rebuild — rebuild only sets CopyEcxFile=true on the first shard, then relies on auto-select to land later shards on the same disk. Without an on-disk check has_ec_volume returns false (no mount yet) and the fallback picked "any HDD with free space," splitting shards from their .ecx across disks of the same node and producing the orphan-shard layout seaweedfs/seaweedfs#9212 reports. Implementation walks store.locations once with tier scoring; the highest-tier disk wins, ties broken by free count. The earlier 4-pass waterfall in find_free_location_predicate would have re-acquired locks per pass. ec_free_shard_count returns the free count in shard slots (not volume-equivalent slots). The pre-existing find_free_location* helpers divide by DATA_SHARDS_COUNT at the end; that truncation can exclude a disk that has room for several individual shards (MaxVolumeCount=1, EcShardCount=1, dsc=10 → reports 0 despite 9 free slots), which would re-route subsequent shards off the .ecx-owning disk and re-introduce the orphan layout. Keep the result in shard slots throughout. The unlimited-disk branch (MaxVolumeCount==0) reports a synthetic large free count decremented by current usage so unlimited disks stay eligible and tie-breaks still prefer the less-loaded one. data_shard_count is taken as a parameter rather than read from DATA_SHARDS_COUNT so custom-ratio builds can swap the default without touching this helper. Tests cover: pinning to .ecx on disk, mounted-wins-over-stray-.ecx, HDD fallback, MaxVolumeCount=0 unlimited handling, and the tight-provisioning truncation case. * fix(seaweed-volume): route EC shard auto-select through new helper VolumeEcShardsCopy and the ReceiveFile EC branch both used a 3-tier inline waterfall: in-memory has_ec_volume → any HDD → any disk. That checked in-memory state only and missed disks that own the .ecx on disk but haven't been mounted yet — the orphan-shard placement hazard from seaweedfs/seaweedfs#9212. Replace both with a single call to Store::find_ec_shard_target_location, which adds the .ecx-on-disk tier between mounted and HDD, and accounts for free space in shard slots so tight-provisioning configurations don't incorrectly skip a disk that still has room for individual shards. Pass DATA_SHARDS_COUNT as the data-shard count for free-slot maths; the helper takes it as a parameter so custom-ratio builds can swap the default without touching this file. * fix(seaweed-volume): grow UNLIMITED_FREE budget and saturate the math ec_free_shard_count's unlimited branch (MaxVolumeCount=0) used to clamp to a constant `1` once usage exceeded `1 << 30 ≈ 1e9` shard slots. With several unlimited disks all past that threshold, every placement decision among them tied at 1 — tie-break degraded to "first eligible disk." Bump the synthetic budget to `1 << 60 ≈ 1.15e18` and use saturating arithmetic so even pathological usage never wraps i64. Clamp the return value to `≥ 1` so the disk stays eligible for placement at any load. Tie-breaks among unlimited disks now keep preferring the less-loaded one across all realistic deployments. Reported in PR #9250 review by @gemini-code-assist. |
||
|
Chris LuandGitHub
|
f50917224a |
fix(iceberg): default namespace location so fresh CTAS does not race metadata write (#9074) (#9246)
* fix(iceberg): advertise default namespace location for REST clients
Trino's REST catalog has two code paths for CREATE TABLE depending on
whether a table location can be resolved before the catalog call:
// TrinoRestCatalog.newCreateTableTransaction (Trino 479)
if (location.isEmpty()) {
return tableBuilder.create().newTransaction(); // EAGER: REST POST now
}
return tableBuilder.withLocation(...).createTransaction(); // DEFERRED
`tableLocation` resolves to null when the user does not pass
`location = '...'` AND `defaultTableLocation` returns null. The latter
happens whenever the namespace's `loadNamespaceMetadata` response has no
`location` property — and our handler returned exactly that.
In the eager branch Trino calls REST POST /v1/.../tables immediately, so
our handleCreateTable persists `<location>/metadata/v1.metadata.json` to
the filer. Trino's IcebergMetadata.beginCreateTable then runs
`fileSystem.listFiles(location).hasNext()` on the same path, finds the
metadata file we just wrote, and throws "Cannot create a table on a
non-empty location" — even on a brand-new schema and table name.
Synthesize a default `location` of `s3://<bucket>/<flattened-namespace>`
on Get/Create namespace responses when one is not stored. With a non-
null `defaultTableLocation`, Trino takes the deferred branch, picks a
unique `<namespace-location>/<table>-<UUID>` path (UUID added by the
standard `iceberg.unique-table-location=true` setting), and the empty-
location check passes. The actual REST POST is deferred to commit time,
so our metadata write lands alongside the data files Trino has already
produced.
Existing namespaces with an explicit `location` property are untouched —
the synthesis only kicks in when the property is absent.
Refs #9074
* test(trino): regression for fresh CREATE TABLE without explicit location
Exercises the follow-up scenario reported on issue #9074: a CREATE TABLE
on a brand-new schema and brand-new table name with NO `location = '...'`
clause, followed by a CTAS on top — exactly the SQL pattern from the
report. Before advertising a default namespace location, the first
CREATE TABLE failed with
Cannot create a table on a non-empty location:
s3://iceberg-tables/<schema>/<table>, set
'iceberg.unique-table-location=true' in your Iceberg catalog
properties to use unique table locations for every table.
even though the bucket was genuinely empty. The bug surfaced because our
handleCreateTable persists `<location>/metadata/v1.metadata.json` during
Trino's eager-create branch, and Trino's post-create listFiles
emptiness check then trips on the metadata file we just wrote.
The test asserts the CTAS succeeds AND the resulting table contains the
source rows, since the reporter saw the table get created with empty
data when the query failed.
Refs #9074
|
||
|
Chris LuandGitHub
|
cba2f7b1dd |
fix(volume_server): load orphan EC shards across disks on startup (#9212) (#9244)
* fix(volume_server): load orphan EC shards across disks on startup (#9212) When ec.balance / ec.rebuild copies an EC shard onto a destination node without also pinning subsequent shards to the disk that holds .ecx, the shard ends up on a different physical disk than its index files. The per-disk loadAllEcShards has no visibility into other DiskLocations on the same store, so those orphan shards were silently left out of ecVolumes and never reported to master — volume.list showed partial counts, and ec.rebuild reported the volume as unrepairable even though all shards were physically present. After every DiskLocation finishes its initial pass, sweep the store for shard files that are on disk but not yet in any EcVolume, look up the .ecx-owning sibling disk, and load each shard against its physical disk with dirIdx pointing at the sibling. Each shard is still registered on its own disk's ecVolumes map so heartbeat reporting carries the right DiskId per shard (master fix #9219 already aggregates per-disk messages correctly). Also fall back to dirIdx for .vif lookup when dir != dirIdx, so the reconciliation path doesn't write a stub .vif on the shard disk and lose the real EC config and datFileSize. * fix(volume_server): track actual .ecx dir in cross-disk reconcile indexEcxOwners scans both IdxDirectory and Directory to find each volume's .ecx — the second scan covers the legacy case where index files were written into the data dir before -dir.idx was configured (removeEcVolumeFiles already accounts for this in disk_location_ec.go). But the returned map dropped which directory matched, and reconcile unconditionally passed owner.IdxDirectory to loadEcShardsWithIdxDir. When the owner's .ecx is in Directory and IdxDirectory != Directory (server later re-configured with -dir.idx pointing at a fresh path), NewEcVolume opens IdxDirectory/.ecx → ENOENT, retries the same-disk fallback at dataBaseFileName+.ecx — but dataBaseFileName uses the *orphan* disk's data dir, not the owner's, so it ENOENTs again and the orphan shards stay unloaded. Track which scan dir matched in indexEcxOwners and pass it through. Adds TestLoadEcShardsWhenOwnerEcxIsInDataDir as the regression. Reported in PR #9244 review by @gemini-code-assist and @coderabbitai. * refactor(storage): thread dataShardCount as a parameter into calculateExpectedShardSize The helper used erasure_coding.DataShardsCount directly, but tests in store_ec_orphan_shard_test.go save .vif with a local dataShards=10 constant. If the package default ever diverged from 10 (e.g. an enterprise build), the test would write a .vif for one layout while sizing shard files for another and silently break. Take dataShardCount as a parameter. Existing callers (validateEcVolume + size-validation tests + real-world tests) pass erasure_coding.DataShardsCount unchanged. The orphan-shard tests pass the same dataShards local they save into .vif, so the persisted shape and the on-disk shape stay consistent. Reported in PR #9244 review by @coderabbitai. |
||
|
Chris LuandGitHub
|
5fbe39320c |
fix(volume_server): pin EC shard auto-select to the .ecx-owning disk (#9212) (#9245)
* fix(volume_server): pin EC shard auto-select to the .ecx-owning disk (#9212) ec.rebuild only sets CopyEcxFile=true on the first shard sent to the rebuilder; subsequent shards rely on VolumeEcShardsCopy / ReceiveFile auto-select to land on the same disk. The old auto-select used FindEcVolume (in-memory) to detect the "already has this volume" case. Mid-rebuild, no EC volume has been mounted yet on the destination, so FindEcVolume returns nothing and the fallback picks "any HDD with free space" — which can split shards from their .ecx across disks of the same node and feed the orphan-shard layout reported in #9212 / fixed on the loader side in #9244. Add Store.FindEcShardTargetLocation as the canonical placement primitive: prefer a mounted EC volume, then a disk that has the .ecx on disk, then any HDD, then any disk. DiskLocation.HasEcxFileOnDisk is the new on-disk check, and it looks at IdxDirectory first with a fallback to Directory to handle .ecx written before -dir.idx was configured. Both VolumeEcShardsCopy and ReceiveFile now route through the new helper, dropping their duplicated 4-level fallback ladder. No protocol changes; explicit DiskId callers are unaffected. * fix(volume_server): treat directories named *.ecx as no-match in HasEcxFileOnDisk os.Stat(".ecx") succeeds for both files and directories. If something happens to leave a directory named X.ecx in the data or idx folder, HasEcxFileOnDisk would currently report true and FindEcShardTargetLocation would route shards to that disk — where NewEcVolume's eventual OpenFile(O_RDWR) on the same path errors out. Add a !info.IsDir() check on both stat sites. Cheap and conservative. Suggested in PR #9245 review by @gemini-code-assist. * refactor(volume_server): collapse EC placement helper to a single pass FindEcShardTargetLocation called FindFreeLocation up to four times. Each call iterates s.Locations and acquires VolumesLen / EcShardCount RLocks per disk — for a typical 4-disk node that's 32 RLock cycles per placement decision. Walk s.Locations once, score each disk by tier (mounted > .ecx-on-disk > HDD > any-disk), break ties by free count. The free-slot math is factored into a small helper that mirrors FindFreeLocation's formula without re-entering the location's locks. Behaviour is unchanged: each existing tier still wins over later tiers, and within a tier the disk with the most free count still wins, matching the original max-tracking in FindFreeLocation. Suggested in PR #9245 review by @gemini-code-assist. * refactor(volume_server): thread dataShardCount as a parameter through EC placement ecFreeShardCount and FindEcShardTargetLocation referenced erasure_coding.DataShardsCount directly. Take it as a parameter so custom-ratio builds (e.g. enterprise) can swap the default without touching the helper itself, and so unit tests can pin a specific ratio independent of the package constant. Default callsites in VolumeEcShardsCopy and ReceiveFile now pass the package default explicitly; tests pass a literal 10 for clarity. * fix(volume_server): treat MaxVolumeCount=0 as unlimited in EC placement ecFreeShardCount computed `MaxVolumeCount - VolumesLen()` and went negative when MaxVolumeCount was 0 — the "unlimited disk" sentinel already honoured by Store.hasFreeDiskLocation and friends. With a negative free count, FindEcShardTargetLocation's `freeCount <= 0` guard skipped the disk entirely, so unlimited disks could never receive EC shards via the placement helper. Special-case MaxVolumeCount<=0: report a synthetic large free count that decrements with current usage, so unlimited disks are eligible and tie-breaks still prefer the less-loaded one. Added TestFindEcShardTargetLocation_HonoursUnlimitedDisk as the regression. Reported in PR #9245 review by @gemini-code-assist. * fix(volume_server): account in shard slots, not volume slots, in ecFreeShardCount FindFreeLocation in store.go ends with `free /= DataShardsCount`, converting "shard slots free" back to "volume-equivalent slots." The truncation is harmless there, but my new ecFreeShardCount inherited the same final divide and re-introduced exactly the orphan-shard hazard #9245 was meant to prevent: with MaxVolumeCount=1, VolumesLen=0, EcShardCount=1 the formula reports 0 even though the disk has room for 9 more shards, so subsequent shards route off the .ecx-owning disk into the HDD-fallback tier. Drop the trailing divide and return the count directly in shard slots. Same shape, finer granularity; tie-breaks still order by free count. The unlimited branch's "used" calculation is updated to match (mix volume-slots and shard-slots in shard units). Added TestFindEcShardTargetLocation_TightProvisioningKeepsEcxDisk as the regression. Reported in PR #9245 review by @coderabbitai. |
||
 Lars LehtonenandGitHub
|
1add6cbbca | chore(weed/topology): prune unused functions (#9249) | ||
 dependabot[bot]andGitHub
|
4d8ddd8ded | build(deps): bump aquasecurity/trivy-action from 0.35.0 to 0.36.0 (#9248) | ||
 Lisandro PinandGitHub
|
2c404f66bc |
Export file_read_invalid_needles metric for REST read requests on invalid file IDs. (#9241)
Provides a straightforward metric to count read requests with incorrect file/needle IDs, which can indicate client issues. Note that the metric does not cover gRPC calls, as the current proto service API does not support seeking files by ID. |
||
 Chris Lu
|
ac3a756dae |
test(s3-retention): force-drop collection after deleteBucket to free volumes
COMPLIANCE-mode retention leaves objects that BypassGovernanceRetention cannot clear, so the test's DeleteBucket keeps returning BucketNotEmpty and the underlying SeaweedFS collection (with its 7 reserved volumes) leaks. After a few leaks on the single-node `weed mini` server, the master logs "Not enough data nodes found" and every subsequent PutObject 500s, timing the suite out. Call the master's /col/delete admin endpoint from deleteBucket so the collection's volumes are reclaimed even when S3-level cleanup is blocked. |
||
|
Chris LuandGitHub
|
3f5b4814b7 |
fix(kafka): evict expired group members on rejoin to unblock fast restart (#9243)
fix(kafka): evict expired group members on JoinGroup and during cleanup Phantom members lingering past their session timeout were blocking TestOffsetManagement/ConsumerGroupResumption: the cleanup goroutine runs every 30s but session timeouts can be as short as 6s, so a lost LeaveGroup or ungraceful disconnect could leave a dead member holding the leader slot or a partition until the next cleanup tick. New consumers rejoining within that window saw stale group composition and burned all 5 test-level join retries (~118s, generation churned to 22 in the failing CI run). Add EvictExpiredMembersLocked on GroupCoordinator and call it from handleJoinGroup so each rejoin sees a fresh member list. Refactor performCleanup to share the helper, and on either path move surviving members to PreparingRebalance, bump generation, clear cached assignments (so the non-leader SyncGroup path returns REBALANCE_IN_PROGRESS instead of serving stale partitions), re-elect a leader from survivors when needed, and rebuild SubscribedTopics so topics only the evicted member subscribed to are dropped. |
||
|
Chris Lu
|
fa492a9eed |
fix(admin): wrap plugin URLs with basePath for subdir deployments
Two more spots that broke under a subdirectory deployment: - plugin.templ pluginRequest() called fetch(url) with relative API paths from 14+ callers; wrap once inside the helper so they all honor window.__BASE_PATH__. - plugin_lane.templ generated <a href="/plugin/configuration?job=..."> with an absolute path; wrap with basePath() so the link stays inside the deployment prefix. Follow-up to a6adf530c. |
||
|
Chris Lu
|
3ea489d013 |
fix(admin): wrap plugin lane fetch URL with basePath
Plugin lane page fetches API endpoints with raw absolute URLs, breaking deployments under a subdirectory. Wrap the fetch URL with basePath() so window.__BASE_PATH__ is honored, matching other admin pages. Addresses https://github.com/seaweedfs/seaweedfs/issues/9240 |
||
|
Chris LuandGitHub
|
7f770b1553 |
fix(filer): return 503 + Retry-After when remote object not cached yet (#9236)
* extend cache-not-ready handling to filer HTTP path Mirror the s3api change for the native filer HTTP handlers. When the filer GET hits a remote-only object whose cache fill hasn't completed, return 503 Service Unavailable with Retry-After: 5 instead of 500 Internal Error, and treat client disconnects as silent cancellations rather than logging them as errors. Adds an ErrCacheNotReady sentinel and a small helper used at the prepareWriteFn-error sites in ProcessRangeRequest, so the same classification (cancel / not-ready / other) applies to plain GETs, single-range, and multi-range requests. * clear Content-Range on prepareWriteFn error The single-range path sets Content-Range before calling prepareWriteFn. If prepareWriteFn fails, http.Error is about to write a fresh body for 503 or 500, but the stale Content-Range header would still go out and no longer match. Drop it alongside Content-Length in the shared helper so all current and future callers are covered. * strip success-path headers and forward NotFound on prepareWriteFn error When ProcessRangeRequest writes an error response, the previously-set success headers (Content-Disposition, ETag, Last-Modified, in addition to Content-Length/Content-Range) shouldn't ride along on the new body. With ?dl=1 a stale Content-Disposition would even cause browsers to save the error message under the object's filename. Strip them all in the shared helper. Also forward filer_pb.ErrNotFound through the cache-failure branch so a mid-cache entry deletion surfaces as 404, not as a 503 retry-loop. Permanent upstream cloud errors (403/404 from the cloud SDK) still come back as opaque wrapped strings via FetchAndWriteNeedle and remain mapped to 503; distinguishing those would need a wider refactor. |
||
|
Chris LuandGitHub
|
4c4d53ce23 |
fix(seaweed-volume): accept redb aliases for --index (#9237)
fix(seaweed-volume): accept redb aliases for --index and rename kinds The Rust volume server's disk-backed index uses redb internally (see RedbNeedleMap), but --index only accepted the legacy `leveldb` spellings, contradicting the wiki and forcing users to read source to figure out what value to pass. - --index now accepts memory|redb|redbMedium|redbLarge as the canonical names, with leveldb/leveldbMedium/leveldbLarge kept as aliases. - Rename NeedleMapKind variants LevelDb*->Redb* so the in-tree names match the actual backend. - Update help text and add a parse-table test covering both names. Refs #9234. |
||
|
Chris LuandGitHub
|
045ace29d5 |
fix(seaweed-volume): parse host:port.grpcPort in master address (#9235)
The Go ServerAddress format encodes an optional explicit gRPC port as host:port.grpcPort. The Rust heartbeat client only handled host:port (falling back to port+10000), so feeding it host:port.grpcPort yielded a malformed gRPC target like "host:port.grpcPort", which manifests as checkWithMaster transport errors. Mirror pb.ServerToGrpcAddress(): if the part after the last ':' contains a '.' followed by a valid u16, treat that suffix as the explicit gRPC port; otherwise keep the +10000 default. Refs #9234. |
||
|
Chris LuandGitHub
|
a2639b533e |
fix(s3api): return 503 + Retry-After when remote object not cached yet (#9233)
* fix(s3api): return 503 with Retry-After when remote object not cached yet When a GET hits a remote-only object whose cache fill timed out or was canceled, the handler returned 500 InternalError. SDK clients treat 500 as a server bug and surface it as a fatal error (boto3 S3DownloadFailedError), even though the cache is often still filling in the background and the next request would succeed. Return 503 ServiceUnavailable with Retry-After: 5 instead, matching AWS S3's "try again later" semantics. AWS SDKs already classify 503 as retryable and apply exponential backoff transparently, so clients recover without changes. Refs https://github.com/seaweedfs/seaweedfs/discussions/9174 * treat client cancel as cancellation, not 503 If r.Context() is already canceled when the cache attempt returns no chunks, the cache failure is almost certainly a side-effect of the client disconnecting, not real backpressure. Surface the context error so GetObjectHandler logs at V(3) and skips writing a response, instead of synthesizing a 503 that nobody will read. Addresses Gemini review feedback on #9233. * simplify comments |
||
|
Chris LuandGitHub
|
503b6f2744 |
fix(seaweed-volume): ceil EC shard slots in maybe_adjust_volume_max (#9232)
Mirrors the volume-server side of seaweedfs/seaweedfs#9196: compute the EC-shard contribution to maxVolumeCount with proper ceiling division ((N + D - 1) / D) instead of (N + D) / D, which over-counts by one slot whenever the per-location EC-shard count is zero or an exact multiple of DataShardsCount (10). The most common case -- a location with no EC shards -- silently inflated maxVolumeCount by 1 on every recalculation. The matching low-disk effective_max_count path in heartbeat.rs already uses the correct ceiling form, and the master-side topology changes from that PR have no Rust counterpart. |
||
 qzhandGitHub
|
21fadf5582 |
fix(shell): correct volume.list -writable filter unit and comparison (#9231)
* fix(shell): correct volume.list -writable filter unit and comparison * fix(shell): correct volume.list -writable filter unit and comparison |
||
|
Chris Lu
|
0b3cc8d121 | 4.22 4.22 | ||
|
Chris Lu
|
6cbcdf488c |
chore(mount,fuse-test): diagnostics for FUSE ConcurrentReadWrite ENOENT flake
PR #9230 attempt 1 hit an intermittent TestConcurrentFileOperations/ConcurrentReadWrite failure where stat returned ENOENT for a path all writers had just succeeded against, and the captured mount.log carried no signal about which layer dropped the entry because the relevant lookup logged at V(4). Two diagnostic-only changes (no behavior change on the happy path): - weed/mount/weedfs.go: in lookupEntry, when filer GetEntry returns ErrNotFound for a path whose inode is still tracked locally with no in-flight create or flush, log Warningf with inode + dirtyHandle + pendingFlush + localCache + dirCached. This surfaces layer-by-layer state at the moment of the suspicious ENOENT. - test/fuse_integration/framework_test.go: on AssertFileExists failure, dump five 100ms-spaced stat retries, a parent ReadDir, and a direct O_RDONLY open before failing. Triangulates kernel dentry caching vs mount lookup vs filer state. |
||
|
Chris LuandGitHub
|
c934b5dab6 |
fix(credential/postgres,s3api/iam): rename safety + pgxutil follow-ups to #9226 (#9230)
* refactor(util): extract pgx OpenDB + DSN builder into shared pgxutil
The postgres filer store had OpenPGXDB plus duplicated key=value DSN
assembly across postgres/ and postgres2/. Move the connection helper to
weed/util/pgxutil and add BuildDSN so the credential postgres store can
land on the same code path.
filer/postgres/pgx_conn.go keeps OpenPGXDB as a thin alias so postgres2
keeps building unchanged.
* refactor(credential/postgres): use shared pgxutil for connection setup
Replace the bespoke fmt.Sprintf DSN + sql.Open("pgx", ...) path with
pgxutil.BuildDSN + pgxutil.OpenDB so the credential store mirrors the
postgres filer store. This also drops the leaky RegisterConnConfig-style
init in favor of stdlib.OpenDB(*config), which doesn't accumulate
entries in the global pgx config map.
Adds parity knobs the filer store already exposes: sslcrl, and
configurable connection_max_idle / connection_max_open /
connection_max_lifetime_seconds (with the previous hardcoded 25/5/5min
as defaults). Also moves the jsonbParam helper here so other store
files can reuse it. (Helper is also referenced by postgres_identity.go,
which is migrated to it in the next commit.)
* refactor(credential/postgres): use jsonbParam helper across all writers
Consolidate JSONB write handling on the new pgxutil-adjacent helper
jsonbParam(b []byte) interface{}, which returns nil (driver writes SQL
NULL) when the marshaled JSON is empty and string(b) otherwise.
postgres_identity.go: replace the inline 'var fooParam any' /
'fooParam = string(b)' pattern with the helper. Same in CreateUser
and UpdateUser.
postgres_inline_policy.go, postgres_policy.go, postgres_service_account.go,
postgres_group.go: every JSONB writer was still passing []byte. Under
pgx simple_protocol (pgbouncer_compatible=true), []byte is encoded as
bytea and Postgres rejects that against a JSONB column with "invalid
input syntax for type json". Route them through jsonbParam too.
* fix(credential/postgres): rework SaveConfiguration to handle rename + UNIQUE access keys
The IAM rename path (s3api UpdateUser) renames an identity in place
and keeps its access keys. With the previous flow — upsert each user,
then per-user delete-and-insert credentials, then prune absent users —
the renamed user's access keys were still owned by the old row when
the INSERT for the new name ran, tripping credentials.access_key's
global UNIQUE constraint and failing every rename of a user with
credentials.
Reorder the SaveConfiguration body so the prune step runs BEFORE the
credential replace. CASCADE on the old user releases its access keys
in the same transaction, and the new name can then claim them.
While here:
- Replace the per-user loop DELETE FROM users WHERE username = $1 with
a single DELETE ... WHERE username = ANY($1), one round trip instead
of N inside the transaction.
- Surface inline-policy CASCADE losses: count user_inline_policies for
the prune set and emit a Warningf when the count is non-zero so
rename-driven drops are visible in operator logs (the structural
fix for renames lives at the IAM layer in a follow-up commit).
- Two-pass credential replace: clear credentials for every user we are
about to rewrite first, then insert, so an access key can be moved
between two users in the same SaveConfiguration call.
- credErr := credRows.Err() before credRows.Close() in
LoadConfiguration — Err() is documented as safe after Close, but
the leading-capture pattern matches the rest of the file.
* fix(s3api/iam): preserve inline policies when renaming a user
EmbeddedIamApi.UpdateUser renames an identity in place and the caller
persists via SaveConfiguration, which prunes the old username and
CASCADE-drops its rows from user_inline_policies. GetUserPolicy and
ListUserPolicies then return nothing under the new name even though
the API reported success — silent data loss.
Before flipping sourceIdent.Name, list the user's stored inline
policies and re-attach each one under the new name. The subsequent
SaveConfiguration prune still CASCADE-removes the old-name rows; only
the duplicates we just wrote under the new name survive. Adds a
regression test that puts a policy on the old name, renames, and
asserts the policy is readable under the new name.
* perf(credential/postgres): batch the credential clear in SaveConfiguration
The two-pass credential replace was clearing each incoming user's
credentials with its own DELETE statement — N round-trips inside the
transaction. Match the pattern already used for the user prune and
issue a single DELETE FROM credentials WHERE username = ANY($1)
instead.
* refactor(s3api/iam): plumb context through UpdateUser
UpdateUser was synthesizing a fresh context.Background() inside the
inline-policy migration block, which discards the request deadline,
cancellation, and tracing carried by the caller. Add ctx as the first
parameter and pass r.Context() in via the ExecuteAction dispatcher,
mirroring the signature already used by CreatePolicy /
AttachUserPolicy / DetachUserPolicy.
* fix(util/pgxutil): quote DSN values per libpq rules
BuildDSN was concatenating values directly, so any password / cert path
/ database name with a space, single quote, or backslash produced a
malformed connection string and pgx.ParseConfig either errored or
mis-parsed the remainder. Critical now that the helper is shared with
the credential store: mTLS deployments routinely sourcing passwords or
secret-mounted cert paths from a vault are exactly the case where
spaces and quotes show up.
Add quoteDSNValue: empty values and values containing whitespace, `'`,
or `\` are wrapped in single quotes with `'` and `\` escaped per
PostgreSQL libpq rules; plain alphanumeric values pass through
unchanged. Apply it to every variable field in BuildDSN.
Adds a test that round-trips a password containing spaces, quotes and
backslashes through pgx.ParseConfig and confirms the parsed Config
matches the input.
* fix(credential,s3api/iam): atomic UserRenamer to avoid FK violation on rename
The previous IAM rename path called PutUserInlinePolicy(newName, ...)
before SaveConfiguration created the new users row. user_inline_policies
has a non-deferrable FOREIGN KEY (username) REFERENCES users(username),
which Postgres validates at statement time, so every rename of a user
that owned at least one inline policy failed with an FK violation. The
existing memory-store regression test missed it because the memory
backend has no FK enforcement.
Add an optional credential.UserRenamer interface plus a
CredentialManager.RenameUser thin shim that returns (supported, err).
Implement it on PostgresStore as an atomic in-transaction migration:
INSERT the new users row by SELECT-copying from the old, UPDATE
credentials.username and user_inline_policies.username to the new
name (FK satisfied because both rows now exist), then DELETE the old
row. ErrUserNotFound / ErrUserAlreadyExists are surfaced cleanly.
Implement it on MemoryStore by re-binding store.users / store.accessKeys
/ store.inlinePolicies under the new name. Also fixes a small leak in
DeleteUser, which was forgetting to drop the user's inline-policy
bucket.
EmbeddedIamApi.UpdateUser now calls RenameUser first; if the store
implements the interface, that's the whole migration. If it doesn't
(stores without FK enforcement), fall back to the previous
list / get / put copy.
Adds a focused test for MemoryStore.RenameUser that asserts the
identity, the access-key index, and the inline policies all land
under the new name.
|
||
|
Chris LuandGitHub
|
4f628ff4e5 |
fix(s3api): stream multipart-SSE chunks lazily to avoid truncated GETs (#8908) (#9228)
* fix(s3api): stream multipart SSE-S3 chunks lazily to avoid truncated GETs (#8908) buildMultipartSSES3Reader opened a volume-server HTTP response for EVERY chunk upfront, then walked them with io.MultiReader. For a multipart SSE-S3 object with N internal chunks (e.g. a 200MB Docker Registry blob with 25+ chunks), N volume-server bodies sat live at once; chunks 1..N-1 were idle while io.MultiReader drained chunk 0. Under concurrent load the volume server's keep-alive logic closed those idle responses mid-flight, and the S3 client saw `unexpected EOF` partway through the GET. Truncated bytes hash to the wrong SHA-256, which is exactly the "Digest did not match" symptom Docker Registry reports in #8908 (and which persisted even after the per-chunk metadata fix in #9211 and the completion backfill in #9224). Introduce lazyMultipartChunkReader + preparedMultipartChunk{chunk, wrap}: a generic lazy chunk streamer with a per-chunk wrap closure for the SSE-specific decryption setup. Per-chunk metadata is still validated UPFRONT so a malformed chunk fails fast without opening any HTTP connection -- the eager validation contract callers and tests rely on is preserved. The volume-server GET and the SSE-specific decrypt wrap, however, fire LAZILY: at most one chunk body is live at any time, regardless of object size. This commit applies the new pattern to buildMultipartSSES3Reader only; the SSE-KMS and SSE-C multipart readers retain their eager form for now and will be migrated in follow-up commits, since the same shape exists there too. Tests: - TestBuildMultipartSSES3Reader_LazyChunkFetch pins the new contract: zero chunks opened at construction, peak liveness == 1, all closed after drain. - TestBuildMultipartSSES3Reader_RejectsBadChunkBeforeAnyFetch (replaces ClosesAppendedOnError) asserts a malformed chunk in position N causes zero fetches for chunks 0..N -- the previous test pinned a weaker contract (cleanup after eager open). - TestBuildMultipartSSES3Reader_InvalidIVLength updated for the same reason: the fetch callback must NOT be invoked at all on a bad-IV chunk. - TestMultipartSSES3RealisticEndToEnd round-trips multiple parts encrypted the way putToFiler writes them (shared DEK + baseIV, partOffset=0, post-completion global offsets) and walks them through buildMultipartSSES3Reader. * fix(s3api): stream multipart SSE-KMS chunks lazily Apply the same fix as the previous commit to createMultipartSSEKMSDecryptedReaderDirect: per-chunk SSE-KMS metadata is validated upfront, but volume-server GETs fire lazily through lazyMultipartChunkReader. At most one chunk body is live at any time. This is the same eager-open-all-chunks shape that produced #8908's truncated GETs for SSE-S3; SSE-KMS multipart objects with many chunks were exposed to the same idle-keepalive failure mode under concurrent load. The wire format on disk is unchanged (same per-chunk metadata, same encrypted bytes, same object Extended attributes). Existing SSE-KMS multipart objects read back identically -- only when the volume-server GETs fire changes. * fix(s3api): stream multipart SSE-C chunks lazily Apply the same fix as the previous two commits to createMultipartSSECDecryptedReaderDirect: per-chunk SSE-C metadata is validated upfront (IV decode, IV length check, non-negative PartOffset), but the volume-server GET and CreateSSECDecryptedReader- WithOffset wrap fire lazily through lazyMultipartChunkReader. At most one chunk body is live at any time. This is the same eager-open-all-chunks shape that produced #8908's truncated GETs for SSE-S3; SSE-C multipart objects with many chunks were exposed to the same idle-keepalive failure mode under concurrent load. The pre-existing TODO note about CopyObject SSE-C PartOffset handling is preserved verbatim. The wire format on disk is unchanged (same per-chunk metadata, same encrypted bytes); existing SSE-C multipart objects read back identically. After this commit all three multipart SSE read paths (SSE-S3, SSE-KMS, SSE-C) share lazyMultipartChunkReader as their streaming engine. * test(s3): add Docker Registry-shape multipart SSE-S3 GET regression Pin the end-to-end fix for #8908 with a test that mirrors what Docker Registry actually does on pull: a 25-part * 5MB upload with bucket- default SSE-S3, then a full GET, then SHA-256 over the streamed body must match SHA-256 over the uploaded bytes. The eager-multipart-reader bug was specifically a streaming truncation under load: the response status was 200 with a Content-Length matching the object size, but the body short-circuited mid-stream because later chunks' volume-server connections had already been closed by keepalive. The hash check is the symptom Docker Registry surfaces ("Digest did not match"), so this is the most faithful regression we can pin without spinning up a registry. uploadAndVerifyMultipartSSEObject already byte-compares the GET body, but hashing on top is intentionally explicit -- it documents WHY the test exists, and matches the failure mode reported in the issue. * test(s3): add range-read coverage matrix across SSE modes and sizes Existing range-read coverage in test/s3/sse was scoped to small (<= 1MB) single-chunk objects, with one ad-hoc range case per SSE mode and one 129-byte boundary-crossing case in TestSSEMultipartUploadIntegration. Nothing exercised: - Range reads on single-PUT objects whose content crosses the 8MB internal chunk boundary (medium size class). - Range reads on multipart objects whose parts each span multiple internal chunks (large size class) -- the shape #8908 originally surfaced for full-object GETs and the most likely site of any future regression in per-chunk IV / PartOffset plumbing for partial reads. - A consistent range-pattern set applied uniformly across SSE modes, so any divergence between modes (SSE-C uses random IV + PartOffset; SSE-S3/KMS use base IV + offset) is comparable at a glance. TestSSERangeReadCoverageMatrix introduces a parameterized matrix: modes: no_sse, sse_c, sse_kms, sse_s3 sizes: small (256KB single chunk), medium (12MB single PUT crossing one internal boundary), large (5x9MB multipart, ~10 internal chunks, every part itself spans an 8MB boundary) ranges: single byte at 0, prefix 512B, single byte at last, suffix bytes=-100, open-ended bytes=N-, whole object, AES-block boundary 15-31, mid straddling one internal boundary (medium+large), mid spanning many internal boundaries (large only) Per case it asserts: body bytes equal the expected slice, Content-Length matches the range length, Content-Range matches start-end/total, and the SSE response headers match the mode. The sse_kms branch probes once with a 1-byte SSE-KMS PUT and t.Skip's the remaining sse_kms subtests with a clear reason if the local server has no KMS provider configured -- the default `weed mini` setup lacks one; the Makefile target `test-with-kms` provides one via OpenBao. Other modes always run. Verified locally: 75 subtests pass under no_sse / sse_c / sse_s3 against weed mini, sse_kms cleanly skipped. * test(s3): conform new test names to TestSSE*Integration so CI runs them The two tests added in the previous commits had names that did NOT match the patterns the test/s3/sse Makefile and .github/workflows/s3-sse-tests.yml use to discover SSE integration tests: - test/s3/sse/Makefile `test` target: TestSSE.*Integration - test/s3/sse/Makefile `test-multipart`: TestSSEMultipartUploadIntegration - .github/workflows/s3-sse-tests.yml: ...|.*Multipart.*Integration|.*RangeRequestsServerBehavior Result: SSE-KMS coverage I added to TestSSERangeReadCoverageMatrix and the Docker-Registry-shape multipart regression in TestSSES3MultipartManyChunks_DockerRegistryShape were silently invisible to CI even though the underlying test setup (start-seaweedfs-ci using s3-config-template.json with the embedded `local` KMS provider) already has SSE-KMS configured. Renames: TestSSERangeReadCoverageMatrix -> TestSSERangeReadIntegration TestSSES3MultipartManyChunks_... -> TestSSEMultipartManyChunksIntegration Both names now match `TestSSE.*Integration` (Makefile `test` target) and TestSSEMultipartManyChunksIntegration additionally matches `.*Multipart.*Integration` (CI's comprehensive subset). No behavior change; only the function names move. Verified locally against `weed mini` with s3-config-template.json: TestSSERangeReadIntegration runs 96 leaf subtests across 4 SSE modes (none, SSE-C, SSE-KMS, SSE-S3) x 3 size classes x 7-9 range patterns, all passing, 0 skipped. The probe-and-skip in the SSE-KMS arm now only fires for ad-hoc local setups that don't load any KMS provider; the project's standard test setup loads the local provider, so CI has full SSE-KMS range coverage. * fix(s3api): validate SSE-KMS chunk IV during prep, before any fetch Addresses CodeRabbit review on PR #9228: in createMultipartSSEKMSDecryptedReaderDirect the per-chunk SSE-KMS metadata was deserialized in the prep loop but the IV length was only validated later, inside CreateSSEKMSDecryptedReader, which runs from the wrap closure -- AFTER the chunk's volume-server fetch has already started. That weakens the new "reject malformed chunks before any fetch" contract for SSE-KMS specifically: a chunk with a missing/short/long IV would fire its HTTP GET, then fail mid-stream during decrypt. The fix moves the existing ValidateIV check into the prep loop, matching the SSE-S3 and SSE-C paths. Drive-by: extract the SSE-KMS prep loop into a free buildMultipartSSEKMSReader helper that mirrors buildMultipartSSES3Reader, so the new contract is unit-testable without an S3ApiServer. The exported method (createMultipartSSEKMSDecryptedReaderDirect) stays a thin caller, so behavior for production callers is unchanged. New tests in weed/s3api/s3api_multipart_ssekms_test.go pin the contract: - TestBuildMultipartSSEKMSReader_RejectsBadIVBeforeAnyFetch covers missing IV, empty IV, short IV, long IV. Each case asserts both that an error is returned AND that the fetch callback is never invoked. - TestBuildMultipartSSEKMSReader_RejectsMissingMetadataBeforeAnyFetch pins the analogous behavior when SseMetadata is nil on a chunk in position N: chunks 0..N-1 must not be fetched (the earlier eager implementation depended on a closeAppendedReaders cleanup path; the new contract is stronger -- nothing is opened in the first place). - TestBuildMultipartSSEKMSReader_RejectsUnparseableMetadataBeforeAnyFetch covers the JSON-unmarshal failure branch. - TestBuildMultipartSSEKMSReader_SortsByOffset smoke-tests the documented sort-by-offset contract by recording the order in which fetch is invoked. All four pass under `go test ./weed/s3api/`. Existing weed/s3api unit suite + the SSE integration suite (with the local KMS provider enabled via s3-config-template.json) continue to pass. * test(s3): address CodeRabbit nitpicks on range coverage matrix Three small follow-ups on the range-read coverage matrix from the previous commit, per CodeRabbit nitpicks on PR #9228: 1. Promote the body-length check from `assert.Equal` to `require.Equal` so a truncation regression -- the canonical #8908 failure mode -- aborts the subtest immediately. Previously the assertion logged a length mismatch and then `assertDataEqual` ran on differently-sized slices, producing a noisy byte-diff on top of the actual symptom. The redundant trailing `t.Fatalf` block becomes dead and is removed. 2. Broaden the SSE-KMS probe-skip heuristic. The probe previously produced the friendly "KMS provider not configured" message only for 5xx responses; KMS-misconfig surfaces also include 501 NotImplemented, 4xx KMS.NotConfigured, and error messages containing "KMS.NotConfigured" / "NotImplemented" / "not configured". The behaviour change is purely cosmetic (the caller t.Skip's on any non-empty reason either way) but the new diagnostic is more useful in CI logs. 3. Add `t.Parallel()` at the mode and size-class levels of the matrix. Each (mode, size) writes an independent object key under the shared bucket, with no cross-talk, so parallel execution is safe. Local wall time on the full matrix dropped from ~2.0s to ~1.1s (~45%); the savings scale with chunk count and CI machine concurrency. Verified locally against `weed mini` with s3-config-template.json: - go test ./weed/s3api/ -count=1 PASS - TestSSERangeReadIntegration -v 112 PASS, 0 SKIP - TestSSEMultipartUploadIntegration etc. PASS * fix(s3api): tighten lazy reader error path; unify SSE IV validation Three CodeRabbit nitpicks on PR #9228: 1. lazyMultipartChunkReader: mark finished on non-EOF Read errors The Read loop's three earlier failure paths (chunk index past end, fetch error, wrap error) all set l.finished = true before returning. The non-EOF Read path -- where l.current.Read itself errors mid-chunk -- did not, leaving l.current/l.closer set and l.finished = false. A caller that retried Read after an error would re-enter the same broken stream instead of advancing or giving up. Set l.finished = true on non-EOF Read error so post-error state is consistent across all four failure sites; Close() (which the GetObjectHandler defers) still releases the chunk body. 2. Unify IV-length validation across SSE-S3, SSE-KMS, SSE-C prep paths The previous commit moved SSE-KMS to the shared ValidateIV helper but left SSE-S3 and SSE-C with bespoke inline `len(...) != AESBlockSize` checks. All three are enforcing the same invariant; inconsistency obscures the symmetry. Move SSE-S3 and SSE-C to ValidateIV too, with the same `<algo> chunk <fileId> IV` name convention. Error message wording shifts from "<algo> chunk X has invalid IV length N (expected 16)" to ValidateIV's "invalid <algo> chunk X IV length: expected 16 bytes, got N". The substring "IV length" is preserved across both, so the existing TestBuildMultipartSSES3Reader_InvalidIVLength substring assertion is loosened to match either form. 3. TestBuildMultipartSSEKMSReader_SortsByOffset: verify full ordering The test previously drove Read() to observe fetch-call order, but CreateSSEKMSDecryptedReader requires a live KMS provider to unwrap the encrypted DEK -- unavailable in unit tests -- so the wrap closure failed on the first chunk and only one fetch was ever recorded. The test asserted only fetchOrder[0] == "c0", which is weaker than the comment promised. Switch to a static check: type-assert the returned reader to *lazyMultipartChunkReader (same package so unexported fields are accessible) and inspect the prepared chunks slice directly. This pins the entire [c0, c1, c2] sort order in one place, doesn't depend on KMS, and runs in zero fetch calls. The fetch closure now asserts it is never invoked during preparation. All weed/s3api unit tests pass; integration suite (with KMS provider configured via s3-config-template.json) passes. * test(s3): switch range coverage cleanup to t.Cleanup; tighten KMS probe Two CodeRabbit comments on PR #9228, both about test/s3/sse/s3_sse_range_coverage_test.go: 1. CRITICAL: defer + t.Parallel() race in TestSSERangeReadIntegration The test creates one bucket up front, then runs subtests that call t.Parallel() at the mode and size levels (added in |
||
 Jon E NesvoldandGitHub
|
dc462a80d7 |
feat(credential/postgres): inline policies, mTLS and pgbouncer connection support (#9226)
* feat(credential/postgres): mTLS + pgbouncer support, InlinePolicyStore implementation, upsert SaveConfiguration * fix(credential/postgres): add rows.Err() checks, inline policy tests, memory store LoadInlinePolicies * fix(credential/postgres): cast JSONB params to string for pgbouncer simple protocol * fix(credential/postgres): wrap tx.Commit errors with context * fix(credential/postgres): use any type for JSONB params to preserve SQL NULL for nil fields |
||
 Parviz MiriyevandGitHub
|
f407bdaa36 |
fix(admin): use TLS-aware HTTP client for /dir/status fetch (#9227)
fetchPublicUrlMap() in weed/admin/dash/cluster_topology.go uses a
dedicated &http.Client{} that doesn't honor security.toml client TLS
configuration, and hardcodes "http://" in the URL. When master is
configured HTTPS-only ([https.master] set), every cluster topology
cache refresh logs:
NOTICE: http: TLS handshake error from <admin-ip>:<port>: client
sent an HTTP request to an HTTPS server
The function falls through to glog.V(1).Infof and returns nil, so the
admin UI loses PublicUrl enrichment for data nodes. Cosmetic but noisy.
Switch to util_http.GetGlobalHttpClient() whose Do() calls
NormalizeHttpScheme(), which automatically rewrites http:// to https://
when [https.client] is enabled and presents the configured client cert.
Preserve the 5-second timeout via context.WithTimeout().
Same pattern as weed/admin/handlers/file_browser_handlers.go,
weed/server/master_server.go, weed/shell/command_volume_fsck.go.
|
||
|
Chris LuandGitHub
|
0716577ec8 |
fix(upload): rewind request body when retrying on connection reset (#9139) (#9222)
* fix(upload): rewind request body when retrying on connection reset (#9139) When httpClient.Do() returned "connection reset by peer" or "use of closed network connection", upload_content retried with the same *http.Request. But the body is a *bytes.Reader the first attempt already consumed, so the retry sent 0 bytes and Go's transport surfaced "http: ContentLength=N with Body length 0". http.NewRequestWithContext populates req.GetBody for *bytes.Reader bodies; use it to attach a fresh body before retrying. Reproduces the issue with a unit test (asserts both attempts see the same payload bytes); the test fails without the fix. * upload: skip inner retry when body cannot be rewound Per review feedback: if req.GetBody is nil or returns an error, the inner retry would call Do(req) with an already-consumed body and the "connection reset" error would be replaced by the misleading "ContentLength=N with Body length 0" — the very symptom this PR set out to fix. Skip the inner retry on rewind failure and let the outer retriedUploadData loop reissue with a fresh request, and log when GetBody is unavailable for observability. * upload: log the actual transport error in the inner retry log line Per review feedback: the diagnostic glog at the top of the inner retry branch was logging postErr — the request-construction error from http.NewRequestWithContext, which is necessarily nil there because the function returns early at line 423 if it isn't. Operators were seeing "<nil>" instead of the transient transport error that triggered the rewind. Reference post_err so the connection-reset / closed-connection cause is actually visible. |
||
|
Chris LuandGitHub
|
654292b57d |
fix(volume): cap leveldb OpenFilesCacheCapacity per index DB (#9139) (#9223)
* fix(volume): cap leveldb OpenFilesCacheCapacity per index DB (#9139) The leveldb opt.Options for NeedleMapLevelDb / Medium / Large never set OpenFilesCacheCapacity, so each leveldb instance defaulted to goleveldb's 500. On servers with thousands of volumes, that ceiling stacks across DBs and exhausts even high ulimits, starving WAL rotation: failed to write leveldb: open .../000006.log: too many open files CompactionTableSizeMultiplier=10 already keeps the SST count low, so a small per-DB cache is sufficient. Cap at 16 / 32 / 64 for the small / medium / large variants so per-DB FD usage is bounded. * storage: hoist leveldb FD-cap values into named constants Per review feedback: replace the inline 16/32/64 literals with LevelDb{,Medium,Large}OpenFilesCacheCapacity, and move the rationale (why 500 is too high per-DB on busy servers, what the tradeoff is) into a package-level comment so future readers see the memory vs. performance picture at the constant declaration instead of inline. |
||
|
Chris LuandGitHub
|
525900dfe4 |
fix(s3api): backfill multipart SSE-S3 metadata at completion (#9224)
* fix(s3api): backfill missing per-chunk SSE-S3 metadata at completion When a part of an SSE-S3 multipart upload lands with SseType=NONE on its chunks (e.g. a transient failure to apply SSE-S3 setup in PutObjectPart), the completed object inherits NONE-tagged chunks and detectPrimarySSEType then misses the chunked SSE-S3 encryption. The read path falls through to the unencrypted serve and GET returns ciphertext, producing the SHA mismatch reported in #8908. Recover at completion using the base IV and key data the upload directory recorded at CreateMultipartUpload: - extractMultipartSSES3Info validates upload-entry metadata up front and hard-fails completion if the base IV or key data are malformed; serializing chunk metadata we then could not decrypt is worse than rejecting the upload. - completedMultipartChunk re-derives a per-chunk IV from baseIV + chunk.Offset (matching what putToFiler would have written) and serializes per-chunk SSE-S3 metadata when the chunk has no tag. Existing per-chunk metadata is left alone; we cannot recover an already-derived IV from the upload-entry alone. The IV formula intentionally has no partNumber term: putToFiler hardcodes partOffset=0 when it calls handleSSES3MultipartEncryption for every part, so each chunk's encryption IV is calculateIVWithOffset(baseIV, chunk.Offset_part_local). PartOffsetMultiplier is defined in s3_constants but is not consumed by the encryption path. Adopting (partNumber-1)*PartOffsetMultiplier + chunk.Offset would produce IVs that fail to decrypt the bytes on disk - a stronger failure mode than the bug being fixed. Tests pin this: - TestCompletedMultipartChunkBackfilledIVDecryptsActualCiphertext runs the round trip across the encryption boundary: encrypt parts with CreateSSES3EncryptedReaderWithBaseIV (the call putToFiler uses), drop chunk metadata to reproduce #8908, backfill, decrypt with backfilled IV, assert plaintext intact. - TestCompletedMultipartChunkRejectsPartNumberMultiplierFormula constructs the IV the partNumber formula would produce and shows it does not decrypt the actual ciphertext. This commit covers the chunk-level recovery only. The companion fix for the object-level Extended attributes (SeaweedFSSSES3Key / X-Amz-Server-Side-Encryption) follows separately. * fix(s3api): backfill canonical SSE-S3 attributes onto multipart object The previous commit ensures every chunk of an SSE-S3 multipart upload carries SseType=SSE_S3 with a per-chunk IV, so the multipart-direct read path can decrypt. The completed object's Extended map can still miss the canonical pair detectPrimarySSEType and IsSSES3EncryptedInternal look at: - X-Amz-Server-Side-Encryption (the AmzServerSideEncryption header detectPrimarySSEType reads on inline / small-object reads) - x-seaweedfs-sse-s3-key (SeaweedFSSSES3Key, required by IsSSES3EncryptedInternal and by the read-path key lookup) When a part of the upload was written by a path that did not set those (the same #8908 race that produced the NONE chunks), copySSEHeadersFromFirstPart finds nothing to copy and the final entry ends up with only the multipart-init keys (SeaweedFSSSES3Encryption / BaseIV / KeyData). The read path then mis-detects the object as unencrypted. applyMultipartSSES3HeadersFromUploadEntry writes the canonical pair from the multipart-init metadata in all three completion paths (versioned, suspended, non-versioned), only when the keys are missing so a healthy first part still wins. extractMultipartSSES3Info already ran in prepareMultipartCompletionState, so the data is reused without re-decoding. Tests: TestApplyMultipartSSES3HeadersFromUploadEntry covers backfill, do-not-clobber, and nil-info no-op cases. * fix(s3api): drop double IV adjustment in SSE-KMS chunk view decrypt decryptSSEKMSChunkView was pre-adjusting the SSE-KMS chunk IV (calculateIVWithOffset(baseIV, ChunkOffset)) and then handing the adjusted IV to CreateSSEKMSDecryptedReader, which itself runs calculateIVWithOffset(IV, ChunkOffset) on whatever it receives. The offset was being applied twice for any chunk with a non-zero ChunkOffset, corrupting the keystream for range reads that cross multipart chunk boundaries. Pass the raw SSE-KMS key (with base IV and the original ChunkOffset field) into CreateSSEKMSDecryptedReader so the offset is applied exactly once, and remove the now-dead intra-block skip that was compensating for the double adjustment. Add an anti-test inside TestSSEKMSDecryptChunkView_RequiresOffsetAdjustment that decrypts the same ciphertext with a deliberately double-adjusted IV and asserts the output is corrupted, so any regression that re-introduces the double application fails the unit test. * test(s3): cover multipart SSE across chunk-spanning parts and ranges Adds an integration subtest "Multipart Parts Larger Than Internal Chunks Across SSE Types" to TestSSEMultipartUploadIntegration that exercises the end-to-end S3 path for the bugs fixed in this branch: - Two-part multipart upload with each part larger than the 8MB internal SeaweedFS chunk, so each part itself spans multiple underlying chunks. - Subtests for SSE-C, SSE-KMS, explicit SSE-S3, and bucket-default SSE-S3 - the four paths multipart parts can take through the SSE pipeline. - Each subtest does a full GET (verifying every byte and the response Content-Length / SSE response headers) plus a 129-byte range read straddling the 8MB internal chunk boundary, which is the path that produced the SSE-KMS double-IV corruption (fix in the previous commit) and the SSE-S3 chunk-tag loss (fix in the earlier commits). Factored the request shape behind multipartSSEOptions / uploadAndVerifyMultipartSSEObject so all four SSE flavors share the same upload+verify code; only the SSE-specific input/output configuration differs per subtest. * test(s3): abort orphan multipart uploads on test failure Address coderabbit nitpick on uploadAndVerifyMultipartSSEObject. The helper used require.NoError after CreateMultipartUpload, UploadPart and CompleteMultipartUpload, so a failure in any of those (or in the later GET / range read on a still-incomplete upload) called t.Fatal without aborting the in-flight MPU, leaving an orphan upload in the bucket. Harmless in CI where the data dir is wiped on shutdown, but a real annoyance when iterating locally and a textbook AWS S3 caveat in production. Register a t.Cleanup that calls AbortMultipartUpload unless a "completed" flag was set right after a successful CompleteMultipartUpload. Use context.Background for the abort call since the parent ctx may already be cancelled at cleanup time, and t.Logf the abort error rather than failing the test so the original failure remains visible in the run output. |
||
|
Chris LuandGitHub
|
5eead9409a |
fix(admin): S3 Tables CSRF token + non-empty 409 status (#9221)
* fix(admin): attach CSRF token to S3 Tables write requests Several POST/PUT/DELETE calls in s3tables.js were sent without an X-CSRF-Token header while the corresponding handlers in weed/admin/dash/s3tables_management.go enforce CSRF via requireSessionCSRFToken, so authenticated users hit "invalid CSRF token" on actions like creating a table bucket (#9220), updating policies, and managing tags. Add an s3tWriteHeaders helper that pulls the token from the existing csrf-token meta tag and use it on every write to /api/s3tables/buckets, /bucket-policy, /tables, /table-policy, and /tags. The Iceberg-page write paths already attached the token and are unchanged. Fixes #9220 * fix(admin): map BucketNotEmpty/NamespaceNotEmpty to 409 for S3 Tables DELETE on a non-empty table bucket or namespace returned HTTP 500 because s3TablesErrorStatus didn't list ErrCodeBucketNotEmpty or ErrCodeNamespaceNotEmpty in its conflict case, even though the backend handler emits them with 409 Conflict (matching AWS S3 Tables). Add both codes to the existing conflict mapping. * refactor(admin): route Iceberg S3 Tables writes through s3tWriteHeaders Iceberg namespace/table create and Iceberg table delete were still hand-rolling CSRF headers. Replace those blocks with the existing s3tWriteHeaders() helper so every S3 Tables write uses the same code path. Drop the now-unused csrfTokenInput.value population in initIcebergNamespaces and initIcebergTables (the templ hidden inputs have no server-rendered value, and nothing reads the input now that the JS reads the token from the meta tag via getCSRFToken()). |
||
|
Chris Lu
|
a14cbc176b | debug(kafka): add restart flake diagnostics | ||
|
Chris LuandGitHub
|
f1f720f5da |
fix(master): register EC shards per physical disk on full heartbeat sync (#9212) (#9219)
* refactor(types): add DiskId type for physical-disk identifiers Names the uint32 physical-disk index that volume servers carry in VolumeEcShardInformationMessage / VolumeInformationMessage, so EC shard tracking that needs to distinguish disks within a DataNode can use a dedicated type instead of an untyped uint32. No behaviour change. * fix(master): register EC shards per physical disk on full heartbeat sync (#9212) When a volume's EC shards are spread across multiple physical disks on the same volume server (common after ec.balance / ec.rebuild on multi-disk nodes), the volume server emits one VolumeEcShardInformationMessage per (disk, volume) in its heartbeat. The master's DataNode.UpdateEcShards was building a `map[VolumeId]*EcVolumeInfo` with last-write-wins, and doUpdateEcShards then overwrote `disk.ecShards[vid]` once per message, so all but the final disk's shards were silently dropped. Only the topology-global ecShardMap (built via RegisterEcShards in a per-message loop) stayed correct, which hid the problem from `topo.LookupEcShards` but broke everything that reads the DataNode/Disk view — volume.list, admin UI, ec.rebuild dry-run ("only 6 shards, skipping"), and `DiskInfo.EcShardInfos` which the shell's ec.balance / ec.rebuild planners group by `eci.DiskId`. Change the shape of `Disk.ecShards` from map[VolumeId]*EcVolumeInfo to map[VolumeId]map[types.DiskId]*EcVolumeInfo so every physical disk keeps its own entry. UpdateEcShards aggregates incoming messages by (vid, diskId) rather than vid alone; Add/Delete/ HasVolumesById and HasEcShards consult the nested map; doUpdateEcShards rewrites the nested structure from the aggregated map. Per-physical-disk attribution survives through DataNode.ToDataNodeInfo -> DiskInfo.EcShardInfos, matching the wire format the volume server produces and what downstream admin tooling expects. Delta sync (AddOrUpdateEcShard / DeleteEcShard) already merged via ShardsInfo.Add, so this only affects the full-sync path that runs on heartbeat reconnect. Adds data_node_ec_multi_disk_test.go with two regression tests that fail on pre-fix master: - TestEcShardsAcrossMultipleDisksOnSameNode: volume 15 spread over 3 disks (matches the bug report's volume-2 row); asserts every shard visible via LookupEcShards, DataNode.GetEcShards, and ToDataNodeInfo's per-disk EcShardInfos entries. - TestEcShardsAfterRestartHeartbeat: minimal 2-disk full sync case. * fix(topology): tighten locking around EC shard map access Addresses review comments on #9219: * DataNode.UpdateEcShards now holds dn.Lock for the full read-diff-write cycle, matching UpdateVolumes' model, so concurrent heartbeats can no longer interleave their getOrCreateDisk / UpAdjustDiskUsageDelta updates with each other. Introduces a private getEcShardsLocked helper for reads under the held lock; renames doUpdateEcShards to doUpdateEcShardsLocked for the same reason. * DataNode.HasEcShards now takes each disk's ecShardsLock while reading disk.ecShards, closing a pre-existing map race with concurrent Add/Delete/Update writers. * doUpdateEcShardsLocked takes each disk's ecShardsLock around the reset-and-rewrite so readers (GetEcShards, HasEcShards) see a consistent map state rather than a partially-rebuilt one. * Disk.GetEcShards' slice-capacity hint now accounts for the nested per-physical-disk entries (sum of inner lengths) instead of underestimating by the unique-volume count. |
||
|
Chris LuandGitHub
|
d65c568cbb |
fix(s3api): validate SSE-S3 chunk IV length; add multipart direct reader tests (#9218)
* fix(s3api): validate SSE-S3 chunk IV length; add multipart direct reader tests DeserializeSSES3Metadata does not require an IV, and a corrupted or legacy chunk without one would have flowed into cipher.NewCTR and panicked. Validate that each per-chunk IV is exactly AESBlockSize bytes before decryption, closing the current and any already-appended chunk readers on error. Factor the per-chunk decryption loop out of createMultipartSSES3DecryptedReaderDirect into buildMultipartSSES3Reader so it can be driven with a mock chunk fetcher, and add tests covering: the happy path with two parts (distinct per-chunk DEKs/IVs, out-of-order chunks) to lock in the fix from #9211; missing-IV and short-IV metadata rejection without panic; and reader cleanup when a later chunk fails. * address review: sort chunks copy; close encryptedStream on error - buildMultipartSSES3Reader now sorts a copy of the chunks slice so callers do not observe entry.Chunks reordered (other code paths, e.g. ETag computation, can rely on the original order). - createMultipartSSES3DecryptedReaderDirect now closes encryptedStream on the error path from buildMultipartSSES3Reader. All current callers pass nil, but this keeps cleanup symmetric with the success path. - Extend TestBuildMultipartSSES3Reader_PerChunkKeys to assert the input slice is not mutated. * address review: defer single close; extend chunk-copy + IV-guard pattern - createMultipartSSES3DecryptedReaderDirect: collapse the duplicated encryptedStream.Close() calls into a single nil-guarded defer so the error and success paths share cleanup. - createMultipartSSECDecryptedReaderDirect, createMultipartSSEKMSDecryptedReaderDirect: sort a copy of entry.Chunks instead of mutating the caller's slice, matching the SSE-S3 helper. - createMultipartSSECDecryptedReaderDirect: validate per-chunk IV length before handing it to cipher.NewCTR; a base64-decoded empty or short IV from malformed/corrupt metadata would otherwise panic. - SSE-KMS needs no IV guard: CreateSSEKMSDecryptedReader already calls ValidateIV before cipher.NewCTR. Note recorded in the sort comment. * address review: close appended readers on SSE-C/SSE-KMS error paths createMultipartSSECDecryptedReaderDirect and createMultipartSSEKMSDecryptedReaderDirect only closed the current chunk reader on error and leaked any chunk readers already appended to the local readers slice, mirroring the leak previously fixed in the SSE-S3 helper. Add the same closeAppendedReaders() closure pattern to both functions and invoke it on every error return inside the loop so failed requests do not leak volume-server HTTP connections. * address review: defer encryptedStream close in SSE-C/SSE-KMS; drop chunks reassignment - Move encryptedStream.Close() to a nil-guarded defer at the top of createMultipartSSECDecryptedReaderDirect and createMultipartSSEKMSDecryptedReaderDirect so the stream is closed on every return path (including error returns from inside the per-chunk loop), mirroring the SSE-S3 helper. - In buildMultipartSSES3Reader, iterate sortedChunks directly instead of reassigning chunks = sortedChunks. |
||
|
Chris LuandGitHub
|
fe1d7a404d |
fix(iam): substitute dynamic jwt:/saml:/oidc: claim variables in policies (#9217)
* fix(iam): expand arbitrary jwt:/saml:/oidc: claim variables in policies
The policy engine gated variable substitution on a fixed allowlist
(jwt:sub, jwt:iss, jwt:aud, jwt:preferred_username), so patterns like
arn:aws:s3:::softs/${jwt:project_path}/* were passed through as literals
and never matched the requested resource. Dynamic claims from OIDC
providers (e.g. GitLab CI's project_path / namespace_path) could not be
used to scope policies.
Allow any jwt:/saml:/oidc: prefixed variable to be substituted when the
claim is present in RequestContext. These values originate from a
cryptographically verified identity token (the STS session JWT or
federated assertion), and the claim names are controlled by the trusted
identity provider, so the dynamic prefix is safe. Missing claims keep
the placeholder intact so the statement still fails to match.
Numeric JWT claims (JSON-decoded as float64) are now stringified so
patterns like ${jwt:project_id} work the same as string claims.
Fixes #9214
* fix(iam): cover all integer widths in claim stringification
Address PR review: stringifyClaimValue only handled int/int32/int64 on
the signed side and nothing on the unsigned side, so int8, int16, uint,
uint8, uint16, uint32, and uint64 claim values fell through to the
default branch and the placeholder was left unsubstituted.
JSON's generic decoder produces float64/json.Number for numbers, but
RequestContext can also be populated from typed sources (custom
providers or internal code), so cover all common integer widths -
signed and unsigned - explicitly. Extend TestStringifyClaimValue to
assert each supported type.
|
||
|
8815844278 |
fix(s3api): correct SSE-S3 decryption key handling in multipart uploads (#9211)
* fix(s3api): correct SSE-S3 decryption key handling in multipart uploads * fix(s3api): preallocate readers and close on error in SSE-S3 direct path Address review feedback on createMultipartSSES3DecryptedReaderDirect: preallocate the readers slice with the known chunk count, and close any already-appended chunk readers on error returns so failed requests do not leak volume-server HTTP connections. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Lisandro PinandGitHub
|
93247d6de4 |
Export REST file_{read,write}_failures metrics on volume servers (#9215)
* Export gRPC `file_{read,write}_failures` metrics on volume servers.
Allows to track overall R/W errors in real time through Prometheus.
Will follow up with a PR for Seaweed's REST API.
* Export REST `file_{read,write}_failures` metrics on volume servers.
|
||
 |
352ffdffe1 |
build(deps): bump rustls-webpki from 0.103.10 to 0.103.13 in /seaweed-volume (#9216)
build(deps): bump rustls-webpki in /seaweed-volume Bumps [rustls-webpki](https://github.com/rustls/webpki) from 0.103.10 to 0.103.13. - [Release notes](https://github.com/rustls/webpki/releases) - [Commits](https://github.com/rustls/webpki/compare/v/0.103.10...v/0.103.13) --- updated-dependencies: - dependency-name: rustls-webpki dependency-version: 0.103.13 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
|
29e14f89f1 |
fix(weed/command) address unhandled errors (#9208)
* fix(weed/command) address unhandled errors * fix(command): don't log graceful-shutdown sentinels; plug response-body leak - s3: Serve on unix socket treated http.ErrServerClosed as fatal; now excluded like the other Serve/ServeTLS paths in this file. - mq_agent, mq_broker: filter grpc.ErrServerStopped so clean shutdown doesn't log as an error. - worker_runtime: the added decodeErr early-continue skipped resp.Body.Close(); drop it since the existing check below already surfaces the decode error. - mount_std: the pre-mount Unmount commonly fails when nothing is mounted; demote to V(1) Infof. - fuse_std: tidy panic message to match sibling cases. * fix(mq_broker): filter grpc.ErrServerStopped on localhost listener The localhost listener goroutine logged any Serve error unconditionally, which includes grpc.ErrServerStopped on graceful shutdown. Match the main listener's check so clean stops don't surface as errors. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris LuandGitHub
|
88c2f3c34d |
fix(iam): accept bare "*" resource in PutUserPolicy (#9209) (#9210)
AWS IAM treats a bare "*" in a statement's Resource as "any resource",
but the embedded IAM resource parser required a 6-segment S3 ARN and
silently skipped anything else. With a policy like
{Action: "s3:*", Resource: "*"}, every resource was dropped and the
statement produced no actions, so PutUserPolicy rejected the document
with "no valid actions found in policy document".
Short-circuit Resource == "*" to the same full-wildcard path that
"arn:aws:s3:::*" already takes.
|
||
|
Chris LuandGitHub
|
da2e90aefd |
fix(mount): sanitize non-UTF-8 filenames; keep marshal errors per-request (#9207)
* fix(mount): sanitize non-UTF-8 filenames; keep marshal errors per-request (#9139) A single file with invalid-UTF-8 bytes in its name (e.g. a GNOME Trash "partial" like \x10\x98=\\\x8a\x7f.trashinfo.9a51454f.partial) made every FUSE-initiated filer RPC fail with: rpc error: code = Internal desc = grpc: error while marshaling: string field contains invalid UTF-8 and then produced an avalanche of "connection is closing" errors on unrelated LookupEntry / ReadDirAll / UpdateEntry calls, causing the volume-server QPS dips reported in #9139. Root cause is twofold: 1. Proto3 `string` fields require valid UTF-8, but the FUSE kernel passes raw name bytes. Create/Mknod/Mkdir/Unlink/Rmdir/Rename/Lookup/Link/ Symlink all forwarded those bytes directly into CreateEntryRequest.Name, DeleteEntryRequest.Name, StreamRenameEntryRequest.{Old,New}Name and Entry.Name. saveDataAsChunk also copied the FullPath into AssignVolumeRequest.Path unchecked. 2. When the marshal failed, shouldInvalidateConnection treated the resulting codes.Internal as a connection problem and dropped the shared cached ClientConn — canceling every other in-flight RPC on it. Fix: - Add sanitizeFuseName (strings.ToValidUTF8 with '?' replacement, matching util.FullPath.DirAndName) and make checkName return the sanitized name. Apply at every FUSE entry point that passes a name to the filer RPC, including Unlink/Rmdir (which did not previously call checkName) and both oldName/newName in Rename. Add a backstop scrub for AssignVolumeRequest.Path so async flush paths cannot reintroduce invalid bytes from a pre-sanitization cached FullPath. - In weed/pb.shouldInvalidateConnection, detect client-side marshal errors via the gRPC library's "error while marshaling" prefix and return false: the connection is healthy, only the request is bad. Refs: https://github.com/seaweedfs/seaweedfs/issues/9139#issuecomment-4301184231 * fix(mount,util): use '_' for invalid-UTF-8 replacement (URL-safe) Sanitized filenames flow downstream into HTTP URLs (volume-server uploads, filer HTTP API, S3/WebDAV gateways). '?' is the URL query-string delimiter and would split the path the first time the name lands in one, so swap every invalid-UTF-8 replacement to '_'. This covers the two pre-existing sites in weed/util/fullpath.go as well, keeping all paths sanitized the same way. * refactor(pb): detect client-side marshal errors via errors.As, not substring Replace the raw `strings.Contains(err.Error(), ...)` check with a type-based carve-out: use errors.As against the `GRPCStatus() *Status` interface to pull the original Status out of any fmt.Errorf("...: %w") wrapping, then match the library-owned "grpc:" prefix on that Status's Message. Why not errors.Is against a proto-level sentinel: gRPC's encode() collapses the inner proto error with "%v" (stringification) before wrapping it in a Status, so the original error type does not survive into the caller. The Status itself is the structural signal that does survive. Why not status.FromError: when the caller wraps the Status error with fmt.Errorf("...: %w", ...), status.FromError rewrites Status.Message with the full err.Error() of the outermost wrapper, which defeats a prefix check on the library-owned message. errors.As gives us the original Status whose Message is still verbatim from the gRPC library. A new test asserts that a plain errors.New("grpc: error while marshaling: …") — i.e. the same text attached to something that is NOT a gRPC status — does not short-circuit invalidation, so we never silently keep a cached connection alive based on a coincidental substring match. * refactor(util): centralize UTF-8 sanitization; add FullPath.Sanitized Addresses review feedback on PR #9207. Nitpick: every invalid-UTF-8 replacement across the codebase (DirAndName, Name, mount.sanitizeFuseName, the weedfs_write.go backstop) now goes through a single util.SanitizeUTF8Name helper, so the replacement char ('_' — URL-safe) is chosen in one place. Outside-diff: three proto fields took raw FullPath strings that could break marshaling if an entry ever carried invalid UTF-8 (CreateEntryRequest.Directory in Mkdir, DeleteEntryRequest.Directory in Unlink, AssignVolumeRequest.Path in command_fs_merge_volumes). The reviewer's suggested fix — using DirAndName() — would have silently changed Directory from parent to grandparent, because DirAndName sanitizes only the trailing component. Added FullPath.Sanitized(), which scrubs every component, and applied it at the three sites. Exposure is narrow in practice (FUSE-boundary sanitization and the gRPC-side isClientSideMarshalError carve-out already cover the #9139 cascade), but the defense-in-depth is cheap and consistent with the existing AssignVolume backstop. New tests in weed/util/fullpath_test.go document: - SanitizeUTF8Name: valid UTF-8 passes through unchanged; invalid bytes become '_' (not '?', which is URL-special). - FullPath.Sanitized: scrubs bytes in any component, not just the last. - FullPath.DirAndName: dir remains raw on purpose — callers needing a clean full path must use Sanitized(). The test pins this behavior so it is not accidentally "fixed" in a way that changes the (dir, name) semantics callers depend on. |
||
|
Chris Lu
|
a0be40e070 | Merge branch 'master' of https://github.com/seaweedfs/seaweedfs | ||
|
Chris Lu
|
b94ad82472 |
fix(test): stabilize ConcurrentLockContention; warn on coherence drift
TestPosixFileLocking/ConcurrentLockContention failed in CI (run 24857323067) with ENOENT when re-opening the file after all 8 workers had successfully written and closed. The 20s openWithRetry budget was exhausted, pointing at a real but unproven metaCache/parent-cache coherence issue in the mount under bursts of concurrent Release. Test: hold the initial fd open for the whole subtest; use it for the post-workers Sync() and the verification read. Workers still exercise the concurrent-flock invariant and per-record write correctness; the re-open path is no longer load-bearing. On Eventually failure, dump ReadDir of the parent, Stat, and a fresh O_RDONLY open so a future recurrence has state to debug from. Drop the darwin-only ENOENT t.Skip branches that hid this same flake. Mount: in weedfs.lookupEntry, when returning ENOENT from the "parent cached but child missing" branch, log at Warningf instead of V(4) when the kernel is still tracking this path's inode. That combination is the smoking-gun signal for cache drift and is rare enough in normal use not to spam the log. |
||
|
|
cd5004cfbd |
build(deps): bump github.com/Azure/go-ntlmssp from 0.1.0 to 0.1.1 in /test/kafka (#9204)
build(deps): bump github.com/Azure/go-ntlmssp in /test/kafka Bumps [github.com/Azure/go-ntlmssp](https://github.com/Azure/go-ntlmssp) from 0.1.0 to 0.1.1. - [Release notes](https://github.com/Azure/go-ntlmssp/releases) - [Commits](https://github.com/Azure/go-ntlmssp/compare/v0.1.0...v0.1.1) --- updated-dependencies: - dependency-name: github.com/Azure/go-ntlmssp dependency-version: 0.1.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
|
5cbcfd311c |
build(deps): bump github.com/Azure/go-ntlmssp from 0.1.0 to 0.1.1 (#9205)
Bumps [github.com/Azure/go-ntlmssp](https://github.com/Azure/go-ntlmssp) from 0.1.0 to 0.1.1. - [Release notes](https://github.com/Azure/go-ntlmssp/releases) - [Commits](https://github.com/Azure/go-ntlmssp/compare/v0.1.0...v0.1.1) --- updated-dependencies: - dependency-name: github.com/Azure/go-ntlmssp dependency-version: 0.1.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
Chris LuandGitHub
|
76f361fa77 |
fix(helm): gate S3 TLS cert args on httpsPort to stop probe failures (#9202) (#9206)
* fix(helm): gate S3 TLS cert args on httpsPort to stop probe failures (#9202) With `global.seaweedfs.enableSecurity=true` and the default `s3.httpsPort=0`, the chart was unconditionally passing `-cert.file` / `-key.file` to the S3 frontend. In `weed/command/s3.go`, when `tlsPrivateKey != ""` and `portHttps == 0`, the server promotes its main `-port` (8333 by default) into an HTTPS listener. The pod's readiness / liveness probes still use `scheme: HTTP`, so every kubelet probe produces http: TLS handshake error from <node-ip>:<port>: client sent an HTTP request to an HTTPS server in the pod log, as reported in #9202. `enableSecurity=true` is supposed to activate security.toml / gRPC mTLS, not silently flip the S3 HTTP port to HTTPS. Move the `seaweedfs.s3.tlsArgs` include inside the `if httpsPort` guard in all three templates that wire up an S3 frontend (standalone S3 deployment, filer with S3 sub-server, all-in-one deployment). The TLS cert args are now emitted only when the user explicitly opts into an HTTPS port; the main `-port` stays HTTP so probes work. Also add a regression test to `.github/workflows/helm_ci.yml` that renders all three templates with and without `httpsPort` and asserts the cert/key/ `-port.https` args are emitted together or not at all. * test(helm): add bash -n parse check to the S3 TLS-gating regression test Addresses gemini-code-assist review comment on #9206 flagging a potential "dangling backslash" shell-syntax risk in the rendered all-in-one command script when httpsPort is set but most S3/SFTP args are defaulted off. In practice bash -n accepts a trailing `\<newline><EOF>` (it's line-continuation to an empty line), so no current rendering is broken. Locking that contract down in CI so a future helper change that leaves a dangling backslash — or any other shell-syntax regression in the rendered command — fails loudly instead of silently shipping broken pods. |
||
|
Chris LuandGitHub
|
3d39324bc1 |
fix(nfs): make Linux mount -t nfs work without client workaround (#9199) (#9201)
* fix(nfs): make Linux `mount -t nfs` work without client-side workaround (#9199) The upstream go-nfs library serves NFSv3 + MOUNT on a single TCP port and does not register with portmap. Linux mount.nfs queries portmap on port 111 first, so the plain `mount -t nfs host:/export /mnt` form failed with "portmap query failed" / "requested NFS version or transport protocol is not supported" against a default `weed nfs` deployment. - Add a minimal PORTMAP v2 responder (weed/server/nfs/portmap.go) with TCP+UDP listeners implementing PMAP_NULL, PMAP_GETPORT, PMAP_DUMP, and proper PROG_MISMATCH / PROG_UNAVAIL / PROC_UNAVAIL responses. Advertises NFS v3 TCP and MOUNT v3 TCP at the configured NFS port. - New CLI flag `-portmap.bind` (empty, disabled by default) to opt into the responder. Binding port 111 requires root or CAP_NET_BIND_SERVICE and must not collide with a system rpcbind. - Extended `weed nfs -h` help with the two supported ways to mount from Linux (client-side portmap bypass, or server-side `-portmap.bind`). - Startup log now prints a copy-pasteable mount command tailored to whether portmap is enabled. Unit tests cover RPC/XDR parsing, accept-stat paths, and a TCP+UDP round-trip against the real listener. Verified in a privileged Debian 12 container: with `-portmap.bind=0.0.0.0` the exact command from #9199 (`mount -t nfs -o nfsvers=3,nolock host:/export /mnt`) now succeeds and both read and write work. * fix(nfs): harden portmap responder per review feedback (#9201) Addresses three review findings on the portmap responder: - parseRPCCall: validate opaque_auth length against the record limit before applying the XDR 4-byte padding, so a near-uint32-max authLen can no longer overflow (authLen + 3) and bypass the bounds check. (gemini-code-assist) - serveTCP/Close: track live TCP connections and evict them on Close() so shutdown does not block on idle clients waiting for the read deadline to trip. serveTCP also no longer tears the listener down on a non-fatal Accept error (e.g. EMFILE); it logs and retries after a small back-off. Replaces the atomic.Bool closed flag with a mutex-guarded one so closed, conns, and the shutdown transition stay consistent. (coderabbit, minor) - handleTCPConn: apply per-IO read/write deadlines (30s idle, 10s in-flight) so a peer that opens the privileged port 111 and stalls cannot pin a goroutine indefinitely. (coderabbit, major) Adds TestPortmapServer_CloseEvictsIdleTCPConn, which holds a TCP connection idle and asserts Close() returns within 2s (well under the 30s idle deadline) and that the client sees the eviction. All existing tests still pass, including under -race. * fix(nfs): keep portmap UDP responder alive on transient read errors (#9201) - serveUDP: on a non-shutdown ReadFromUDP error, log, back off, and continue instead of returning. Matches how serveTCP now treats non-fatal Accept errors so a transient network blip doesn't take UDP portmap down until restart. (coderabbit) - Rename portmapAcceptBackoff -> portmapRetryBackoff now that both paths use it. - pmapProcDump: fix the pre-allocation capacity to match the actual encoding (20 bytes per entry + 4-byte terminator), replacing the old over-estimate of 24 per entry. No behavior change; just documents intent. (coderabbit nit) * docs(nfs): clarify encodeAcceptedReply body semantics (#9201) The prior comment said body is "nil when the accept_stat is itself an error", which was misleading: the PROG_MISMATCH branch already passes an 8-byte mismatch_info body. Rewrite to enumerate which error accept_stat values omit the body and call out PROG_MISMATCH as the exception, referencing RFC 5531 §9. Comment-only. (coderabbit nit) * fix(nfs): make portmap retry backoff interruptible by Close() (#9201) serveTCP and serveUDP both sleep portmapRetryBackoff (50ms) after a non-fatal listener error. If Close() races in during that sleep, the goroutine can't be interrupted, so Close() has to wait out the remaining backoff before wg.Wait() returns. Add a done channel that Close() closes once, and replace both time.Sleep calls with a select on ps.done + time.After. The window was tiny in practice but the select makes shutdown strictly bounded by Close()'s own work. (coderabbit nit) |
||
|
20f4fd9985 |
fix(storage): use ceil division for EC shard slots in maxVolumeCount (#9196)
* fix(storage): use ceil division for EC shard slots in maxVolumeCount * fix(topology): use ceil division for EC shard slots consistently Applies the same ceiling-division formula used in store.go to the four remaining master-side sites that computed volume-slots consumed by EC shards with off-by-one approximations: - disk.go ToDiskInfo / Disk.ToDiskInfo used (n+1)/d, which under-counts slots for non-multiples of DataShardsCount, over-reporting FreeVolumeCount. - DiskUsageCounts.FreeSpace and NodeImpl.AvailableSpaceFor subtracted n/d + 1, which over-counts slots at multiples of DataShardsCount, under-reporting free space (and suppressing volume growth on nodes that still had room). All four now use (n + DataShardsCount - 1) / DataShardsCount, matching store.go:393, store.go:810, and command_ec_decode.go:422. * refactor(topology): extract ecShardSlots helper Deduplicates the (n + DataShardsCount - 1) / DataShardsCount ceiling expression now used by ToDiskInfo, DiskUsageCounts.FreeSpace, Disk.ToDiskInfo, and AvailableSpaceFor. Addresses PR review feedback. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
 
|
0fcd5173be |
fix(admin): use basePath for API fetches when urlPrefix is set (#9197)
* fix(admin): use basePath for API fetches when urlPrefix is set * fix(admin): drop duplicate iam-utils script on Groups page * fix(admin): route topics page fetches through basePath The Topics page missed two fetch() calls that still used root-relative URLs, so create-topic and view-details still broke when -urlPrefix was set. --------- Co-authored-by: Maksim Babkou <maksim.babkou@innovatrics.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris LuandGitHub
|
749430dceb |
fix(filer.meta.tail): include extended metadata in Elasticsearch docs (#9200)
* fix(filer.meta.tail): include extended metadata in Elasticsearch docs The -es sink flattened only the FUSE attributes, so xattrs (including S3 user metadata like X-Amz-Meta-*) never reached Elasticsearch. Add an Extended field and convert map[string][]byte to map[string]string so the values index as text; non-UTF-8 values fall back to base64. Addresses #9190 follow-up. * fix(filer.meta.tail): prefix base64-encoded extended values with "base64:" Addresses review feedback: a plain UTF-8 xattr and a base64 fallback are otherwise indistinguishable to a consumer reading the ES doc. |
||
|
Chris Lu
|
036191c78a | Merge branch 'master' of https://github.com/seaweedfs/seaweedfs | ||
|
Chris Lu
|
34b236acfa |
test(s3api): look up NewUser by name in CreateAccessKey collision test
The memory credential store backs LoadConfiguration with a map, so the identity order is not stable across a save/load round trip. Indexing Identities[1] intermittently pointed at the owner identity and produced a spurious credential leak. |
||
 steve.weiandGitHub
|
1a7ab2ea82 |
fix(upload): keep Content-MD5 on 204 unchanged writes (#9198)
Return Content-MD5 in the volume unchanged-write response and read it in the uploader 204 path so multipart chunk ETag metadata is preserved. |