mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-13 01:50:40 +02:00
fd4fa72289ef09f9012c1dbdfeda16dc5e646ab3
1391
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chris Lu
|
b88156fe6b |
fix(s3api): evaluate aws:SourceIp from the direct TCP peer, not forwarded headers (#11231)
* fix(s3api): use direct peer IP for aws:SourceIp in bucket policy engine extractSourceIP in the bucket-policy engine trusted X-Forwarded-For and X-Real-Ip whenever the TCP peer looked private (loopback/RFC1918/link-local), with no configurable trusted-proxy allowlist. In containerized deployments the gateway peer is almost always private, so any caller reaching it directly or from a co-located workload could spoof aws:SourceIp and bypass IpAddress/NotIpAddress bucket-policy restrictions. Always return the direct peer address (r.RemoteAddr), matching AWS S3 semantics. Remove the now-unused isPrivateIP helper and header-trust branch. Update TestExtractConditionValuesFromRequestSourceIPPrecedence to assert the peer IP is used regardless of forwarding headers, and add regression tests TestExtractSourceIP_IgnoresForwardedHeaders and TestExtractSourceIP_EnforcesIPRestrictionPolicy. * fix(s3api): use direct peer IP for aws:SourceIp in IAM role/session policies The IAM middleware's extractSourceIP trusted X-Forwarded-For and X-Real-IP whenever the TCP peer looked private (loopback/RFC1918/link-local), with no configurable trusted-proxy allowlist. In containerized deployments the gateway peer is almost always private, so any caller reaching it directly or from a co-located workload could spoof aws:SourceIp and bypass IpAddress/NotIpAddress conditions on role and session policies (IsPrincipalActionExplicitlyDenied). Always return the direct peer address (r.RemoteAddr), matching AWS S3 semantics. Remove the now-unused isPrivateIP helper, privateNetworks table, and its init(). Update TestRequestContextExtraction and TestIPBasedPolicyEnforcement to assert the peer IP is enforced regardless of forwarding headers, and add regression test TestUserInlinePolicySourceIpCondition_IgnoresForwardedHeaders. |
||
|
Chris Lu
|
557fffa350 |
iam: preserve native Admin when IAM policies are attached (#11226) (#11232)
* iam: expose tri-state result from attached policy evaluation evaluateIAMPolicies returned a bool that collapsed explicit Deny and no-match into a single false, so the authorization path could not tell "policies forbid this" from "policies say nothing". Introduce evaluateAttachedIAMPolicies returning Allow/Deny/NoMatch and keep evaluateIAMPolicies as a bool projection for existing callers. This is preparation for unioning native permissions with attached policies while preserving deny-always-wins. * iam: preserve native Admin when IAM policies are attached Attaching an IAM policy routed authorization exclusively to the attached policies, dropping the identity native permissions. A user with native Admin lost all access after attaching a non-granting policy, and stayed locked out if that policy was deleted without being detached first (#11226). Treat a native bare Admin grant as a permission floor that survives attached policies: when the attached policies do not explicitly allow, fall back to isAdmin() on the attached-policy path, and on the IAM integration path allow unless an attached policy explicitly denies. Explicit Deny still wins on both paths. Only bare Admin is consulted because inline policies flatten lossily into Actions (dropping conditions), so scoped actions are not unambiguously native and must keep flowing through the policy engine. * iam: regression tests for native Admin surviving attached policies Reproduces issue #11226: - TestNativeAdminSurvivesAttachedPolicy: a user with native Admin keeps Write access after attaching a policy that does not grant it. - TestNativeAdminSurvivesDeletedPolicy: the same user keeps Write access after the attached policy is deleted without being detached. - TestAttachedPolicyExplicitDenyOverridesNativeAdmin: an explicit Deny in an attached policy still constrains a native admin (deny-always-wins). * iam: apply native Admin floor before IAM principal validation The native Admin floor in authorizeWithIAM ran after the auth-path switch, which denies when no session principal or PrincipalArn is present. An Admin identity without a PrincipalArn (no session token) was therefore denied before the floor executed. Move the floor ahead of the switch and derive the principal for its explicit-deny check with buildPrincipalARN, which already handles identities without a PrincipalArn. Adds a regression case for an Admin identity with an empty PrincipalArn. Addresses CodeRabbit review feedback on PR #11232. |
||
|
Chris Lu
|
c0a7dbb2bb |
iam: bind CreateServiceAccount ParentUser to the caller (#11218)
* iam: bind CreateServiceAccount target to caller in AuthorizeIamAction A non-admin holding iam:CreateServiceAccount could pass an arbitrary ParentUser and mint a service account for any identity, inheriting that identity permissions. Add a self-target category so a granted non-admin may only target their own identity; admins remain unrestricted. * iam: authorize CreateServiceAccount against its ParentUser target AuthIamManagement passed UserName as the authorization target for every action, so CreateServiceAccount was authorized with an empty target and the self-target binding never saw the caller-supplied ParentUser. Pass ParentUser for that action so the binding takes effect on the live path. * iam: test CreateServiceAccount binds target to caller Regression test: a non-admin holding iam:CreateServiceAccount may target itself but is denied targeting another identity; admins remain unrestricted. * iam: authorize CreateServiceAccount against ParentUser on the S3 port UnifiedPostHandler passed UserName as the authorization target for every IAM action, so CreateServiceAccount was authorized with an empty target on the S3-port route and the self-target binding never saw the caller ParentUser. Extract iamTargetUserName (ParentUser for CreateServiceAccount, UserName otherwise) and use it from both IAM dispatch surfaces so the binding applies on the live S3-port path as well as the standalone iam server. * iam: test CreateServiceAccount ParentUser binding on the S3 port End-to-end regression test through UnifiedPostHandler: a non-admin holding iam:CreateServiceAccount is denied (403) when targeting another identity and passes authorization when targeting itself. |
||
|
dependabot[bot]andChris Lu
|
2d4b730a2f |
build(deps): bump github.com/twmb/avro from 1.7.2 to 1.8.0 (#11210)
* build(deps): bump github.com/twmb/avro from 1.7.2 to 1.8.0 Bumps [github.com/twmb/avro](https://github.com/twmb/avro) from 1.7.2 to 1.8.0. - [Commits](https://github.com/twmb/avro/compare/v1.7.2...v1.8.0) --- updated-dependencies: - dependency-name: github.com/twmb/avro dependency-version: 1.8.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * iceberg: adapt to twmb/avro v1.8.0 and iceberg-go defensive copies avro v1.8.0 changes Schema.Root() to return *SchemaNode, which breaks iceberg-go v0.6.0's internal avro_schemas.go. The fix (apache/iceberg-go#1843) is only on iceberg-go's main branch, unreleased, so bump iceberg-go to that commit (c210509) alongside the avro bump. That iceberg-go revision also changes two behaviors seaweedfs worked around: - It now infers a manifest list's format version from the embedded writer schema, so a list missing the "format-version" header entry (DuckDB's shape is read as v2, not v1. ReadManifestList's header patching is now a redundant safety net; tests updated to expect v2. - It returns defensive copies from DataFile.Partition(), so the ReadManifest shim's in-place partition normalization was silently discarded. Rebuild the entry through NewDataFileBuilder when any partition value is normalized, copying every other DataFile field so manifest round-trips are preserved. - It converts day-transform partitions to iceberg.Date on read (applyDayTransformDates), so the day-partition cases the shim and tests guarded now convert without help; tests updated to expect iceberg.Date from the raw read. EOF ) * iceberg: accept assert-ref-snapshot-id without snapshot-id iceberg-go's new nullableInt64 parser rejects an assert-ref-snapshot-id requirement whose "snapshot-id" field is absent from the JSON, even though the Iceberg REST spec makes it optional (null means the ref must not already exist). v0.6.0 used a plain *int64, so absent was nil and accepted. ClickHouse sends the requirement without snapshot-id when asserting a branch does not yet exist, so its writes fail with "missing required field \"snapshot-id\"". normalizeRequirements splices an explicit null into any assert-ref-snapshot-id requirement missing the field before handing the JSON to iceberg-go's parser, restoring the v0.6.0 behavior across both iceberg-go versions. * iceberg: fix v1 block_size_in_bytes default in rebuilt manifest entries rebuildManifestEntry set block_size_in_bytes to 0, but the v1 manifest schema requires the default of 64 MiB ("Always write default in v1"). The original value is not exposed on the DataFile interface, so use the spec default. Also clarify the fallback comment to note that empty (zero-record / zero-byte) files also trigger it, not just a nil spec. Add a round-trip test that writes a rebuilt entry as v1 and verifies block_size_in_bytes is 64 MiB via Avro decoding. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
70a26cb5d2 |
s3: gate IAM-cache gRPC RPCs behind admin Bearer auth (#11190)
* s3: gate IAM-cache gRPC RPCs behind admin Bearer auth The SeaweedS3IamCacheServer registered on the S3 gateway's internal gRPC port (default 0.0.0.0:18333) accepted PutIdentity/RemoveIdentity/PutPolicy/ DeletePolicy/GetPolicy/ListPolicies/PutGroup/RemoveGroup with no per-RPC authentication. An unauthenticated network peer could call PutIdentity with Actions:[Admin] and write straight into the live accessKeyIdent map that the SigV4 path reads, bypassing S3 authentication entirely. Mirror the filer's IamGrpcServer.checkAdminAuth: require a Bearer token signed with jwt.filer_signing.key (read from the existing s3a.filerGuard) at the top of every IAM-cache RPC. With no key configured the check is a no-op, matching the rest of SeaweedFS's gRPC surface. * credential: attach admin Bearer token to S3 IAM-cache propagation The filer's PropagatingCredentialStore fans IAM mutations out to peer S3 servers over the SeaweedS3IamCache gRPC service. Now that the S3 handlers require a Bearer token signed with jwt.filer_signing.key, attach one to the outgoing propagation context (mirroring shell/iamAdminAuthContext). With no key configured it is a no-op, so deployments that run without the signing key keep working. * credential: mint IAM-cache admin token after master discovery propagateChange attached the admin Bearer token before ListClusterNodes, so master-client retries could run down the (default 10s) token lifetime before the peer S3 fan-out began, leaving peers to reject an expired token and IAM caches stale. Move withIamCacheAdminAuth to after discovery succeeds, immediately before the propagation timeout is derived. * credential: cap IAM-cache propagation timeout below JWT lifetime The propagation fan-out used a fixed 10s timeout. If an operator configures jwt.filer_signing.expires_after_seconds below 10, the admin token can expire while slower S3 peers are still being contacted, leaving their IAM caches stale. Derive the propagation deadline as min(10s, tokenTTL) so it never outlives the token. withIamCacheAdminAuth now returns the token's lifetime (0 = no expiry) for this purpose. |
||
|
Chris Lu
|
f35e2ccf21 |
s3: warn when a lifecycle change leaves fast-path-stamped objects on their old TTL (#11184)
* s3: warn when a lifecycle change leaves fast-path-stamped objects on their old TTL The per-write TTL fast path (opt-in via s3.bucket.lifecycle.fastpath) stamps a volume TTL at PutObject time that can't be taken back. When an operator lengthens or removes an Expiration.Days rule (or deletes the bucket lifecycle) on a fast-path-enabled bucket, objects already written keep their baked-in TTL and won't be rescued by the change — unlike the default worker-driven path, which re-evaluates the current rules each pass. This is the data-loss direction described in #11183. Surface it: Put/DeleteBucketLifecycle now emit a glog warning and set X-Seaweed-Lifecycle-Fastpath-Warning on the response when the change removes, disables, lengthens, or re-scopes a fast-path-eligible rule. Shortening a rule does not warn (old objects simply expire later, not data loss). Tag-only and overflow-day rules are never on the fast path and never warn. Addresses the warning half of option 2 in #11183. * s3: address review — emit warning after mutation succeeds, fix ID-rename false positive Two issues raised by CodeRabbit, Greptile, and Devin reviews: 1. Failed mutations retained the warning header. The warning was set on the ResponseWriter before storeBucketLifecycleConfiguration / clearStoredBucketLifecycleConfiguration was called; if that failed, the error response carried a warning for a change that was never applied. Now the reason is computed before the mutation but the log and header are emitted only after it succeeds. 2. Rule renames produced false "removed" warnings. fastpathRuleKey used Rule.ID as the sole identity when present, so renaming a rule (same prefix/size/days, different ID) treated the old rule as removed. Replaced with two-pass matching: first by ID, then by fast-path predicates (prefix + size). An ID-only rename with unchanged predicates and days no longer warns. Greedy matching ensures each new rule is consumed by at most one old rule. Added regression tests: ID-only rename (no warn), rename + lengthen (warn), rename + shorten (no warn). |
||
|
|
f99c4a1f14 |
s3: make RenameObject idempotent for a retried request (#11178)
* feat(s3): make RenameObject idempotent for a retried request - #10661 A rename that succeeds but whose response is lost leaves the client with no safe move: retrying returned 404, because the source is already gone, so a retry was indistinguishable from a rename that never happened. The destination now carries what the rename that created it was, under x-seaweedfs-rename-token: the client's token, the source key and the time. A retry that names the same token and the same source and destination is answered 200 without touching anything. The same token sent for a different rename is refused with 409 rather than silently answered, and a token older than 24 hours is treated as unrelated so a key cannot answer for a request indefinitely. Requests without the header behave exactly as before. * s3: answer a reused rename token with 409, not 400 The PR promised Conflict and the code returned Bad Request. 400 tells a client its request was malformed and invites it to give up; this request is well formed and resending it unchanged will not help, because what it collides with is a rename the same token already stands for. The status code is now asserted in a test, since it is the part of this behaviour a client actually acts on. * Update weed/s3api/s3err/s3api_errors.go Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: fix rename token review notes - ErrIdempotentParameterMismatch returns 409 Conflict, not 400. The comment and TestRenameTokenReuseAnswersConflict both expect 409; the code regressed to 400 in a later commit. - stampRenameToken: clarify that markRenameToken mutates srcEntry in place, so the token reaches the destination via the move regardless of whether the UpdateEntry succeeds. The precondition only guards the pre-move write, not the move itself. - Extract the handler retry branch into retryRenameDecision and add TestRetryRenameDecision, covering the source-still-exists fallthrough that was previously reasoned about but not tested. * s3: IdempotentParameterMismatch returns 400, matching AWS docs The AWS S3 RenameObject API documentation specifies HTTP Status Code: 400 for IdempotencyParameterMismatch. Revert the previous 409 change and align the comment and test with the documented behavior. --------- Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
8a68337256 |
filer: pack SSE chunks into manifests (#11175)
* filer: pack SSE chunks into manifests * s3: resolve encrypted manifests before reads * s3: scope encrypted manifest resolution to ranges |
||
|
Chris Luanddevin-ai-integration[bot]
|
811b8b5734 |
make the remote-mount cache wait configurable per mount (#11168)
* add a per-mount cache_wait_ms to the remote storage mount mapping A read of an uncached remote-only object waits on a hardcoded size tier before it can fall back to the origin, so every ranged read of a large remote-only object pays that wait. Carry the wait in the mount mapping so it can be tuned, or set to zero, per mount. * resolve the cache wait of an uncached remote-only read from its mount The wait came only from the object size, so an operator could not trade cache hits for time to first byte. Both read paths now resolve the mount covering the object and let its cache_wait_ms replace the size tiers. * read straight from the remote when a mount waits zero for its cache A mount used as a streaming source pays the cache wait on every ranged read of an object too large to finish caching, and the caching itself is wasted work. A zero wait now skips the cache call, so both read paths go to the origin immediately. * let remote.mount set the cache wait of a mount remote.mount -cacheWait=0 turns a mount into a streaming source, and any other duration trades cache hits against time to first byte. * keep the size based wait for a version-specific read A read pinned to a version cannot fall back to the origin, since the mounted remote only holds the current key, so a mount that opts out of caching would leave it on the 503 retry loop forever. * let the operator allow a remote-only read to dial an internal endpoint The remote-mount read paths in the filer and the S3 gateway always refused an endpoint resolving to a loopback or private host, so a mount backed by an internal S3 could never be read from its origin, only through the local cache. Both now take the allowance the volume server already has, still off by default. * skip the background cache of a mount that waits zero for its cache GetObjectHandler kicks off caching for every remote-only read, so a mount serving as a streaming source kept downloading whole objects even though no read ever waited for them. * cover a zero cache wait end to end The read has to reach a real origin, so the harness also opts the filer and the S3 gateway into dialing the loopback remote it already allows for the volume server. * resolve the S3 cache wait once so the background cache follows it too The background cache that GetObjectHandler starts read the mount on its own, so it skipped a version-specific read that the foreground path still waits for. Both now ask the same resolver. * answer 404 when the origin of a zero-wait read is gone Metadata can outlive the object it points at, and with no cache to fill the read would sit on the 503 retry path forever. The remote backends already report a missing object as ErrRemoteObjectNotFound. * open the origin at write time for a multipart range Every part of a multipart Range is prepared before any is written, so opening eagerly would hold one origin connection per part and leak the ones already opened when a later part fails to open. * reject a cache wait shorter than a millisecond The mapping stores milliseconds, so -cacheWait=500us truncated to zero and silently turned caching off instead of waiting. * restore the doc comment of cacheRemoteObjectForStreamingWithShortTimeout Extracting the wait resolver left its comment on the new function. * stat the origin before committing a multipart range Opening at write time keeps no connection through the preparation, but it also moved a failure past the point where the multipart body picks the response status, so a gone origin truncated a 206 instead of answering 404. One stat up front puts the status back. * stat the origin once per request Every part of a multipart Range is prepared on its own, so the preflight ran once per range instead of once per read. * map Azure and GCS stream not-found to ErrRemoteObjectNotFound ReadFileAsStream on Azure and GCS returned provider-specific not-found errors instead of ErrRemoteObjectNotFound, so a zero-wait read of a deleted object was misclassified as a transient cache failure and retried indefinitely. Map BlobNotFound and ErrObjectNotExist the same way StatFile already does. * Update weed/remote_storage/gcs/gcs_storage_client.go Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> |
||
|
Eliah RusinandChris Lu
|
cfa8afec92 |
filer: guard FoundationDB 100KB value limit and pack earlier (#11161)
* filer: guard the FoundationDB value size limit, not the transaction limit An entry's whole chunk list is one FoundationDB value, and FDB caps a value at 100,000 bytes while a transaction may reach 10MB. UpdateEntry checked the transaction limit, so every entry between the two limits passed the guard and was rejected by FDB itself with error 2103 (Value length exceeds limit). The failure surfaced inside the store rather than at the guard, so the S3 layer dropped the connection and clients saw a network fault instead of an error. Check the value limit in UpdateEntry and KvPut instead, after gzip and before the transaction, with an error that names the limit it hit. The removed transaction-size constant guarded nothing else: DeleteFolderChildren batches by entry count. Refs #11158 * filer: fold at 500 chunks in the foundationdb build Manifest packing is what keeps a large file's entry small, but it only ran once a flat chunk list reached 10000 chunks. A FoundationDB value stops at 100,000 bytes and an entry's whole chunk list is one value, which at ~100 bytes per chunk record is about 1000 chunks -- so on FDB the write always failed before packing could help: a 3.3 GiB PutObject at the default -maxMB=4 was already past the limit. FoundationDB support is its own build (`go build -tags foundationdb`, shipped as its own image), so the batch is a build-time choice and needs no negotiation at run time. The tagged build folds at 500, every other build keeps 10000 and is untouched. 500 is not arbitrary: a single fold level leaves (chunks/batch) manifest pointers plus up to (batch-1) unfolded chunks in the entry, so the reachable chunk count is highest when the two terms are near equal. For a 100,000-byte budget that optimum is 500, which holds an entry inside the limit up to ~250,000 chunks -- ~1 TB at -maxMB=4, against ~4 GB before. Larger files need nested packing, which no batch size substitutes for. One binary serves every role in that image, so the filer and each client that folds -- S3, mount, WebDAV, weed shell, filer.copy -- agree on the batch by construction. A binary built with the tag but pointed at another store folds earlier than that store requires, costing one manifest blob per 500 chunks and one read to resolve it. Fixes #11158 * filer: fold with rollback inside MaybeManifestize, not beside it A fold that fails midway has already uploaded manifest blobs for its earlier batches, and returns only the data chunks -- dropping the manifests it had separated out of the caller's list. Both were wrong in ways that mattered: - AppendToEntry assigned that shortened list straight to entry.Chunks and created the entry, so an append to an already-folded file whose fold failed lost every previously folded chunk. weed mount had the same shape. - cleanupChunks logged the error as "not good, but should be ok" and then returned it through a named result, failing the whole CreateEntry or UpdateEntry, while the blobs it had written stayed behind referenced by nothing. The S3 path was alone in handling this, through a private helper beside MaybeManifestize. A second entry point next to the one everything else calls just means the wrong one gets used, so the behaviour moves inside MaybeManifestize: on failure it returns inputChunks as it received them, and hands the blobs it saved to a deleteChunks callback. The filer, S3 and filer.copy pass their existing deleters -- filer.copy already cleans up this way after a failed upload -- and mount, WebDAV and weed shell pass nil, which reports the blobs rather than collecting them, as before. Each caller keeps its own error policy: the filer HTTP PUT path and filer.copy still fail the request, the rest still continue with the flat list, which is a correct entry. --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
Chris Lu
|
5a515adab2 |
s3: HeadObject with partNumber returns the part's size and 206 (#11166)
* s3: HEAD with partNumber reports the part's size and range HeadObject set its headers from the total object size and then only validated the partNumber, so a client probing part 1 with HEAD got the whole object's Content-Length and a 200 while the same GET returned the part's size, a Content-Range and a 206. Resolve the part's byte range before the headers are written, through the range logic GetObject already used, and answer a partNumber HEAD as the ranged HEAD that AWS documents. * s3: answer an unsatisfiable partNumber with 416 InvalidPartNumber GET and HEAD rejected a partNumber past the number of parts with 400 InvalidPart, the code for a missing part in CompleteMultipartUpload. AWS answers a read of a part that does not exist with 416 InvalidPartNumber, which lets a client probing for the part count tell the two apart. The ceph suite pins RGW's 400 InvalidPart here, so the s3tests jobs patch that expectation the way they already patch prefix ordering. * s3: keep the whole-object checksum off a partNumber response The stored checksum covers the whole object, so it is already withheld from a ranged read. A partNumber HEAD now describes one part while the request carries no Range header, so exclude it there too rather than handing a client a checksum that does not match the bytes described. * s3: resolve a partNumber against the parts the object records Completion accepts ascending, not consecutive, part numbers, so the part count is not the highest part number. Comparing the two rejected an uploaded part 3 of a two-part object, and let a request for the absent part 2 fall through to the positional chunk lookup and serve part 3's bytes. Ask the recorded boundaries for the part instead, and keep the count comparison for objects written before boundaries were stored. * s3: apply a client Range within the part on HEAD too GET narrowed the part by a Range sent alongside partNumber; HEAD reported the whole part, so the two disagreed again for a request that carries both. Move the narrowing into the shared range lookup so either verb describes the same bytes. |
||
|
Chris Luandbruce-zzz
|
0f05957bc4 |
filer: self-heal chunk manifest reads when volume locations go stale (#11107)
* filer: self-heal fetchWholeChunk on stale volume locations Upstream #10156/#10800 wired cache invalidation into the buffer-based read paths, but manifest resolution still goes through fetchWholeChunk, which returns the raw error on failure. When cached volume locations are stale (volume tiered to remote storage, server rolled), resolving a large multipart file fails permanently even though other locations are healthy. Thread the ChunkGroup's cacheInvalidator through ResolveChunkManifest / ResolveOneChunkManifest / fetchWholeChunk, and on failure invalidate, re-lookup and retry once via the existing retryFetchWithFreshLocations helper. The streaming bytesBuffer is reset before the retry so partial bytes from the failed attempt cannot corrupt the manifest proto.Unmarshal. Non-mount callers pass nil and keep their semantics. * filer: move the manifest self-heal tests in with the other manifest tests Also make the stale server stream a prefix and then abort mid-body, which is what actually leaves partial bytes in the buffer: an HTTP error status returns before ReadUrlAsStream ever calls the writer, so a 500 never exercised the Reset the tests claimed to cover. Claude-Session: https://claude.ai/code/session_01FK3oGC5ZVeJYvNBWgb9JUD * filer: keep the cached volume locations when a manifest read is cancelled A cancelled or timed-out read says nothing about where the volume lives, so dropping the location and going back to the master only costs the next reader a round trip. PrepareStreamContentWithThrottler already guards its self-heal this way. The guard also goes inside retryFetchWithFreshLocations, since the caller can be cancelled between its own check and the invalidation, and that covers the reader cache and prefetch paths too. fetchWholeChunk returns the context error rather than the stream failure it provoked, and ResolveOneChunkManifest wraps with %w so errors.Is still sees it. That matters even where no invalidator is passed: volume.fsck resolves manifests with nil and tells its own abort from a corrupt manifest that way, so the cancellation check sits ahead of the nil-invalidator return. Claude-Session: https://claude.ai/code/session_01FK3oGC5ZVeJYvNBWgb9JUD * filer: self-heal manifest reads on the filer and s3 paths too Every caller that already holds the location cache backing its lookup function can hand it over: the filer's read, copy and deletion paths and the log cache have the MasterClient right there, and s3api has the FilerClient. MinusChunks takes one for the same reason, since the deletion path resolves manifests through it. Only the shell tools and the replication sinks, whose lookup functions cache privately with nothing to invalidate, keep passing nil. Claude-Session: https://claude.ai/code/session_01FK3oGC5ZVeJYvNBWgb9JUD --------- Co-authored-by: bruce-zzz <bruce.zou@hhy-data.com> |
||
|
Dmitriy PavlovandChris Lu
|
1d0b97f4c6 |
avro: field time.Time <> iceberg.date (#11091)
* iceberg: normalize foreign day partitions during manifest rewrite * test: cover manifest rewrite with foreign day partitions * iceberg: restore every foreign partition value, not just day transforms iceberg-go takes a partition field's logical type from the last branch of its Avro union, so a writer that spells an optional partition [<type>, null] rather than [null, <type>] leaves the value as whatever the Avro decoder produced. A day or date partition then arrives as a time.Time the manifest writer cannot encode, and a time partition is worse: time.Duration converts to int64 nanoseconds and silently records the wrong value. ReadManifest sits next to ReadManifestList, the other shim for what foreign writers put on the wire, and converts each partition value back to the Iceberg representation for its field type. Claude-Session: https://claude.ai/code/session_01FdQyRuWF9SuCnPn21iH9yR * iceberg: read manifests that carry partition values through the shim Compaction, delete rewrite and their detection passes read entries and write the same partition values back into new manifests, so they fail on a foreign day partition exactly as manifest rewrite does. Where filters see it too: literalMatchesActual falls through to fmt.Sprint, so a time.Time renders as a timestamp and never matches the day the user asked for. The two remaining readers, orphan collection and the admin preview, only look at file paths and stay on iceberg.ReadManifest. Claude-Session: https://claude.ai/code/session_01FdQyRuWF9SuCnPn21iH9yR * iceberg: convert partition values before the writer rebinds logical types Dimonyga checked the manifests of a live Doris table: every input spells the partition union null-first, with the date logical type present, so the union ordering is not what breaks the merge. The conversion is lazy. iceberg-go converts what the Avro decoder returned on the first Partition() call, using the logical types read from the manifest being parsed, and ManifestWriter.addEntry rebinds them to the manifest it is about to write before it makes that call. A day partition is where the two disagree -- iceberg-go's day transform reports an int32 result type, so the manifest it writes carries no date logical type at all -- and an entry nobody looked at in between converts against that and keeps its time.Time. That is why only rewrite_manifests failed: compaction and delete rewrite group entries by partitionKey(df.Partition()) first, which converts them, and a where filter does the same. Reading every entry's partition here converts them all while the manifest's own logical types are still in place. Claude-Session: https://claude.ai/code/session_01FdQyRuWF9SuCnPn21iH9yR --------- Co-authored-by: Chris Lu <chris.lu@gmail.com> |
||
|
|
96242a2be2 |
s3: ListParts on a completed or unknown upload answers NoSuchUpload (#11081)
* s3: ListParts on a completed or unknown upload answers NoSuchUpload complete/abort delete the .uploads/<id> directory, but most filer stores list a missing directory as empty rather than erroring, so listObjectParts answered 200 with an empty Parts list for an upload that no longer exists -- the same response an open upload with no parts yet gets. AWS (and Ceph/RGW, MinIO) answer NoSuchUpload, and clients lean on that: tusd derives the resumable upload offset from the ListParts part sizes, so every completed upload read back as zero bytes received. Probe the upload record before listing, the way completeMultipartUpload already does: not found, or a directory a late part write resurrected without the destination key, answers NoSuchUpload. An open upload with no parts keeps answering 200 with an empty list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GGQEfVUvoATtwRCR8oC2jG * s3: have the ListParts test filer refuse a directory it was not asked about The fake answered the upload lookup on the name alone and the part listing regardless of directory, so a wrong genUploadsFolder or upload-id suffix would still have passed. Both calls now refuse any other directory with an Internal error, which surfaces as ErrInternalError rather than the NoSuchUpload the tests expect. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01StsRz9wbu5dUCMGbFgPoRM --------- Co-authored-by: tomislavcivcija <9787657+tomislavcivcija@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Chris Lu
|
eed5e8cdf6 |
s3: return the multipart object checksum in the CompleteMultipartUpload response (#11101)
* s3: return the multipart object checksum in the CompleteMultipartUpload body S3 carries the flexible-checksum members of CompleteMultipartUploadResult in the XML body, not in response headers, so every SDK read back an empty checksum from an upload that asked for one. Claude-Session: https://claude.ai/code/session_01Huux1uh7JxAbf8yypMYrMk * s3: echo the checksum algorithm and type from CreateMultipartUpload The upload directory already records both, but the response dropped them, so a client could not confirm which checksum its parts had to carry. Claude-Session: https://claude.ai/code/session_01Huux1uh7JxAbf8yypMYrMk * test: multipart upload reports the object checksum it was asked for Covers every algorithm end to end: the create response echoes the algorithm and type, the complete response carries the checksum, and it matches what a later HEAD reports. Claude-Session: https://claude.ai/code/session_01Huux1uh7JxAbf8yypMYrMk |
||
|
Chris Lu
|
86761cc7d5 |
filer: keep empty folders that are s3tables catalog entries (#11102)
* s3tables: build the catalog attribute keys from one shared prefix Every attribute the catalog stores on a bucket, namespace, table or view entry is spelled out with the same literal prefix. Name it once in s3_constants so code outside the package can recognize a catalog entry without repeating the string. Claude-Session: https://claude.ai/code/session_01GfZsc4cyNB2yr6KYLRv9q1 * filer: keep empty folders that are s3tables catalog entries A namespace, table or view is a directory whose extended attributes are the catalog record. Its files can live elsewhere - a rename moves only the catalog pointer and leaves the data at the old path, and a view has no files at all - so an empty one is still a live entry. Drop a table, then rename another table onto that name: the drop queues the old table's folders, the rename recreates the name path, and two minutes later the cleaner deletes it and cascades into the namespace, losing a table the catalog still lists. Claude-Session: https://claude.ai/code/session_01GfZsc4cyNB2yr6KYLRv9q1 * filer: drop a queued cleanup when the folder is created again A cleanup is queued against the folder that was found empty. If that folder is deleted and a new one takes its name, the queue entry outlives the folder it was about and the next pass deletes the replacement. A drop followed by a rename onto the dropped name does exactly this: the name path comes back as a live catalog entry two minutes before the queue is read. Claude-Session: https://claude.ai/code/session_01GfZsc4cyNB2yr6KYLRv9q1 |
||
|
Chris Lu
|
9ea52db219 |
s3: validate the version-id header used as a filer path segment (#11097)
* s3: reject a version-id header that is not a valid path segment
putToFiler stored the client-supplied Seaweed-X-Amz-Version-Id header
verbatim into object metadata. That value is later read back and used
as a filer path component when building the .versions/v_<id> path, so a
value containing "/", "\" or ".." could steer retention/legal-hold
writes and remote-cache reads outside the object's own bucket tree.
Validate the header with isValidVersionID before storing it, the same
check the versioned read paths already apply, and reject the request
otherwise. Server-set version ids ("null" and generated hex) pass.
Claude-Session: https://claude.ai/code/session_011QqNaxZwnpHgMoAZNp3RkY
* s3: validate a stored version-id before using it as a path
The retention and legal-hold sinks build a .versions/v_<id> path from a
version id read back out of object metadata, and the remote-cache path
builder does the same from either the request or the stored id, without
the isValidVersionID check the other version-id consumers apply. Guard
these so a value that is not a valid path segment falls back to the
regular / unversioned path instead of steering the write or read out of
the bucket tree.
Claude-Session: https://claude.ai/code/session_011QqNaxZwnpHgMoAZNp3RkY
|
||
|
Chris Lu
|
23adeb37e2 |
s3: check Object Lock on directory-marker keys before bucket deletion (#11096)
recursivelyCheckLocksWithClient tested EntryHasActiveLock only on non-directory entries, so a directory-marker object (an S3 key ending in "/") that carries retention or a legal hold was recursed into but never lock-checked. DeleteBucket then saw no locks and removed the bucket, destroying an object under active Object Lock along with the rest of the bucket. DeleteObject already enforces the lock on the same key, so the two paths disagreed. Check the directory entry for an active lock before recursing. Claude-Session: https://claude.ai/code/session_011QqNaxZwnpHgMoAZNp3RkY |
||
|
Chris Lu
|
77a9dd4b9e |
s3: route per-key object authorization through a shared helper (#11072)
* s3: share the per-key object authorization across copy and delete AuthorizeCopySource and AuthorizeObjectDelete both authorize a key the request URL does not name by evaluating the bucket policy and IAM against a synthetic per-key request; only the method and action differed. Extract that into authorizeObjectKeyAction and make the two callers thin wrappers. No behavior change. Claude-Session: https://claude.ai/code/session_01Qo7p6VsoWxMo8816ogJFk5 * s3: route POST Object uploads through the shared object authorization POST Object uploads (presigned-POST / HTML form) authorized the write with only the coarse per-identity Write action, unlike the other write paths which also check the resolved object against the bucket policy and IAM. Route POST through authorizeObjectKeyAction via a new AuthorizeObjectWrite so it is authorized like the equivalent PUT. Claude-Session: https://claude.ai/code/session_01Qo7p6VsoWxMo8816ogJFk5 * s3: test POST Object per-key authorization Drives a signed POST upload and checks the per-key authorization decision for a denied, permitted, and admin caller. Claude-Session: https://claude.ai/code/session_01Qo7p6VsoWxMo8816ogJFk5 |
||
|
Chris Lu
|
34f5442e9b |
s3api: push the listing prefix down to the filer in ListObjectVersions (#11070)
The version walk listed every directory with no prefix, transferring all 1024-entry batches over gRPC and filtering gateway-side - and kept paging past the point where names can no longer match. On wide directories (many sibling orgs/jobs next to the requested prefix) that is most of the transfer, decode, and CPU cost of every page. Derive the next path component of the requested prefix per directory level and hand it to the filer listing. A name holds no slash, so a directory whose name does not start with the component cannot contain a matching key and a file that does not cannot be one; stores with native prefixed listing (sql, leveldb) turn this into a range scan and stop the stream at the end of the prefix zone. Claude-Session: https://claude.ai/code/session_01FquvGtTD2zA3uMZGQHAuV4 |
||
|
Chris Lu
|
81ca5cb6c6 |
s3api: drop two redundant filer round-trips per listed version entry (#11068)
* s3api: drop two redundant filer round-trips per listed version entry ListObjectVersions paid two avoidable getEntry calls while walking a bucket, both re-fetching data the walk already held: - getObjectVersionList re-read the .versions directory entry that every caller had just received from listing the parent directory (or from its own sibling probe). Pass the entry down instead: one RPC saved per object listed. - getObjectOwnerFromVersion, on a version with no stamped owner, re-fetched the same version entry its OwnerID had been extracted from. The refetch cannot answer differently, so data written before owners were stamped cost one futile RPC per listed version, forever. All round-trips on this path are sequential, so on large versioned buckets (Veeam-style workloads) they add up to a visible share of per-page latency and gateway CPU. Claude-Session: https://claude.ai/code/session_01FquvGtTD2zA3uMZGQHAuV4 * s3api: treat a nil .versions entry as an empty version list filer_pb.GetEntry's contract permits (nil, nil) for an absent entry, and the old internal lookup answered that case with an empty list. Keep that answer now that the entry arrives from the caller. Claude-Session: https://claude.ai/code/session_01FquvGtTD2zA3uMZGQHAuV4 |
||
|
Chris Lu
|
4c9cbf72bc |
s3api: stop retrying a definitive NotFound in getLatestObjectVersion (#11067)
The .versions lookup retried every error through the full backoff ladder, so a missing key spent 12.7s (8 attempts, 100ms..6.4s) before the pre-versioning fallback could answer. NotFound is an answer, not a transient failure: gate the retries on isRetryableFilerErr, the same classifier retryFilerOp already uses, which also stops retrying for callers whose context is canceled or past its deadline. GetObject already treats NotFound on .versions/ as definitive; this brings the retention/tagging/ACL/attributes/delete/copy paths that go through getLatestObjectVersion in line with it. Claude-Session: https://claude.ai/code/session_01FquvGtTD2zA3uMZGQHAuV4 |
||
|
Chris Lu
|
87474c2f21 |
s3: let attached policies authorize CreateBucket (#11049)
* s3: resolve admin bucket subresources to their specific S3 actions Encryption, requestPayment, publicAccessBlock and ownershipControls requests reached the policy engines as s3:*, so only a policy granting all of s3 could authorize them. Map each subresource to its AWS action, with DELETE sharing the PUT permission as AWS does. Claude-Session: https://claude.ai/code/session_01XH7iM88ZqWMEvsLB8tkWPQ * s3: authorize CreateBucket as s3:CreateBucket in the policy engine A plain bucket-level PUT is registered with ACTION_ADMIN, which resolved to s3:*, so no attached policy short of s3:* could match it. Federated sessions whose policy explicitly allowed s3:CreateBucket were always denied while the same policy worked for object operations. Resolve it to s3:CreateBucket, like DeleteBucket already resolves. Claude-Session: https://claude.ai/code/session_01XH7iM88ZqWMEvsLB8tkWPQ |
||
|
Chris Lu
|
ba5b14b457 |
master, filer, s3api: bound the collection deletes that strand a caller (#11026)
* master: bound each volume server DeleteCollection, and finish the fan-out A collection delete fanned out to every volume server holding it with context.Background(), so a server that accepted the connection and then went quiet held the whole delete open with nothing to end it. Each RPC is bounded now, on the same budget allocateVolumeTimeout gives the other master-to-volume-server admin RPC. The volume server runs the delete to completion regardless of the request context, so giving up costs the confirmation and not the deletion. The walk itself is the caller's, not a per-server one: - It outlives the caller. A cancelled request must not abandon a destructive fan-out part-done, with volumes left behind and no request still running to come back for them. - It no longer stops at the first server that refuses, which left the collection on every server after it in the list. The first failure is still what is reported, and the collection stays in the topology so a later delete comes back for the rest. - It sends one RPC per server rather than one per replica. ListVolumeServers reports a node once for every replica it holds, while DeleteCollection removes the whole collection from the server it reaches, so a collection with thousands of volumes repeated the same whole-collection delete thousands of times over. Both passes run too. Returning after a failed normal pass left the collection's EC shards in place with nothing left to retry them. Claude-Session: https://claude.ai/code/session_01EnB1fbryyKc2LetRZxQPTP * master: delete the EC shards behind /col/delete too The HTTP handler carried its own copy of the volume-server walk and only ever ran the normal pass, so a collection deleted through it kept its EC shards. It shares the gRPC path now, which also gets it the bounded RPCs and the one-per-server fan-out. Claude-Session: https://claude.ai/code/session_01EnB1fbryyKc2LetRZxQPTP * filer: bound the collection delete a bucket delete leaves behind Deleting a bucket entry deletes its collection afterwards, deliberately detached from the request so a client that hangs up cannot strand the bucket's volumes. Detached meant unbounded, though: with the master down or mid-election the wait for a leader has nothing to end it, so the handler parks, and the client retrying behind it parks another. It keeps outliving the request and now carries a deadline of its own. The budget bounds the wait, not the work: the master keeps deleting on its own fan-out once asked, so giving up costs the confirmation. Claude-Session: https://claude.ai/code/session_01EnB1fbryyKc2LetRZxQPTP * s3api: bound the collection RPCs a bucket creation and deletion issue Neither carried a deadline, so a transient failure anywhere down the chain held the S3 request open until the client gave up on it. Both budgets are taken outside the filer failover walk, so one budget covers the whole walk rather than granting each filer a fresh one. The walk itself stops when that budget is spent, and stops without blaming anyone: the caller's own expiry is not evidence against the filer that was answering, and the next filer has no time left to answer in either. Recorded as a filer failure, a slow master upstream would flag every filer in the walk, and the three failures that open the circuit take unrelated object reads down with them. Claude-Session: https://claude.ai/code/session_01EnB1fbryyKc2LetRZxQPTP * s3api: a failed collection listing no longer fails a bucket creation PutBucket lists collections to notice a leftover one it is about to reuse. The result feeds a warning and nothing else -- s3a.exists is what decides whether the bucket already exists -- yet a transient failure of that listing returned 500 and refused the creation. It is advisory now, so a failure is logged and the creation continues, exactly as it does when the listing returns false. Claude-Session: https://claude.ai/code/session_01EnB1fbryyKc2LetRZxQPTP |
||
|
Chris Lu
|
7dc3835b02 |
s3: an abort answered mid-part no longer leaves the upload completable (#11025)
* s3: reject a part whose upload was aborted while its body was in flight The upload-exists check runs before the part body is read. An abort answered during the read deletes the upload directory, and the part write that follows re-creates it, so the aborted upload is listed nowhere yet completes. Re-check after the write: only createMultipartUpload stamps the destination key on .uploads/<id>, so a directory without it is one the part write resurrected. Drop it along with the part and answer NoSuchUpload. Claude-Session: https://claude.ai/code/session_01ByZ49KQdUtHhmjG6SpNczT * s3: reject a copied part whose upload was aborted mid-copy UploadPartCopy has the same window as UploadPart: the upload-exists check runs before the bytes are copied, and the part write that follows re-creates the directory an abort removed. Both the re-encryption and the raw-copy path re-check before answering. Claude-Session: https://claude.ai/code/session_01ByZ49KQdUtHhmjG6SpNczT * s3: do not complete an upload whose directory holds no upload record A .uploads/<id> directory that a part write created rather than createMultipartUpload carries no destination key, no owner and no encryption settings. Completing one turned stray parts into an object; answer NoSuchUpload instead. Claude-Session: https://claude.ai/code/session_01ByZ49KQdUtHhmjG6SpNczT * s3: log the part left behind when the resurrected directory survives abortMultipartUpload can fail to remove what the part write re-created. The client still hears NoSuchUpload, since the upload is gone either way and a retry would only write another part, but the leftover is worth a line. Claude-Session: https://claude.ai/code/session_01ByZ49KQdUtHhmjG6SpNczT |
||
|
Chris Lu
|
7bb0a1c127 |
s3: replay a delete whose reply the transport dropped (#11022)
* s3: stop retrying a delete the filer refused for a non-empty folder The filer looked and the children are there, so the answer will not change. retryFilerOp spent six attempts and up to 3.1s of backoff on it before the caller could act on the condition it was already holding. Claude-Session: https://claude.ai/code/session_01XqaJrwgXQ5GSUpyzRbe5nD * s3: thread the request context through the unversioned delete path doDeleteEntry issued every DeleteEntry on context.Background(), so an S3 client that hung up left the gateway working on its behalf, out of reach of both cancellation and the per-request retry allowance that DeleteMultipleObjectsHandler installs. Claude-Session: https://claude.ai/code/session_01XqaJrwgXQ5GSUpyzRbe5nD * s3: treat a cancelled filer RPC as terminal, not transient isRetryableFilerErr matched context.Canceled and DeadlineExceeded by sentinel, which only holds while the error is still local. Once it has crossed gRPC it is a status, so an abandoned request was retried six times on behalf of a caller that had already gone. Claude-Session: https://claude.ai/code/session_01XqaJrwgXQ5GSUpyzRbe5nD * s3: replay a delete whose reply the transport dropped A delete is idempotent at the filer, which answers an entry that is already gone with an empty resp.Error, so a reply lost in transit can be reissued rather than surfaced. Surfaced, it becomes a 500 on the bucket delete, which boto3 resends and is then answered NoSuchBucket, or a per-key InternalError inside the 200 of a multi-object delete, which no SDK retries at all. The replay runs through retryFilerOp, so it draws on the allowance the request already installs rather than paying a backoff per key, and stops for a caller that has gone. rm and rmObject re-enter WithFilerClient per attempt, so each one walks the failover list again on a connection the failed attempt had invalidated; the multi-object loop holds one client for the batch, so there the replay reuses it. Classification stays structural. The filer reports its own refusals in resp.Error, which carries no status and has the deleted path - and, for a recursive delete, the children it stopped on - formatted into it, so no key name can steer the decision either way. rm and rmObject now take the caller's context. Cleanup and rollback paths pass context.Background() deliberately: they have to run whether or not the caller is still waiting. Claude-Session: https://claude.ai/code/session_01XqaJrwgXQ5GSUpyzRbe5nD * s3: share one retry allowance across multipart completion cleanup The unused-entry loop deletes once per entry, and each delete now retries, so a filer that stays unavailable held the response for 3.1s per entry after the object was already committed. Claude-Session: https://claude.ai/code/session_01XqaJrwgXQ5GSUpyzRbe5nD |
||
|
Chris Lu
|
60893c5ef3 |
Classify a filer error before a user-controlled path is wrapped into it (#11004)
* util, pb: classify a filer error by the status the server sent DoSeaweedListWithSnapshot wrapped a failed ListEntries with %v, dropping the gRPC status, so IsTransientError fell back to matching substrings against a message that now held the caller's path. Keep the status with %w and let it decide, reading the server's own text rather than the wrapper's. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * s3: keep the bucket and prefix out of the list retry decision A bucket named transport, or a prefix under logs/unavailable/, made a PermissionDenied listing look transient and got it retried; a key holding the not-found sentence suppressed a retry that should have run. Both checks now read the filer's status, and only fall back to the text when there is none. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * filer, s3: classify a delete failure before the path is wrapped into it The filer put the non-empty-folder marker behind its own "delete directory %s" wrapper and the gateway matched it as a substring, so a key named after the marker turned a real delete failure into the demote-the-marker no-op and the request answered 204. Keep the marker leading the message that crosses the wire, turn it back into a sentinel where the response is read, and match that. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU |
||
|
Chris Lu
|
9e06e1d0f9 |
Report a delete the filer rejected instead of answering success (#11003)
* s3tables: report a delete the filer rejected deleteDirectory discarded DeleteEntryResponse and checked only the transport error, so DeleteTable, DeleteNamespace, DeleteView and DeleteTableBucket answered 200 for a delete the filer refused. Call filer_pb.DoRemove, which reads resp.Error and still treats a missing entry as success. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * admin: report a delete the filer rejected The bucket delete, the file browser handlers and the topic retention purger all discarded DeleteEntryResponse, so a delete the filer refused came back as success. Call filer_pb.DoRemove, which reads resp.Error. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * credential: report a delete the filer rejected DeleteUser, DeletePolicy and the full-sync cleanup loops discarded DeleteEntryResponse, so a rejected delete answered success and left the credential file in place. The service account path in the same store already read resp.Error; the rest now do too, via filer_pb.DoRemove where not-found is already tolerated. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * shell: report a delete the filer rejected remote.configure -delete, remote.cache and the remote metadata sync discarded DeleteEntryResponse, so a rejected delete printed as removed. Call filer_pb.DoRemove, which reads resp.Error. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * mq: report a delete the filer rejected The consumer offset group purge and the coordinator assignment delete discarded DeleteEntryResponse. Call filer_pb.DoRemove, which reads resp.Error. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * iam: count only the revocation entries the filer actually deleted The expiry sweep discarded DeleteEntryResponse, so a rejected delete was counted as purged and the entry stayed. Call filer_pb.DoRemove, which reads resp.Error, matching the role and provider stores beside it. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * mount: fail rmdir when the unary fallback delete was rejected The streaming branch turns DeleteEntryResponse.Error into an error, the unary fallback dropped it, so rmdir of a non-empty directory answered OK off the stream and ENOTEMPTY on it. Surface it in both. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU * s3tables: fail DeleteTableBucket when the directory delete is refused The handler only failed when both the leaf entry and the directory delete failed, so a refused bucket directory delete still answered 200 with the bucket in place. The directory is the bucket, so it decides; the leaf entry stays best-effort. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU |
||
|
Chris Lu
|
742b2f5896 |
s3: share one retry allowance across a batch delete (#11001)
Every key in a multi-object delete drives its own retryFilerOp, so a filer that is briefly unhealthy multiplied one op's ~3.1s of backoff by a key count the client picks. The batch now carries a single allowance in its context, sized to one op's worst case; once it is spent the remaining keys fail fast with a per-key error instead of holding the request goroutine. A single-object delete carries no allowance and keeps its full retries. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU |
||
|
Chris Lu
|
d850f36513 |
s3: distinguish a failed bucket lookup from a missing bucket on HEAD (#11000)
HeadBucket treated any lookup error as ErrNoSuchBucket, so a transient filer failure answered 404 instead of 500 and clients stopped retrying. Split the two cases the way the bucket policy handlers already do. Claude-Session: https://claude.ai/code/session_01BjDWtZsCoZY6x4pdDmGWxU |
||
|
Chris Lu
|
fdd8bd9478 |
s3: reject a request that names two operations (#10987)
The router matches bucket subresource routes in registration order while the IAM action resolver matches its own list in a different order, so a request carrying two operation subresources is authorized as one operation and served as another. `PUT /bucket?policy&tagging` resolves to s3:PutBucketTagging and runs PutBucketPolicy, letting an identity delegated bucket tagging install an arbitrary bucket policy. The same mismatch reaches PutBucketCors, PutBucketLifecycle, PutBucketVersioning, PutObjectLockConfiguration, PutBucketRequestPayment and the policy and cors deletes. Reject the ambiguity where the other pre-routing checks live, so neither list has to stay in step with the other. Keys that modify an operation rather than select one -- versionId, partNumber, prefix -- still combine freely. |
||
|
Chris Lu
|
2a97e08caa |
s3: cover the directory marker key with object lock (#10988)
* s3: enforce object lock when deleting a directory marker The key "dir/" is deleted the unversioned way, ahead of the branches that enforce Object Lock, so a principal with plain delete permission could remove a key the gateway was reporting as COMPLIANCE-retained -- retention set through PutObjectRetention is stored on the directory entry and served back by GetObjectRetention, only the delete ignored it. The same path also takes any key ending in "/" regardless of size, while a PUT only makes a marker of one up to 1KiB. A larger one is a genuine versioned object, and deleting it here dropped its whole history after the versioned delete of the same key had been refused. Enforce in the marker delete itself, so the single, versioned and multi-object delete paths are all covered. * s3: apply object lock headers on a directory marker PUT The trailing-slash branch runs before the versioning and Object Lock handling, so it accepted x-amz-object-lock-* headers and stored none of them: a bucket owner could believe a key was retained while nothing recorded it, and an invalid mode or a past retention date that a regular key rejects came back 200 here. Validate the headers the way the regular path does, store what they ask for beside the owner the same callback already sets, and refuse to replace a key that is already retained. * s3: check every version a marker delete would remove The marker delete clears any history under the key in one recursive removal, while the lock check ahead of it resolves the latest version only. A version retained under an unretained one was taken with the rest, so enforce against each version the removal covers. * test: pin the marker lock refusals to AccessDenied A bare require.Error passes on any failure, including one that has nothing to do with the lock. Assert the code, the key the batch delete reports, and that the marker survives each refusal. * s3: check the history entries a version list leaves out The version list skips an entry without a version id, while the removal takes it with the rest, so an entry an older build left unnamed escaped the check. Walk the history directly instead, and refuse when an unnamed entry is still under a retention or a legal hold of its own. * s3: let a governance bypass reach an unnamed history entry The unnamed branch refused every active retention, so a caller allowed to bypass governance could not clear one, which the named path lets through. Refuse a legal hold and compliance mode as before, and take the bypass into account for governance. * s3: keep the object lock decision in one place The unnamed history entry had to repeat the retention and legal hold rules inline because the enforcement helper only takes a key to look up. Split the part that judges an entry out of it and call that from both. * s3: guard a marker PUT on the entry it replaces The overwrite check resolved the key's latest version, but mkdir builds a fresh entry for the marker itself, dropping the lock metadata the old one carried. Once the key had a history, an unlocked version answered for a retained marker and a plain PUT replaced it. Judge the entry the write is about to replace instead; a versioned write of the same key still adds a version, which is its own to allow. * s3: guard a marker delete on the entry it removes The check ran against the key rather than the entry, so once the key had a history it answered with a version and the retention recorded on the marker itself went unseen. Judge the entry that is about to be removed, the same way the PUT side now does; the versions under it are still covered by the walk that follows. * s3: take the object write lock for a marker PUT The overwrite check read the entry that the mkdir after it replaces, so two marker PUTs could both pass while one was still unlocked. The marker delete already runs under this lock; hold it across the check and the mkdir so the entry cannot change in between, and so the two paths are serialized against each other. |
||
|
Chris Lu
|
ab8b34720a |
s3tables: delete only the location the dropped table owns (#10986)
DeleteTable authorizes the named table, then recursively purges the data path derived from its stored MetadataLocation. That location is supplied by the caller at create/register time and never bound to the table, so a tenant allowed to drop one table could point it at a table in a sibling namespace and have the delete destroy that table's catalog entry and data files. A legitimately decoupled location -- a rename source, or a leftover the name was reused over -- has had its catalog attributes stripped, so a surviving metadata marker identifies a path that belongs to another entry. Refuse those, alongside the existing ancestor refusal. |
||
|
Chris Lu
|
0b5fff2ccd |
filer, s3: reuse the volume server's guarded remote-storage client builder (#10990)
* volume: build the guarded remote storage client through a shared helper Fold the endpoint validation, credential check and rebinding-safe dialer that FetchAndWriteNeedle applies before dialing a caller-supplied remote storage endpoint into a single BuildGuardedRemoteStorageClient helper, so other callers that dial the same endpoints can reuse it. No behavior change on this path. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN * filer: build the remote-mount stream client through the guarded helper streamFromRemote serves a cold remote-only entry straight from its mounted origin. Build its client through BuildGuardedRemoteStorageClient so the same endpoint checks the volume server applies cover this read path too. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN * s3: build the remote-mount stream client through the guarded helper openRemoteStream serves a remote-mounted object straight from its origin when the local read cannot. Build its client through the same guarded helper so the endpoint checks apply here as well. Claude-Session: https://claude.ai/code/session_01AiH1FU3rmshSbFFTbJpaZN |
||
|
Chris Lu
|
28862c866e |
Authorize an Iceberg table create before it writes (#10991)
* s3tables: share one CreateTable authorization gate CreateTable and RegisterTable each carried their own copy of the name validation, policy load and permission check. Fold them into authorizeCreateTable, and expose it on the Manager for callers that write into a table bucket before the table itself is registered. Claude-Session: https://claude.ai/code/session_01QiJkka1T2NAWDWq4JQ8Vuy * iceberg: authorize a table create before it writes Stage-create returns before the S3Tables registration that authorizes a create, and the plain create writes its metadata file before reaching it, so a caller who may not create the table could still leave a staged template, a marker and a v1.metadata.json in the target bucket - and get vended credentials for a location of their choosing. Run the CreateTable gate as soon as the table is known to be absent. Claude-Session: https://claude.ai/code/session_01QiJkka1T2NAWDWq4JQ8Vuy * iceberg: authorize a create-on-commit the same way A commit against a table that does not exist creates it, writing the metadata file first and only then reaching the registration that checks the caller may create it. Denied callers saw a 500 for what is a 403. Claude-Session: https://claude.ai/code/session_01QiJkka1T2NAWDWq4JQ8Vuy * iceberg: pin that identity actions reach the create gate The manager request is built from the caller's own context, so an identity whose actions carry the permission still passes. Worth a test: a fresh context here would silently deny every such caller. Claude-Session: https://claude.ai/code/session_01QiJkka1T2NAWDWq4JQ8Vuy |
||
|
Chris Lu
|
d8a189f07f |
s3: keep a missing object a 404 under If-Match and If-Unmodified-Since (#10985)
* s3: keep a missing object a 404 under If-Match and If-Unmodified-Since GET and HEAD resolved the target before evaluating the conditional headers, and a missing target failed If-Match and If-Unmodified-Since outright, so absence surfaced as 412 PreconditionFailed. AWS reports the missing object instead: 404 for HeadObject, NoSuchKey for GetObject, and 412 only when a live object fails the condition. Clients cannot tell absence from a stale precondition without an extra racy HEAD, so OpenDAL disabled its four conditional stat/read capabilities against SeaweedFS. A precondition now only fails against an object that exists; a missing one -- including a latest version that is a delete marker -- returns NoSuchKey. Claude-Session: https://claude.ai/code/session_01X4kEbuwxd9DFsTnSXjfjgv * s3: evaluate a conditional read against the version the request names GET and HEAD resolved the latest version before evaluating the conditional headers, so a request carrying versionId had its If-Match compared against a different version than the one it was asking for: a live version whose ETag the client held failed once a newer version -- or a delete marker -- became the latest. resolveObjectEntry now resolves the named version on a versioned bucket, the way DELETE already does. A named version that resolves to nothing is left to the handler, which alone knows whether the bucket is versioned and so whether it owes NoSuchVersion. Claude-Session: https://claude.ai/code/session_01X4kEbuwxd9DFsTnSXjfjgv |
||
|
Chris Lu
|
f5f1dcbd8c |
s3: keep verifying the request host when externalUrl is set (#10970)
* s3: keep verifying the request host when externalUrl is set externalUrl was the only host candidate once set, so a client that dialed the gateway directly instead of through the proxy always got SignatureDoesNotMatch. Make it lead the candidate walk instead: every candidate still needs a valid signature, and the request-derived hosts are already trusted when the flag is unset, so a mixed proxy plus in-cluster topology can now advertise a public endpoint and verify both planes. * s3: cover virtual-hosted addressing behind externalUrl The old pin also rejected an external client that signed bucket.api.example.com, since only the bare externalUrl host was ever tried. The candidate walk covers it; pin the case down. |
||
|
Chris Lu
|
3431bdcb74 |
s3: fix UploadPartCopy with volume-data encryption (#10971)
* operation: give an encrypted chunk the plaintext ETag With -encryptVolumeData the volume server stores ciphertext, so it cannot echo a Content-MD5 back and the chunk lands with an empty ETag. Every ETag derived from those chunks then comes out empty for a single chunk, or d41d8cd98f00b204e9800998ecf8427e-N for several. The caller already hashes the plaintext to send as Content-MD5, so keep that digest as the chunk ETag instead of dropping it, and compute it for a WantMd5 caller under cipher too. * s3: re-encrypt a part copy from a volume-encrypted source UploadPartCopy raw-copies source chunks when neither side uses SSE, which also caught -encryptVolumeData sources. Those chunks are ciphertext a whole-chunk cipher key decrypts, so copying a byte range out of one and keeping the key leaves a destination that fails authentication on GET, and the copied chunks carry no ETag for the part result to report. Route them through the re-encrypting path already used for SSE: it reads the source as plaintext, hashes the part, and writes the destination under the gateway's own encryption. * s3: fetch only the range a part copy asked for The re-encrypting UploadPartCopy path opened the source at offset 0 and threw the prefix away, so assembling an object part by part read the source once per part. Now that volume-encrypted sources take this path too, that is the common case rather than an SSE corner. The chunk stream already seeks, so hand it the range. * s3: reject an unsatisfiable copy-source-range A part copy has no way to report a short part, so a range reaching past the source cannot be clamped the way a GET clamps one. The fast path silently produced a part shorter than asked for, or an empty one; the re-encrypting path pads with zeros, so a 2 MiB source copied as bytes=1048576-9999999 came back as 1 MiB of data followed by 7.5 MiB of nothing. Answer InvalidRange instead, which is what s3-tests' test_multipart_copy_invalid_range expects. |
||
|
Chris Lu
|
368b2035b2 |
s3: deny anonymous access when the identity config loads no identities (#10954)
* s3: deny anonymous requests when the identity config loads no identities Naming a config file is the operator asking for authentication. A file that yields no identity - an unpopulated secret mount, or a mistyped top-level key the proto parser silently drops - left the gateway open to every anonymous caller: ListBuckets returned 200, and anonymous PUT could create buckets and write objects. * s3: name the unknown top-level keys in an identity config The proto parser discards what it does not recognise, so a mistyped "identites" loads as an empty config. Naming the dropped keys at startup turns the resulting lockout into a one-line diagnosis. * s3: isolate the auth-enforcement tests from AWS environment credentials * s3: use a singular "identity" as the unrecognised-key example Codespell rejects the misspelling the example used. * s3: cover the empty identity config alongside the unrecognised key * s3: cover a config file whose body is an empty object |
||
|
Chris Lu
|
2a70532d0d |
s3: log each request at -v=2 (#10931)
* s3: log each request at -v=2 * s3: quote requester and path in the access log line * s3: record the post-policy signing identity as the requester |
||
|
Chris Lu
|
d2c470af1b |
S3: commit SSE GET status only after the first read succeeds (#10935)
The SSE streaming path kept writing 200/206 from filer metadata before fetching or decrypting anything, so a missing needle or failed decrypt setup surfaced as a broken 200 body. Same deferral as the plain path: the status commits on the first body write, and every failure before that returns to the handler for a clean S3 error response. |
||
|
Chris Lu
|
d9d5fab35b |
S3: commit GET status only after the first read succeeds (#10930)
streamFromVolumeServers wrote the 200/206 status from filer metadata before any byte had been fetched from a volume server, so a missing or corrupted needle surfaced as a broken 200 body and the request metrics recorded a success. Defer the status commit to the first body write: a failed first read now returns a clean 500 before headers, while the wire timing of successful responses is unchanged since net/http buffers the status line until body bytes arrive anyway. |
||
|
Chris Lu
|
863fec6c3f |
S3: let a key that is a prefix of other keys be an object (#10912)
* filer: keep the sentinel when CreateEntry reports an update failure CreateEntry flattened the error UpdateEntry wraps, so errors.Is stopped matching and ErrExistingIsDirectory and ErrExistingIsFile never reached the S3 mapper, which answered a retryable 500 instead. * s3: let a key that is a prefix of other keys be an object S3 keys are flat, so "a/b" and "a/b/c" are independent objects that coexist in either write order. The filer stores a key as a path, so one of them has to live on the directory the other is nested under. Writing the nested key first refused the prefix key outright. Writing it second promoted the file to a directory, which kept its data but lost the key: an empty object left nothing to recognise it by and disappeared, and one with data listed under a trailing slash it never had. Mark the directory that carries such a key, and write the object onto it when the path is already a directory. The mark makes an empty prefix object visible to listings and readable by GET and HEAD, keeps the empty folder cleaner off it, and lists it under the key it was written with. Deleting the key strips the mark back off along with the data. * filer: keep a TTL off a directory that stands for an object An expired entry is deleted a row at a time, so expiring a directory removes it and leaves everything under it unreachable. Promoting a file to a directory carried its TTL across, and a promoted file is exactly the one that has keys nested under it. Drop the TTL on promotion, and leave one an older build wrote alone. The lifecycle worker still expires the object, through the delete that leaves the directory behind. * s3: delete the null version of a key other keys are nested under The routed delete cannot remove an entry that other keys live under, and answered a retryable 500 rather than falling back to the lock path the unversioned delete already falls back to. That path then looked the entry up under the bucket with the whole key as its name, so the demote wrote it back one directory too high and failed as not found. Fall back on any non-precondition error, and split the key before deleting it. Trailing-slash directory markers with children reach the same delete. * filer: keep the sentinel when MkFile and Mkdir report a create failure Same flattening one layer out: every mkFile caller lost the sentinel, so a CopyObject onto a key that other keys are nested under answered a retryable 500 where a PutObject of the same key answers 409. * s3: copy and rename a key that other keys are nested under Such a key is stored on the directory those keys live in, and copy and rename both refused it: the source lookup maps every directory entry to NoSuchKey, so a key a plain GET serves could not be copied or moved, and the destination side refused it as a directory conflict. The source is read through a view of the entry as the object it names. The destination is written the way a PutObject of that key writes it. A rename at either end copies the object's own data across and strips it off the source key rather than going through AtomicRenameEntry, which moves a directory by moving everything under it - the nested keys are not part of what is being renamed. |
||
|
Chris Lu
|
69cc2869ad |
Fixes from the review of the admin bucket policy UI (#10907)
* admin: treat a missing S3 Tables policy as an empty load, not an error
The bucket/table policy GET relayed the backend's 404 NoSuchPolicy to the
dialog, whose loader treats any non-OK response as a load failure and
keeps Save and Delete blocked. A bucket or table without a policy could
never be given one. Return policy null instead, the same contract
ShowBucketPolicy uses for classic buckets.
* admin: reject policy documents the structured editor would misread
A top-level JSON array passed the object guard (typeof [] is 'object')
and loaded as a zero-statement policy, which the next commit would
rewrite to an empty document. Object elements in Action/Resource were
coerced to '[object Object]' and saved that way on the s3tables surface,
which stores policies verbatim. Both now throw, which routes the
document to the JSON tab like other unrepresentable shapes.
* admin: let the JSON tab save documents the structured editor can't model
Save with the JSON tab active required a round-trip through
policyDocToEditorState, so exactly the documents the dialogs shunt to
'JSON tab only' mode (unrepresentable Effect, Resource+NotResource, and
the like) could never be saved - Delete was the only mutation left.
Invalid JSON still blocks; an unrepresentable document now saves and the
editor state stays marked unparsed.
* admin: pin the policy editor to what each consumer's backend supports
The s3tables evaluator has no NotResource/NotPrincipal fields - it
silently drops them, turning Allow+NotResource into allow-everything and
making Deny+NotPrincipal inert - and it only matches s3tables: actions
against s3tables ARNs, while the editor suggested s3: actions and
arn:aws:s3::: resources. New registerPolicyEditor knobs: allowNegation
hides the Not* modes and routes documents using them to the JSON tab;
resourceSuggestions pins the Resource autocomplete to the open
resource's ARN; the S3 Tables dialogs get an s3tables-only action
datalist. requirePrincipal now also hides NotPrincipal, which
policy_engine.ValidateBucketPolicy always rejects, and the client-side
check requires Principal specifically to match that server rule.
* admin: save S3 Tables policies from a button, not form submission
The multi-input structured editor sits inside a form whose Save button
was type=submit, so Enter in any single-line editor input - accepting an
autocomplete suggestion, say - implicitly submitted whatever half-built
statement the editor held, and the backend stores the document verbatim.
A lone statement with no Principal matches nobody, locking out every
non-owner. Save is now an ordinary button and the form ignores
submission.
* admin: block zero-statement policy saves
Committing the active tab before the emptiness check made 'Policy JSON
is required' dead code: an empty editor serializes to {"Statement":[]},
which the s3tables backend stores verbatim - evaluated default-deny for
every non-owner, while the statement-count column keeps showing 'Not
configured'. All three policy dialogs now refuse a save with no
statements and point at Delete instead. The classic bucket modal only
gained a clearer message; the server already rejected the document.
* admin: guard S3 Tables policy mutations against stale and overlapping requests
The save/delete completions ran against whatever resource the shared
modal happened to show by then: a slow PUT for one bucket would hide the
modal mid-edit of another and misattribute its alerts, a late DELETE
cleared the shared textarea over the newly opened resource with its
loaded flag set, and nothing stopped a double-click from firing two
overlapping mutations. Ported the classic modal's pattern: capture the
target on start, flag the mutation in flight with the buttons disabled,
and only touch the UI when the completion still matches the open
resource. Success now reloads the page, which also keeps the Policy
column's statement count honest.
* admin: confirm before deleting an S3 Tables policy
Delete Policy sat next to Save and fired on a single click; with
default-allow enabled one stray click silently dropped the resource
policy and left the bucket open to every principal. Same confirmation
the classic bucket modal already has.
* admin: let a corrupt stored bucket policy be shown, fixed, and deleted
A stored document the decoder rejects made the policy GET 500, and with
the loaded flag never set the modal blocked both Save and Delete - the
one policy an operator most needs to remove was the one they couldn't,
even though the delete path never reads the document. The GET now
returns the raw bytes alongside a null policy; the dialog hands them to
the JSON tab and unblocks the buttons.
* admin: url-encode the bucket name in the policy API calls
The filer lists any directory under the buckets path, names S3 would
never allow included; one carrying '#' or '%' broke the fetch URL or
addressed a different name than the modal shows.
* admin: drop stale edit-policy responses on the IAM policies page
The same race the bucket and S3 Tables dialogs already guard against:
open one policy's editor while its GET stalls, open another, and the
late response populates the editor under the second policy's name -
Update then saves the first policy's statements over the second.
* admin: warn before a bucket policy save drops unsupported fields
The editor tracks unmodeled top-level keys precisely so
confirmPolicyFieldDiscard can warn before the server's Version+Statement
decode discards them, but only the IAM page called it; the bucket modal
saved a pasted document with e.g. a console-generated Id without a word
while the editor kept displaying the field.
* s3: enforce the bucket policy size cap on both surfaces
The 20KB cap lived only in the admin UI, so a larger policy stored via
the S3 API displayed there but could never be re-saved, desyncing the
two writers the cap comment claimed could not desync. The constant now
lives in policy_engine next to the shared validator and PutBucketPolicy
rejects oversized documents with PolicyTooLarge, matching AWS.
* admin: ship the policy editor's fieldset styles with the editor
The .policy-stmt-* rules that undo Bootstrap's full-width legend reset
stayed behind in policies.templ when the editor markup moved to the
shared script, so the bucket and S3 Tables dialogs rendered Actions/
Resource/Principal as full-width jumbo headings. PolicyDatalists is the
component every consumer already renders once; the styles live there
now.
* s3: mirror bucket policy changes into the IAM store from the metadata subscription
The advanced-IAM path appends the bucket-policy:<bucket> document to
every STS/session evaluation, but only this gateway's own PutBucketPolicy
maintained that mirror - a policy tightened or created through the admin
UI (or another gateway) never reached it, so revoked access stayed live
indefinitely, and the delete side was an unimplemented TODO in any case.
The metadata subscription now diffs the stored policy on every bucket
entry change and updates or removes the mirror, covering all writers and
deletion with one mechanism; IAMManager gains the missing
RemoveBucketPolicy.
* admin: deduplicate the bucket policy write path
Set and Delete carried line-for-line identical filer closures;
bucketPolicyMutation already treats nil as clear-the-key. The shared
helper sits below Set's validation, since ValidatePolicy cannot take the
nil document Delete passes.
* s3: drop ValidateBucketPolicy's re-checks of ValidatePolicy rules
Both callers run ValidatePolicy first, which already enforces the
version and at-least-one-statement rules; the duplicates were dead code
with drifted error text.
* admin: seed a new statement's Resource from the pinned suggestions
A fresh statement on the S3 Tables dialogs started with no resource row
at all; seed it with the broadest pinned ARN the same way cfg.bucket
already seeds the classic modal.
* admin: refuse to save Not* fields the backend would silently drop
Hiding the NotResource/NotPrincipal modes was not enough where negation
is disallowed: the JSON tab accepts any valid document (that is its
job), and a statement's Advanced-fields box can reintroduce the keys, so
an s3tables save could still store fields the evaluator drops - turning
Allow+NotResource into allow-everything. commitPolicyActiveTab now runs
a final document-level check over what would actually be saved; Delete
stays available for cleanup.
* s3: move the IAM bucket policy mirror on a bucket rename
A same-directory rename delivers one event carrying both entries, and
the byte-equality short-circuit skipped the new name's mirror when the
policy was unchanged - while the replayed delete for the old name
removed its mirror, leaving the renamed bucket unmirrored. The mirror
decision is now a pure function that removes the old name and writes the
new one regardless of byte equality, with the rename cases unit tested.
* s3: backfill the IAM bucket policy mirror on lazy bucket loads
The metadata subscription only mirrors changes, so a policy that
predates the IAM integration never reached the bucket-policy:<bucket>
mirror and its grants did not bind on the IAM path until the policy was
next modified. The gateway is deliberately lazy at startup (nothing
lists all buckets), so the backfill hooks the same place a bucket's
policy first becomes known: the cold bucket-config load. EnsureBucketPolicy
writes only when no mirror is stored, so repeat loads cost one cached
read.
* s3: reconcile the bucket policy backfill against concurrent changes
The backfill's check-then-write could race an event-driven mirror update
or removal and re-store bytes that were already stale, with no later
event to heal it. EnsureBucketPolicy now reports whether it wrote, and a
write is reconciled against a fresh authoritative entry read: a changed
policy is re-mirrored, a removed one is removed. Anything changing after
that read fires its own event, which finds the backfill's write already
present and supersedes it. The backfill also carries the entry's raw
bytes rather than a re-marshaled document, so the reconcile can
byte-compare.
* s3: prime the bucket policy mirror before advanced-IAM authorization
The backfill ran from the lazy bucket-config load, but IAM authorization
evaluates the bucket-policy:<bucket> mirror before any handler runs - a
grant carried only by a not-yet-mirrored policy denied forever, and the
denied request never reached the code that would have loaded the bucket.
authorizeWithIAM now primes the bucket config first (an in-memory cache
hit once warm), and the backfill runs synchronously on the cold load so
the very first authorization already sees the mirror.
|
||
|
Mathieu Arnold
|
e931cccc7b |
Manage bucket policies via the admin ui (#10895)
* admin: manage S3 bucket policies from the admin UI
Bucket policies were only manageable through the S3 PutBucketPolicy API;
the admin UI had no equivalent to the quota/owner/lifecycle editors it
already offers. Add GET/PUT/DELETE for a bucket's policy, sharing the
exact validation the S3 gateway uses.
- Extract validateBucketPolicy/validateResourceForBucket out of
s3api_bucket_policy_handlers.go into policy_engine.ValidateBucketPolicy /
ResourceMatchesBucket so both the S3 API and the admin UI enforce
identical rules.
- weed/admin/dash/bucket_policy.go: Get/Set/DeleteBucketPolicy, writing
through ObjectTransaction + PATCH_EXTENDED (the lifecycle pattern) so a
concurrent owner/quota/lifecycle change on the same bucket entry isn't
clobbered. Propagation to every S3 gateway is automatic via the existing
filer metadata log subscription. The S3 gateway's IAM policy mirror is
deliberately not replicated here (its delete path is already an
unimplemented TODO on the S3 side).
- New GET/PUT/DELETE /api/s3/buckets/{bucket}/policy routes, CSRF-guarded
on writes.
- Bucket list and details modal now show a statement-count badge, read
from the entry already fetched (no extra RPC).
- UI: a JSON-textarea policy editor modal, matching the lifecycle modal's
structure.
* admin: reuse the visual policy editor for bucket policies
Extract the structured policy editor (add/remove statement, action/
resource/principal rows with autocomplete, JSON tab kept in sync) out of
policies.templ's inline script into a shared
weed/admin/static/js/policy_editor.js, and wire the bucket policy modal
in s3_buckets.templ up to it instead of a bare JSON textarea.
- registerPolicyEditor(which, config) replaces the hardcoded create/edit
id derivation with a per-instance config (textarea/tab/body ids,
datalist ids, requirePrincipal, bucket). The IAM policies page keeps its
exact pre-extraction ids via two registerPolicyEditor calls, so its
markup is unchanged.
- New policy_datalists.templ exposes the three shared <datalist>s
(actions/resources/principals) as @PolicyDatalists(), now rendered by
both policies.templ and s3_buckets.templ.
- requirePrincipal seeds new bucket-policy statements with Principal: "*"
and adds a client-side check before save (the server, via
policy_engine.ValidateBucketPolicy, remains the actual authority); the
bucket config pins the Resource autocomplete to the open bucket instead
of fetching every bucket in the cluster.
- layout.templ loads policy_editor.js globally, after admin.js/
modal-alerts.js (basePath/escapeHtml/showAlert) which it depends on.
3a (the extraction) is a byte-preserving move verified against the
unchanged policies.templ behavior before layering 3b's parameterization
and the bucket-policy wiring on top.
* admin: migrate S3 Tables bucket/table policy editors to the shared editor
Third consumer of the shared visual policy editor: the S3 Tables bucket
and table policy modals (a bare JSON textarea each) now get the same
structured Editor/JSON tabs as the bucket policy and IAM policy pages,
via registerPolicyEditor('s3tablesBucketPolicy'/'s3tablesTablePolicy',
{ textareaId: ... }). Storage and validation are untouched - S3 Tables
policies still go through their own s3tables.PolicyDocument type and the
s3tables.policy extended attribute, unrelated to policy_engine and
s3-bucket-policy; only the editor UI is shared.
Fix a real bug surfaced by adding this second load path: the bucket
policy modal (and the naive first draft of this s3tables port) called
commitPolicyTextareaToEditor() right after a GET and then force-switched
to the Editor tab. commitPolicyTextareaToEditor() is designed to leave
the current tab in place and the editor state untouched when a document
fails to parse (so an in-progress edit survives a bad tab switch), so
forcing the Editor tab afterwards could show empty/stale editor state
that a careless Save would then serialize over a perfectly valid but
structurally-unusual stored policy. Add
loadPolicyTextareaIntoEditor(which) to policy_editor.js, which has no
"current tab" to defer to and instead falls back to the JSON tab with an
alert on a document the structured editor can't represent - the same
safety editPolicy already had in policies.templ - and use it at all three
"populate the editor right after a GET" call sites (bucket policy,
S3 Tables bucket policy, S3 Tables table policy).
* admin: show policy statement count on the S3 Tables buckets page
Mirrors the "Policy" column already added to the classic S3 buckets
list: a clickable badge with the statement count when the table bucket
has a resource policy, "Not configured" otherwise. S3 Tables policies
are a separate mechanism (s3tables.PolicyDocument under the
s3tables.policy extended attribute) from the S3 bucket policy work
elsewhere in this branch (policy_engine.PolicyDocument /
s3-bucket-policy), so this is a parallel implementation of the same
pattern rather than shared code.
- S3TablesBucketSummary gains PolicyStatementCount, populated in
GetS3TablesBucketsData from entry.Entry.Extended[s3tables.ExtendedKeyPolicy]
via the new extractS3TablesPolicyStatementCountFromEntry - no extra RPC,
the entry is already fetched for ExtendedKeyMetadata.
- The badge reuses the existing .s3tables-bucket-policy-btn class, so it
opens the same policy modal as the row's action button with no JS
changes.
* admin: don't let a failed policy GET open the door to an empty overwrite
loadS3TablesBucketPolicy/loadS3TablesTablePolicy cleared the textarea,
then unconditionally called loadPolicyTextareaIntoEditor() regardless of
whether the GET actually succeeded - including when fetch() rejected or
the response was not ok, silently logged to console only. That leaves
the structured editor holding a legitimate-looking empty policy
({version, statements: []}), with the Editor tab active by default.
If Save is then clicked, commitPolicyActiveTab() serializes that empty
state into the textarea as `{"Version":"2012-10-17","Statement":[]}` -
a non-empty string - before the "Policy JSON is required" guard ever
sees it, so the guard passes and the transient load failure gets
written over whatever policy was actually stored.
Add s3tablesBucketPolicyLoaded/s3tablesTablePolicyLoaded, set true only
once a GET has actually completed (ok, including a genuinely empty
policy) and false on any failure path (fetch rejection or a non-ok
response, which previously fell through silently). Both submit handlers
now check the flag before touching the editor at all, and a failed load
surfaces via alert() instead of only a console.error - the user
previously had no visible indication the load had failed.
Verified with a jsdom simulation driving the real rendered page against
a stubbed fetch: a failed GET followed by Save now sends no PUT at all
(previously it sent Statement: []); a successful GET followed by Save
still PUTs the loaded policy unchanged.
* admin: address code review findings on the policy editor
1. policy_editor.js: policyEditors is only pre-populated for 'create'/
'edit'; every other `which` (bucket, s3tablesBucket, s3tablesTable)
stays undefined until its first successful async load. Nothing in
this file enforces that a page hide its Editor/JSON tabs and
Add-statement button until that load completes - the S3 Tables policy
modals don't - so a click in that window (e.g. Add statement, or
switching to the JSON tab) threw "Cannot read properties of undefined
(reading 'unparsed')". Add policyEditorState(which), which lazily
initializes a default state, and route addPolicyStatement, the
jsonTabBtn 'show.bs.tab' handler, commitPolicyActiveTab, and
renderPolicyEditor through it. Verified with a jsdom simulation
against a never-resolving fetch: the exact click threw on the
pre-fix code and no longer does.
2. s3_buckets.templ: the bucket-policy Save handler checked the
textarea for emptiness before calling commitPolicyActiveTab(), which
is what actually serializes the structured Editor tab's fields into
that textarea. A policy entered entirely through the Editor tab (the
primary path - never touching the JSON tab) left the textarea at
whatever it was at load time, so creating a new policy this way hit
"Enter a policy document" and Save silently did nothing. Move the
commit before the emptiness check, preserving the existing alert and
early-return. Verified with a jsdom simulation: Add-statement then
Save (no tab switch) now PUTs the entered statement; before the fix
the same sequence never reached fetch().
3. s3tables_buckets.templ / s3tables_tables.templ: the policy Editor/
JSON nav-tabs were missing the ARIA roles Bootstrap's own tab pattern
expects (role="tab"/"tabpanel", aria-selected, aria-controls,
aria-labelledby) - screen readers had no way to tell these were tabs
or which pane went with which button. Added the standard Bootstrap 5
tab markup to both.
* admin: guard policy load/save flows against overlapping requests
1. s3tables.js: loadS3TablesBucketPolicy/loadS3TablesTablePolicy had no
protection against overlapping loads. Opening one bucket's (or
table's) policy dialog and then another's before the first GET
resolved let the late response write its document into the shared
textarea and mark the dialog "loaded" while it was now targeting the
second resource - a subsequent Save would then push the first
resource's policy onto the second. Add a per-load monotonic sequence
number (s3tablesBucketPolicyRequestSeq / s3tablesTablePolicyRequestSeq,
the same pattern already used for the classic bucket-policy load in
s3_buckets.templ); a response is only applied - textarea, loaded flag,
editor state - if its captured sequence still matches the latest one
issued.
Verified with a jsdom simulation: bucket A's policy load (artificially
slow) followed immediately by bucket B's (fast) previously left A's
policy in the textarea once A's late response landed; it now correctly
keeps B's.
2. s3_buckets.templ: the bucket-policy Save button lives outside the
(initially hidden) editor wrapper, so it stays clickable while a load
is still in flight - the existing policyRequestSeq guard only protects
the *load* from a stale response, not Save from firing before any
load for the current bucket has completed. Add bucketPolicyLoaded,
reset before each GET and set only once the matching response lands,
and check it at the top of the Save handler.
Verified with a jsdom simulation: clicking Save immediately after
opening the dialog, before a (deliberately never-resolving) GET
settles, now sends no PUT; a normal load-then-save sequence still
PUTs the loaded policy unchanged.
* admin: address further code review findings on the policy editor
1. s3tables.js: loadS3TablesBucketPolicy/loadS3TablesTablePolicy only
reset the JSON textarea when a new load starts; the structured editor
kept showing the previously loaded resource's statements (Editor tab
is the default active one) until the new fetch resolved. Call
loadPolicyTextareaIntoEditor() against the now-cleared textarea
immediately, so switching resources visibly resets the editor right
away instead of only once its own load completes. Verified with jsdom:
opening bucket A (loads fully) then bucket B (GET never resolves) no
longer leaves A's statements visible in B's editor.
2. s3tables.js: deleteS3TablesBucketPolicy/deleteS3TablesTablePolicy had
no loaded-state check, so a failed GET (which already blocks Save)
left Delete fully able to remove the resource's stored policy sight
unseen. Add the same s3tablesBucketPolicyLoaded/s3tablesTablePolicyLoaded
guard Save already uses. Verified with jsdom: delete after a failed
load now sends no DELETE; delete after a successful load is unaffected.
3. s3_buckets.templ: the bucket-policy Editor/JSON nav-tabs were missing
the same ARIA roles already added to the S3 Tables policy tabs in an
earlier round (role="tab"/"tabpanel", aria-selected, aria-controls,
aria-labelledby) - this instance was out of scope for that review
comment but is the same gap. Bootstrap's own tab.js already manages
aria-selected on tab switch once the attribute exists, so no extra JS
was needed.
4. s3_buckets.templ: neither the bucket-policy Save nor Delete handler
guarded against a double-click, or against firing while the other was
still in flight - two overlapping PUT/DELETE requests for the same
bucket could land in either order. Add a shared
bucketPolicyMutationInFlight flag: set (and both buttons disabled)
before each fetch, cleared (and buttons re-enabled) on failure so the
user can retry, left set through the existing success hide-and-reload
path, and also reset when a new bucket's dialog opens so an abandoned
in-flight request from a closed dialog can't leave the buttons stuck
disabled. Verified with jsdom: double-clicking Save now sends exactly
one PUT, and a Delete click while that PUT is still pending sends no
DELETE.
* admin: scope bucket-policy mutation completions to the bucket that started them
1. The previous round's fix reset bucketPolicyMutationInFlight whenever a
new bucket's policy dialog opened, to avoid leaving Save/Delete stuck
disabled if the modal was closed mid-request. That traded one bug for
a worse one: if bucket A's PUT/DELETE was still in flight when the
user opened bucket B's dialog, the reset let B's Save/Delete fire
immediately, and A's completion handler - unaware anything had
changed - would still hide the (now B's) modal and reload the page
out from under whatever the user was doing with B, on success, or
alert a message with no bucket context, on failure.
Stop resetting on reopen, so a pending mutation for a previous bucket
keeps this bucket's Save/Delete blocked until it settles (matches the
"preventing overlapping mutations" the review comment describes).
Instead, capture policyEditorBucket as targetBucket right before each
fetch and compare it against policyEditorBucket again in the
completion handler: the in-flight flag is always released so the
buttons never get stuck, but the modal-hide/reload/alert only fire if
this bucket is still the one showing; a stale completion for an
abandoned bucket just logs to the console instead.
Verified with a jsdom simulation: opening bucket B while bucket A's
Save is still pending leaves B's Save button disabled and a click on
it a no-op; once A's PUT resolves, B's button re-enables but no
modal.hide()/reload() fires (previously both fired unconditionally).
2. bucketPolicyDeleteBtn had no bucketPolicyLoaded check, unlike Save -
a failed GET blocked Save but left Delete free to remove a policy the
client never actually saw (the same gap already fixed for the S3
Tables policy modals in an earlier round). Added the same guard,
ahead of the confirm() dialog. Verified with jsdom: Delete after a
failed load now sends no DELETE request.
* admin: fix spelling mistake
|
||
|
孙超
|
c80664ec21 |
s3: propagate storage rule fsync to volume server uploads (#10906)
The storage rule's fsync decision was computed by the filer (detectStorageOption -> rule.Fsync) and applied on the filer's own HTTP write path, but was never carried onto the chunk uploads S3 issues: the AssignVolumeResponse had no fsync field, so the s3api client could not learn the decision, and the chunked upload URL was hardcoded without it. Every S3 write to a path with fsync configured went to the volume server as a non-fsync write. Carry the decision through the assign response: - filer.proto: AssignVolumeResponse gains bool fsync, filled from the storage option the assign resolved. - operation.AssignResult gains Fsync, so uploadChunk can append ?fsync=true to the volume server upload URL (single and replica fan-out paths). - The S3 PUT/UploadPart assignFunc, the S3 copy path, the admin file browser upload, and the Iceberg worker assign functions all forward the response field. Adds TestUploadReaderInChunksAppendsFsyncWhenAssigned. |
||
|
Chris Lu
|
36c97344ef |
s3: confine a Lance catalog table location to the caller's own bucket (#10901)
The Lance namespace gateway took the request-body location field, trimmed a trailing slash, and passed it straight to the marker sink. That location feeds TableDataDirFromMetadataLocation, which joins it under /buckets and collapses any ../ segments, and writeMarker's CreateEntry then auto-creates every missing parent. A caller could point the location at another tenant's bucket, or escape /buckets entirely, and plant a fixed-name marker (recursively creating the parents) or hide a victim's live table with .lance-deregistered. Confine the declared location the way the Iceberg gateway already does: require an s3:// URI whose bucket is the caller's own and whose path carries no traversal segment, on both the declare and register handlers. |
||
|
Chris Lu
|
cf0dba334c |
s3api: no filer failover after the callback has consumed part of a response (#10902)
s3api: no filer failover after fn has consumed part of a response withFilerClientFailover replays fn verbatim on the next filer, so a filer that died mid-stream followed by a healthy peer returned success with the callback's closure-captured accumulator holding the dead filer's prefix twice; the per-attempt accumulator in listWithRetry could not close this, because the replay happens inside a single attempt. Track delivery on the connection handed to fn: once a unary reply or streamed message has reached the callback, surface the transport error unwrapped instead of failing over, and let callers replay from a clean slate. A filer that fails before delivering anything fails over exactly as before. |
||
|
Chris Lu
|
8d8a25b1cf | s3api: remove the duplicated listing retry helpers left by overlapping merges | ||
|
Chris LuandJunker der Provinz
|
c58795354a |
s3api: retry a transient filer failure on metadata listings (#10890)
* s3api: retry a transient failure when listing multipart uploads/parts A blip on the way to the filer failed the whole ListMultipartUploads or ListParts request. Both reported failure points sit inside one streaming listing: the ListEntries call that opens the stream, and the stream.Recv calls that drain it. Neither retried, so a single Unavailable answer from a filer that was restarting turned into a 500 for the S3 client. Replay the listing instead, bounded to three attempts with a 100ms backoff that doubles. Only a transient failure is replayed. A not-found answer stays authoritative so the empty-list branch still works, and every other error still reaches the client on the first attempt. This is scoped to (*S3ApiServer).list rather than added inside DoSeaweedListWithSnapshot, which mount, the shell and the other object listings share, and where a retry after a partial stream would re-deliver entries the callback had already seen. Within one call to list, a replay is safe: it collects into a fresh slice each time, so it can neither duplicate nor drop entries. That guarantee does not extend past this function. withFilerClientFailover already re-runs its callback against the next filer on any non-NotFound error without resetting the caller's accumulator, so on a multi-filer gateway a mid-listing failover can itself produce a duplicated result with err == nil, independent of this change and not fixed by it. Noted in the PR rather than silently left for someone to rediscover. Fixes #7221 References #7235 * s3api: move the listing retry inside list itself --------- Co-authored-by: Junker der Provinz <jdp@braethoria.com> |