mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-12 19:16:18 +00:00
fc75f16c30c465ed3d2fd2d4e6846f4d021eac91
8819
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  Chris LuandGitHub
|
f16353de0b |
feat(mini): add -bucket flag to pre-create an S3 bucket on startup (#9302)
* feat(mini): add -bucket flag to pre-create an S3 bucket on startup Lets users hand a pre-provisioned object store to clients/CI without a post-start `weed shell s3.bucket.create` step. The flag is a no-op when empty (default) and idempotent on subsequent starts. * mini: bound bucket-creation RPCs with a timeout off miniClientsCtx Address PR review feedback: derive the lookup/mkdir context from miniClientsCtx() so Ctrl+C cancels the bucket RPCs, and cap with a 5s timeout so a stalled filer cannot block the welcome message indefinitely. Also wrap the DoMkdir error for parity with the lookup path. * mini: fall back to S3_BUCKET env var for -bucket Mirrors the existing -s3.externalUrl / S3_EXTERNAL_URL pattern so container/Kubernetes deployments can pre-create the bucket via env without overriding the entrypoint command. * docs(readme): lead weed mini quick start with credentials + bucket Promote the one-line setup (env vars + bucket) so users get a ready-to-use S3 endpoint without hopping between sections to find credential and bucket setup. * mini: accept comma-separated -bucket list Lets a single startup pre-create multiple S3 buckets, e.g. -bucket=bucket1,bucket2 (or S3_BUCKET=bucket1,bucket2). Names are trimmed and deduped; per-bucket errors are logged and the loop continues so one bad name does not block the rest. * mini: add -tableBucket flag for pre-creating S3 Tables buckets Mirrors -bucket but creates S3 Tables (Iceberg) buckets via s3tables.Manager so users can hand the all-in-one binary a ready-to-use table catalog without a follow-up weed shell call. Comma-separated, env fallback to S3_TABLE_BUCKET, idempotent on restart, owned by the DefaultAccountID placeholder. * mini: use errors.Is for ErrNotFound check in bucket lookup Matches the rest of the codebase (~20 call sites in weed/s3api). The direct equality works today because LookupEntry returns ErrNotFound unwrapped, but errors.Is future-proofs against any future wrapping. |
||
|
Chris LuandGitHub
|
1f6f473995 |
refactor(worker): co-locate plugin handlers with their task packages (#9301)
* refactor(worker): co-locate plugin handlers with their task packages
Move every per-task plugin handler from weed/plugin/worker/ into the
matching weed/worker/tasks/<name>/ package, so each task owns its
detection, scheduling, execution, and plugin handler in one place.
Step 0 (within pluginworker, no behavior change): extract shared helpers
that previously lived inside individual handler files into dedicated
files and export the ones now consumed across packages.
- activity.go: BuildExecutorActivity, BuildDetectorActivity
- config.go: ReadStringConfig/Double/Int64/Bytes/StringList, MapTaskPriority
- interval.go: ShouldSkipDetectionByInterval
- volume_state.go: VolumeState + consts, FilterMetricsByVolumeState/Location
- collection_filter.go: CollectionFilterMode + consts
- volume_metrics.go: export CollectVolumeMetricsFromMasters,
MasterAddressCandidates, FetchVolumeList
- testing_senders_test.go: shared test stubs
Phase 1: move the per-task plugin handlers (and the iceberg subpackage)
into their task packages.
weed/plugin/worker/vacuum_handler.go -> weed/worker/tasks/vacuum/plugin_handler.go
weed/plugin/worker/ec_balance_handler.go -> weed/worker/tasks/ec_balance/plugin_handler.go
weed/plugin/worker/erasure_coding_handler.go -> weed/worker/tasks/erasure_coding/plugin_handler.go
weed/plugin/worker/volume_balance_handler.go -> weed/worker/tasks/balance/plugin_handler.go
weed/plugin/worker/iceberg/ -> weed/worker/tasks/iceberg/
weed/plugin/worker/handlers/handlers.go now blank-imports all five
task subpackages so their init() registrations fire.
weed/command/mini.go and the worker tests construct the handler with
vacuum.DefaultMaxExecutionConcurrency (the constant moved with the
vacuum handler).
admin_script remains in weed/plugin/worker/ because there is no
underlying weed/worker/tasks/admin_script/ package to merge with.
* refactor(worker): update test/plugin_workers imports for moved handlers
Three handler constructors moved out of pluginworker into their task
packages — update the integration test files in test/plugin_workers/
to import from the new locations:
pluginworker.NewVacuumHandler -> vacuum.NewVacuumHandler
pluginworker.NewVolumeBalanceHandler -> balance.NewVolumeBalanceHandler
pluginworker.NewErasureCodingHandler -> erasure_coding.NewErasureCodingHandler
The pluginworker import is kept where the file still uses
pluginworker.WorkerOptions / pluginworker.JobHandler.
* refactor(worker): update test/s3tables iceberg import path
The iceberg subpackage moved from weed/plugin/worker/iceberg/ to
weed/worker/tasks/iceberg/. test/s3tables/maintenance/maintenance_integration_test.go
still imported the old path, breaking S3 Tables / RisingWave / Trino /
Spark / Iceberg-catalog / STS integration test builds.
Mirrors the OSS-side fix needed by every job in the run that
transitively imports test/s3tables/maintenance.
* chore: gofmt PR-touched files
The S3 Tables Format Check job runs `gofmt -l` over weed/s3api/s3tables
and test/s3tables, then fails if anything is unformatted. Files this
PR moved or modified had import-grouping and trailing-spacing issues
introduced by perl-based renames; reformat them with gofmt -w.
Touched files:
test/plugin_workers/erasure_coding/{detection,execution}_test.go
test/s3tables/maintenance/maintenance_integration_test.go
weed/plugin/worker/handlers/handlers.go
weed/worker/tasks/{balance,ec_balance,erasure_coding,vacuum}/plugin_handler*.go
* refactor(worker): bounds-checked int conversions for plugin config values
CodeQL flagged 18 go/incorrect-integer-conversion warnings on the moved
plugin handler files: results of pluginworker.ReadInt64Config (which
ultimately calls strconv.ParseInt with bit size 64) were being narrowed
to int32/uint32/int without an upper-bound check, so a malicious or
malformed admin/worker config value could overflow the target type.
Add three helpers in weed/plugin/worker/config.go that wrap
ReadInt64Config and clamp out-of-range values back to the caller's
fallback:
ReadInt32Config (math.MinInt32 .. math.MaxInt32)
ReadUint32Config (0 .. math.MaxUint32)
ReadIntConfig (math.MinInt32 .. math.MaxInt32, platform-portable)
Update each flagged call site in the four moved task packages to use
the bounds-checked helper. For protobuf uint32 fields (volume IDs)
the variable type also becomes uint32, removing the trailing
uint32(volumeID) casts and changing the "missing volume_id" check
from `<= 0` to `== 0`.
Touched files:
weed/plugin/worker/config.go
weed/worker/tasks/balance/plugin_handler.go
weed/worker/tasks/erasure_coding/plugin_handler.go
weed/worker/tasks/vacuum/plugin_handler.go
* refactor(worker): use ReadIntConfig for clamped derive-worker-config helpers
CodeQL still flagged three call sites where ReadInt64Config was being
narrowed to int after a value-range clamp (max_concurrent_moves <= 50,
batch_size <= 100, min_server_count >= 2). The clamp is correct but
CodeQL's flow analysis didn't recognize the bound, so it flagged them
as unbounded narrowing.
Switch to ReadIntConfig (already int32-bounded by the helper) for
those three sites, drop the now-redundant int64 intermediate variables.
Also drops the now-unused `> math.MaxInt32` clamp in
ec_balance.deriveECBalanceWorkerConfig (the helper covers it).
|
||
|
Chris LuandGitHub
|
31e5e0dee2 |
fix(mount): keep async flush when LockOwner has no POSIX locks (#9300)
FlushIn.LockOwner is populated by the kernel for any fd that may have participated in locking, not only when locks were actually taken. The previous Flush logic treated any non-zero LockOwner as a closing lock holder and forced a synchronous flush, which silently disabled the writebackCache async-flush path (introduced in #8727) for most ordinary close() calls. Consult the POSIX lock table before forcing sync: only owners that currently hold a non-flock byte-range lock need the synchronous path to coordinate with blocked SetLkw waiters. Other closes go async as intended. |
||
|
Chris LuandGitHub
|
f2c3bd7b77 |
fix(admin/view): define basePath in plugin IIFE scopes (#9298)
The plugin.templ and plugin_lane.templ components use basePath() in their IIFE (Immediately Invoked Function Expression) scopes to handle subdirectory deployments. However, basePath was not defined locally, causing "basePath is not defined" errors when accessing plugin pages. Added local basePath function definitions in both files, matching the pattern from admin.js. This function checks window.__BASE_PATH__ (set by the layout during page initialization) and prepends it to API paths. |
||
 Lars LehtonenandGitHub
|
913f98db10 | refactor(weed/storage): log volume file removal failures (#9297) | ||
 
|
1127672d10 |
fix(s3api,iamapi): avoid full SaveConfiguration when creating a single IAM user (#9261)
* fix(s3api,iamapi): avoid full SaveConfiguration when creating a single IAM user
When CreateUser was called, both the embedded IAM path (s3api_embedded_iam.go)
and the standalone IAM path (iamapi_management_handlers.go) would:
1. Load the full config (all N users)
2. Append the new user
3. Call SaveConfiguration, which re-writes ALL N user files in the filer_etc
store, triggering N file-change events and N reload cycles.
The fix replaces the full-save path with credentialManager.CreateUser, which
writes only the single new user's file. Set changed=false to skip the redundant
SaveConfiguration call, and add "CreateUser" to the reload-after-targeted-write
block so the in-memory config is refreshed.
Also adds a nil guard around iama.iam.GetCredentialManager() in DoActions to
avoid a nil-pointer panic in legacy test fixtures that leave iam unset.
Add SetCredentialManagerForTest to IdentityAccessManagement so tests can inject
an in-memory credential store without touching production code paths.
* test(s3api,iamapi): regression tests for CreateUser targeted-write fix
Add TestCreateUserDoesNotSaveAllUsers (iamapi) and
TestEmbeddedIamCreateUserDoesNotSaveAllUsers (s3api) to guard against the
regression where CreateUser would call SaveConfiguration and re-write all
N existing user files.
Both tests:
- Pre-populate 3 existing users
- Invoke CreateUser via the HTTP API
- Assert PutS3ApiConfiguration (full-config save) was NOT called
- Assert the new user is visible in the credential store
- Assert all pre-existing users are still intact
Also update TestEmbeddedIamExecuteAction to verify persistence via the
credential manager directly (mockConfig is no longer updated on CreateUser
since we skip the SaveConfiguration path).
* refactor(s3api,iamapi): share credential-error to IAM-code mapping
Move the credentialErrToIamErrCode helper from weed/iamapi to
weed/s3api as exported CredentialErrToIamErrCode and call it from
both the standalone IAM handler (iamapi) and the embedded IAM
handler (s3api). Previously the standalone path used the helper
while the embedded path duplicated the same switch inline; the two
sites could drift out of sync.
Also extend the mapping to cover ErrUserNotFound and
ErrAccessKeyNotFound (404 NoSuchEntity) so non-CreateUser callers
that opt into the helper get the right HTTP status.
* test(s3api): seed credential store explicitly in CreateUser regression
Previously the test relied on getS3ApiConfigurationFunc's syncOnce
side effect to populate the credential store with mockConfig
identities. If that fixture ever stopped seeding, the post-test
"pre-existing users still exist" assertion would silently start
passing for the wrong reason (the users were never created).
Seed cm.SaveConfiguration directly and assert the seed precondition
before the API call so any seeding regression fails loudly.
* fix(s3api): honor skipPersist in embedded CreateUser targeted path
ExecuteAction documents that skipPersist=true means "the changed
configuration is not saved to the persistent store", but the
targeted credentialManager.CreateUser added for the avoid-bulk-save
fix ran unconditionally. Gate it on !skipPersist so no-persist
callers don't silently leak a write to the credential store. Leave
changed=true in the skipPersist branch so the tail's existing
`if changed { if !skipPersist { ... } }` block keeps suppressing
persistence the same way it does for every other action.
Add TestEmbeddedIamCreateUserSkipPersist to pin the contract: a
CreateUser invocation with skipPersist=true must not call
PutS3ApiConfiguration and must not leave the user in the
credential store.
---------
Co-authored-by: Chris Lu <chris.lu@gmail.com>
|
||
|
Chris LuandGitHub
|
6572b472c3 |
fix(s3): honor X-Forwarded-For in audit log remote_ip (#9295)
* fix(s3): honor X-Forwarded-For in audit log remote_ip When SeaweedFS S3 sits behind a reverse proxy (e.g., Caddy), the audit log's `remote_ip` was reporting the proxy's address because only `X-Real-IP` and `r.RemoteAddr` were consulted. Caddy and most other proxies set `X-Forwarded-For` by default but not `X-Real-IP`, so the real client IP was lost. Check `X-Forwarded-For` first (using the left-most non-empty entry as the originating client), then fall back to `X-Real-IP`, then `r.RemoteAddr`. Fixes #9293 * fix(s3): strip port from RemoteAddr fallback in audit log Address PR review: the X-Forwarded-For and X-Real-IP paths return host-only values, while the RemoteAddr fallback was returning "host:port", making the remote_ip field inconsistent with both the other code paths and the "192.0.2.3" example in the AccessLog struct. Use net.SplitHostPort to strip the port, falling back to the raw RemoteAddr for non-IP markers (e.g., "@" for unix sockets). |
||
|
Chris LuandGitHub
|
d265274e13 |
fix(nfs): accept dirpath any-where under the export, mirroring rclone (#9291)
* fix(nfs): accept any MOUNT3 dirpath, mirroring rclone's permissive policy
weed nfs has exactly one export per process, so the MOUNT3 dirpath
argument has no second export to disambiguate against. Strict
comparison only translated PV-path typos into the inconsistent
"mount succeeds but empty" / "mount fails completely" split that
operators see.
Match rclone's serve nfs Handler.Mount: ignore the dirpath, log an INFO
line when it differs from the configured export, and always serve the
export root. Apply the same change to the UDP MOUNT3 path so kernel
clients defaulting to mountproto=udp see identical behaviour. Access
control still goes through -allowedClients / -ip.bind, and file-handle
scoping in FromHandle is unchanged so handles still cannot escape the
export.
Replace the prior single-path reject tests with table tests covering
the shapes operators commonly hit: root, parent, sibling, deeper child,
unrelated, empty, relative form, exact match, and trailing slash, at
the Handler.Mount, UDP MOUNT3, and full RPC layers.
* feat(nfs): mount at subdirectory when MOUNT3 dirpath is under the export
Make the dirpath argument meaningful when the client asks for a subtree
of the configured export. With -filer.path=/buckets, a client mounting
<server>:/buckets/data lands directly inside /buckets/data instead of
at the export root.
- dirpath equals the export root: serve the export root.
- dirpath strictly under the export, directory entry: serve that
subdirectory; the returned filehandle encodes its inode.
- dirpath strictly under the export, missing or non-directory: reject
with NoEnt or NotDir.
- dirpath outside the export: keep the rclone-style fallback to the
export root.
TCP returns a sub-rooted seaweedFileSystem and lets go-nfs's onMount
call ToHandle to encode the FH; UDP encodes the FH itself. FromHandle
is unchanged: handles are content-addressed by inode and resolve via
the inode index, so they remain stable across mounts and across
process restarts.
The trimmed permissive tests keep their outside-export shapes; new
subexport tests cover under-export directories, missing entries, and
non-directory entries on Handler.Mount, the UDP MOUNT3 wire, and
through the full RPC stack.
* nfs: propagate request context through MOUNT3 resolution
Mount now accepts the gonfs context and threads it through
resolveMountFilesystem and lstatExportStatus so a slow filer call
during MOUNT cannot outlive a cancelled or timed-out request.
lstatExportStatus uses fileInfoForVirtualPath(ctx, "/") directly
instead of billy.Filesystem.Lstat, which would otherwise drop the
context on the floor by calling fileInfoForVirtualPathWithOptions
with context.Background().
Lower the successful subexport-mount log from V(0) to V(1). The
fallback log stays at V(0) so operator typos still surface; the
success line is per-mount churn that adds up on NFS-CSI deployments.
* nfs: mirror TCP defensive checks on the UDP MOUNT3 path
Two transport-parity bugs the rabbit caught:
(1) The exact-export-root and outside-export branches were returning
MNT3_OK unconditionally, while the TCP handler runs lstatExportStatus
on those same branches. If the configured -filer.path has been
removed from the filer, TCP returns NoEnt/ServerFault but UDP would
still hand out a synthetic root handle pointing at nothing. Add
rootMountStatus as the UDP analogue and call it on both branches.
(2) resolveSubexportFileHandle did filer I/O on the single UDP serve
loop with context.Background(). One slow filer round-trip would
block every later MOUNT packet. Wrap each MOUNT call's filer work in
context.WithTimeout(mountUDPLookupTimeout) and thread that ctx
through both rootMountStatus and resolveSubexportFileHandle.
Lower the successful subexport log to V(1) to match the TCP side.
* nfs: assert TCP/UDP MOUNT3 produce byte-identical filehandles
The existing UDP subexport assertions only checked the decoded inode
and kind. A regression that drifted the generation, exportID, or
encoding format on one transport but not the other would have slipped
through. Build the TCP Handler from the same Server, drive its Mount
with the same dirpath, and require ToHandle to match the raw UDP FH
bytes for every OK case.
* nfs: take MOUNT3 dirpath as string in resolveMountFilesystem
Convert req.Dirpath to string once at the call site instead of
sprinkling string(...) casts through every log line and conversion
inside the function. Behavior unchanged.
* nfs: share rootFS lifecycle between TCP and UDP MOUNT handlers
Server.rootFilesystem() lazily constructs the seaweedFileSystem rooted
at the configured export the first time anything asks for it, then
hands the same instance to every subsequent caller. newHandler() and
mountUDPServer.rootMountStatus() now both go through it, so:
- Both transports observe the same chunk reader cache and chunk
invalidator without depending on call order during startup.
- The UDP defensive Lstat doesn't allocate a fresh wrapper per
MOUNT request anymore; one struct lives for the life of the
Server.
The sub-rooted seaweedFileSystem the subexport branch builds in
resolveSubexportFileHandle is still per-request because actualRoot
varies with the requested dirpath.
* nfs: drive rootFilesystem before reading sharedReaderCache on UDP
The UDP listener is started before serve() calls newHandler(), so an
under-export MOUNT3 request can reach resolveSubexportFileHandle before
Server.sharedReaderCache has been assigned. Reading it directly would
hand newSeaweedFileSystem a nil cache and the sub-fs would build a
throwaway ReaderCache that never gets shared with the TCP path.
Take rootFS off Server.rootFilesystem() (which drives the sync.Once
that initializes the shared cache) and read readerCache off that
instead, so subexport sub-fs instances always share the same reader
cache as rootFS regardless of which transport sees the first MOUNT.
* nfs: collapse exact-match and outside-export MOUNT branches
The two branches return the same filesystem (export root) and the
same status; only the log line differs. Combine the conditions and
guard the fallback log inline. Behavior unchanged.
|
||
|
Chris Lu
|
14cd426cf9 | templ | ||
|
Lars LehtonenandGitHub
|
89b07f3e12 | chore(weed/mq/kafka/protocol): prune no-op test (#9287) | ||
|
Lars LehtonenandGitHub
|
02da928e25 | chore(weed/mq/kafka/protocol): prune dead code (#9288) | ||
|
be451d22b5 |
feat(filer.sync): add -verifySync mode to filer.sync for cross-cluster file comparison (#9284)
* Add -verifySync flag to filer.sync for cross-cluster file comparison
Add a verification mode to filer.sync that compares entries between two
filers without performing actual synchronization. Uses directory-level
sorted merge of ListEntries to detect missing files, size mismatches,
and ETag mismatches. Supports -isActivePassive for unidirectional check
and -modifyTimeAgo to skip recently modified files during sync lag.
* Add mtime annotation and JSON output to filer.sync -verifySync
Add automatic mtime relation analysis for SIZE_MISMATCH and
ETAG_MISMATCH diffs, and an NDJSON output mode for external tooling.
mtime classification:
- B_NEWER => "late_updates_skip_likely" hint. Surfaces the case
where target has a stub entry whose mtime is ahead of source's
real file, causing UpdateEntry's mtime guard in filersink to
permanently skip the update.
- A_NEWER => "sync_lag_or_event_miss" hint.
- EQUAL => no hint (chunk-level issue suspected).
Text output example:
[SIZE_MISMATCH] /path (a=996, b=0, B newer +274d [late-updates skip likely])
Add -verifyJsonOutput flag. When set, emits one JSON object per
line (NDJSON) for diffs and a final SUMMARY object, suitable for
piping into external diagnostic pipelines.
Concurrent writes from the directory worker pool are now serialized
via outputMu to keep both text lines and JSON records atomic.
* fix(filer.sync): use shared global semaphore in verifySync to bound goroutine explosion
Replace the per-call local semaphore in compareDirectory with a single
shared semaphore created in runVerifySync. The old per-level semaphore
applied a limit of verifySyncConcurrency only within each directory level,
allowing effective concurrency to grow as verifySyncConcurrency^depth on
deep trees.
The shared semaphore is held only for each directory's I/O phase
(listEntries + merge) and released before recursing into subdirectories,
so a parent never blocks waiting for children to acquire slots — which
would deadlock once tree depth exceeds the semaphore capacity.
Extract the capacity into a named constant (verifySyncConcurrency = 5)
with a comment explaining the memory vs. performance trade-off.
Add unit tests:
- correctness: missing file, only-in-B, size mismatch, active-passive mode
- concurrency bound: peak concurrent listings ≤ verifySyncConcurrency
- no-deadlock: binary tree of depth 10 completes within timeout
* fix(filer.sync): stream directory entries to prevent OOM on large directories
Replace the listEntries helper (which accumulated all entries into a
single []filer_pb.Entry slice) with an entryStream type that pages
through the directory in the background and forwards entries one at a
time through a buffered channel. Memory per directory comparison is now
O(channel buffer size = 64) regardless of how many entries the directory
contains.
Key design points:
- entryStream wraps a goroutine + buffered channel with a one-entry
lookahead (peek/advance) so the two-pointer sorted merge in
compareDirectory can work without buffering any full listing.
- A child context (mergeCtx) is passed to both stream goroutines so
they are cancelled promptly if compareDirectory returns early (e.g.
on error); the ctx.Done() select arm in the callback prevents
goroutine leaks when the consumer stops reading.
- stream.err is written by the goroutine before close(ch), so it is
safe to read after the channel is exhausted (Go memory model:
channel close happens-before the zero-value receive).
- countMissingRecursive is rewritten to use ReadDirAllEntries with a
direct callback, eliminating its own slice allocation.
- listEntries is removed; it is no longer called anywhere.
* fix(filer.sync): address verifySync review findings
Four real bugs found and fixed; one finding already resolved (shared
semaphore was introduced in a prior commit).
path.Join for child paths (filer_sync_verify.go)
fmt.Sprintf("%s/%s", dir, name) produced "//name" when dir was "/".
Replace all child-path concatenations with path.Join so root-level
walks emit clean paths.
cutoffTime check for ONLY_IN_B entries (filer_sync_verify.go)
The B-only branch ignored -modifyTimeAgo, so files recently written
to B were reported as ONLY_IN_B instead of being skipped. Mirror the
A-side mtime guard: skip and increment skippedRecent when the entry
is newer than cutoffTime.
Summary emitted before error check (filer_sync_verify.go)
A filer I/O error mid-walk still caused a SUMMARY record (or text
summary) to be printed, making partial runs appear complete. Move the
error check to before summary emission; on error, return immediately
without printing any summary.
Return false on verification failure (filer_sync.go)
runVerifySync returned true (exit 0) even when diffs were found or the
walk failed. Return false so the main binary sets exit status 1,
consistent with how all other commands signal failure.
* test(filer.sync): add missing verifySync test coverage
Four new tests covering gaps identified during review:
TestVerifySyncETagMismatch
Verifies that two files with identical size but different Md5 checksums
are counted as etagMismatch (not sizeMismatch). Exercises the second
branch of compareEntries that was previously untested.
TestVerifySyncCutoffTime (4 subtests)
A-only recent — recent file skipped (skippedRecent++), not MISSING
A-only old — old file reported as MISSING
B-only recent — recent file skipped (skippedRecent++), not ONLY_IN_B
B-only old — old file reported as ONLY_IN_B
The B-only subtests specifically cover the cutoffTime fix added in the
previous commit.
TestVerifySyncRootPath
Regression for the path.Join fix: walks from "/" and verifies that the
child directory is reached and compared correctly (the old Sprintf
produced "//data" which would silently produce wrong results).
Asserts dirCount=2 and fileCount=1 to confirm the full tree is walked.
* fix(filer.sync): use os.Exit(2) instead of return false on verify failure
return false triggered weed.go's error handler which printed the full
command usage — appropriate for invalid arguments, not for a completed
verification that found differences. Use os.Exit(2) consistent with
the existing pattern in filer_sync.go (lines 251, 293).
* refactor(filer.sync.verify): split verify into its own command
The verify mode is a one-shot batch operation with a fundamentally
different lifecycle from the long-running sync subscriber, and most of
filer.sync's flags (replication, metrics port, debug pprof, concurrency,
etc.) do not apply to it. Extract it into a sibling command alongside
filer.copy/filer.backup/filer.export rather than a flag mode on
filer.sync.
Also rename modifyTimeAgo to modifiedTimeAgo (grammatical) and drop the
verifyJsonOutput prefix to plain jsonOutput now that the verify context
is implicit in the command name.
* fix(filer.sync.verify): address review comments
- Bounded worker pool: cap subdirectory goroutines per level via a
jobs channel and min(verifySyncConcurrency, len(subDirs)) workers
instead of spawning one goroutine per child. Wide directories no
longer park ~2KB per queued goroutine.
- Don't gate recursion on a directory's mtime: a fresh child write
bumps the parent mtime, but older files inside should still be
reported as missing. Always recurse for missing-in-B directories
and apply the cutoff per-file inside countMissingRecursive.
- Apply -modifiedTimeAgo symmetrically: matched-name files now skip
the comparison when EITHER side is recently modified, not just A.
This restores lag tolerance when B was just rewritten.
Adds tests for both new behaviors and a shared isTooRecent helper.
---------
Co-authored-by: Chris Lu <chris.lu@gmail.com>
|
||
 
|
eebffd9df6 |
fix(storage): fix verifyDeletedNeedleIntegrity using wrong offset (#9273)
* fix(storage): fix verifyDeletedNeedleIntegrity using wrong offset verifyDeletedNeedleIntegrity was ignoring the offset recorded in the index file and instead always reading from fileSize-DiskSize(0), which is only correct when the deleted needle happens to be the very last entry in the .dat file. When a deletion tombstone is written in the middle of the volume (e.g. after subsequent writes), the function reads the wrong bytes, causing the key-mismatch check to either silently pass (if the bytes happen to form a valid needle with the same ID) or produce a spurious integrity error. Fix: accept the actual offset from the index entry and use types.TombstoneFileSize as the size, mirroring the approach used by verifyNeedleIntegrity for live needles. * fix(storage): fix error message in doCheckAndFixVolumeData The error message in the deleted-needle branch referenced verifyNeedleIntegrity, but the function being called is verifyDeletedNeedleIntegrity. This makes log output misleading when a deleted-needle integrity check fails. Suggested by gemini-code-assist review on PR #9273. --------- Co-authored-by: chenshi5012 <chenshi5012@github.com> |
||
  
|
db34e8b6fd |
feat(admin): prefer stored S3 Content-Type metadata over key-extension MIME inference (#9286)
* feat(admin): prefer stored filer mime in file browser and properties * feat(mime): enhance MIME type registration and improve fallback logic Co-authored-by: Copilot <copilot@github.com> --------- Co-authored-by: baracudaz <baracudaz@users.noreply.github.com> Co-authored-by: Copilot <copilot@github.com> |
||
 qzhelloandGitHub
|
60c76120fc |
fix(shell): use exact match for volume.balance -racks/-nodes filter (#9279)
* fix(shell): correct volume.list -writable filter unit and comparison * fix(shell): correct volume.list -writable filter unit and comparison * chore(shell): fix typo in EC shard helper param names * fix(shell): use exact match for volume.balance -racks/-nodes filter The old strings.Contains-based filter quietly included any id that was a substring of the user-supplied flag value (e.g. -racks=rack10 also matched rack1). Replace it with an exact-match set parsed from the comma-separated flag value, and add regression tests for both -racks and -nodes paths. Also fix a small typo in the "remote storage" error returned by maybeMoveOneVolume. * fix(shell): use exact match for volume.balance -racks/-nodes filter The old strings.Contains-based filter quietly included any id that was a substring of the user-supplied flag value (e.g. -racks=rack10 also matched rack1). Replace it with an exact-match set parsed from the comma-separated flag value, and add regression tests for both -racks and -nodes paths. Also fix a small typo in the "remote storage" error returned by maybeMoveOneVolume. * refactor(shell): drop nil sentinel in splitCSVSet, use len() in callers |
||
|
Chris LuandGitHub
|
1f515f9d02 |
fix(s3api): cross-SSE copy operations and bring them back into CI (#9281) (#9282)
* fix(s3api): cross-SSE copy operations and bring them back into CI (#9281) Four cross-SSE copy tests were broken on master and excluded from CI with the comment "pre-existing SSE-C issues": - TestSSECObjectCopyIntegration/Copy_SSE-C_to_SSE-C_with_different_key - TestSSEKMSObjectCopyIntegration/Copy_SSE-KMS_with_different_key - TestCrossSSECopy/SSE-S3_to_SSE-C - TestSSEMultipartCopy/Copy_SSE-KMS_Multipart_Object Each surfaced as a different symptom — 500 InternalError, CRC32 mismatch, "unexpected EOF", MD5 mismatch — but they were all instances of the same root pattern that #8908 hit on UploadPartCopy: copy paths writing destination chunks tagged inconsistently with the bytes on disk, so detectPrimarySSEType / IsSSE*Encrypted disagreed about what the read path should do. Five fixes in this PR, each with its own targeted test: 1. SSE-C IV format: putToFiler stored entry.Extended[SeaweedFSSSEIV] as raw bytes (with a comment saying so), but StoreSSECIVInMetadata stored it base64-encoded. The two readers (the GET handler reading it raw, and GetSSECIVFromMetadata reading it base64-decoded) each matched one writer but not the other. Standardise on raw bytes everywhere; GetSSECIVFromMetadata accepts the legacy base64 form for backward compat. 2. SSE-C single-part copy chunk tagging: copyChunkWithReencryption re-encrypted the bytes for the destination but never set the destination chunk's SseType / SseMetadata. With chunks left SseType=NONE, detectPrimarySSEType returned "None" and the GET served still-encrypted volume bytes raw without decryption. Tag the chunk after re-encryption. 3. SSE-KMS single-part copy chunk tagging: same shape as (2). Also, the function discarded the destSSEKey returned from CreateSSEKMSEncryptedReaderWithBucketKey (with `_`) — that key carries the freshly-minted EncryptedDataKey + IV the read path needs, so it must be captured and serialized into the destination chunk's per-chunk metadata (and bubbled up to the entry-level SeaweedFSSSEKMSKey for single-chunk objects whose read path falls back to the entry-level key). 4. SSE-KMS multipart source decryption: copyChunkWithSSEKMSReencryption decrypted every source chunk with the entry-level sourceSSEKey. For multipart SSE-KMS objects each chunk has its own EDK + IV in per-chunk metadata, so the entry-level key is wrong. Decrypt with per-chunk metadata when present. 5. Same-key copy fast path chunk tagging: copySingleChunk uses createDestinationChunk which dropped SseType / SseMetadata. For same-key copies (e.g. SSE-KMS source → SSE-KMS dest with the same KMS key) the fast path reuses the source ciphertext as-is, so the destination chunks must keep the source's SSE tagging. Add a createDestinationChunkPreservingSSE helper for the fast path; the re-encryption paths still call createDestinationChunk and then overwrite the SSE fields after re-encrypting. CI: extend the comprehensive-test TEST_PATTERN to include the four test families that were previously excluded (`.*ObjectCopyIntegration`, `TestCrossSSECopy`, `TestSSEMultipartCopy`) so this category of regression is caught going forward. The exclusion comment is removed. Tests: - All four originally-failing tests pass. - The full pre-existing TestSSE* / TestCrossSSE / TestGitHub7562 / TestCopyToBucketDefaultEncryptedRegression / TestSSEMultipart suite still passes. - go test -race ./weed/s3api/ passes. Refs #8908, #9280. * fix(s3api): SSE-KMS copy ChunkOffset must stay 0 (review feedback on #9282) CreateSSEKMSEncryptedReaderWithBucketKey initialises a fresh CTR stream at counter 0 with a per-chunk random IV — there is no base-IV-plus-offset relationship. The previous commit on this branch wrote `destSSEKey.ChunkOffset = chunk.Offset` onto the per-chunk metadata, which the read-side CreateSSEKMSDecryptedReader applies as calculateIVWithOffset(IV, ChunkOffset) — i.e. it advances the decryption IV by chunk.Offset/16 blocks beyond where the encryption actually wrote. The bug only manifests for SSE-KMS-to-SSE-KMS-with-different-key copies of multipart sources (where source chunks live at non-zero offsets), which is why the existing TestSSEKMSObjectCopyIntegration (single-chunk source) and TestSSEMultipartCopy/Copy_SSE-KMS_Multipart_Object (same-key copy that takes the fast preserving path, not the re-encrypt path) both happened to pass. Set ChunkOffset to 0 to match the actual encryption position. Existing tests still pass; the dangerous case is only reachable with a multipart SSE-KMS source and a different destination key, which is not currently exercised in CI. Found by gemini-code-assist review on PR #9282. * fix(s3api): use first dst chunk's full key for entry-level SSE-KMS metadata in remaining copy paths (review feedback on #9282) Earlier this branch fixed copyChunksWithSSEKMSReencryption to populate the entry-level SeaweedFSSSEKMSKey from the first destination chunk's fully-formed metadata (with EDK + IV) instead of a stub key with only KeyID + EncryptionContext + BucketKeyEnabled. The same fix needs to apply to the other two paths that build entry-level SSE-KMS metadata: - copyMultipartCrossEncryption() — cross-encryption to SSE-KMS dest. Per-chunk metadata comes from copyCrossEncryptionChunk's CreateSSEKMSEncryptedReaderWithBucketKey call, so chunks[0] has a real EDK + IV. Use it. - copyChunksWithSSEKMS() direct (same-key) branch. After createDestinationChunkPreservingSSE in copySingleChunk, dst chunks carry the source's per-chunk SSE-KMS metadata. Use chunks[0] for the entry-level key so single-chunk same-key copies don't fall back to a stub key on the read path. Without this, single-chunk SSE-KMS reads through these two paths failed at GET with "Invalid ciphertext format" — KMS unwrap was called on an empty EDK. Found by coderabbitai review on PR #9282. * fix(s3api): add 0-byte fallback to SSE-KMS reencryption entry-level metadata (review feedback on #9282) copyChunksWithSSEKMSReencryption was missing the fallback for 0-byte objects (where dstChunks is empty), inconsistent with the fallback in copyChunksWithSSEKMS direct branch and copyMultipartCrossEncryption. Without it, a 0-byte SSE-KMS copy would land with no entry-level SeaweedFSSSEKMSKey, so the read path's IsSSEKMSEncryptedInternal check would not recognise the empty object as SSE-KMS. Mirror the existing fallback: build a stub SSEKMSKey with KeyID, context and bucket-key state; serialize it as the entry-level key. Found by gemini-code-assist review on PR #9282. * fix(s3api): SSE-KMS direct copy must check encryption context + bucket-key, not just key ID (review feedback on #9282) DetermineSSEKMSCopyStrategy / CanDirectCopySSEKMS only compares the source and destination KMS key IDs, but the destination request can also change the encryption context or the BucketKey flag. Both are embedded in the source ciphertext's wrapped EDK; preserving the source metadata verbatim does not satisfy a destination request that asks for different settings, so the destination object would silently report the source's context/flag instead of what was requested. Add srcSSEKMSStateMatchesDest: deserialize the source's stored SSEKMSKey and compare its EncryptionContext + BucketKeyEnabled to the destination request. If either differs, force the slow re-encrypt path (SSEKMSCopyStrategyDecryptEncrypt) so the destination gets a freshly-wrapped EDK bound to the requested context/flag. A malformed source key is treated as non-matching (conservative). nil and empty encryption-context maps are treated as equal to avoid spurious divergence when the request omits the context header. Found by coderabbitai review on PR #9282. * fix(s3api): copyMultipartSSEKMSChunk falls back to entry-level key + entry-level metadata uses first chunk's full key (review feedback on #9282) Two related issues in copyMultipartSSEKMSChunks / copyMultipartSSEKMSChunk: 1. copyMultipartSSEKMSChunks built the destination's entry-level SeaweedFSSSEKMSKey from a stub (KeyID + context + bucket-key only), missing the EDK + IV. Single-chunk reads through this path fall back to entry-level keyData and would fail at GET because KMS would be asked to unwrap an empty EDK. Mirrors the fix in copyChunksWithSSEKMS / copyMultipartCrossEncryption / copyChunksWithSSEKMSReencryption: prefer the first dst chunk's full per-chunk metadata, fall back to the stub only for 0-byte objects. 2. copyMultipartSSEKMSChunk hard-failed when chunk.GetSseMetadata() was empty. Newer multipart SSE-KMS uploads populate per-chunk metadata, but legacy objects may have only entry-level metadata and would now be impossible to copy. Add a sourceEntrySSEKey fallback parameter (deserialized once by the caller from entry.Extended[SeaweedFSSSEKMSKey]); use it when per-chunk metadata is absent. Found by coderabbitai review on PR #9282. * refactor(s3api): extract entry-level SSE-KMS deserialization and per-chunk fallback into helpers (review feedback on #9282) Three medium-priority maintainability comments from gemini-code-assist: - The same "deserialize entry.Extended[SeaweedFSSSEKMSKey]" pattern appeared in srcSSEKMSStateMatchesDest, copyMultipartSSEKMSChunks and copyChunksWithSSEKMSReencryption. - The "prefer per-chunk metadata, fall back to entry-level key" selection logic appeared inline in copyMultipartSSEKMSChunk and copyChunkWithSSEKMSReencryption with subtly different shapes. - encryptionContextEqual hand-rolled a map comparison. Pull both patterns out into named helpers: - deserializeEntrySSEKMSKey: returns the entry-level SSEKMSKey or nil on missing/malformed data, with a single V(2) log line. - resolveChunkSSEKMSKey: centralises the chunk-vs-entry-level selection so all sites use the same decryption-side selection logic (which must mirror the encryption side). Replace encryptionContextEqual's manual loop with reflect.DeepEqual, keeping the empty-vs-nil shortcut at the top because DeepEqual treats those as different. No behaviour change; existing copy tests still pass. |
||
|
Chris LuandGitHub
|
82cf60a44f |
fix(s3api): re-encrypt UploadPartCopy bytes for the destination's SSE config (#8908) (#9280)
* fix(s3api): re-encrypt UploadPartCopy bytes for the destination's SSE config (#8908) The remaining failure mode in #8908 was that Docker Registry's blob finalization (server-side Move via UploadPartCopy) silently corrupts SSE-S3 multipart objects. Reproduces with `aws s3api upload-part-copy` under bucket-default SSE-S3: the GET on the completed object returns deterministic wrong bytes (correct length, same wrong SHA-256 across runs). The metadata is mathematically self-consistent — every chunk's stored IV equals `calculateIVWithOffset(baseIV_dst, partLocalOffset)` — but the bytes on disk were encrypted with the SOURCE upload's key+baseIV. Root cause: - `copyChunksForRange` (and `createDestinationChunk`) constructs new chunks for UploadPartCopy without copying `SseType` / `SseMetadata`, so destination chunks are written with `SseType=NONE`. - At completion, `completedMultipartChunk` (PR #9224's NONE→SSE_S3 backfill, intended to recover from a different missing-metadata bug) sees those NONE chunks under an SSE-S3 multipart upload and backfills SSE-S3 metadata derived from the destination upload's baseIV. The chunk metadata is now internally consistent and the GET path applies decryption — but the bytes on disk are encrypted with the source upload's key, not the destination's. Decryption produces deterministic garbage. Docker Registry pulls then fail with "Digest did not match". Fix: when either the source object or the destination multipart upload has any SSE configured, take a slow-path UploadPartCopy that (1) opens a plaintext reader of the source range — decrypting the source's per-chunk SSE-S3 metadata if needed via a reused `buildMultipartSSES3Reader`, and (2) feeds that plaintext through `putToFiler`'s existing encryption pipeline by staging the destination upload entry's SSE-S3/SSE-KMS headers on a cloned request. Encryption then matches PutObjectPart's contract: every part starts a fresh CTR stream from counter 0 with `baseIV_dst`, and each internal chunk's metadata records `calculateIVWithOffset(baseIV_dst, chunk.partLocalOffset)`. The `non-SSE → non-SSE` case still takes the existing fast raw-byte copy path — bytes on disk are plaintext on both sides, so chunk-level metadata is irrelevant. Cross-encryption from SSE-KMS / SSE-C sources is left as TODO — the new path returns an explicit error rather than the previous silent corruption. SSE-S3 (the user-reported case) round-trips correctly. Tests: - test/s3/sse/s3_sse_uploadpartcopy_integration_test.go pins three UploadPartCopy shapes against bucket-default SSE-S3: * Docker-Registry-shape 32MB+tail (the user's exact 5-chunk / 2-part metadata layout) * single full-object UploadPartCopy * many small range copies Each round-trips SHA-256. - test/s3/sse/s3_sse_concurrent_repro_test.go covers the parallel multipart-upload shape from the user report (5 blobs in parallel, full GET and chunked range GET both hash-checked) — pre-existing coverage; added here as a regression sentinel. * test(s3-sse): rename UploadPartCopy regression test so CI matches it The CI workflow .github/workflows/s3-sse-tests.yml dispatches on the TEST_PATTERN ".*Multipart.*Integration" — i.e. the test name must contain both "Multipart" and "Integration" for CI to run it. The previous name TestSSES3UploadPartCopyIntegration had only "Integration"; "UploadPart" isn't "Multipart". Rename to TestSSES3MultipartUploadPartCopyIntegration so the regression test actually runs in CI rather than only locally. * fix(s3api): map unsupported UploadPartCopy SSE source to 501, not 500 (review feedback on #9280) openSourcePlaintextReader explicitly rejects SSE-KMS and SSE-C sources (SSE-S3 is the only one wired up in this slow path so far). Earlier the caller blanket-mapped that to ErrInternalError, which collapses "this shape isn't implemented yet" into the same 500 response a real server failure would produce. Clients can no longer tell whether they hit a feature gap or a bug. Introduce a sentinel errCopySourceSSEUnsupported and have copyObjectPartViaReencryption errors.Is-check it; on match, return ErrNotImplemented (501) instead of ErrInternalError (500). Other failures still map to 500. Found by coderabbitai review on PR #9280. * fix(s3api): UploadPartCopy must fail with NoSuchUpload when upload entry is missing (review feedback on #9280) CopyObjectPartHandler's earlier checkUploadId call only verifies that the uploadID's hash prefix matches dstObject; it does not prove the upload directory exists in the filer. The previous logic silently swallowed filer_pb.ErrNotFound from getEntry(uploadDir) and fell through with uploadEntry=nil, which then skipped the destination SSE check and could route a plain-source copy through the raw-byte fast path even though the destination's encryption state is unknown. Treat ErrNotFound as ErrNoSuchUpload so the client sees the right status, matching the AWS S3 contract for UploadPartCopy on a non-existent upload. Found by coderabbitai review on PR #9280. * feat(s3api): set SSE response headers on UploadPartCopy slow path (review feedback on #9280) PutObjectPartHandler writes x-amz-server-side-encryption (and the KMS key-id header for SSE-KMS) on every successful part response so clients can confirm the destination's encryption state. The new UploadPartCopy slow path was missing this — it returned only the ETag in the response body and no SSE response headers. Plumb putToFiler's SSEResponseMetadata back through copyObjectPartViaReencryption to the handler, then call setSSEResponseHeaders before writing the XML response, matching the PutObjectPart contract. Found by gemini-code-assist review on PR #9280. * fix(s3api): map transient filer errors on UploadPartCopy upload-entry fetch to 503 (review feedback on #9280) Earlier non-ErrNotFound errors from getEntry(uploadDir, uploadID) all returned 500 InternalError, which most SDKs treat as fatal — even though a transient filer outage (gRPC Unavailable, leader election in flight, deadline exceeded) is exactly the kind of failure SDK retry logic is supposed to recover from. Add an isTransientFilerError helper that recognises: - context.DeadlineExceeded / context.Canceled - gRPC codes.Unavailable, DeadlineExceeded, ResourceExhausted, Aborted When the upload-entry fetch fails for one of those reasons, return 503 ServiceUnavailable so the client retries; everything else still maps to 500. Log line now also carries dstObject (in addition to dstBucket and uploadID) to make incident triage easier. Found by gemini-code-assist review on PR #9280. |
||
|
Chris LuandGitHub
|
7a461ffc2f |
fix(mount): copy xattr value bytes to avoid FUSE buffer aliasing (#9278)
fix(mount): copy xattr value bytes to avoid FUSE buffer aliasing (#9275) SetXAttr stored the caller-supplied `data` slice directly into entry.Extended. That slice aliases go-fuse's per-request input buffer, which is returned to a pool the moment the handler returns. When a file is open during setxattr (the open-fh path defers persistence to flush), the next FUSE request recycles the buffer and silently overwrites the stored xattr bytes; flushMetadataToFiler then ships the corrupted bytes to the filer. `cp -a` reproduces this because it issues a setxattr while holding an open fh, then continues to issue follow-up FUSE ops that reuse the same buffer. The path-based setxattr (e.g. setfattr without an open fh) saves synchronously inside the same handler, so the bytes were marshalled before the buffer could be reused — that is why the source file in the report looked fine and only the cp -a destination was garbage. Defensively copy the bytes when storing them, and add a unit test that mutates the caller buffer after SetXAttr returns to lock in the invariant. |
||
|
qzhelloandGitHub
|
108e42fb8b |
chore(shell): fix typo in EC shard helper param names (#9277)
* fix(shell): correct volume.list -writable filter unit and comparison * fix(shell): correct volume.list -writable filter unit and comparison * chore(shell): fix typo in EC shard helper param names |
||
|
Chris LuandGitHub
|
d9b86fb495 |
fix(s3api): clear stale latest-version pointer when .versions dir cleanup is blocked (#9269)
* fix(s3api): clear stale latest-version pointer when post-deletion cleanup is blocked In versioned buckets, when the only remaining children of a .versions directory are orphan entries (v_<id> files that lack the version-id extended attribute - e.g. left behind by older code paths or interrupted writes), updateLatestVersionAfterDeletion's selectLatestVersion finds zero promotable versions and falls through to s3a.rm. The non-recursive rm fails because the orphan blocks the directory deletion. Previously the code logged the failure and returned, leaving the LatestVersionId pointer pointing at the version we just deleted. For Veeam-style workloads that GET-PUT-GET-DELETE a small lock object on every checkpoint, the stale pointer poisons every subsequent run: the next GET re-enters getLatestObjectVersion's 13x retry loop on the missing pointer file plus the self-heal rescan, all to return the same 404. The cycle is self-perpetuating until the orphan is removed by hand. When rm fails, additionally clear the LatestVersionId / LatestVersionFile pointer fields on the .versions directory entry. The orphan files stay in place (an operator can audit and remove them); from the S3 API perspective the object is now correctly absent and subsequent reads short-circuit to ErrNotFound on the fast path instead of replaying the heal cycle. * fix(s3api): clear stale latest-version pointer on read-side self-heal failure healStaleLatestVersionPointer is invoked by getLatestObjectVersion when the pointed-at version file is missing. The rescan path can find no remaining version-id-tagged entries (e.g. when only orphan v_<id> files lacking the version-id extended attribute remain). Prior code returned ErrNotFound but left the stale pointer in place, so every subsequent read replayed the same 13x retry loop on the missing file and re-entered self-heal, all to return the same 404. Reuse the same pointer-clear logic introduced for updateLatestVersionAfterDeletion. The two call sites are now identical, so factor the body out into clearStaleLatestVersionPointer. The caller parameter carries the source function name so the log lines operators were already grepping (updateLatestVersionAfterDeletion: cleared stale ... and healStaleLatestVersionPointer: cleared stale ...) keep the same prefix. * fix(s3api): re-validate before clearing latest-version pointer (CAS) Reviewer feedback (gemini-code-assist, coderabbitai) on PR #9269 flagged a TOCTOU in clearStaleLatestVersionPointer: between the caller loading versionsEntry and this function persisting the cleared map, a concurrent PUT could promote a fresh version. Persisting the caller's snapshot then silently overwrites the live pointer and re-introduces the missing-pointer state on a now-existing object. Make the persist CAS-style: 1. Re-scan .versions for any version-id-tagged entry. If one now exists, abort - the concurrent writer has populated the directory and either already updated the pointer or the next read's self-heal will pick up the new entry. 2. Re-fetch the live .versions directory entry and only proceed if its latest-pointer fields still match the stale id the caller observed. A concurrent pointer update by another writer is detected here and the clear is skipped. 3. Persist with mkFile using the live Extended map (minus the two pointer fields and the cached metadata) so any other Extended fields written concurrently between (2) and the persist are preserved. A note on the literal suggestion of mutating updatedEntry.Extended in the mkFile callback: that does not work because mkFile constructs a fresh *filer_pb.Entry rather than reading the live entry first (weed/pb/filer_pb/filer_client.go:247). The callback's updatedEntry is nil at invocation, so a delete on it would be a no-op and we would lose all Extended fields, not just the two we want to clear. The correct shape - re-fetching the live entry before mkFile and carrying its Extended map into the persist - is what this change implements. True atomic CAS would require filer-level conditional update support; this change narrows the race window from the full caller scope to the ~ms gap between (2) and (3), which is the best we can do without that. |
||
|
chenshiandGitHub
|
c93018d987 | fix(s3api): fix uint16 overflow in doListFilerEntries Limit calculation (#9271) | ||
|
Chris LuandGitHub
|
35fe3c801b |
feat(nfs): UDP MOUNT v3 responder + real-Linux e2e mount harness (#9267)
* feat(nfs): add UDP MOUNT v3 responder
The upstream willscott/go-nfs library only serves the MOUNT protocol
over TCP. Linux's mount.nfs and the in-kernel NFS client default
mountproto to UDP in many configurations, so against a stock weed nfs
deployment the kernel queries portmap for "MOUNT v3 UDP", gets port=0
("not registered"), and either falls back inconsistently or surfaces
EPROTONOSUPPORT — surfacing as the user-visible "requested NFS version
or transport protocol is not supported" reported in #9263. The user has
to add `mountproto=tcp` or `mountport=2049` to mount options to coerce
TCP just for the MOUNT phase.
Add a small UDP responder that speaks just enough of MOUNT v3 to handle
the procedures the kernel actually invokes during mount setup and
teardown: NULL, MNT, and UMNT. The wire layout for MNT mirrors
handler.go's TCP path so both transports produce the same root
filehandle and the same auth flavor list for the same export. Other
v3 procedures (DUMP, EXPORT, UMNTALL) cleanly return PROC_UNAVAIL.
This commit only adds the responder; portmap-advertise and Server.Start
wire-up follow in subsequent commits so each step stays independently
reviewable.
References: RFC 1813 §5 (NFSv3/MOUNTv3), RFC 5531 (RPC). Existing
constants and parseRPCCall / encodeAcceptedReply helpers from
portmap.go are reused so behaviour stays consistent across both UDP
listening goroutines.
* feat(nfs): advertise UDP MOUNT v3 in the portmap responder
The portmap responder advertised TCP-only entries because go-nfs only
serves TCP, but with the new UDP MOUNT responder in place we can now
honestly advertise MOUNT v3 over UDP as well. Linux clients whose
default mountproto is UDP query portmap during mount setup; if the
answer is "not registered" some kernels translate the result to
EPROTONOSUPPORT instead of falling back to TCP, which is exactly the
failure pattern reported in #9263.
Add the entry, refresh the doc comment, and extend the existing
GETPORT and DUMP unit tests so a regression that drops the entry shows
up at unit-test granularity rather than only in an end-to-end mount.
* feat(nfs): start UDP MOUNT v3 responder alongside the TCP NFS listener
Plug the new mountUDPServer into Server.Start so it comes up on the
same bind/port as the TCP NFS listener. Started before portmap so a
portmap query that races a fast client never returns a UDP MOUNT entry
the responder isn't actually answering, and shut down via the same
defer chain so a portmap-or-listener startup failure doesn't leave the
UDP responder dangling.
The portmap startup log now reflects all three advertised entries
(NFS v3 tcp, MOUNT v3 tcp, MOUNT v3 udp) so operators can confirm at a
glance that the UDP MOUNT path is up.
Verified end-to-end: built a Linux/arm64 binary, ran weed nfs in a
container with -portmap.bind, and mounted from another container using
both the user-reported failing setup from #9263 (vers=3 + tcp without
mountport) and an explicit mountproto=udp to force the new code path.
The trace `mount.nfs: trying ... prog 100005 vers 3 prot UDP port 2049`
now leads to a successful mount instead of EPROTONOSUPPORT.
* docs(nfs): note that the plain mount form works on UDP-default clients
With UDP MOUNT v3 now served alongside TCP, the only path that ever
required mountproto=tcp / mountport=2049 — clients whose default
mountproto is UDP — works against the plain mount example. Update the
startup mount hint and the `weed nfs` long help so users don't go
hunting for a mount-option workaround that no longer applies.
The "without -portmap.bind" branch is unchanged: that path still has
to bypass portmap entirely because there is no portmap responder for
the kernel to query.
* test(nfs): add kernel-mount e2e tests under test/nfs
The existing test/nfs/ harness boots a real master + volume + filer +
weed nfs subprocess stack and drives it via go-nfs-client. That covers
protocol behaviour from a Go client's perspective, but anything
mis-coded once a real Linux kernel parses the wire bytes is invisible:
both ends of the test use the same RPC library, so identical bugs
round-trip cleanly. The two NFS issues hit recently were exactly that
shape — NFSv4 mis-routed to v3 SETATTR (#9262) and missing UDP MOUNT v3
— and only surfaced in a real client.
Add three end-to-end tests that mount the harness's running NFS server
through the in-tree Linux client:
- TestKernelMountV3TCP: NFSv3 + MOUNT v3 over TCP (baseline).
- TestKernelMountV3MountProtoUDP: NFSv3 over TCP, MOUNT v3 over UDP
only — regression test for the new UDP MOUNT v3 responder.
- TestKernelMountV4RejectsCleanly: vers=4 against the v3-only server,
asserting the kernel surfaces a protocol/version-level error rather
than a generic "mount system call failed" — regression test for the
PROG_MISMATCH path from #9262.
The tests pass explicit port=/mountport= mount options so the kernel
never queries portmap, which means the harness doesn't need to bind
the privileged port 111 and won't collide with a system rpcbind on a
shared CI runner. They t.Skip cleanly when the host isn't Linux, when
mount.nfs isn't installed, or when the test process isn't running as
root.
Run locally with:
cd test/nfs
sudo go test -v -run TestKernelMount ./...
CI wiring follows in the next commit.
* ci(nfs): run kernel-mount e2e tests in nfs-tests workflow
Wire the new TestKernelMount* tests from test/nfs into the existing
NFS workflow:
- Existing protocol-layer step now skips '^TestKernelMount' so a
"skipped because not root" line doesn't appear on every run.
- New "Install kernel NFS client" step pulls nfs-common (mount.nfs +
helpers) and netbase (/etc/protocols, which mount.nfs's protocol-
name lookups need to resolve `tcp`/`udp`).
- New privileged step runs only the kernel-mount tests under sudo,
preserving PATH and pointing GOMODCACHE/GOCACHE at the user's
caches so the second `go test` invocation reuses already-built
test binaries instead of redownloading modules under root.
The summary block now lists the three kernel-mount cases explicitly
so a regression on either of #9262 or this PR's UDP MOUNT change is
traceable from the workflow run page.
|
||
|
Chris LuandGitHub
|
735e94f6ba |
mount: expose -fuse.maxBackground and -fuse.congestionThreshold flags (closes #9258) (#9268)
* mount: expose `-fuse.maxBackground` flag (closes #9258) The Linux FUSE driver caps in-flight async requests via `/sys/fs/fuse/connections/<id>/max_background` (and a derived `congestion_threshold = 3/4 * max_background`). Heavy upload workloads need this raised, but the cap currently lives only in `/sys`, so it resets on reboot/remount. `weed mount` was hardcoding `MaxBackground: 128`. Promote it to a flag, default unchanged. Setting `-fuse.maxBackground=2048` reproduces the manual `echo 2048 > .../max_background` (and gives 1536 for congestion_threshold automatically) persistently across remounts. `congestion_threshold` is not exposed as a separate flag because go-fuse derives it as 3/4 of MaxBackground in InitOut and offers no hook to override; users wanting a different ratio can still write /sys/fs/fuse/connections/<id>/congestion_threshold post-mount. * mount: add `-fuse.congestionThreshold` flag, bump go-fuse to v2.9.3 go-fuse v2.9.3 exposes CongestionThreshold as a separate MountOption, so we can now let users override the kernel's default 3/4-of-max_background ratio at mount time instead of having to write /sys/fs/fuse/connections/<id>/congestion_threshold post-mount on every remount/reboot. Default 0 preserves existing behavior (kernel derives it as 3/4 * max_background). Non-zero is sent to the kernel verbatim; the kernel clamps it to max_background if higher. |
||
 Lisandro PinandGitHub
|
3f3aaa7cc8 |
Export Prometheus metrics for scrubbing operations. (#9264)
This PR introduces three new metrics... - `scrub_last_time_seconds` - `scrub_volume_failures` - `scrub_shard_failures` ...capturing overall volume scrub results, and allowing to construct alerts and dashboards to monitor scrubbing progress. Note that these metrics are aggregated at the volume/EC shard level, and not intended for fine-grained tracking of scrubbing operations. |
||
|
Chris LuandGitHub
|
294f7c3d04 |
shell: expand ~ in local file path arguments (#9265)
* shell: expand `~` in local file path arguments The weed shell parses commands itself instead of going through an OS shell, so a path like `~/Downloads/foo.meta` was passed verbatim to `os.Open`, which fails because no `~` directory exists. Users had to spell out absolute home paths in every command. Add an `expandHomeDir` helper that resolves a leading `~` or `~/...` to the user's home directory, and run user-supplied local file paths in the affected shell commands through it: fs.meta.load (positional file) fs.meta.save (-o) fs.meta.changeVolumeId (-mapping) s3.iam.export (-file) s3.iam.import (-file) s3.policy (-file) s3tables.bucket (-file) s3tables.table (-file, -metadata) volume.fsck (-tempPath) Filer-namespace path flags (`-dir`, `-path`, `-locationPrefix`, etc.) are unaffected; they live in the filer, not on the local FS. * shell: reuse util.ResolvePath instead of a new helper util.ResolvePath already does tilde expansion; drop the local expandHomeDir helper and route every shell call site through it. |
||
|
Chris LuandGitHub
|
e2c8791441 |
fix(nfs): reject NFSv4 calls with PROG_MISMATCH so clients fall back to v3 (#9262)
* feat(nfs): add NFSv3-only RPC version filter The upstream willscott/go-nfs library dispatches RPC calls by (program, procedure) only — it does not validate the program version. A client sending NFSv4 (prog 100003 vers 4 proc 1 COMPOUND) lands on the same handler map as NFSv3 and gets routed to v3 SETATTR, which parses the COMPOUND args as SETATTR3args and writes a malformed reply. The kernel then returns EPROTONOSUPPORT and mount.nfs prints "requested NFS version or transport protocol is not supported" without retrying v3. This commit adds a listener wrapper that peeks the first RPC frame on each new TCP connection. If the program is NFS or MOUNT and the version is not 3, it writes a protocol-correct PROG_MISMATCH reply (supported range 3..3, per RFC 5531) directly to the socket and closes the connection. v3 frames are replayed unchanged via a bufio reader so go-nfs sees the original bytes. Unknown programs pass through so go-nfs's own PROG_UNAVAIL handling stays in charge. The filter is not yet wired into the server; the next commit activates it. Tests cover NFSv4 reject, MOUNTv4 reject, NFSv3 pass-through, and unknown-program pass-through. * fix(nfs): wire NFSv3 version filter into the listener chain Place the version filter after the optional client allowlist so that unauthorized peers are still rejected first by IP/CIDR before we look at RPC content. With the filter active, a Linux client doing the default v4-first probe gets a clean PROG_MISMATCH reply pointing at v3, which lets mount.nfs (and the in-kernel client) skip v4 and reuse the same v3 mountOptions that already work for rclone serve nfs against this deployment. * test(nfs): exercise MOUNT v4 in the v4-rejection test, not v1 TestVersionFilterRejectsMOUNTv4WithProgMismatch was sending mountProgramID with version 1, so the test never actually covered the "reject MOUNT v4" path it claims to exercise. The filter does reject any non-v3 version uniformly, so the test still passed, but a future change that tightened the version check (for example, only rejecting v4) would let this test silently lie about coverage. Bump the call to version 4 so the name matches what is actually exercised. * refactor(nfs): reuse package RPC constants and io.ReadFull in version filter The RPC numeric constants (msg_type=CALL/REPLY, MSG_ACCEPTED, PROG_MISMATCH, AUTH_NONE, the NFS/MOUNT program numbers) are already named in portmap.go alongside the portmap responder. Reuse them here instead of defining a parallel set in rpc_version_filter.go: keeping one source of truth per package means a future correction in one spot can't drift away from the other. The filter-only constants (peek timeout, peek length, supportedNFSVer) stay local because they have no portmap analog. In the test, drop the bespoke readFull loop in favor of io.ReadFull. The custom version was a near-identical reimplementation that did not return io.ErrUnexpectedEOF on short reads, so the standard library is both shorter and more diagnostic-friendly. * fix(nfs): move RPC peek off the Accept path The previous wrapper called filterFirstRPCFrame inline inside versionFilterListener.Accept, which meant a single slow or idle TCP connect could hold rpcVersionFilterPeekTimeout (10s) of head-of-line blocking against every other accept: gonfs.Serve calls Accept serially, so each in-flight peek stalled the next legitimate client until the deadline expired. An attacker who simply opens a TCP connection without sending any RPC payload could trivially throttle accept throughput. Restructure the wrapper so a background goroutine drives the inner Accept loop and hands each raw conn to its own short-lived goroutine that runs the peek. Validated conns are sent on a buffered-once channel, which the wrapper's Accept reads from; rejected conns finish their PROG_MISMATCH reply and disappear without ever reaching the channel. This means N concurrent slow clients only block themselves, not the N+1th fast client that connects after them. Add Close coordination — sync.WaitGroup for the accept loop and per-conn peek goroutines, plus a closed channel so Accept unblocks immediately on shutdown — so the wrapper now satisfies the full net.Listener contract instead of relying on the embedded listener. Add a regression test that opens a slow conn (TCP only, never writes) and a fast conn (sends a v3 frame) and asserts the fast conn reaches the inner accept handler well below the peek timeout. * test(nfs): assert io.EOF (not just any error) after PROG_MISMATCH close The post-rejection check was only failing when conn.Read succeeded; any error — including a deadline timeout because the server kept the socket open — let the test pass. That defeats the point of the assertion: a regression where the filter replies but forgets to close would slip through silently. Match against io.EOF explicitly. The TCP semantics are deterministic here: the server writes PROG_MISMATCH, calls conn.Close(), the client reads what's left in flight and then sees a clean FIN, which surfaces as io.EOF on the next zero-byte read. * fix(nfs): reject short first fragments before parsing RPC header fields bufio.Reader.Peek(28) is willing to read across record boundaries to satisfy the requested length, so a final fragment whose body is shorter than the 24-byte fixed RPC CALL header (xid + msg_type + rpcvers + prog + vers + proc) leaves the trailing peek bytes pointing at the next RPC's framing or whatever bytes happen to follow on the wire. Indexing hdr[16:24] for prog/vers in that state can spuriously reject (or pass through) traffic based on data that doesn't belong to the request being classified. Drop those frames out of the filter early: if the first fragment can't possibly hold a full CALL header, pass the connection straight to go-nfs, which has its own framing-error handling for malformed input. Add a regression test that crafts a 12-byte first fragment whose trailing peek bytes are deliberately shaped like an NFSv4 CALL — without the length check the filter sends a PROG_MISMATCH; with it, the conn passes through silently. Verified by stashing the production-code change and running the test in isolation: it fails as expected without the fix. * fix(nfs): retry transient Accept() errors instead of treating any error as terminal acceptLoop previously exited on the first error returned by the inner listener's Accept(). That conflates two very different failure modes: permanent shutdown (the listener was Close()d, OS-level fatal failure) and transient resource pressure (EMFILE, EAGAIN, ECONNABORTED on accept). The transient case should not take the entire NFS server down — a single fd-table-full event would leave the deployment offline until restart. Classify the error: errors.Is(err, net.ErrClosed) is the permanent signal we already wanted to surface to Accept(); everything else is transient. Log at V(1) and back off rpcVersionFilterAcceptBackoff (50ms, mirroring portmap.go's portmapRetryBackoff) before retrying. The backoff sleep is interruptible via the closed channel so Close() still shuts the loop down promptly. Add a regression test that wraps a real listener with one that injects 3 fake transient errors before delegating, and asserts Accept() still delivers the next real connection. Verified the test fails on the old "any error is terminal" loop and passes with this change. * fix(nfs): only synthesize PROG_MISMATCH for ONC RPC v2 traffic The filter was rejecting any CALL-shaped record with prog=100003 or 100005 and vers!=3, regardless of the rpcvers field. If the caller is speaking some other protocol that happens to share the port — or just sending garbled bytes — pretending to be an NFSv3 server replying PROG_MISMATCH is misleading at best, and at worst fabricates a coherent RPC reply for traffic we don't actually understand. Add an rpcvers==2 check between the msg_type and prog/vers parses. Any non-v2 record now passes through to go-nfs, whose RFC 5531 §9 RPC_MISMATCH handling is the correct place to reject mis-versioned RPC. Regression test takes a normal v3 NFS CALL frame, overwrites the rpcvers field with 99, and asserts no PROG_MISMATCH-shaped reply lands on the client and that the conn is delivered to the inner accept handler. Verified the test fails on the previous code (filter still rejected on prog/vers alone) and passes with the guard in place. * fix(nfs): bound Close() latency by evicting in-flight prefilter conns Close() does wg.Wait() to drain handleConn goroutines, but each of those goroutines can be parked inside filterFirstRPCFrame's bufio.Peek for up to rpcVersionFilterPeekTimeout (10s) waiting for the very first RPC header. A client that completes the TCP handshake but never sends a byte therefore stretched shutdown by 10s per such conn — a real regression for stop/restart paths and for tests that just want to tear the listener down. Track raw (pre-peek) conns in versionFilterListener.inFlight as handleConn enters, untrack on exit, and have Close() forcibly close every tracked conn before wg.Wait. Closing the underlying conn breaks its Peek immediately, so handleConn returns within a single scheduler hop. trackInFlight also short-circuits if shutdown has already started, so a conn accepted after signalClose can't slip past the eviction. Black-box regression test opens 4 idle TCP-handshake-only conns, lets their handleConn goroutines settle into Peek, and asserts Close() returns under 2s. Verified: same test fails on the previous code with Close taking ~9.9s; passes here at ~100ms. |
||
|
Chris LuandGitHub
|
0fa0a56a5a |
filer(mysql): TLS hostname/SNI knobs + MariaDB upsert documentation (#9260)
* refactor(filer/mysql): set tls.Config per-instance via Connector instead of global registry
Replace the use of `mysql.RegisterTLSConfig("mysql-tls", ...)` and the
`&tls=mysql-tls` DSN suffix with a per-instance setup that assigns the
`*tls.Config` directly to `mysql.Config.TLS` and opens the database via
`mysql.NewConnector` + `sql.OpenDB`.
The driver's TLS-config registry is process-wide; if a second `MysqlStore`
were ever initialized with different TLS settings (e.g., a filer plus a
separately configured store) the second registration would silently
overwrite the first. The connector pattern keeps the TLS configuration
attached to the connector and avoids that global side effect.
Behavior is otherwise unchanged: TLS is enabled when `enable_tls=true`,
the same `ca_crt`/`client_crt`/`client_key` knobs are honored, and the
TLS minimum version remains 1.2.
* filer(mysql): use system root CAs when ca_crt is empty
Previously, enabling `enable_tls=true` without setting `ca_crt` returned an
unhelpful empty-path read error. Many managed MySQL/MariaDB providers serve
certificates that chain to a public CA already in the host's trust store, so
requiring an explicit CA bundle adds friction with no security benefit.
Leave `RootCAs` unset when `ca_crt` is empty so Go's `tls.Config` falls back
to the system trust store, matching the standard behavior of `mysql --ssl`.
Existing setups with `ca_crt` configured are unaffected.
Also wraps the CA read/parse errors with the file path for easier diagnosis.
* filer(mysql): fail loudly when client_crt / client_key are unreadable
The previous implementation called `tls.LoadX509KeyPair` and silently
discarded any error, falling back to a non-mTLS connection. A typo or
permissions problem in `client_crt` / `client_key` therefore appeared as a
confusing server-side handshake error rather than as a config error,
because the server was expecting a client cert that the filer never sent.
Treat the keypair as required when either path is set, and surface the
underlying load error with both filenames so the misconfiguration is
obvious. The default (both paths empty) is unchanged: no client cert is
sent.
* filer(mysql): add tls_insecure_skip_verify and tls_server_name knobs
When the filer connects to a MySQL/MariaDB cluster whose server
certificate's SAN does not match the connection address (common with
internal load balancers, IP-only connection strings, or self-signed
cluster certs), the TLS handshake fails with `x509: certificate is valid
for X, not Y`. There was previously no way to fix this short of reissuing
the cert.
Expose two new optional knobs on `[mysql]`:
- `tls_server_name` overrides the SNI / cert hostname used for
verification — the standard fix when the cert SAN is correct but the
connection address is not.
- `tls_insecure_skip_verify` disables verification entirely as an escape
hatch for testing or for clusters with no usable SAN.
Both default to off, so existing configurations continue to verify the
server certificate against the connection address as before.
* docs(scaffold/filer.toml): document mysql TLS knobs and MariaDB upsert override
- Document the new `tls_insecure_skip_verify` and `tls_server_name` options.
- Update the `ca_crt` comment to reflect that it is optional and that the
system trust store is used when the path is empty (matches the runtime
behavior in mysql_store.go).
- Reword the client cert comments to make the mTLS pairing requirement
explicit (both `client_crt` and `client_key` must be set together).
- Add a commented-out MariaDB / MySQL 5.7 alternative for `upsertQuery`,
noting that the default (`AS new` row alias) requires MySQL 8.0.19+.
* filer(mysql): drop redundant blank import of go-sql-driver/mysql
The package was imported twice: once with the `mysql` alias (used for
`mysql.MySQLError`, `mysql.Config`, `mysql.NewConnector`, etc.) and once
as `_` to register the driver. The named import already triggers
`init()` and registers the driver, so the blank import is dead weight.
|
||
|
Chris LuandGitHub
|
135af25b55 |
fix(grpc): require host match before routing dials to local Unix socket (#9254) (#9257)
* fix(grpc): require host match before routing dials to local Unix socket (#9254) resolveLocalGrpcSocket keyed the Unix-socket hijack on port alone, so a remote peer reusing a local gRPC port (e.g. a standalone `weed volume` defaulting port.grpc=17334 against a `weed server` whose in-process volume socket is also on 17334) had its inbound RPCs silently rerouted to the local socket. In a cross-DC replication=100 cluster this surfaced as persistent volume-grow failure: both AllocateVolume RPCs landed on the local volume server, the second returned "Volume Id N already exists!", the grow rolled back, and S3 PUTs to the new collection returned 500. Track a per-port set of host strings that count as "this machine" and require the dial host to be in that set before redirecting. Loopback aliases (localhost, 127.0.0.1, ::1, "") are always included so same-process dials via loopback still take the socket fast path. * test(grpc): cover empty-host bare-port dial in local-socket regression test The empty alias is registered explicitly so SplitHostPort outputs like ":17334" (which can occur when a caller dials a bare port) take the local-socket fast path. Add a case so that path is exercised. |
||
 Parviz MiriyevandGitHub
|
e2f96687ff |
fix(admin): use protocol-relative URLs for component links so HTTPS clusters don't break clicks (#9256)
* fix(admin): use protocol-relative URLs for component links Hardcoded http:// in admin UI templates breaks browser-initiated clicks to master / volume / filer / EC shard / Iceberg REST URLs whenever the target component runs HTTPS-only via security.toml [https.X] sections. The browser sends plain HTTP to a TLS-only endpoint and gets 400 "client sent an HTTP request to an HTTPS server". Same root pattern as #9227 (admin's own backend /dir/status fetch); this PR is the browser-facing equivalent. Replace fmt.Sprintf("http://%s...") with fmt.Sprintf("//%s...") and the JS-string '<a href="http://' with '<a href="//' so the browser uses the same scheme as the page hosting the link. Backwards compatible: - HTTPS-only deployments: links now work - HTTP-only deployments: identical behavior to before - Mixed: edge case, addressed by future per-component public-URL work Affected templates (9 files), each kept in lockstep with its generated _templ.go sibling so reviewers don't need to run templ generate: - weed/admin/view/app/admin.templ - weed/admin/view/app/cluster_filers.templ - weed/admin/view/app/cluster_masters.templ (Go templ + JS modal) - weed/admin/view/app/cluster_volume_servers.templ (Go templ + JS modal) - weed/admin/view/app/cluster_volumes.templ - weed/admin/view/app/ec_volume_details.templ - weed/admin/view/app/volume_details.templ - weed/admin/view/app/iceberg_catalog.templ - weed/admin/view/app/s3tables_buckets.templ 17 link constructions total, +32/-32 lines. * fix(admin): protocol-relative URLs in iceberg + s3tables JS overrides Per Gemini code review on this PR: the JS scripts in iceberg_catalog and s3tables_buckets templates overwrite the href attribute of the "Open Iceberg REST" links after page load, replacing the protocol-relative URL set by the templ render with a hardcoded http://<host>:<port>/v1/config. Apply the same protocol-relative fix to the JS template literals so they don't undo the templ-side change. Browser uses the page scheme (http or https) to fill in the protocol. Mirrored in iceberg_catalog_templ.go and s3tables_buckets_templ.go. * fix(admin): displayed Iceberg endpoint scheme follows page protocol Per CodeRabbit review on this PR: the on-page guidance text in iceberg and s3tables templates still showed a literal `http://` even after the clickable link was switched to a protocol-relative URL. In HTTPS-only deployments operators see `http://host:8181/v1` as the suggested endpoint, copy it, and get a broken connection. Wrap the scheme in <span id="iceberg-protocol"> (and the s3tables counterpart) and have the existing inline script set its innerText to window.location.protocol minus the trailing colon. Same pattern as the existing dynamic host substitution. Mirrored in *_templ.go so reviewers do not need templ generate. SQL/JSON code-block examples (CREATE EXTERNAL TABLE ... ENDPOINT 'http://...', "uri": "http://..." ) are intentionally left as-is — they are starter snippets users adapt to their environment, not clickable or copy-paste-into-runtime values. Happy to follow up with server-side scheme threading if requested. |
||
|
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) | ||
|
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 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
|
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 |
||
|
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 | ||
|
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()). |