mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-19 04:50:54 +02:00
08d5daf0c1dbbd947835ce26774253c19d1533c8
9916
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chris Lu
|
4ec564469a |
s3tables: hide inaccessible catalog resources (#11365)
* s3tables: hide inaccessible table buckets * s3tables: hide inaccessible namespaces * s3tables: hide inaccessible tables * s3tables: hide inaccessible resources in rename and namespace delete RenameTable/RenameView denied on the source now report the same not-found as a missing source, and the destination name conflict is checked only after destination authorization so a denied caller cannot distinguish an existing destination namespace or name from a missing one. DeleteNamespace denials use the same formatted message as a missing namespace. |
||
|
Chris Lu
|
66f1754896 |
s3: enforce dedicated Object Lock actions (#11362)
s3: enforce dedicated object lock actions |
||
|
Chris Lu
|
994e1f7d64 |
admin: replace Font Awesome with MIT-licensed icons (#11364)
admin: replace Font Awesome with MIT icons |
||
|
David Christopher
|
1a285c1334 |
filer: join shutdown paths before closing metadata store (#11363)
Serve can return when its listener closes while HTTP requests are still draining. The main path could then close the metadata store before those requests finish. Make signal, context, and Serve-exit paths join one shutdown sequence. Drain gRPC and HTTP concurrently with 15-second default limits, then close the store. Test both completion orders. |
||
|
Chris Lu
|
3ebc05930d |
s3: separate Object Lock configuration permission (#11361)
* s3: separate object lock configuration permission * test: synchronize manifest cancellation setup |
||
|
Chris Lu
|
0c7beec697 | server: add filer-specific disableHttp flag (#11360) | ||
|
David Christopher
|
a859f0a019 |
filer: preserve accepted metadata log records on shutdown (#11359)
fix: flush metadata log before closing filer store Serialize sealed-batch handoffs with shutdown, reject late appends, and wait for log-buffer workers before closing the filer metadata store. Cover queued writes, interval and explicit flushes, late-write rejection, and pending persistence with shutdown tests. |
||
|
Nguyễn Đăng Minh LựcandChris Lu
|
71f8128d75 |
shell: fs.verify -pruneEntries deletes entries whose needles are lost (#11338)
* shell: fs.verify -pruneEntries deletes entries whose needles are lost
* shell: harden fs.verify -pruneEntries guards; VolumeNeedleStatus returns NotFound for absent needles
* shell: resolve chunk manifests in fs.verify metadata path; require confirmed deletion before counting prunes
* shell: anchor fs.verify legacy missing-needle error matching
* shell: keep fs.verify metadata scan alive on manifest resolution failures
* shell: classify EC missing needles and keep manifest failures unverified
VolumeNeedleStatus now canonicalizes erasure_coding.NotFoundError to
codes.NotFound, so absent needles in EC volumes reach the prune path
through the same stable contract as regular volumes. The client-side
isNeedleMissingError keeps recognizing the legacy wrapped EC shape
("locate in local ec volume: ... needle not found") for mixed-version
clusters.
A chunk manifest that fails to resolve is now an entry-level
verification failure even when the raw top-level chunks are healthy:
the file is not fully readable without the manifest. Raw chunks are
still verified on a resolution failure so a missing top-level manifest
needle is classified and can be pruned. The per-entry logic is
extracted into resolveAndVerify for testability.
* shell: trim fs.verify prune comments
---------
Co-authored-by: Chris Lu <chris.lu@gmail.com>
|
||
|
|
1f037e48f9 |
s3: a list marker that sorts before the prefix excludes nothing (#11322)
* s3: a list marker that sorts before the prefix excludes nothing ListObjects `marker` and ListObjectsV2 `start-after` are a plain key cutoff: list the keys that sort after it. A marker that sorts before the prefix and is not under it therefore excludes no key carrying the prefix, and the listing must equal the one with no marker at all. normalizePrefixMarker treated every marker that does not start with the prefix as "something wrong" and the listing came back empty. Clients send this shape routinely: docker/distribution's S3 storage driver walks prefix "<root>/<path>/" with start-after "<root>" (its rootdirectory), so on SeaweedFS a registry walk saw an empty bucket. zot read that as "no repositories": /v2/_catalog was empty, GC/scrub/retention never saw a repo, and on restart its storage parse deleted every repository's metadata as "no longer in storage". listFilerEntries now lists as if no marker were given when the marker sorts before the prefix; the response still echoes the marker the client sent. A marker that sorts after the prefix's subtree is left alone: it may legitimately sit inside a partial-name prefix's match set, which normalizePrefixMarker already handles, and otherwise correctly lists nothing. Reproduce on 4.44 and 4.47: curl -s "$S/zot?list-type=2&prefix=zot/zot/&start-after=zot/zot/" # all keys curl -s "$S/zot?list-type=2&prefix=zot/zot/&start-after=zot" # KeyCount 0 curl -s "$S/zot?list-type=2&prefix=zot/zot/&start-after=a" # KeyCount 0 * s3: keep the prefix's own key excluded by a marker that names it Fold the before-prefix marker rule into normalizePrefixMarker, which now also derives prefixEndsOnDelimiter from the effective marker instead of each cursor rebuilding the expression. A marker equal to the prefix is no longer trimmed to a subtree cutoff: start-after "a/b/" with prefix "a/b/" excludes only the "a/b/" key, so the walk starts inside that directory and its children still list. Adds a listing-level test that walks the whole path for both start-after shapes a registry sends, and covers the new normalization cases. * s3: leading slashes do not hide a marker that names the prefix * s3: echo the V1 marker the client sent, not the walk's cutoff * s3: filter only the walk's cutoff from the V1 page, not the echoed marker * s3: skip the key an exclusive marker names as it streams --------- Co-authored-by: Zuse <be9c90a8-c104-4be2-b7a4-9f92eb833ac8@forge.local> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
beaf96a51d |
s3: cover object lock retention on version deletes (#11335)
* s3: cover WORM guarded version deletes * s3: trim version delete comments |
||
|
Chris Lu
|
93d4a6aefd |
s3: drain request body before error response (#11334)
* s3: drain request body before error response * s3: keep oversized request bodies drainable |
||
|
|
87332eb60b |
Cloud/remote storage & tiering: configurable multipart upload/download concurrency (#11319)
* pb: add multipart concurrency fields to RemoteConf and tier move requests RemoteConf gains upload_concurrency/download_concurrency (0 = client default); VolumeTierMoveDatToRemote/FromRemote requests gain a concurrency field (0 = backend default). * remote storage: honor RemoteConf upload/download concurrency in s3 and azure clients s3 client: ReadFile passes conf download_concurrency to the downloader, WriteFile uses upload_concurrency for the uploader; previously hard-coded 1 upload / 5 download parts. 0 keeps defaults. Same for azure client. * storage: plumb concurrency through backend interface and tier upload/download BackendStorage.CopyFile/DownloadFile take a concurrency hint (<=0 = backend configured default); s3 backend reads upload_concurrency/download_concurrency from scaffold config with parseConcurrency fallback, rclone updated to the new signature. Tier move gRPC handlers forward the request concurrency to the backend. * shell: -upload_concurrency/-download_concurrency for remote.configure, -concurrent for volume.tier remote.configure exposes upload/download concurrency persisted into RemoteConf; volume.tier move/evict commands forward -concurrent to the tier move requests. Documented in master-cloud.toml scaffold. * test: cover concurrency propagation in remote tier integration test * remote.configure: merge existing config on partial update Load the stored RemoteConf before saving so a partial update (e.g. only -upload_concurrency) preserves credentials, endpoints, and type instead of replacing them with new-config defaults. Only treat a confirmed ErrNotFound as a new configuration; propagate all other load errors so a transient filer failure does not overwrite stored settings. On a type transition, reset backend-specific fields to the destination type's new-config defaults rather than inheriting the old backend's empty values. Bound configured concurrency to a sane maximum. * remote storage: honor configured download concurrency in S3 and Azure ReadFileWithConcurrency now resolves a zero request override against the client's configured download_concurrency (new downloadConcurrency() helpers), so the remote-mount/cache read path honors RemoteConf.DownloadConcurrency instead of the hard-coded default. Azure also clamps the resolved value to math.MaxUint16 regardless of whether the fallback was used, preventing uint16 wraparound when a configured value exceeds 65535. * shell: rename -concurrent to -concurrency and validate tier transfer bounds Rename the -concurrent flag to -concurrency across volume.tier.upload, volume.tier.download, and volume.tier.compact to match the proto field and RemoteConf field names. Add validateTierConcurrency to reject values that would wrap int32 or exceed a 1024 cap before constructing the request. * server: clamp tier move concurrency in gRPC handlers Add clampTierConcurrency to both VolumeTierMoveDatToRemote and VolumeTierMoveDatFromRemote handlers so a direct gRPC caller cannot spawn an unbounded number of network workers. * trim verbose comments added with concurrency feature Remove redundant doc comments on the backend interface, rclone backend, s3_backend parseConcurrency, and test helpers that restated the obvious. * remote.configure: apply type defaults before re-parse so explicit flags win applyTypeDefaults ran after the second flag parse, overwriting explicit destination flags (e.g. -s3.region=eu-west-1) with new-config defaults. Move the type-transition default reset before the re-parse so user-supplied flags override the destination defaults. * remote.configure: only treat explicit -type as a type transition The first parse defaults -type to s3, so a concurrency-only update on an existing non-S3 config captured requestedType=s3 and wrongly triggered a type transition, resetting the stored backend to S3. Use fs.Visit to detect whether -type was explicitly supplied; an omitted -type keeps the stored backend. --------- Co-authored-by: Jack Meredith <9480542+jackusm@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
David ChristopherandChris Lu
|
e4ca0d09e7 |
s3: preserve versions for POST policy uploads (#11316)
* s3: preserve versions for POST policy uploads Route POST policy uploads through the existing version-aware write helpers and validate promoted Object Lock headers before writing. Return the generated version ID when versioning is enabled, return x-amz-version-id: null when versioning is suspended, and omit the header when versioning has never been enabled. * s3: reuse versioning helpers in POST policy handler Route the POST policy handler through the existing getVersioningState and isObjectLockEnabled helpers instead of open-coding the object-lock forces-versioning-enabled rule, matching the PUT path. Drop the x-amz-version-id: null response header for suspended versioning; the PUT handler omits it and the S3 PutObject sample response for suspended buckets does not include it. Trim the moved fileSize comment. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
dependabot[bot]andChris Lu
|
01433e801d |
build(deps): bump github.com/redis/go-redis/v9 from 9.21.0 to 9.22.0 (#11306)
* build(deps): bump github.com/redis/go-redis/v9 from 9.21.0 to 9.22.0 Bumps [github.com/redis/go-redis/v9](https://github.com/redis/go-redis) from 9.21.0 to 9.22.0. - [Release notes](https://github.com/redis/go-redis/releases) - [Changelog](https://github.com/redis/go-redis/blob/master/RELEASE-NOTES.md) - [Commits](https://github.com/redis/go-redis/compare/v9.21.0...v9.22.0) --- updated-dependencies: - dependency-name: github.com/redis/go-redis/v9 dependency-version: 9.22.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * test(redis_conf): track go-redis 9.22.0 default read timeout of 5s go-redis 9.22.0 raised the default ReadTimeout from 3s to 5s (part of the cross-SDK configuration alignment). Update TestUnsetKeepsGoRedisDefaults to expect the new default so the bump in #11306 stops failing CI. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
0e82b4e351 |
s3: populate Initiated timestamp in ListMultipartUploads (#11313)
* s3: populate Initiated timestamp in ListMultipartUploads ListMultipartUploads returned each upload with only Key and UploadId, omitting the Initiated timestamp. Clients such as GeeseFS rely on this field to expire stale uploads and crash on its absence. Set Initiated from the upload directory entry creation time so repeated listings preserve the original initiation time. * test/s3: verify Initiated timestamp in ListMultipartUploads Add an integration test that initiates a multipart upload, lists it, and asserts the Initiated field is populated and preserved across repeated listings rather than reflecting the listing time. |
||
|
Chris Lu
|
c997e54096 |
admin: default to 0.0.0.0 for authenticated HTTP, keep worker gRPC on loopback (#11314)
* admin: extract isFlagExplicitlySet helper from applyViperFallback No behavior change; the inline flag-visit check becomes a reusable helper so the upcoming bind-address default can share it. * admin: default to 0.0.0.0 for authenticated HTTP, keep worker gRPC on loopback PR #11185 made the admin HTTP server default to 127.0.0.1 to stop exposing the unauthenticated admin API on the network by accident. That also locked out operators who already authenticated with -adminPassword: their UI became unreachable from the network after upgrade unless they added -ip=0.0.0.0 (see #11303). An authenticated deployment is safe to expose, so auto-upgrade the -ip default to 0.0.0.0 when -adminPassword or [https.admin] mTLS is configured. The loopback default stays for the unauthenticated case, so the unauthenticated API is never exposed on the network. An explicit -ip is always honored. The worker gRPC control plane has no password auth (only mTLS), so it must not follow the HTTP upgrade. Give it a separate bind address that stays on loopback unless -ip is explicit, so adminPassword no longer re-exposes the unauthenticated worker stream. * admin: hint loopback-only bind in startup banner When the admin server binds to loopback (the default for the unauthenticated case), print a one-line hint that it is not reachable from other hosts and how to expose it. This helps operators who, after the #11185 loopback default, can no longer reach the UI from another machine quickly see the cause and the fix without reading the docs. * admin: keep worker gRPC on loopback, decouple from https.admin mTLS The worker gRPC auto-upgrade to 0.0.0.0 was gated on hasMTLS, which reads the https.admin (HTTP) mTLS config. The worker gRPC mTLS comes from grpc.admin + grpc.ca, a separate config, so: - https.admin mTLS without grpc.admin mTLS widened the worker gRPC to 0.0.0.0 unauthenticated (re-exposing the control plane), and - grpc.admin mTLS without https.admin mTLS left the worker gRPC on loopback, blocking authenticated remote workers. Drop the worker gRPC auto-upgrade entirely. The worker gRPC keeps the raw -ip value (loopback by default), matching the pre-existing behavior; an operator who wants remote workers sets -ip explicitly. Only the HTTP admin listener auto-upgrades to 0.0.0.0 when authenticated. Addresses review feedback on #11314 from Devin and Greptile. |
||
|
Chris Lu
|
02749c1192 |
s3api: configurable trusted-proxy allowlist for aws:SourceIp (#11302) (#11315)
* s3api: add TrustedProxies allowlist helper for aws:SourceIp extraction Introduces a policy_engine.TrustedProxies type that parses a comma-separated list of bare IPs and CIDRs (mirroring Guard.UpdateWhiteList) and extracts the client IP for aws:SourceIp condition evaluation. When the direct TCP peer is in the allowlist, X-Forwarded-For is walked right-to-left skipping trusted hops (then X-Real-Ip); otherwise the direct peer address is returned. This is the building block for restoring configurable forwarded-header trust removed in |
||
|
Nguyễn Đăng Minh LựcandChris Lu
|
ac03d3fd78 |
shell: warn when fs.mergeVolumes source holds only orphan needles (#11310)
* shell: warn when fs.mergeVolumes source holds only orphan needles fs.mergeVolumes traverses filer entries, so a source volume whose needles are all orphans — filer entries lost to a crashed write or a wiped filer store — produces only the plan header and exits 0: no move, no skip, no error. Operators read that as a successful merge while the real cleanup (volume.fsck) never runs, and dat>idx volumes keep coming back read-only after restarts. Count the source-volume needles seen during traversal and, when a plan source was never seen but its index still reports needles, print a warning pointing at volume.fsck. Dry-run warns too. * shell: make needle counting concurrency-safe and count manifest sub-chunks TraverseBfs runs its callbacks from five workers, so the plain needlesSeen map raced between source-heavy merges (fatal concurrent map writes). All increments now funnel through a mutex-guarded recordSeen closure. Manifest sub-chunks that live on planned source volumes are now recorded too — rewriteManifestChunk visits them (including dry-run and capacity-skipped ones) but previously never marked their source, which produced false 'orphan needles' warnings for sources whose chunks were all reached through manifests. * shell: extract sourceNeedleCounter so the concurrency test covers the production path The orphan-warning recording was a closure local to Do, so TestWarnUnreferencedSources_ConcurrentRecording could only exercise a test-local copy of it — a regression in the production mutex would pass the test. Lift the map and mutex into a sourceNeedleCounter type with record/count methods and use it from Do and the test, so the -race test now drives the actual recording path. Trim the verbose comments added with the warning while here. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
cf38c01978 |
admin: bind worker gRPC listener to -ip instead of wildcard (#11300)
* admin: bind worker gRPC listener to -ip instead of wildcard
The worker/plugin gRPC control plane called net.Listen("tcp", ":port")
directly, so it wildcard-bound every interface and ignored the -ip setting.
A cluster bound to loopback still exposed the unauthenticated
WorkerService/PluginControlService streams on 0.0.0.0. Bind through
util.JoinHostPort(bindIp, port) so the listener honors -ip like the
master, filer, and volume gRPC listeners.
* admin: warn when worker gRPC is exposed off loopback without mTLS
The worker gRPC stream has no password auth, so grpc.admin mTLS is the
only effective control once the listener leaves loopback. An operator who
sets -adminPassword and binds -ip=0.0.0.0 authenticates the HTTP API but
still exposes the unauthenticated worker control plane. Log a startup
warning naming the port and the mTLS knobs so the exposure is not silent.
* admin: address review on worker gRPC bind fix
- mini: reserve the admin gRPC port with util.JoinHostPort so an IPv6
bindIp (e.g. ::1) does not form an invalid unbracketed address and
lose the reservation.
- worker gRPC: track whether grpc.admin mTLS credentials actually loaded
rather than only whether they were configured, and gate the
non-loopback exposure warning on that. A cert/key that fails to load
now still warns instead of silently suppressing.
|
||
|
Bruce ZouandChris Lu
|
ea179963c0 |
filer: clean up manifest resolve error propagation and add webdav tes… (#11297)
filer: clean up manifest resolve error propagation and add webdav test (#78) Drop GitHub issue references from comments and trim verbose comments. Replace the viewFromChunksOrErr helper with the existing NonOverlappingVisibleIntervals + ViewFromVisibleIntervals at the stream call sites, and add a WebDavFile.Read regression test for the manifest resolution failure path. Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> |
||
|
github-actions[bot]
|
c507336000 | 4.47 | ||
|
Chris Lu
|
38c14d3c13 |
filer: apply SSRF guard to the lazy-remote fetch/list/delete paths (#11294)
* filer: add guarded remote-storage client builder hook for lazy fetch The lazy-remote fetch path (maybeLazyFetchFromRemote) resolved its remote-storage client through the unguarded shared cache, bypassing the SSRF chokepoint (BuildGuardedRemoteStorageClient) that the CVE-2026-73080 remediation wired into the volume, filer stream and s3 stream dial paths. Add a RemoteStorageClientBuilder hook on Filer plus conf-only lookups on FilerRemoteStorage, and route the lazy fetch through the builder when set (endpoint deny-list + DNS-rebinding-safe dialer), falling back to the shared cache otherwise. The filer server wires the builder in a follow-up. * filer: route lazy directory listing through the guarded remote client maybeLazyListFromRemote shared the unguarded client resolution of the fetch path, so a caller-supplied remote endpoint was dialed without the SSRF deny-list or rebinding-safe dialer. Resolve the conf and build the client through buildRemoteStorageClient so the same guard covers listing. * filer: route lazy remote delete through the guarded remote client maybeDeleteFromRemote issued outbound DELETE/RemoveDirectory requests through the unguarded client, giving a write-side SSRF to a caller-chosen endpoint. Resolve the conf and build the client through buildRemoteStorageClient so the endpoint deny-list and rebinding-safe dialer apply to the delete path as well. * filer server: wire the guarded remote client builder into the filer Set Filer.BuildGuardedRemoteClient to BuildGuardedRemoteStorageClient and forward AllowUntrustedRemoteEndpoints so the lazy-remote fetch, list and delete paths apply the same SSRF endpoint checks as the volume and streaming read paths. * filer: test lazy fetch honors the guarded remote client builder Add a regression test that sets BuildGuardedRemoteClient to a rejecting builder and asserts maybeLazyFetchFromRemote returns no entry without reaching the remote, covering the SSRF guard wired in the prior commits. * filer: skip remote client for local-only lazy deletes maybeDeleteFromRemote resolved and validated the mount's remote client before checking entry.Remote, so a local-only file (no Remote entry) under a mount whose endpoint the guard rejects failed to delete: the guard error aborted the metadata deletion, leaving a file that needs no remote operation undeletable. Move the local-only check ahead of client construction so only remote-backed files and directories pay the guard. * filer: build the guarded remote client inside the lazy singleflight The lazy fetch and list paths built the guarded client before their singleflight blocks, so concurrent requests for the same key each allocated a fresh SDK client and HTTP transport even though only one remote operation ran. Move client construction inside the singleflight so the deduplicated operation builds it once, matching the per-request guard semantics of the sibling streaming paths without the duplicate transport churn. * filer: test guarded rejection for the lazy list and delete paths Add regression tests that set BuildGuardedRemoteClient to a rejecting builder and assert the lazy list does not reach the remote, a remote-backed file delete is blocked, and a local-only file under a rejected mount still deletes (covering the local-only fix). * filer: decouple lazy guarded-client build from the first caller's context Building the guarded client inside the singleflight made concurrent fetches share the first caller's context. If that caller canceled while endpoint DNS validation was running, the builder returned an error and published a not-found result to other callers whose contexts were still valid. Build with context.WithoutCancel so the guard's DNS validation is not tied to any single caller's cancellation, matching the list path's existing decoupling for the remote operation itself. * filer: reject remote-storage confs that dial blocked endpoints at load The filer's lazy-fetch / lazy-list / remote-delete paths resolve remote storage clients by name from FilerRemoteStorage.storageNameToConf and dial them via remote_storage.GetRemoteStorage, which bypasses the SSRF deny-list the volume server (BuildGuardedRemoteStorageClient) and the filer's own direct-read path apply. A RemoteConf planted under /etc/remote with a loopback / private / IMDS S3 endpoint is reloaded into storageNameToConf on the next metadata-change event and then dialed on the next cache miss — server-side request forgery from the filer. Apply the volume server's SSRF deny-list at conf load time, the single chokepoint that populates storageNameToConf: - Add RemoteStorageConfValidator, injected into FilerRemoteStorage by the filer server (the filer package cannot import the server package). A conf that fails validation is dropped from storageNameToConf, so the name-based client resolution on the lazy paths returns "not found" instead of dialing the blocked endpoint. - Add ValidateRemoteConfForLoad in weed_server, which mirrors BuildGuardedRemoteStorageClient's gcs credential + endpoint checks (validateRemoteEndpoint via guardedRemoteClient) without building a client. allowUntrusted skips the check, mirroring the volume server opt-out (-filer.allowUntrustedRemoteEndpoints). - The filer server injects the validator at construction. A conf whose type dials a fixed provider host (no caller-supplied endpoint) passes; only caller-influenced endpoints are denied. * filer: skip DNS resolution in the load-time SSRF validator ValidateRemoteConfForLoad resolved hostnames during /etc/remote reload, so a transient DNS failure (2s timeout) dropped the conf from the fresh map that replaces the live map, disabling a working mount until the next metadata event. The build-time guard (BuildGuardedRemoteStorageClient) already re-resolves and re-validates the endpoint at dial time with the rebinding-safe dialer, so DNS at load is redundant for security. Split the static checks (scheme, IMDS hostnames, IP-literal blocked addresses, gcs credentials) into validateRemoteEndpointForLoad, which does no DNS. Hostname endpoints pass at load and are caught at dial if they resolve to a blocked address. This preserves fail-fast for statically-blocked confs (loopback IPs, IMDS hostnames) without letting transient DNS failures disable mounts. * filer: accept empty S3 endpoints in the guarded remote client builder guardedRemoteClient returned ok=true with an empty endpoint for a standard AWS S3 config (no custom S3Endpoint), so BuildGuardedRemoteStorageClient and ValidateRemoteConfForLoad rejected it with "remote endpoint is empty" — breaking standard AWS S3 mounts on the lazy paths and the sibling streaming read paths that already use the guarded builder. An empty endpoint is not caller-supplied: the AWS SDK derives the regional endpoint from the region, so there is nothing for the SSRF guard to validate. Return ok=false for empty S3-compatible endpoints so the builder falls through to the shared unguarded cache, matching the historical behavior for standard AWS S3. |
||
|
Chris Lu
|
92c379e5b4 |
filer: accept gcs credentials file paths in the guarded remote client builder (#11296)
* filer: accept gcs credentials file paths in the guarded remote client builder checkGcsCredentials rejected all filesystem paths, so a gcs mount configured with remote.configure -gcs.appCredentialsFile (which stores a path in GcsGoogleApplicationCredentials) was rejected by BuildGuardedRemoteStorageClient with "gcs credentials must be inline JSON". This broke existing gcs mounts on the volume, filer, and s3 remote-mount read paths that use the guarded builder. Read and validate the file content instead of rejecting the path, mirroring what the gcs client itself does in MakeWithHTTPClient. A path that does not exist or does not contain valid gcs credentials is still rejected before any client is built. guardedRemoteClient now reads the file to extract the token exchange URL for the SSRF deny-list, so the rebinding-safe dialer still guards the token endpoint. * filer: resolve gcs credential paths and avoid leaking file existence loadGcsCredentialsContent passed the raw credentials string to os.ReadFile, so a documented ~/path (as written by remote.configure -gcs.appCredentialsFile=~/...) was rejected because os.ReadFile does not expand ~. It also wrapped the os.ReadFile error, which includes the file path, exposing file existence to a caller who planted a conf with an arbitrary path. Resolve the path with util.ResolvePath, matching the gcs client's own behavior in MakeWithHTTPClient. Return a generic sentinel error on read failure so the path is not reflected in the error message. The credential type validation still runs on the file content, so a path that does not contain valid gcs credentials is rejected before any client is built. |
||
|
Chris Lu
|
bea10e269f |
iceberg/s3tables: confine stored metadataLocation to the authorized table bucket (#11292)
* iceberg: confine commit/transaction/view-update write paths to authorized bucket The create, register, and createView handlers already confine the client- supplied metadata location to the caller table bucket and reject ".." segments. The commit, create-on-commit, transaction, and view-update paths read the stored metadataLocation back from the catalog and skipped the same guard, so a location poisoned via the raw S3Tables UpdateTable API (which persists metadataLocation verbatim) could escape the caller bucket through a ".." segment that path.Join collapses in saveMetadataBlob. Add confineMetadataLocation and apply it after parseS3Location on every commit/update/transaction/view write path, mirroring the create/register/ createView check. Reject with 400 so a poisoned stored location fails the commit instead of writing into another tenant bucket tree. * s3tables: validate metadataLocation at the store layer The raw S3Tables API (CreateTable, RegisterTable, UpdateTable, CreateView, UpdateView) persisted the client-supplied metadataLocation verbatim with no bucket-confinement or traversal check, so a caller could store a location pointing outside its own bucket. The Iceberg REST gateway commit paths then read that stored value back and wrote through it. Add ValidateMetadataLocation and call it in every s3tables store handler that accepts a metadataLocation, rejecting locations whose bucket differs from the caller table bucket or whose path contains traversal segments. This prevents a poisoned location from ever being persisted, complementing the per-write-path guard added to the Iceberg commit handlers. * iceberg/s3tables: validate location before repair and after idempotency check Address review feedback: - Move the commit-path confinement check ahead of repairManifests so a poisoned stored location cannot reach manifest repair I/O before the commit is rejected. - Move ValidateMetadataLocation in CreateTable/CreateView to after the existing-resource check so idempotent retries that do not consume the requested location are not rejected for an unused bad location. - Assert HTTP 400 in the cross-tenant reproduction tests so an unrelated failure cannot satisfy them. * iceberg: confine staged metadata location before load in create-on-commit The create-on-commit path parsed the staged metadata location from the stage-create marker and called loadMetadataFile before validating that the staged bucket/path stay within the authorized bucket. Add the same confineMetadataLocation guard before the read so a tampered marker cannot direct a cross-tenant metadata read. * iceberg/s3tables: reject bucket-only metadata locations ValidateMetadataLocation and confineMetadataLocation accepted s3://bucket with an empty table path. metadataDirPath then maps every such table to the shared <TablesPath>/<bucket>/metadata directory, so tables could overwrite or read each other's metadata files. Require a non-empty table path in both validators; the empty-location case (where the catalog derives one) is unaffected. * iceberg/s3tables: reject slash-only table paths in location validation s3://bkt/// parses to tablePath="/" which passed the empty-string check but path.Join cleans it away, mapping to the bucket-level metadata directory shared across tables. Update isValidTablePath to require at least one non-empty segment and mirror the same check in ValidateMetadataLocation, closing the gap in all callers. |
||
|
Chris Lu
|
10c0857476 |
s3: gate internal LifecycleDelete gRPC behind admin Bearer auth (#11291)
* s3/lifecycle: attach admin Bearer token on internal LifecycleDelete clients Export credential.WithS3InternalAdminAuth (renamed from withIamCacheAdminAuth) and use it in the worker and shell lifecycle RPC adapters so lifecycle calls carry the same admin token the IAM-cache propagation already attaches. No-op when jwt.filer_signing.key is unset, matching the server-side checkAdminAuth. Prepares the internal clients for the server-side auth gate that follows. * s3/lifecycle: gate LifecycleDelete behind admin Bearer auth Add checkAdminAuth to LifecycleDelete, matching the SeaweedS3IamCache handlers on the same internal gRPC listener (PR #11190). No-op when jwt.filer_signing.key is unset; rejects unauthenticated callers when it is. The internal worker/shell clients already attach the token in the previous commit. |
||
|
Nguyễn Đăng Minh LựcandChris Lu
|
c462fffce6 |
master: name the unlabeled disk layout plainly in assign errors (#11290)
* master: name the unlabeled disk layout plainly in assign errors When no volume server serves the layout an assign targets, the error named the empty disk type as "hdd" (HardDriveType is the empty string), sending operators looking for servers labeled hdd when the actual mismatch is labeled (e.g. -disk=ssd) servers versus unlabeled clients. - describe the layout as "default (unlabeled)" when the disk type is empty, keep %q naming for labeled types - log the unserved-layout condition once per option instead of letting every failing write repeat an unactionable line Observed in production: volume servers started with -disk=ssd while CSI mounts assign with the unlabeled layout; the per-write error stream pointed at a nonexistent hdd fleet. * master: bound and expire the unserved-layout warning dedupe The dedupe map retained every distinct option key permanently. Option keys embed request-derived fields (collection, disk type), so repeated assignments with distinct options would grow master memory without bound, and a retained key suppressed the warning if the same option went unserved again after the topology recovered. Remember last-warned timestamps instead, expiring after an hour, with a hard cap that resets the set when a client-driven key flood fills it. * master: silence per-retry unserved-layout log and name explicit hdd Addresses Devin Review comments on #11290. - The unserved-layout branch already rate-limits its warning via assignUnservedLayoutWarning.Do, but the common epilogue still logged lastErr at V(0) on every retry, so the flood the dedup was meant to stop continued. Skip the epilogue log when the unserved-layout branch owns the logging; the error is still returned to the client. - describeDiskLayout took the canonicalized option.DiskType, but ToDiskType folds both "" and "hdd" into HardDriveType, so an explicit disk=hdd request was mislabeled "default (unlabeled)". Pass the original request disk type instead: only an empty request is the unlabeled default; an explicit hdd is named "hdd". Adds TestAssignFailsFastNamesExplicitHdd covering the explicit-hdd wording. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
99d2479528 |
fix(vacuum): batch fsync in makeupDiff to prevent test timeout (#11289)
makeupDiff called dstDatBackend.Sync() (fsync) per needle in the loop over incrementedHasUpdatedIndexEntry. With 20000 entries in TestLDBIndexCompaction this resulted in up to 20000 fsync calls, which on slow CI disks exceeded the 10-minute test timeout. Batch the sync: write all needles/tombstones first, then fsync the dat file once in the defer alongside the existing idx fsync. The durability guarantee is unchanged — both files are still synced before CommitCompact writes the .cpc commit marker and swaps the files. |
||
|
|
8db41d0217 |
[Mount] Cache Chunk Manifest Resolution for Repeated File Opens (#11266)
* cache resolved chunk manifests for Mount * Address PR review: per-mount cache, singleflight, reuse ResolveOneChunkManifest - Own the manifest cache per WFS mount instead of a process-global variable, so manifests from one filer backend are never served to another (Devin/CodeRabbit major bug). - Coalesce concurrent cold misses via singleflight so only one fetch runs during a cold burst (Greptile P2). - Copy cached data after releasing the mutex so a large copy does not block concurrent hits, inserts, and evictions (CodeRabbit nitpick). - Reuse the existing ResolveOneChunkManifest function name instead of introducing a new resolveOneChunkManifest wrapper. - Validate (unmarshal) manifest bytes before caching so malformed manifests do not poison the cache. - Add TestChunkGroupManifestResolutionCoalescesColdMisses covering the singleflight cold-miss path. * Address round 2 review: coalesced-miss cancellation, test overlap - Use singleflight.DoChan in fetchOrLoad and select on ctx.Done() so a caller whose context is canceled while waiting for an in-flight fetch returns ctx.Err() promptly instead of blocking for the leader's result (Devin BUG). - Add TestResolveOneChunkManifestCanceledWaiterReturnsDuringCoalescedMiss covering the canceled-waiter path. - Delay the cold-miss fixture response so the leader's fetch is still in flight when concurrent opens join the singleflight, making the one-fetch assertions reliable (CodeRabbit Minor). * Address review: keep ResolveOneChunkManifest four-argument Restore the exported ResolveOneChunkManifest to its original four-argument signature so external callers keep compiling. Move the cache-aware resolution into an unexported resolveOneChunkManifest helper that accepts the per-mount ChunkManifestCache. The exported function delegates to the helper with a nil cache, preserving the historical uncached behavior for every non-Mount caller. The Mount path (ChunkGroup.SetChunks) now calls the unexported helper with the mount-owned cache. Tests and benchmarks that exercise the cache path call the unexported helper directly. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> |
||
|
Bruce Zou
|
eb6a7e93ca |
Fix mount eio on manifest resolve failure (#11287)
* mount: fail reads with error when chunk manifest resolution fails When SetChunks fails to resolve a chunk manifest (e.g. the volume is on a remote tier with reads disabled), the sections map stays empty and readDataAtSequential/readDataAtParallel zero-fill every missing section as if it were a sparse hole. Reads then return all-zero data with no error, so a plain cp of a large manifest-based file silently produces a completely zero-filled file. Remember the resolve error in ChunkGroup (guarded by sectionsLock) and return it from ReadDataAt. A later successful SetChunks clears it. Fixes the mount path of #11286. * filer: propagate manifest resolve errors in streaming read paths ViewFromChunks discards the chunk manifest resolve error returned by NonOverlappingVisibleIntervals. On failure the chunk views come back empty, and the streaming paths zero-fill the entire requested range, serving HTTP 200 / WebDAV 200 responses whose body is all zeros. Propagate the error in PrepareStreamContentWithThrottler, PrepareStreamContentWithPrefetch and the WebDAV read path so these requests fail with 500 instead. Fixes the filer HTTP and WebDAV paths of #11286. * mount: fail lseek with EIO when chunk manifest resolution fails SearchChunks still consulted the stale section map after SetChunks recorded a manifest resolution failure, so SEEK_DATA/SEEK_HOLE would describe the unresolved regions as sparse holes or return ENXIO. Return the recorded error from SearchChunks and map it to EIO in Lseek. Also add regression tests for the stream preparation error paths. Addresses review feedback on #11287. |
||
|
|
5b2fe374fc |
[Volume] Scrub every disk's EC shards for a volume id, not just the first (#11258)
* storage: add Store::find_all_ec_volumes for split-disk EC lookups Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: add merge_ec_runtimes to resolve a vid's per-disk shard set Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: replace dead slots.get(14) assertion with a width-14 pin Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: build the checksum scrub plan from every per-disk runtime Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: build the local scrub plan from every per-disk runtime Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: prove the local scrub plan reaches every runtime's slots Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: make the scrub plan tests falsifiable Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: report unverifiable protection when the sidecar predates the scrubbed encode Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: commit sidecar provenance with the sidecar it describes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: scrub every disk's EC shards for CHECKSUM and LOCAL Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: run the FULL/READS parity check across split-disk shards Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: report fenced-out runtimes in FULL/READS scrubs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: tighten verify_ec_shards ordering and missing-shard coverage Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: visit each EC volume id once in node-wide scrubs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: cover split-disk scrub aggregation end to end Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * volume server: pin fenced-out disks and sibling-disk shards in EC scrubs Three scrub behaviors shipped without a test at the RPC seam. Task 8 showed the seam exists, so close them here. FULL/READS (mode 2|5) now marks a volume broken when the identity fence excludes a runtime, where it previously reported clean. Pinned against a control fixture whose two disks AGREE and scrub clean, so the test fails on the clean->broken transition, not only on the message text. That needs a structurally valid, tombstone-only .ecx (so the needle walk finds nothing to complain about) and a seeded shard-location cache (so the absent master does not short-circuit the scrub with an error of its own). LOCAL (mode 3) and CHECKSUM (mode 4) now build their plans from every per-disk runtime. Made observable by moving shard 0 -- the shard the volume's single needle spans and the one the checksum sidecar is checked against -- to the SIBLING disk, leaving shard 5 on the disk the singular find_ec_volume lookup returns. Built from that disk alone, neither scrub ever looks at shard 0. The split-disk fixture grows a config struct rather than more positional arguments; its defaults reproduce the existing layout byte for byte, so the node-wide dedupe test is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: report fenced-out disks on a malformed sidecar too `errors.extend(self.skipped)` sat below the whole status match, so only `(Some(p), On)` ever reached it. The Invalid arm already returns a non-empty error vector of its own, so the Go-parity contract that silences the Off arm (`case BitrotOff: return 0, nil, nil`) does not reach it -- appending the fence lines there costs nothing that contract protects. A volume with BOTH a malformed sidecar and a disk the identity fence excluded reported only the sidecar, hiding the unscanned disk behind an unrelated integrity error. Off stays byte-identical, and so does the `(None, On)` arm that is documented as treating a missing payload defensively as protection off. Off is now the ONLY status that drops the report, and the comment at the On-path copy says so: that is the one place the parity constraint costs us coverage. Also corrects a false claim in the FULL/READS test's doc comment. It said a fenced-out disk "is a disk this scrub did NOT read", which is true only of the merge-driven parity half. The per-needle walk still resolves `store.find_ec_volume` (store_ec.rs:281) and binds `expected_encode_ts_ns` to that runtime (:311) -- position 0, the EXCLUDED one on that fixture -- so `read_local_intervals`' generation filter (:1204) makes it read the excluded disk and treat the anchor's shards as non-local, the inverse of what `skipped` reports. The fixture's tombstone-only .ecx walks nothing, so the test cannot tell the two apart; the comment now says that rather than implying coverage it does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: take CHECKSUM's bitrot protection from the disk that has the sidecar `EcChecksumScrubPlan::for_volumes` read `(prot, status)` off the ANCHOR. The anchor is the first shard-bearing runtime at the maximum `encode_ts_ns`, chosen with no regard for which disk holds the `.ecsum`. That sidecar is deliberately NOT mirrored across disks -- `ec_metadata_dirs()` exists so one authoritative copy stays reachable rather than being duplicated -- and at mount `EcVolume::new` resolves it via `load_active_bitrot_sidecar(&[])` with no sibling directories at all; only the `VolumeEcShardsMount` RPC ever passes `ec_metadata_dirs()`. So after EVERY volume-server restart, the split-disk runtime that does not physically hold the sidecar mounts `BitrotStatus::Off`. When the one copy lives on disk 1 and the anchor is disk 0, `run()` hit `case BitrotOff` and returned `(0, [], [])`: the whole volume scrubbed clean, silently. That is the steady state for roughly half of all mirrored split-disk layouts, and it is the exact failure this branch exists to remove. Source protection from the first MERGED runtime that has any -- `On` if one does, else `Invalid`, else the anchor's `Off`. Two facts make that safe, and both are load-bearing: - Every runtime that mounted `On` already passed the `geometry_matches` gate in `load_bitrot_for_generation`, so its manifest agrees with the volume's layout. A sidecar that contradicted it would have failed the mount. - All merged runtimes share the same `encode_ts_ns` by construction of the identity fence, so a sidecar from any of them describes the same encode run. The `unverifiable_sidecar` provenance rule four lines down read `anchor.bitrot_source_dir`; it now reads the SAME runtime `prot` came from. Otherwise the two would describe different sidecars and the rule would vouch for a manifest nobody is scanning against. One consequence worth naming: that source dir is now non-empty by construction (a runtime with protection found a file), where the anchor's was often "" and short-circuited the rule -- so on a fenced volume whose anchor had no sidecar, an unverifiable-protection note now surfaces where previously nothing was reported at all. `run()` is untouched, and the `BitrotStatus::Off` arm still returns `(0, [], [])` exactly, for Go parity with `case BitrotOff: return 0, nil, nil`. `parity_shards` still comes from the anchor while `prot` may come from a sibling; the geometry gate above makes them agree, and slot-width agreement is handled separately. The test drives mode 4 through the real RPC against a split-disk volume whose sidecar exists only on dir1, and asserts up front that the anchor mounted `Off` and the sibling `On` -- otherwise it would prove nothing. Reverting this commit's one-line source change makes it report `[]` instead of `[0, 5]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: pin the slot width, contain the shard-size fallback, and cover multi-disk FULL Five findings from the whole-branch review, none of which changes what a healthy volume reports. Slot width was undefined and the two consumers disagreed (ec_volume.rs). `merge_ec_runtimes` sizes `slots` to the WIDEST merged runtime, but the identity fence keys on `encode_ts_ns` alone and never on geometry -- so two same-generation runtimes whose `.vif`s disagree do merge. The mode 2|5 arm truncates to the anchor's `data+parity` and silently drops the surplus slots, while `EcChecksumScrubPlan::for_volumes` iterated the full width and emitted "present but missing from sidecar manifest" for exactly those ids. Nothing in the volume describes them -- the sidecar manifest and the Reed-Solomon matrix are both the anchor's -- so that message was the width disagreement talking, not a finding. The `slots` field doc now states the contract (the range is the anchor's geometry; every consumer truncates to it) and CHECKSUM truncates. The LOCAL `shard_size` fallback had grown a node-wide blast radius (ec_volume.rs). `anchor.shard_file_size()` returns the anchor's FIRST held shard, not a maximum. Before aggregation the plan read only that runtime's own shards, so a truncated shard was contained to its disk; now that one value sizes every merged sibling's shards, mis-offsetting `locate_data` and manufacturing needle corruption across the node. Take the max over the merged slots, which is how `verify_ec_shards` already answers the same question (`if size > shard_size { shard_size = size }`). Only on the legacy `dat_file_size == 0` path. Multi-disk `all_local` had no end-to-end test (grpc_server.rs). The parity check is gated on every shard being present, and the one all-local fixture keeps them in a single directory, so every entry of `dirs` is the same string and a permutation or off-by-one in the `slots` -> `dirs` mapping is invisible; `test_verify_ec_shards_reads_shards_from_multiple_dirs` builds its `dirs` by hand and never goes through `merge_ec_runtimes`. The new fixture is a real 10+4 encode split 0..=6 / 7..=13 across two store locations (the `.dat`/`.idx` stay outside both, so `prune_incomplete_ec_with_sibling_dat` has nothing to act on), driven through the real RPC: clean first, then a corrupted PARITY shard on the SECOND disk -- which only the parity half can see, and only through a correct mapping. Shifting that mapping by one, or computing `all_local` from the anchor alone, both make it report `[]` instead of `[13]`. Deleted `test_ec_volume_enumeration_is_deduped` (store_ec_reconcile.rs). It built `raw` from `store.locations` and then applied its OWN inline `filter(|v| seen.insert(*v))`, asserting on that -- a property of `HashSet::insert`, never reaching the production dedupe. That path is covered by `test_scrub_ec_volume_node_wide_dedupes_a_split_disk_volume`, which does fail (2 != 1) when the dedupe is removed. Corrected `test_verify_ec_shards_treats_a_none_dir_as_missing`'s docstring (ec_encoder.rs). It claimed the unmounted shard "must not drag the shards that ARE mounted down with it", but `dirs[5] = None` puts shard 5 in `broken_shards` before the block loop, so every iteration takes the `read_failed` arm and the parity comparison never runs: corrupting a mounted shard in that fixture changes nothing about the result. The assertions are unchanged; the docstring now states what they actually establish. Also refreshed two comments that cited `shard_file_size() - 1` as the reason `merge_ec_runtimes` prefers a shard-bearing anchor -- true before this commit, stale after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: correct the Fix 1 rationale and truncate the shard-size scan The safety argument attached to `EcChecksumScrubPlan::for_volumes`'s protection selection was false as written, and it is the argument a reviewer reads first. `geometry_matches` compares a sidecar against the MOUNTING runtime's own data/parity/block size, not the anchor's, and returns true vacuously when `ec_shard_config` is `None` -- so it establishes agreement only when all merged runtimes share one geometry, which an `encode_ts_ns`-only fence does not guarantee and which `test_checksum_scrub_truncates_slots_to_the_anchors_geometry` constructs a counterexample to. The second clause was weaker than stated too: a `.ecsum` records no encode identity at all, so merged runtimes agreeing on `encode_ts_ns` does not transfer to the sidecar. Replace it with the property that is true, checkable from the selection itself, and stronger for what actually matters. `anchor` is an element of `merged`, so the `.unwrap_or(anchor)` fallback is reached only when no merged runtime is `On` and none is `Invalid` -- in which case the anchor is necessarily `Off`. The status can therefore only move `Off -> On`, `Off -> Invalid` or `Invalid -> On`; never `On -> Off`, never `Invalid -> Off`. This selection cannot stop a volume that was being scanned from being scanned, and cannot turn a reported integrity error into silence: every change it makes is toward more verification. The comment now also states what it does NOT establish -- geometry agreement is not guaranteed -- and names geometry fencing as the follow-up that would close it. Second, `EcLocalScrubPlan::for_volumes`'s `shard_size` max scanned the FULL slot width, violating the `slots` contract documented in the same commit that introduced the max: the volume's shard-id range is the anchor's geometry and every consumer must truncate to it. Pre-fix that input could not exist, because `anchor.shard_file_size()` read only the anchor's own anchor-sized vector -- so the max opened a new, narrow path to the same node-wide mis-sizing it exists to close (same-generation runtimes with disagreeing `.vif`s, the wider one holding an out-of-geometry shard larger than the in-geometry ones, `dat_file_size == 0`). `.take(anchor.data_shards + anchor.parity_shards)` mirrors the truncation already applied to the CHECKSUM shard scan. The sibling `shards:` vector is left untruncated on purpose: every access in `EcLocalScrubPlan::run` is `shards.get(sid)` with `sid < data_shards`, so the surplus entries are inert. No behavior change for any healthy volume, and no test added -- the suite is unchanged at 575 passing, 0 failing, 0 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUf2cmVKHNhAZPTNv39rDE * ec: aggregate split-disk runtimes in Go scrubs, mirroring Rust Go volume scrubs previously used FindEcVolume (first runtime only), so a volume whose EC shards are split across multiple disks was scrubbed against just one disk's shards and the others were silently skipped. Node-wide ScrubEcVolume also appended each disk's EcVolumeIds without deduplication, scrubbing a split-disk volume once per disk. Add MergedEcRuntimes/MergeEcRuntimes (Go counterpart to Rust's merge_ec_runtimes): select the maximum EncodeTsNs as the anchor generation, fence out runtimes whose encode generation or geometry (DataShards, ParityShards, BlockSize) disagrees with the anchor, merge shard handles by shard ID, and report excluded runtimes rather than dropping them. Wire it into every scrub mode: - INDEX: scrub the anchor's index, report skipped runtimes. - LOCAL: aggregate local shards across all merged runtimes via a synthetic EcVolume built from the merged shard slots. - FULL/READS: resolve the runtime matching the anchor's encode generation (not the first match) so the needle walk and parity phase inspect one encode run; report skipped runtimes. - CHECKSUM: take bitrot protection from the first merged runtime that has a valid sidecar (On, else Invalid, else anchor's Off), preserve invalid sidecar errors from every other merged runtime, and report skipped runtimes. Deduplicate EC volume IDs in node-wide ScrubEcVolume so each volume is scrubbed exactly once. Refactor ScrubEcVolume to share the per-needle walk via scrubEcVolumeWalk, called by both the legacy first-runtime path and the new merged path. Add Go regression tests covering split-disk deduplication, encode-generation fencing, geometry fencing, sibling-disk LOCAL reach, and merge anchor selection. Rust: keep the previously-landed merge/fence/checksum changes intact; revert incidental cargo-fmt drift from unrelated files so the diff stays focused. * ec: fence merged CHECKSUM on sidecar encode generation and fix legacy shard size Address two review findings on the Go merged-runtime scrub: 1. Sidecar provenance: a merged runtime can load a bitrot sidecar from a sibling metadata directory (ReloadBitrotSidecar), and the merge fence may then exclude the runtime owning that directory. Generation-0 sidecars do not identify the encode run, so geometry validation alone cannot prove the borrowed manifest describes the anchor shards. If the sidecar records a non-zero EncodeTsNs that disagrees with the anchor, refuse the scan instead of applying stale checksums to current shards and reporting false corruption. 2. Legacy shard size: for volumes without datFileSize in .vif, LocateEcShardNeedleInterval derives the shard size from Shards[0].ecdFileSize. The merged shard set is compacted in shard-ID order, so a truncated lowest-ID shard would shrink every interval and misread intact sibling shards. Synthesize a datFileSize from the maximum mounted shard size when the anchor lacks one, so the datFileSize>0 path uses the largest shard size across all merged runtimes. * ec: fix copylocks, legacy shard boundary, and encode-aware Rust lookups Address review findings from CodeRabbit and Devin: Go (ec_volume_merge.go): - Remove bitrotLock copy from the synthetic EcVolume: copying a sync.RWMutex is a go vet copylocks error. The synthetic volume uses its own zero-value mutex; bitrot/bitrotStatus are set directly before ChecksumScrub reads them via BitrotProtection(), so no concurrent access occurs. - Fix legacy shard-size boundary: synthesize datFileSize from (maxShardSize - 1) * DataShards, not maxShardSize * DataShards, to match the legacy fallback in LocateEcShardNeedleInterval (ecdFileSize - 1). An exact large-block boundary is ambiguous; the unadjusted size would select an extra large row and misread intact sibling shards. Rust (store_ec.rs): - Add find_ec_volume_for_scrub helper that resolves by encode generation (not first-match find_ec_volume) and use it in scrub_snapshot_under_lock, write_back_shard_locations, and the post-refresh shard-location read. Previously the encode-aware lookup was only used for the initial runtime selection; the cache write-back and per-needle snapshot still used first-match, so a split-disk volume whose first runtime was from an older encode run would write to and read from the wrong runtime's shard-location cache and falsely abort with 'remounted as a different encode run'. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
c46f82d29a |
fix(master): stop goraft server on shutdown and bump raft to v1.2.1 (#11284)
MasterServer.Shutdown only stopped the Hashicorp raft implementation; when using the default goraft backend, the raft event-loop goroutine (leaderLoop/followerLoop) kept running after the master shut down. In the in-process test harness this leaked goroutines across sequential test runs, and a stale event occasionally reached a leader at term 0 and tripped the goraft "leader.elected.at.same.term" assertion, crashing the whole test binary (CI run 34670959967, PR 11279). Stop the goraft server in Shutdown() so its goroutines exit cleanly, and bump seaweedfs/raft to v1.2.1 which replaces that assertion with a graceful step-down to Follower instead of a panic. |
||
|
Chris Lu
|
5a0e017457 |
s3: reject virtual-host bucket retargeting via X-Forwarded-Host (#11281)
* s3: reject virtual-host bucket retargeting via X-Forwarded-Host SigV4 verification tries the client-supplied X-Forwarded-Host as a signed host candidate, while routing and IAM select the bucket from the actual Host header. A presigned URL for one virtual-host bucket could therefore be retargeted to another bucket accessible to the same signing identity by changing Host and adding X-Forwarded-Host. After the signature matches a host candidate, extract the bucket that the candidate implies (via the configured virtual-host domains) and compare it with the bucket the router selected. Reject when they differ, before returning success. * test(s3api): cover virtual-host presigned URL retargeting Add unit tests for bucketFromVirtualHost and end-to-end tests that reproduce the X-Forwarded-Host retargeting attack for both presigned and signed requests, plus a negative test confirming the legitimate same-bucket case still verifies. * s3: harden bucketFromVirtualHost for case and overlapping domains Compare host and domain suffixes case-insensitively so a mixed-case X-Forwarded-Host cannot bypass the consistency check. Only treat the exact path-style domain as non-virtual-host; subdomains of a path-style domain still match the virtual-host router pattern and must be checked. |
||
|
Chris Lu
|
210afacd12 |
s3: close list-type / ownership-controls routing mismatch (#11280)
* s3: reject list-type paired with another operation subresource ?list-type=2&ownershipControls= routes to ListObjectsV2 (the list-type route is registered first) while the IAM action resolver resolves the ownershipControls selector to s3:GetBucketOwnershipControls. A principal denied s3:ListBucket but allowed s3:GetBucketOwnershipControls would therefore list the bucket. list-type selects an operation just like the other keys in operationSubresources, so add it there and reject the combination before routing, matching the fix for policy&tagging (#10987). * s3: resolve list-type to s3:ListBucket ahead of bucket subresources The router registers the ListObjectsV2 route ahead of the bucket subresource routes, so the action resolver should resolve list-type the same way. Without this, a request carrying list-type and another operation selector resolves to the subresource action (e.g. s3:GetBucketOwnershipControls) while being served by ListObjectsV2. The ambiguity guard rejects such combinations before routing, but resolving list-type to s3:ListBucket keeps the resolver aligned with the router, mirroring how versions is handled. * s3: match list-type=2 exactly in action resolver The router selects ListObjectsV2 only for list-type=2; other values fall through to the subresource routes. Resolve the same way so the action matches the handler for every list-type value, not just 2. |
||
|
Chris Lu
|
9f6feef299 |
feat(s3api): add bucket quota S3 extension via ?seaweedfs-quota (#11279)
* feat(s3api): add bucket quota S3 extension via ?seaweedfs-quota
Add a SeaweedFS-specific S3 subresource for bucket quota management:
PUT /{bucket}?seaweedfs-quota — set bucket quota (s3:PutBucketQuota)
GET /{bucket}?seaweedfs-quota — get bucket quota (s3:GetBucketQuota)
The request/response body is JSON:
{"quota_size": 100, "quota_unit": "GB", "quota_enabled": true}
Quota is stored on the bucket's filer entry (positive = enabled,
negative = disabled but retained, zero = no quota), matching the
existing admin REST API behavior. When quota is cleared, the bucket's
read-only flag is also lifted.
Authentication uses the existing S3 SigV4 flow — no new global secret
is needed. Authorization uses two new dedicated IAM permissions:
s3:PutBucketQuota
s3:GetBucketQuota
This allows integrations like Apache CloudStack to manage per-bucket
quotas through the S3 endpoint with a scoped credential, without
exposing the broad admin REST API or requiring a separate admin token.
The credential can be limited to s3:PutBucketQuota/s3:GetBucketQuota
only, preventing bucket deletion, user management, or cluster topology
changes.
The coarse-grained ACTION_PUT_BUCKET_QUOTA/ACTION_GET_BUCKET_QUOTA
constants are added to s3_constants, and the action resolver maps the
seaweedfs-quota query parameter to the fine-grained s3: actions for
policy evaluation.
* docs: update design for S3 ?seaweedfs-quota extension approach
Replace the broad admin REST API + bearer-token design with the narrow,
scoped S3 ?seaweedfs-quota extension. Update quota, usage reporting, and
SeaweedFS-side changes sections to reflect PR #11279.
* fix(s3api): address review comments on quota handler
Fix four issues identified by Devin, Greptile, and CodeRabbit reviews:
1. Integer overflow in convertQuotaToBytes: large quota_size values
(e.g. 8388608 TB) could overflow int64, wrapping to negative and
being silently treated as zero quota. Now returns an error when
size * multiplier would exceed math.MaxInt64.
2. Disabled quotas returned negative sizes in GET: the GET handler
returned entry.Quota directly, which is negative for disabled-but-
retained quotas. Now returns the absolute magnitude as quota_size
and derives quota_enabled from the sign, making the response
round-trippable.
3. Missing buckets returned 500 instead of NoSuchBucket: the PUT
handler treated all lookup failures as internal errors. Now
distinguishes filer_pb.ErrNotFound and returns ErrNoSuchBucket.
4. Trailing JSON was silently accepted: the decoder read only the
first JSON object without checking for trailing data. Now
requires EOF after the object, rejecting malformed payloads.
Also add tests for overflow detection and trailing data rejection.
* fix(s3api): cast math.MaxInt64 to int64 for 32-bit vet
On 32-bit platforms, math.MaxInt64 is an untyped int constant that
overflows int (32-bit) when used directly in fmt.Errorf with %d.
Cast to int64 explicitly to fix Go Vet 32-bit.
* docs: reconcile design doc with implementation and add AWS tools note
- Resolve open question about IAM endpoint path: driver accepts optional
iamUrl and defaults to <s3Url>/iam
- Add note explaining ?seaweedfs-quota is not callable by standard AWS tools
(aws s3api, s3cmd, rclone), and how this compares to MinIO and Ceph quota
APIs which also live outside the standard S3 API
* docs: fix IAM endpoint default — SeaweedFS IAM is at POST / on S3 endpoint
SeaweedFS registers its embedded IAM API at POST / on the same S3
endpoint (UnifiedPostHandler), not under /iam. The design doc
previously said the driver defaults iamUrl to <s3Url>/iam, which would
send IAM operations to an unregistered path. Correct the default to
s3Url.
Found by Greptile review on PR #11279.
* docs: fix credential model, signer, and GET response shape in design doc
Three issues found by CodeRabbit review on PR #11279:
1. Credential-scope contradiction: the doc claimed the service credential
is scoped to only s3:PutBucketQuota/s3:GetBucketQuota, but the
implementation uses it as the admin credential for all operations
(bucket CRUD, IAM user provisioning, quota). Document the actual
model.
2. S3Signer -> AWSS3V4Signer: the doc said 'S3Signer for SigV4 signing'
but S3Signer is legacy SigV2. Correct to AWSS3V4Signer.
3. GET response shape: the doc showed a single JSON example with 'GB'
for both PUT and GET, but GET always returns quota_unit 'B' and the
absolute byte count. Document PUT input and GET response separately.
|
||
|
Chris Lu
|
79994b69af |
s3: fail closed on unsupported bucket-policy condition operators (#11283)
* s3: support StringEqualsIgnoreCase and related condition operators The S3 bucket-policy condition engine rejected StringEqualsIgnoreCase (and StringNotEqualsIgnoreCase, StringLikeIgnoreCase, StringNotLikeIgnoreCase), which AWS and the IAM policy engine both accept. Add evaluators and register them in GetConditionEvaluator so valid policies using these operators evaluate correctly instead of being skipped. * s3: reject bucket policies with unsupported condition operators validateStatement did not check Condition operators, so a policy with an unknown operator (e.g. a typo or unsupported key) was accepted at upload time and only surfaced at evaluation, where it was silently skipped. Reuse GetConditionEvaluator to reject unknown operators when a policy is parsed or stored, failing closed at the entry point instead of relying on evaluation-time handling. * s3: fail closed on unsupported condition operators at evaluation EvaluateConditions skipped statements whose condition operator was unsupported, logging a warning and continuing. With no remaining conditions to fail, the function returned true, so an Allow statement conditioned on an unrecognized operator became unconditional and granted access to private objects. Return false instead so an unrecognized operator fails the condition block and the statement does not match, matching the fail-closed behavior of the IAM policy engine. * s3: validate condition operators at upload time only, not load time Validating condition operators in validateStatement rejected the whole policy document from ParsePolicy, which SetBucketPolicy uses when loading stored bucket policies. A legacy policy saved before this change could contain an unsupported operator, and rejecting it at load time dropped the entire policy - including unrelated explicit Deny statements - so the bucket lost its protections. Move the operator check into ValidateBucketPolicy, which only the PutBucketPolicy handler and admin UI run at upload time, so legacy policies still load and EvaluateConditions fails the unsupported statement closed instead. * s3: drop non-AWS StringLikeIgnoreCase and StringNotLikeIgnoreCase operators AWS defines StringEqualsIgnoreCase and StringNotEqualsIgnoreCase but not StringLikeIgnoreCase or StringNotLikeIgnoreCase (StringLike and StringNotLike are case-sensitive only). Registering the wildcard IgnoreCase variants made the engine accept operators AWS rejects. Keep only the two AWS-defined IgnoreCase operators and add a test asserting the wildcard IgnoreCase names are unsupported. |
||
|
Konstantin AdzerandChris Lu
|
42b0ca7850 |
s3 sink: report the source read error the SDK hides (#11277)
* s3 sink: report the source read error the SDK hides filer.backup stops for good on an event whose chunks are gone from the volume servers: the uploader reads the body, the read fails with the volume's 404, and the AWS SDK returns "ContentLength=N with Body length 0" without the cause. isIgnorable404 would skip such an event, but it never sees the 404, so the event is retried forever and the checkpoint never advances. ChunkStreamReader keeps its first source failure and the s3 sink returns it when the upload fails. * s3 sink: trim verbose comments on source error propagation --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Da.Sanchezandchrislusf
|
d8aa7ecf04 |
fix(vacuum): stop comparing compact size against the live needle map (#11263)
* fix(vacuum): stop comparing compact size against the live needle map CompactByIndex's post-copy integrity check compared bytes written to the .cpd against v.nm.ContentSize()-DeletedSize(), the live map that keeps mutating for as long as the volume stays writable during the copy. Any write landing after the point-in-time index snapshot was loaded made the live map's tally exceed what got copied, aborting compaction with "unexpected new data size" — even though CommitCompact's makeupDiff exists specifically to reconcile writes that land mid-copy. On a busy volume this can fail every vacuum cycle. Tally the expected live size from oldNm, the same frozen snapshot the copy loop reads from, instead of the live map. This keeps the check's original protection (destination smaller than what should have been copied signals real data loss) while removing the false positive from ordinary concurrent traffic. * fix(vacuum): stop double-subtracting skipped bytes from the size check Unreadable needles return before reaching the expectedLiveBytes tally, so it already excludes them. Subtracting skippedDataBytes again on top loosened the integrity check's margin by that same amount, letting a .cpd short of the true expected size slip past undetected — the exact failure mode the check exists to catch. Flagged independently by three automated PR reviewers (Devin, Greptile, CodeRabbit). Extract the comparison into exceedsExpectedCompactedSize and drop the subtraction entirely; add TestExceedsExpectedCompactedSize to pin the threshold to expectedLiveBytes alone. * fix(vacuum): trim verbose integrity-check comment Reduce the 8-line block comment to a concise 3-line rationale. No behavior change. * fix(vacuum): mirror compact integrity check in Rust volume server Mirror the Go fix in the Rust volume server's do_compact_by_index: tally expected_live_bytes from the frozen index snapshot (not the live needle map) and compare the compacted .dat against it after the copy. Unreadable needles already return before the tally, so no skipped-byte adjustment is needed. Adds exceeds_expected_compacted_size and two regression tests. * fix(vacuum): exercise makeup_diff in Rust concurrent-write test Address CodeRabbit review: write a needle after compaction (before commit), then call commit_compact() and assert the late write survives via makeup_diff. This actually exercises the concurrent-write path rather than just confirming the integrity check passes. --------- Co-authored-by: chrislusf <chris.lu@gmail.com> |
||
|
Chris Lu
|
2ebfeabfce |
mount: rebuild expired directory cache on entry lookup (#11268)
* test: reproduce expired directory cache degrading lookup to N RPCs After cacheMetaTtlSec elapses the kernel can still serve a directory listing from its page cache, so ReadDir never runs and EnsureVisited is not called. Metadata lookups then fall through to one LookupEntry RPC per entry instead of rebuilding the directory cache once. Issue #11262 * mount: add expired-directory rebuild predicate with cooldown to InodeToPath ShouldRebuildExpiredDir distinguishes a TTL-expired cached directory from a never-cached, invalidated, evicted, or read-through one (those clear isChildrenCached, while a plain TTL expiry keeps it set). It also gates retries on a cooldown since the last failed rebuild attempt, recorded by MarkRebuildAttempt, so a transient listing failure does not trigger a full rebuild on every later lookup. Issue #11262 * mount: rebuild expired directory cache on entry lookup When the kernel still serves a directory listing from its page cache past cacheMetaTtlSec, ReadDir never runs and EnsureVisited is not called, so lookupEntry issues one LookupEntry RPC per entry. Rebuild the expired directory once via ensureDirectoryVisited before the cache-hit check so later lookups are served locally. The EnsureVisited singleflight deduplicates concurrent rebuilds. On a non-oversized rebuild failure, record the attempt so the cooldown suppresses repeated rebuilds while the listing keeps failing; once it elapses a later lookup retries, recovering without waiting for ReadDir. Oversized dirs are already marked read-through by ensureDirectoryVisited. Issue #11262 * test: cover concurrent rebuild dedup and rebuild-cooldown fallback Add a test that runs concurrent lookups into the same expired directory behind a gated listing, asserting they share one rebuild via the EnsureVisited singleflight. Add a test that a failed rebuild records the attempt so an immediate retry is suppressed (per-entry RPC fallback), and that once the cooldown elapses and the filer recovers a later lookup rebuilds the cache. Issue #11262 * mount: wait for pending async flush before rebuilding parent cache The rebuild lists the parent directory from the filer, so a pending async flush of the target entry must land first; otherwise the rebuilt cache captures pre-flush metadata and the cache-hit path returns it without the wait that guards the filer-fallback path. waitForPendingAsync Flush is a no-op when no flush is pending, so the common case is unaffected. Issue #11262 |
||
|
Chris Lu
|
a3638e479e |
fix(s3api/audit): surface OIDC identity claim in audit log for STS sessions (#11269)
* Add ResolveIdentityClaim helper for OIDC audit identity ComputeParentUser derives a stable per-identity hash from (sub, iss) for internal keying, but it is opaque and not human-readable. Audit logs for STS-assumed OIDC sessions currently surface that opaque value (or the random session id) as the requester, leaving no authoritative trace of the federated user. Add ResolveIdentityClaim next to ComputeParentUser to recover a human-readable, server-asserted identity attribute from the STS request context populated at federation time. It walks a priority list (preferred_username, email, name, sub) so a federated session always audits against a stable OIDC claim rather than a client-supplied role session name. For #11264 * Surface authoritative OIDC identity claim in S3 audit log For STS-assumed sessions minted from an OIDC web identity, the audit log requester field is the opaque session subject, which cannot be traced back to the federated user who performed the operation. The OIDC identity claims (preferred_username, email, sub) are already carried in the session request context and reach the auth layer as identity.Claims, but they were never surfaced to the audit log. Add a requester_identity field to the S3 access audit log, populated from the authoritative OIDC identity claim resolved via ResolveIdentityClaim. The claim is propagated through the shared identity holder (the same mechanism the requester name and principal ARN already use) so it survives the request-context copy that hides auth-set values from the outer audit middleware. The existing requester field is left unchanged for backward compatibility; requester_identity is empty for non-federated sessions, where requester already carries the real username. For #11264 * Gate OIDC audit identity on federation marker and harden resolver Address review feedback (Devin Review, Greptile) on the initial implementation: - Non-federated STS sessions no longer gain a false requester_identity. ValidateJWTWithClaims merges the JWT registered sub claim (the opaque session id) into RequestContext for sessions without an explicit request context, so the previous ResolveIdentityClaim fallback to sub surfaced that session id as an authoritative identity. Resolution is now gated on SessionInfo.ParentUser, which is set only for OIDC-federated sessions in AssumeRoleWithWebIdentity. The claim is resolved from the original sessionInfo.RequestContext (not the local claims map, whose sub the bearer path overwrites with the session subject) so SigV4 and bearer sessions surface the same identity. - ResolveIdentityClaim now trims whitespace and treats whitespace-only claims as absent, so a blank preferred_username no longer masks a usable email or sub. The resolved claim is carried on Identity.IdentityClaim (and IAMIdentity for the bearer path) rather than re-derived in recordIdentityInContext, making the federation gate explicit at the auth boundary. For #11264 * Resolve OIDC identity claim for external bearer tokens The external OIDC bearer path (a raw OIDC JWT presented directly, not via STS) populates Claims with preferred_username/email/name/sub from the validated token but did not set IdentityClaim, so requester_identity stayed blank for that authentication path. Resolve the claim there too — sub is the real OIDC subject on this path (not an STS session id), so no federation gate is needed. Also drop an ineffectual ctx assignment flagged by ineffassign in the audit test. For #11264 |
||
|
Chris Lu
|
80dae68dbf |
fix: write the new key when a remote-synced file is renamed (#11270)
* refactor: extract update event handling into processUpdateEvent Pull the OldEntry/NewEntry update branch of the remote sync event processor into its own function so the rename skip logic can be exercised by tests with stub clients. No behavior change. * test: reproduce remote sync rename dropping the new key A rename under a remote mount arrives as an update whose NewEntry inherits the source RemoteEntry. shouldSendToRemote returns false for it, so processUpdateEvent skipped the event without writing the new key, while the filer had already deleted the old object. The test runs such an event through processUpdateEvent and expects both a delete of the old key and a write of the new one. Fails before the fix. See #11261. * fix: write the new key when a remote-synced file is renamed A rename under a remote mount arrives as an update whose NewEntry inherits the source RemoteEntry, so shouldSendToRemote returns false (RemoteMtime >= Mtime) and processUpdateEvent skipped the event. That skip is only valid when the destination key is unchanged; a path change always needs a write, and the delete-old/write-new handling below the early return is exactly what a rename needs. Guard the skip with proto.Equal(oldDest, dest) so a rename falls through to it. Fixes #11261. * fix: skip empty upload when renaming a remote-only entry A remote-only entry (no local chunks or content, data lives only on the remote object) carries a positive RemoteSize but nothing for NewFileReader to read. After the previous commit lets a rename fall through to the delete-old/write-new path, such a rename would upload EOF and create a zero-byte object at the new key, then stamp it as synced. Guard the write so a path change on a remote-only entry skips the upload instead of replacing the file with zero bytes. The filer has already deleted the old object, so the data is gone regardless; this avoids leaving a misleading empty object behind. * fix: propagate old-key delete errors except already-deleted When deleting the old key on a rename fails for a non-multipart entry, the error was swallowed and the write proceeded, which could leave both remote keys. Return the error so MetadataProcessor retries the event. The filer deletes the source remote object synchronously during the rename, so the sync delete is redundant and the object may already be gone. GCS reports that as ErrRemoteObjectNotFound (unlike S3/Azure, whose deletes are idempotent), so treat it as a successful deletion and continue to retriedWriteFile rather than pinning the sync offset. |
||
|
Chris Lu
|
5ff49909a0 |
fix(s3api/iam): avoid transient AccessDenied from full reloads on single IAM file changes (#11271)
* fix(s3api/iam): fail config snapshot on empty or malformed IAM files A full IAM reload reads every identity/policy/service-account/group file from the filer. When an external secrets tool rewrites a file, a reload that reads it mid-rewrite sees empty or partially-written content. The identity, policy and service-account loaders silently skipped such files (``continue``), so the snapshot was missing entries that still existed on disk. The atomic swap then installed an incomplete identity set while ``isAuthEnabled`` stayed on, denying unrelated clients mid-reload (#11259). The group loader and the read-error paths already fail the snapshot in this situation (a skipped entry reads as deleted). Apply the same behavior to empty content and unmarshal failures across the identity, policy, service-account and group loaders, so a transient mid-rewrite fails the reload (preserving the last known-good state) instead of silently dropping entries. * fix(s3api/iam): coalesce burst IAM config reloads through the reload queue onIamConfigChange did a full synchronous reload for every identity/policy file change event. When several independently-refreshing credentials rewrite their files within the same second, that produced a burst of dozens of back-to-back full reloads, each reading the whole store and widening the window where a mid-rewrite file is observed (#11259). Route every IAM config change through the existing coalescing reload queue (scheduleReload/reloadRetryLoop) instead. A burst of N events now collapses into a single reload (plus one tail reload for events that arrived while one was in flight). scheduleReload gains a reason argument for the existing log line; the reloadRetryLoop already retries failed reloads, so the per-event failure handoff is no longer needed. Tests that asserted on the synchronous reload now wire up the queue (centralized in newTestS3ApiServerWithMemoryIAM) and poll via waitForIdentity/waitForIdentityGone. Adds TestOnIamConfigChangeCoalescesBurstReloads showing 50 events coalesce into <=3 reloads. * fix(s3api/iam): skip non-JSON auxiliary files before failing IAM snapshot Per review: the multi-file loaders unmarshal every entry in an IAM directory, so a non-JSON auxiliary file (README, .DS_Store, a migration backup such as identity.json.old) would hit the new empty/malformed errors and reject the whole snapshot, blocking all later IAM reloads. Only *.json files are IAM objects (SeaweedFS writes identities, policies, service accounts and groups as <name>.json, and other call sites already gate on the .json suffix). Skip non-.json entries at the top of each loader loop, before reading content, so auxiliary files are ignored while empty/malformed .json files still fail the snapshot. Adds TestLoadConfigurationIgnoresNonJsonAuxiliaryFiles. * fix(s3api/iam): reject IAM files with empty identifiers and skip aux in listing Per review: - ListPolicyNames listed every regular entry in the policies directory as a policy name, including non-JSON auxiliary files, but GetPolicy cannot retrieve them. Apply the same .json suffix filter used by the loader so the list only exposes retrievable policies. - json.Unmarshal accepts `{}` and unknown fields. The identity and group loaders merge by the decoded Name (not the file name), so a `{}` file could install an empty-key record and displace a real one; the service-account loader accepted an empty Id. Validate Identity.Name, Group.Name and ServiceAccount.Id (via validateServiceAccountId) after unmarshal and fail the snapshot on empty identifiers. Adds TestFilerEtcStoreListPolicyNamesSkipsNonJsonAuxiliary and empty-identifier regression tests for identity, group and service-account files. |
||
|
Chris Lu
|
0de9c1f231 |
fix(s3api/sts): respect MaxSessionLength config in DurationSeconds validation (#11267)
* Refactor parseDurationSeconds into a STSHandlers method Convert the parseDurationSeconds wrapper from a package-level function into a method on STSHandlers so it can reach the configured STS service. No behavior change; the three AssumeRole* handlers now invoke it via their receiver. * Respect MaxSessionLength config in STS DurationSeconds validation parseDurationSeconds validated DurationSeconds against a hardcoded 43200s (12h) ceiling, so raising maxSessionLength in iam.json above 12h had no effect on AssumeRole, AssumeRoleWithWebIdentity, or AssumeRoleWithLDAPIdentity — requests were rejected at the handler before reaching the service layer. Derive the upper bound from the configured STS MaxSessionLength, falling back to maxDurationSeconds (43200s) when unset. The service layer (calculateSessionDuration) already caps the issued duration at MaxSessionLength, so this only relaxes the input-validation gate. * Add tests for STS DurationSeconds MaxSessionLength bound Cover the configured MaxSessionLength upper bound, rejection above it, fallback to the 43200s default when STS config is unset, the 900s minimum, and the empty-parameter nil path. * Refactor validateSessionDurationSeconds into a STSService method Convert validateSessionDurationSeconds from a package-level function into a method on STSService so it can reach the configured STS config. No behavior change; the three assume-role entry points in the service (AssumeRoleForPrincipal, validateAssumeRoleWithWebIdentityRequest, validateAssumeRoleWithCredentialsRequest) now invoke it via their receiver. * Respect MaxSessionLength config in STS service DurationSeconds validation The STS service validateSessionDurationSeconds rejected DurationSeconds above a hardcoded 43200s (12h) ceiling, so even after the handler accepted a longer duration it was rejected again in the service layer for AssumeRoleForPrincipal, AssumeRoleWithWebIdentity, and AssumeRoleWithCredentials. Derive the upper bound from the configured MaxSessionLength, falling back to DefaultMaxSessionLength (43200s) when unset. The issued duration is still capped at MaxSessionLength by calculateSessionDuration. * Add tests for STS service DurationSeconds MaxSessionLength bound Cover the configured MaxSessionLength upper bound, rejection above it, fallback to the 43200s default when STS config is unset, the 900s minimum, and the nil DurationSeconds path. * Preserve capping when MaxSessionLength is below the API minimum Deriving the DurationSeconds upper bound directly from MaxSessionLength created an empty valid range when MaxSessionLength is configured below the 900s API minimum, rejecting every explicit DurationSeconds that the old code silently capped via calculateSessionDuration. Only apply the configured MaxSessionLength as the upper bound when it is at least minDurationSeconds; otherwise keep the default bound and let calculateSessionDuration enforce the shorter configured limit. * Add tests for sub-minimum MaxSessionLength capping behavior Verify that a MaxSessionLength below the 900s API minimum keeps the default upper bound so explicit DurationSeconds within the default range are still accepted (and later capped by calculateSessionDuration). |
||
|
ssshr-66andChris Lu
|
e919bec9d1 |
fix(volume): Harden Volume Copy Validation and Failure Handling (#11252)
* fix(volume): harden volume copy validation * fix(volume): use stream context for ReadVolumeFileStatus in VolumeCopy ReadVolumeFileStatus ran on context.Background() while the adjacent VolumeStatus call used stream.Context(), an inconsistency left over from the context revert in #11252. Use stream.Context() consistently so the source status check is cancelled with the VolumeCopy stream. * fix(volume): reserve destination before deleting existing replica FindFreeLocation now runs before DeleteVolume so a full target fails without destroying the existing replica. Previously, when the initial VolumeStatus check failed (advisory) but ReadVolumeFileStatus succeeded, the existing replica was deleted before a destination was reserved, risking data loss if no location had enough free space. Add a regression test verifying the existing replica survives when the destination is full and the initial status check fails. * fix(volume): count replaced replica slot in FindFreeLocation FindFreeLocation now accepts the volume being replaced so its slot is treated as available. Without this, a location at its MaxVolumeCount limit could not replace its sole replica even though deleting it would free the slot. VolumeCopy passes the volume ID so destination selection succeeds before the existing replica is deleted. Add TestVolumeCopyReplacesReplicaAtSlotLimit covering a single-slot location that must replace its only replica. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
|
2cd6c36c54 |
filer: end local-only metadata subscriptions when remote peers appear (#11251)
* filer: end local-only metadata subscriptions when remote peers appear SubscribeMetadata delegates to SubscribeLocalMetadata whenever the MetaAggregator knows no remote peers at stream setup. Peer discovery is asynchronous with the gRPC server accepting streams: the master announces filers after Filer.Init, via ListExistingPeerUpdates and OnPeerUpdate. A subscriber that connects inside that window is pinned to a filer-local stream for its whole life, silently missing every other filer's writes. For filer.remote.sync in a multi-filer cluster this means the remote tier permanently stops receiving writes served by other filers (#11247). End the delegated local stream when the first remote peer appears, so the client reconnects into the aggregated stream. The end surfaces as an error, not a clean EOF: RetryUntil-driven followers (mount meta cache, s3api IAM) treat a clean end as following finished and stop reconnecting. The arrival channel is armed under the same lock as the peer check in RemotePeerArrivedChan, so a peer learned in between sends the stream straight to the aggregated path instead of parking on a channel that would never fire. A standalone filer is unaffected: no peer ever appears, the channel never fires, and the local stream serves indefinitely. Fixes #11247 * filer: interrupt disk replay on peer arrival, trim comments Check upgradeOnRemotePeer inside eachLogEntryFn and chunkDiskPass so a peer arriving during a backlog replay stops the stream before the cursor advances past older remote events. Wrap errAggregationUpgrade with StopReadingError so LoopProcessLogData does not log it. Remove issue references from comments and trim verbose commentary. * filer: check upgrade signal between ref batches Pass upgradeOnRemotePeer to sendRefsBatched so a peer arriving while refs are shipped to a slow client is detected between batches, not only after the full batch completes. * filer: interrupt gap park on peer arrival Pass upgradeOnRemotePeer through gapPass to parkOnGap so a peer arriving during a gap park ends the stream immediately instead of waiting for the retry timer (up to one minute). --------- Co-authored-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
7fa2f75f30 |
s3: bucket-policy Allow must not override an identity explicit Deny (#11256)
* s3: add isActionExplicitlyDeniedByApplicablePolicies helper Add a helper that reports whether any applicable identity-side policy (attached IAM policies, enabled-group policies, or the IAM-integration session policy) explicitly denies an action. It reuses the existing evaluateAttachedIAMPolicies, resolveS3AuthTarget, buildPrincipalARN, and isActionExplicitlyDeniedByIAM helpers, and fails closed on evaluation errors. A nil identity has no identity-side policy plane, so the helper returns false to keep the bucket policy authoritative for anonymous access. No behavior change yet; the next commits apply it to the two bucket-policy Allow short-circuits. * s3: enforce identity explicit Deny before bucket-policy Allow authRequestWithAuthType short-circuits on a matching bucket-policy Allow and skips VerifyActionPermission, so an explicit Deny in an authenticated identity attached, group, or session policy is bypassed. A non-admin principal with s3:PutBucketPolicy can install a bucket-policy Allow for itself and read an object its identity policy explicitly denies. Before honoring a bucket-policy Allow, check the applicable identity-side policies for a matching explicit Deny via the new isActionExplicitlyDeniedByApplicablePolicies helper, and fail closed. The cross-account behavior is preserved: a bucket Allow still supplies the Allow an identity policy omits (implicit denial), and a nil identity keeps the bucket policy authoritative for anonymous access. Regression tests cover the explicit-Deny override, the implicit-deny Allow preservation, and the unmatched-key fall-through control. * s3: enforce identity explicit Deny in secondary object-key auth authorizeObjectKeyAction authorizes keys the request URL does not name (CopySource, DeleteObjects body keys, POST Object form keys) and shares the same bucket-policy Allow short-circuit as the primary path, so an explicit Deny in the identity, group, or session policy is bypassed the same way when a bucket policy allows the secondary key. Apply isActionExplicitlyDeniedByApplicablePolicies before accepting the bucket-policy Allow, mirroring the primary path. A regression test covers AuthorizeCopySource for both the explicit-Deny override and the implicit-deny Allow preservation. |
||
|
Feng Shao
|
13bf056a15 |
Mount req with collection (#11249)
* volume mount req support specify collection * rust mirror change |
||
|
Nguyễn Đăng Minh Lực
|
c968084b34 |
iceberg: fix OAuth token expiry handling (401 + token-exchange + configurable TTL) (#11242)
* iceberg: return 401 for invalid or expired Bearer tokens BUG-0001: when the OAuth JWT expired, Server.Auth fell through to the S3 SigV4 authenticator, which rejects the "Authorization: Bearer" scheme with NotImplemented — a 501. Iceberg clients (Java OAuth2Manager, pyiceberg) only refresh tokens on 401, so they retried the dead token forever: RisingWave sinks stalled and Doris catalog queries failed every token TTL (1h) until the client process was restarted. A request carrying a Bearer header is an Iceberg REST client: answer 401 (+ WWW-Authenticate: Bearer, RFC 6750) when the token fails, and only fall through to the S3 authenticator when no Bearer header is present. * iceberg: make OAuth token TTL configurable via ICEBERG_OAUTH_TOKEN_EXPIRY BUG-0001 follow-up: production evidence shows Iceberg Java 1.10.x clients (RisingWave connector node, Doris FE) never re-fetch tokens on 401 — the sink stalled again on token expiry even with the 501→401 fix, and no POST /v1/oauth/tokens appeared in server logs across dozens of retries. 401 is necessary but not sufficient for these clients. The TTL was hardcoded to 3600 with no knob. Read the expiry (seconds) from ICEBERG_OAUTH_TOKEN_EXPIRY, defaulting to 3600, so deployments can issue longer-lived tokens (e.g. 86400) to survive client restart cycles. * iceberg: support OAuth token exchange (RFC 8693) for client refresh Decompiling the Iceberg Java 1.10.1 client bundled with Doris FE showed the missing half of BUG-0001: OAuth2Manager refreshes via token-exchange (AuthConfig.exchangeEnabled defaults to true — the client_credentials re-fetch branch only runs with exchange disabled), so a server that only accepts client_credentials leaves Iceberg clients unable to ever refresh their token, regardless of 401 correctness. Accept grant_type=urn:ietf:params:oauth:grant-type:token-exchange on POST /v1/oauth/tokens: verify the subject_token signature against the issuing credential, allow exchange within a recovery grace window (max(2*TTL, 1h), capped 24h) so clients holding tokens that expired while the grant was unsupported recover without a restart, and mint a fresh access token with the configured TTL. * iceberg: harden OAuth token exchange and Bearer matching per review - match the Bearer scheme case-insensitively (RFC 7235), like authenticateBearer already does - accept optional client authentication on the token-exchange grant (Basic or form credentials, bound to the subject token's client); expired subject tokens now require it. Iceberg Java's proactive refresh sends Bearer-only headers, so the grant cannot require it - reject subject tokens without an exp claim, and re-check the issuer on the verified claims - unauthenticated exchange cannot extend the lifetime past the subject token's own expiry (no chain-refresh from a leaked token) - return 400 invalid_grant per RFC 6749 §5.2 (was 401) - include issued_token_type on exchange responses (RFC 8693) - clamp ICEBERG_OAUTH_TOKEN_EXPIRY to 365d so Duration math cannot overflow into already-expired tokens * iceberg: give authenticated token exchanges a fresh full TTL The remaining-lifetime cap only guards unauthenticated (Bearer-only) exchanges; an authenticated client renewing a live token must get the full configured TTL, matching client_credentials. * iceberg: reject token exchange when no lifetime remains A Bearer-only exchange with under a second of subject lifetime would mint a token with expires_in: 0. Reject with invalid_grant instead. * iceberg: pin near-expiry test token to the next second boundary jwt/v5 serializes exp at one-second precision, so a 300 ms offset can round into the current second and route the test through the expired branch instead of the ttlSeconds<=0 guard. Mint the subject with the next whole-second expiry: live at exchange time, deterministically under a second of remaining lifetime. * iceberg: drop internal ticket reference from comments * iceberg: clamp oversized OAuth TTLs on 32-bit platforms strconv.Atoi on an int-sized value fails with ErrRange on 386, so an oversized ICEBERG_OAUTH_TOKEN_EXPIRY silently fell back to the default instead of clamping. Parse in 64-bit space and clamp, then narrow. * iceberg: make OAuth TTL narrowing explicit * iceberg: disable legacy OAuth in PyIceberg integration tests |
||
|
Chris Lu
|
01fc31cb71 |
fix(filer): use path.Split instead of filepath.Split for filer paths (#11246)
FullPath.DirAndName() and FullPath.Name() used filepath.Split, which is OS-dependent: on Windows it treats backslash as a path separator, corrupting filer paths that contain literal backslashes. Filer paths always use "/" as the separator, so switch to path.Split and path.Join which only split on "/" regardless of the host OS. This fixes the backslash case from #11243 where a file saved as /test/special\reverseslash4.jpg was stored with a corrupted path on Windows filer builds. The #, ?, and % cases from the same issue are client-side URL-encoding problems (the server never receives the raw characters), but once the client properly percent-encodes them the server now handles the decoded path correctly on all platforms. |
||
|
ssshr-66
|
966692fa23 |
[Volume] Validate record counts after volume copy (#11238)
* validate Volume Copy record counts * Delete s3api_object_versioning_bench_test.go * reply ai comments |
||
|
Chris Lu
|
2ffa696809 |
fix(volume): handle faulty storage media (Go + Rust) (#11233)
* fix(volume): track EC shard read errors and unmount on faulty media Extract the volume EIO tracker into a reusable IoErrorTracker and add the same tracking to EcVolume. Sustained EIO on .ecx lookups or .ecd shard reads now unmounts the EC volume in the heartbeat (without deleting files) so the master re-replicates from healthy peers, mirroring the existing volume replica quarantine. Closes #11227 (EC shard unmount). * rust(volume): mirror EC shard read error tracking and unmount Add EIO tracking to the Rust EcVolume mirroring Go: a streak counter with IO_ERROR_TOLERANCE, a sticky quarantine flag, and unmount (not file deletion) in the heartbeat so the master re-replicates from healthy peers. * feat(metrics): expose storage IO error counter and quarantine gauge Add a storage_io_error_total counter incremented on every EIO recorded by the volume or EC shard tracker, and an io_quarantine gauge labelled by kind (volume/ec_shard) reflecting the count of replicas suppressed in the heartbeat. Mirrored in Go and Rust. * feat(healthz): report 503 when local replicas are IO-quarantined Add Store.HasIoQuarantine (Go) / Store::has_io_quarantine (Rust) and have /healthz return 503 when any local volume or EC shard is quarantined due to sustained storage-media EIO, so a load balancer can drain a server whose underlying media is faulty. Mirrored in Go and Rust. * fix(volume): keep quarantined EC volumes in memory and reset EIO on success Address review feedback: instead of unloading quarantined EC volumes (which discards the quarantine state healthz needs), keep them in memory and just skip them from heartbeat reporting, mirroring the regular volume quarantine. Also clear the EIO streak on successful .ecx reads in Rust so a transient error does not accumulate, and add an ec_shard label to the io_quarantine gauge in both Go and Rust. * fix(volume): exclude quarantined EC shards from heartbeat and add Rust volume tolerance Address review feedback: - Filter quarantined EC volumes from CollectErasureCodingHeartbeat (Go) and collect_ec_shard_delta_messages / collect_live_ec_shards (Rust) so the master stops advertising faulty shards and re-replicates from healthy peers. - Add consecutive EIO count and sticky quarantine to the Rust regular Volume, mirroring Go IoErrorTracker: a single EIO no longer deletes the replica; the heartbeat quarantines after the tolerance threshold and keeps the volume in memory. - Use the quarantine flag (not last_io_error) in has_io_quarantine so /healthz reflects sustained, not transient, failures. * fix(volume): make Rust quarantined volumes read-only and wire recovery Address Devin review: - Set no_write_or_delete on Rust volumes when quarantined in the heartbeat, so cached or direct clients cannot mutate a faulty replica after the master removes it (mirrors Go). - Wire reset_io_error_state into Volume::set_writable so an operator making a volume writable again clears the sticky quarantine and the volume re-enters heartbeat rotation. * fix(volume): clear EC quarantine on shard re-mount for operator recovery Address Greptile review: re-mounting EC shards (Go loadEcShardWithIdxDir / Rust mount_ec_shards_with_idx_dir) now calls ResetIoErrorState on the existing EcVolume, giving operators a documented recovery path that clears the sticky quarantine and returns the EC volume to heartbeat rotation. Mirrored in Go and Rust. * fix(volume): do not clear EC quarantine on routine shard mounts Address review feedback: clearing the EC IO quarantine on every mount (including duplicate, retry, sibling-shard, and reconciliation mounts) is too aggressive and can re-advertise known-bad shards before the storage media has been validated. Remove the automatic reset from the mount path; quarantine clears naturally on restart or full unmount when a fresh EcVolume is created with clean state. * test(volume): update Rust IO error test for quarantine semantics The heartbeat now quarantines a volume with sustained EIO (keeps it mounted, makes it read-only, omits it from heartbeat) instead of deleting it. Update test_collect_heartbeat_deletes_io_error_volume to assert the volume stays in the store with no_write_or_delete set, and update set_last_io_error_for_test to set the consecutive error count at the tolerance threshold so the test reflects a sustained error. * fix(volume): reset EIO streak after full write and match Windows media errors Move the success-side EIO reset from append_needle (after write_all only) to the end of do_write_request, after flush_dat/flush_idx complete, so a successful write_all followed by a failed fsync no longer resets the counter before the EIO is recorded. Repeated fsync EIOs now accumulate toward the quarantine threshold as intended. Recognize Windows storage-media failure codes ERROR_CRC (23) and ERROR_IO_DEVICE (1117) in addition to Unix EIO (errno 5), so quarantined heartbeat behavior is preserved on Windows. Mirrors the change in both Go and Rust volume servers. * fix(volume): preserve checkpoint EIO and clear streak on successful delete maybe_checkpoint_index now returns whether the checkpoint succeeded; the success-side EIO reset in do_write_request and do_delete_request only fires when it did, so a checkpoint media failure is no longer erased by the unconditional reset that followed it. do_delete_request also gains the success reset that was lost when append_needle stopped clearing the streak, so a successful delete still clears an earlier failure streak. is_storage_io_error now uses libc::EIO on Unix instead of a hard-coded 5, and the ECX binary-search read path gains a Windows fallback (seek + read_exact) so the buffer is no longer zeroed on non-Unix targets. |