apache / apache/iceberg-python

Concurrent writes to a remote-signing catalog go out unsigned and fail 403

Đang mở
#3,896 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Python
Star
1.1k
Fork
581
Merge trung bình
1 ngày 17 giờ
Pull request đã merge (30 ngày)
78

Mô tả

### Apache Iceberg version

main (development) — also reproduced on 0.12.0 and 0.11.1.

### Please describe the bug 🐞

`_s3()` unregisters the S3 request signer and re-registers it on an event emitter that fsspec caches and every thread shares. A request signed in that window goes out with **no `Authorization` header** — `_s3()` has already set `config_kwargs["signature_version"] = UNSIGNED`, so botocore does not sign in its place — and the store answers `403 AccessDenied`.

`pyiceberg/io/fsspec.py`, in `_s3()`:

```python
fs = S3FileSystem(**s3_fs_kwargs)

for event_name, event_function in register_events.items():
fs.s3.meta.events.unregister(event_name, unique_id=1925) # opens the window
fs.s3.meta.events.register_last(event_name, event_function, unique_id=1925)
```

`FsspecFileIO.get_fs` caches per thread and every table gets its own `FileIO`, so a short workload makes hundreds of these cycles against the one shared emitter. PyIceberg's writer is concurrent by default, so no unusual usage is needed to reach it.

Roughly **3–4% of appends** fail against a remote-signing catalog, surfacing as an opaque `PermissionError: Access Denied` out of `s3fs`, several frames from its cause. Every unsigned request caught on the wire was a `PUT` of a manifest during commit.

Measured on Lakekeeper 0.13.1 + MinIO (path-style), 90 writes per run:

| configuration | append failures | unsigned on wire |
|---|---|---|
| as shipped | 2 / 4 / 5 | 2 / 4 / 5 |
| signer registered once (client still shared) | 0 | 0 |
| `PYICEBERG_MAX_WORKERS=1` | 0 | 0 |

The second row is the one that matters: the failures stop while the client is still shared, which separates this from a general concurrency problem. The response code is always `AccessDenied` and never `SignatureDoesNotMatch` — what an *unsigned* request produces, not a mis-signed one.

### Suggested fix

Drop the `unregister`. Botocore's `HierarchicalEmitter._register_section` returns early for a `unique_id` it already holds, so re-registering an equivalent signer was already a no-op; the unregister only opens the window.

### Reproduction

Two tests in `tests/io/test_fsspec.py`, no credentials and no network:

- `test_s3_leaves_a_signer_installed_while_reconfiguring_a_shared_client` — drives `_s3()` and observes the emitter the instant it unregisters.
- `test_the_signer_stays_installed_while_another_thread_reconfigures_s3` — the same window seen from another thread. The window is two adjacent statements, so sampling for it blind is a coin flip (20,000 observations caught it zero times); it is held open by delaying only the *re-registration*, so the timing is deterministic while the defect is not manufactured.

Both fail on `main` and pass with the fix.

### Already covered by an open PR

**#3783** (for #3625) removes this `unregister` as part of registering the signer on an `AioSession`, for a different reason — a lazily created client not inheriting handlers, failing `InvalidRequest`. I applied that PR and ran both tests above against it: **both pass**, so it fixes this as well.

Worth recording rather than closing silently, because the line is wrong for two independent reasons and #3783's own test does not cover this one. If #3783 lands, these two tests are the regression cover for the concurrency window; happy to raise them against that PR instead if a maintainer prefers.

### A second, independent defect in the same file — noted, not proposed here

`pyiceberg/io/fsspec.py:160` applies the signing service's headers with `add_header`, which appends, so every header the service echoes back appears twice — 880 of 1615 sign calls ended with a duplicate of a header named in `SignedHeaders`. On MinIO this is **latent**: de-duplicating changed nothing, and it did not contribute to the 403s above. SigV4 combines repeated headers comma-separated, so a stricter store may reject them. Mentioned so it is not lost, not to widen this issue.

### Willingness to contribute

I can contribute a fix to resolve this bug independently.

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Hướng nghiên cứu

Bắt đầu trong pyiceberg/io/fsspec.py tại _s3(), sau đó chạy hai test được nêu tên trong tests/io/test_fsspec.py. Được coi là hoàn tất khi signer vẫn được cài đặt trong lúc reconfiguration của shared-client diễn ra và cả hai concurrency regression test đều vượt qua; PR #3783 đã bao gồm bản sửa lỗi được báo cáo.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
python
Lĩnh vực
cloud
Loại issue
Lỗi
Độ khó
1/5
Thời gian dự kiến
Dưới một giờ
Mức độ hoạt động
Đình trệ
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
35/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.